AMBARI-18404 - Upgrade Summary Endpoint Throws NPEs Due To JPA Cached Entities With Missing IDs (jonathanhurley)

ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java

@@ -344,7 +344,6 @@ public class ActionDBAccessorImpl implements ActionDBAccessor {
       StageEntity stageEntity = stage.constructNewPersistenceEntity();
       stageEntities.add(stageEntity);
       stageEntity.setClusterId(clusterId);
-      //TODO refactor to reduce merges
       stageEntity.setRequest(requestEntity);
       stageDAO.create(stageEntity);
 
@@ -353,9 +352,6 @@ public class ActionDBAccessorImpl implements ActionDBAccessor {
       for (HostRoleCommand hostRoleCommand : orderedHostRoleCommands) {
         HostRoleCommandEntity hostRoleCommandEntity = hostRoleCommand.constructNewPersistenceEntity();
         hostRoleCommandEntity.setStage(stageEntity);
-
-        HostEntity hostEntity = null;
-
         hostRoleCommandDAO.create(hostRoleCommandEntity);
 
         assert hostRoleCommandEntity.getTaskId() != null;
@@ -365,6 +361,7 @@ public class ActionDBAccessorImpl implements ActionDBAccessor {
         String output = "output-" + hostRoleCommandEntity.getTaskId() + ".txt";
         String error = "errors-" + hostRoleCommandEntity.getTaskId() + ".txt";
 
+        HostEntity hostEntity = null;
         if (null != hostRoleCommandEntity.getHostId()) {
           hostEntity = hostDAO.findById(hostRoleCommandEntity.getHostId());
           if (hostEntity == null) {
@@ -372,6 +369,7 @@ public class ActionDBAccessorImpl implements ActionDBAccessor {
             LOG.error(msg);
             throw new AmbariException(msg);
           }
+
           hostRoleCommandEntity.setHostEntity(hostEntity);
 
           try {
@@ -401,9 +399,10 @@ public class ActionDBAccessorImpl implements ActionDBAccessor {
         hostRoleCommandEntity.setExecutionCommand(executionCommandEntity);
 
         executionCommandDAO.create(hostRoleCommandEntity.getExecutionCommand());
-        hostRoleCommandDAO.merge(hostRoleCommandEntity);
+        hostRoleCommandEntity = hostRoleCommandDAO.merge(hostRoleCommandEntity);
+
         if (null != hostEntity) {
-          hostDAO.merge(hostEntity);
+          hostEntity = hostDAO.merge(hostEntity);
         }
       }
 
@@ -411,8 +410,9 @@ public class ActionDBAccessorImpl implements ActionDBAccessor {
         roleSuccessCriteriaDAO.create(roleSuccessCriteriaEntity);
       }
 
-      stageDAO.create(stageEntity);
+      stageEntity = stageDAO.merge(stageEntity);
     }
+
     requestEntity.setStages(stageEntities);
     requestDAO.merge(requestEntity);
   }

ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleCommand.java

@@ -209,6 +209,22 @@ public class HostRoleCommand {
 
     hostRoleCommandEntity.setEvent(event.getEventJson());
 
+    // set IDs if the wrapping object has them - they are most likely
+    // non-updatable in JPA since they are retrieved from a relationship,
+    // however the JPA cache may choose to not refresh the entity so they would
+    // end up being null if not set before persisting the command entity
+    if (requestId >= 0) {
+      hostRoleCommandEntity.setRequestId(requestId);
+    }
+
+    if (stageId >= 0) {
+      hostRoleCommandEntity.setStageId(stageId);
+    }
+
+    if (taskId >= 0) {
+      hostRoleCommandEntity.setTaskId(taskId);
+    }
+
     return hostRoleCommandEntity;
   }
 

ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostRoleCommandEntity.java

@@ -511,8 +511,27 @@ public class HostRoleCommandEntity {
     return stage;
   }
 
+  /**
+   * Sets the associated {@link StageEntity} for this command. If the
+   * {@link StageEntity} has been persisted, then this will also set the
+   * commands stage and request ID fields.
+   *
+   * @param stage
+   */
   public void setStage(StageEntity stage) {
     this.stage = stage;
+
+    // ensure that the IDs are also set since they may not be retrieved from JPA
+    // when this entity is cached
+    if (null != stage) {
+      if (null == stageId) {
+        stageId = stage.getStageId();
+      }
+
+      if (null == requestId) {
+        requestId = stage.getRequestId();
+      }
+    }
   }
 
   public HostEntity getHostEntity() {

ambari-server/src/main/java/org/apache/ambari/server/topology/HostRequest.java

@@ -18,6 +18,17 @@
 
 package org.apache.ambari.server.topology;
 
+import static org.apache.ambari.server.controller.internal.ProvisionAction.INSTALL_AND_START;
+import static org.apache.ambari.server.controller.internal.ProvisionAction.INSTALL_ONLY;
+import static org.apache.ambari.server.controller.internal.ProvisionAction.START_ONLY;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
 import org.apache.ambari.server.actionmanager.HostRoleCommand;
 import org.apache.ambari.server.api.predicate.InvalidQueryException;
 import org.apache.ambari.server.api.predicate.PredicateCompiler;
@@ -37,17 +48,6 @@ import org.apache.ambari.server.state.host.HostImpl;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-
-import static org.apache.ambari.server.controller.internal.ProvisionAction.INSTALL_AND_START;
-import static org.apache.ambari.server.controller.internal.ProvisionAction.INSTALL_ONLY;
-import static org.apache.ambari.server.controller.internal.ProvisionAction.START_ONLY;
-
 /**
  * Represents a set of requests to a single host such as install, start, etc.
  */
@@ -355,9 +355,6 @@ public class HostRequest implements Comparable<HostRequest> {
     for (HostRoleCommand task : logicalTasks.values()) {
       HostRoleCommandEntity entity = task.constructNewPersistenceEntity();
       // the above method doesn't set all of the fields for some unknown reason
-      entity.setRequestId(task.getRequestId());
-      entity.setStageId(task.getStageId());
-      entity.setTaskId(task.getTaskId());
       entity.setOutputLog(task.getOutputLog());
       entity.setErrorLog(task.errorLog);
 

ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java

@@ -27,6 +27,7 @@ import java.util.Collections;
 import java.util.List;
 
 import javax.persistence.EntityManager;
+import javax.persistence.NamedQuery;
 
 import org.apache.ambari.server.AmbariException;
 import org.apache.ambari.server.Role;
@@ -611,6 +612,34 @@ public class TestActionDBAccessorImpl {
 
   }
 
+  /**
+   * Tests that entities created int he {@link ActionDBAccessor} can be
+   * retrieved with their IDs intact. EclipseLink seems to execute the
+   * {@link NamedQuery} but then use its cached entities to fill in the data.
+   *
+   * @throws Exception
+   */
+  @Test
+  public void testEntitiesCreatedWithIDs() throws Exception {
+    List<Stage> stages = new ArrayList<Stage>();
+    Stage stage = createStubStage(hostName, requestId, stageId);
+
+    stages.add(stage);
+
+    Request request = new Request(stages, clusters);
+
+    // persist entities
+    db.persistActions(request);
+
+    // query entities immediately to ensure IDs are populated
+    List<HostRoleCommandEntity> commandEntities = hostRoleCommandDAO.findByRequest(requestId);
+    Assert.assertEquals(2, commandEntities.size());
+
+    for (HostRoleCommandEntity entity : commandEntities) {
+      Assert.assertEquals(Long.valueOf(requestId), entity.getRequestId());
+      Assert.assertEquals(Long.valueOf(stageId), entity.getStageId());
+    }
+  }
 
   private static class TestActionDBAccessorModule extends AbstractModule {
     @Override