Distribution set assignment: Fix NPE in AbstractDsAssignmentStrategy (#777)

* Fix NPE
* Add unit test
* Fix review findings

Signed-off-by: Stefan Behl <stefan.behl@bosch-si.com>

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/AbstractDsAssignmentStrategy.java

@@ -11,6 +11,7 @@ package org.eclipse.hawkbit.repository.jpa;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -31,6 +32,8 @@ import org.eclipse.hawkbit.repository.model.Action.Status;
 import org.eclipse.hawkbit.repository.model.DistributionSet;
 import org.eclipse.hawkbit.repository.model.Target;
 import org.eclipse.hawkbit.repository.model.TargetWithActionType;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.util.CollectionUtils;
@@ -42,6 +45,8 @@ import org.springframework.util.CollectionUtils;
  */
 public abstract class AbstractDsAssignmentStrategy {
 
+    private static final Logger LOG = LoggerFactory.getLogger(AbstractDsAssignmentStrategy.class);
+
     protected final TargetRepository targetRepository;
     protected final AfterTransactionCommitExecutor afterCommit;
     protected final ApplicationEventPublisher eventPublisher;
@@ -215,7 +220,6 @@ public abstract class AbstractDsAssignmentStrategy {
                 .publishEvent(new CancelTargetAssignmentEvent(target, actionId, applicationContext.getId())));
     }
 
-    @SuppressWarnings("squid:S2259")
     JpaAction createTargetAction(final Map<String, TargetWithActionType> targetsWithActionMap, final JpaTarget target,
             final JpaDistributionSet set) {
 
@@ -223,17 +227,21 @@ public abstract class AbstractDsAssignmentStrategy {
         assertActionsPerTargetQuota(target, 1);
 
         // create the action
-        final JpaAction actionForTarget = new JpaAction();
+        return getTargetWithActionType(targetsWithActionMap, target.getControllerId()).map(targetWithActionType -> {
-        final TargetWithActionType targetWithActionType = targetsWithActionMap.get(target.getControllerId());
+            final JpaAction actionForTarget = new JpaAction();
-        actionForTarget.setActionType(targetWithActionType.getActionType());
+            actionForTarget.setActionType(targetWithActionType.getActionType());
-        actionForTarget.setForcedTime(targetWithActionType.getForceTime());
+            actionForTarget.setForcedTime(targetWithActionType.getForceTime());
-        actionForTarget.setActive(true);
+            actionForTarget.setActive(true);
-        actionForTarget.setTarget(target);
+            actionForTarget.setTarget(target);
-        actionForTarget.setDistributionSet(set);
+            actionForTarget.setDistributionSet(set);
-        actionForTarget.setMaintenanceWindowSchedule(targetWithActionType.getMaintenanceSchedule());
+            actionForTarget.setMaintenanceWindowSchedule(targetWithActionType.getMaintenanceSchedule());
-        actionForTarget.setMaintenanceWindowDuration(targetWithActionType.getMaintenanceWindowDuration());
+            actionForTarget.setMaintenanceWindowDuration(targetWithActionType.getMaintenanceWindowDuration());
-        actionForTarget.setMaintenanceWindowTimeZone(targetWithActionType.getMaintenanceWindowTimeZone());
+            actionForTarget.setMaintenanceWindowTimeZone(targetWithActionType.getMaintenanceWindowTimeZone());
-        return actionForTarget;
+            return actionForTarget;
+        }).orElseGet(() -> {
+            LOG.warn("Cannot find targetWithActionType for target '{}'.", target.getControllerId());
+            return null;
+        });
     }
 
     JpaActionStatus createActionStatus(final JpaAction action, final String actionMessage) {
@@ -254,4 +262,14 @@ public abstract class AbstractDsAssignmentStrategy {
                 actionRepository::countByTargetId);
     }
 
+    private static Optional<TargetWithActionType> getTargetWithActionType(
+            final Map<String, TargetWithActionType> targetsWithActionMap, final String controllerId) {
+        if (targetsWithActionMap.containsKey(controllerId)) {
+            return Optional.of(targetsWithActionMap.get(controllerId));
+        } else {
+            return targetsWithActionMap.values().stream()
+                    .filter(t -> controllerId.equalsIgnoreCase(t.getControllerId())).findFirst();
+        }
+    }
+
 }

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaDeploymentManagement.java

@@ -88,9 +88,6 @@ import org.springframework.transaction.annotation.Transactional;
 import org.springframework.util.CollectionUtils;
 import org.springframework.validation.annotation.Validated;
 
-import com.fasterxml.jackson.core.JsonProcessingException;
-import com.fasterxml.jackson.databind.ObjectMapper;
-import com.fasterxml.jackson.databind.ObjectWriter;
 import com.google.common.base.Joiner;
 import com.google.common.collect.Lists;
 
@@ -288,7 +285,7 @@ public class JpaDeploymentManagement implements DeploymentManagement {
                 targetEntitiesIdsChunks);
 
         final Map<String, JpaAction> controllerIdsToActions = createActions(targetsWithActionType, targetEntities,
-                assignmentStrategy, controllerIDs, distributionSetEntity);
+                assignmentStrategy, distributionSetEntity);
         // create initial action status when action is created so we remember
         // the initial running status because we will change the status
         // of the action itself and with this action status we have a nicer
@@ -376,14 +373,12 @@ public class JpaDeploymentManagement implements DeploymentManagement {
 
     private Map<String, JpaAction> createActions(final Collection<TargetWithActionType> targetsWithActionType,
             final List<JpaTarget> targets, final AbstractDsAssignmentStrategy assignmentStrategy,
-            final List<String> controllerIDs, final JpaDistributionSet set) {
+            final JpaDistributionSet set) {
         final Map<String, TargetWithActionType> targetsWithActionMap = targetsWithActionType.stream()
                 .collect(Collectors.toMap(TargetWithActionType::getControllerId, Function.identity()));
 
-        return targets.stream()
+        return targets.stream().map(trg -> assignmentStrategy.createTargetAction(targetsWithActionMap, trg, set))
-                .map(trg -> createTargetAction(assignmentStrategy, targetsWithActionType, controllerIDs, targets,
+                .filter(Objects::nonNull).map(actionRepository::save)
-                        targetsWithActionMap, trg, set))
-                .map(actionRepository::save)
                 .collect(Collectors.toMap(action -> action.getTarget().getControllerId(), Function.identity()));
     }
 
@@ -785,54 +780,6 @@ public class JpaDeploymentManagement implements DeploymentManagement {
         return "#" + Joiner.on(",#").join(elements);
     }
 
-    @SuppressWarnings({ "squid:S1695", "squid:S1696" })
-    private JpaAction createTargetAction(final AbstractDsAssignmentStrategy assignmentStrategy,
-            final Collection<TargetWithActionType> targetsWithActionType, final List<String> controllerIDs,
-            final List<JpaTarget> targets, final Map<String, TargetWithActionType> targetsWithActionMap,
-            final JpaTarget target, final JpaDistributionSet set) {
-        try {
-            return assignmentStrategy.createTargetAction(targetsWithActionMap, target, set);
-        } catch (final NullPointerException e) {
-            dumpDistributionSetAssignment(target, set, targetsWithActionType, controllerIDs, targets,
-                    targetsWithActionMap);
-            throw e;
-        }
-    }
-
-    private void dumpDistributionSetAssignment(final JpaTarget target, final JpaDistributionSet set,
-            final Collection<TargetWithActionType> targetsWithActionType, final List<String> controllerIDs,
-            final List<JpaTarget> targets, final Map<String, TargetWithActionType> targetsWithActionMap) {
-        final ObjectWriter writer = new ObjectMapper().writer();
-        final String correlationID = "DistributionSetAssignmentDump[" + Thread.currentThread().getId() + "]";
-        dump(correlationID, "Target", Objects.toString(target));
-        dump(correlationID, "DistributionSet", Objects.toString(set));
-        if (target != null) {
-            dump(correlationID, "ControllerID", Objects.toString(target.getControllerId()));
-            if (set != null) {
-                dump(correlationID, "Actions", toString(writer,
-                        actionRepository.findByTargetAndDistributionSet(new PageRequest(0, 500), target, set)));
-            }
-        }
-        dump(correlationID, "ControllerIDs", toString(writer, controllerIDs));
-        dump(correlationID, "Targets", toString(writer, targets));
-        dump(correlationID, "TargetsWithActionType", toString(writer, targetsWithActionType));
-        dump(correlationID, "TargetsWithActionMap", toString(writer, targetsWithActionMap));
-    }
-
-    private static void dump(final String prefix, final String key, final String value) {
-        LOG.error("{} >>> {}: {}", prefix, key, value);
-    }
-
-    @SuppressWarnings("squid:S1166")
-    private static String toString(final ObjectWriter writer, final Object o) {
-        try {
-            return writer.writeValueAsString(o);
-        } catch (final JsonProcessingException e) {
-            LOG.error("Object serialization failed", e);
-            return e.getMessage();
-        }
-    }
-
     protected ActionRepository getActionRepository() {
         return actionRepository;
     }

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/OfflineDsAssignmentStrategy.java

@@ -88,8 +88,10 @@ public class OfflineDsAssignmentStrategy extends AbstractDsAssignmentStrategy {
     protected JpaAction createTargetAction(final Map<String, TargetWithActionType> targetsWithActionMap,
             final JpaTarget target, final JpaDistributionSet set) {
         final JpaAction result = super.createTargetAction(targetsWithActionMap, target, set);
-        result.setStatus(Status.FINISHED);
+        if (result != null) {
-        result.setActive(Boolean.FALSE);
+            result.setStatus(Status.FINISHED);
+            result.setActive(Boolean.FALSE);
+        }
         return result;
     }
 

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/OnlineDsAssignmentStrategy.java

@@ -93,7 +93,9 @@ public class OnlineDsAssignmentStrategy extends AbstractDsAssignmentStrategy {
     JpaAction createTargetAction(final Map<String, TargetWithActionType> targetsWithActionMap, final JpaTarget target,
             final JpaDistributionSet set) {
         final JpaAction result = super.createTargetAction(targetsWithActionMap, target, set);
-        result.setStatus(Status.RUNNING);
+        if (result != null) {
+            result.setStatus(Status.RUNNING);
+        }
         return result;
     }
 

hawkbit-rest/hawkbit-mgmt-resource/src/test/java/org/eclipse/hawkbit/mgmt/rest/resource/MgmtDistributionSetResourceTest.java

@@ -273,6 +273,29 @@ public class MgmtDistributionSetResourceTest extends AbstractManagementApiIntegr
                 .as("Five targets in repository have DS assigned").hasSize(5);
     }
 
+    @Test
+    @Description("Ensures that targets can be assigned even if the specified controller IDs are in different case (e.g. 'TARGET1' instead of 'target1'.")
+    public void assignTargetsToDistributionSetIgnoreCase() throws Exception {
+        final DistributionSet createdDs = testdataFactory.createDistributionSet();
+
+        // prepare targets
+        final String[] knownTargetIds = new String[] { "64-da-a0-02-43-8b", "Trg1", "target2", "target4" };
+        final String[] knownTargetIdDifferentCase = new String[] { "64-DA-A0-02-43-8b", "TRG1", "TarGET2", "target4" };
+        for (final String targetId : knownTargetIds) {
+            testdataFactory.createTarget(targetId);
+        }
+        final JSONArray list = new JSONArray();
+        for (final String targetId : knownTargetIdDifferentCase) {
+            list.put(new JSONObject().put("id", targetId).put("type", "forced"));
+        }
+
+        mvc.perform(post(
+                MgmtRestConstants.DISTRIBUTIONSET_V1_REQUEST_MAPPING + "/" + createdDs.getId() + "/assignedTargets")
+                        .contentType(MediaType.APPLICATION_JSON).content(list.toString()))
+                .andExpect(status().isOk());
+        // we just need to make sure that no error 500 is returned
+    }
+
     @Test
     @Description("Ensures that multi target assignment is protected by our 'max targets per manual assignment' quota.")
     public void assignMultipleTargetsToDistributionSetUntilQuotaIsExceeded() throws Exception {