Small Rollout create code improvements (#1754)

Signed-off-by: Marinov Avgustin <Avgustin.Marinov@bosch.com>

hawkbit-repository/hawkbit-repository-core/src/main/java/org/eclipse/hawkbit/repository/RolloutHelper.java

@@ -34,8 +34,7 @@ public final class RolloutHelper {
     /**
      * Verifies that the required success condition and action are actually set.
      * 
-     * @param conditions
+     * @param conditions input conditions and actions
-     *            input conditions and actions
      */
     public static void verifyRolloutGroupConditions(final RolloutGroupConditions conditions) {
         if (conditions.getSuccessCondition() == null) {
@@ -50,8 +49,7 @@ public final class RolloutHelper {
      * Verifies that the group has the required success condition and action and a
      * valid target percentage.
      * 
-     * @param group
+     * @param group the input group
-     *            the input group
      * @return the verified group
      */
     public static RolloutGroup verifyRolloutGroupHasConditions(final RolloutGroup group) {
@@ -71,26 +69,20 @@ public final class RolloutHelper {
     /**
      * Verify if the supplied amount of groups is in range
      * 
-     * @param amountGroup
+     * @param amountGroup amount of groups
-     *            amount of groups
+     * @param quotaManagement to retrieve maximum number of groups allowed
-     * @param quotaManagement
-     *            to retrieve maximum number of groups allowed
      */
-    public static void verifyRolloutGroupParameter(final int amountGroup, final QuotaManagement quotaManagement) {
+    public static void verifyRolloutGroupAmount(final int amountGroup, final QuotaManagement quotaManagement) {
-        if (amountGroup <= 0) {
+        if (amountGroup > quotaManagement.getMaxRolloutGroupsPerRollout()) {
-            throw new ValidationException("The amount of groups cannot be lower than or equal to zero for static rollouts");
-        } else if (amountGroup > quotaManagement.getMaxRolloutGroupsPerRollout()) {
             throw new AssignmentQuotaExceededException(
                     "The amount of groups cannot be greater than " + quotaManagement.getMaxRolloutGroupsPerRollout());
-
         }
     }
 
     /**
      * Verify that the supplied percentage is in range
      * 
-     * @param percentage
+     * @param percentage the percentage
-     *            the percentage
      */
     public static void verifyRolloutGroupTargetPercentage(final float percentage) {
         if (percentage <= 0) {
@@ -104,8 +96,7 @@ public final class RolloutHelper {
      * Modifies the target filter query to only match targets that were created
      * after the Rollout.
      * 
-     * @param rollout
+     * @param rollout Rollout to derive the filter from
-     *            Rollout to derive the filter from
      * @return resulting target filter query
      */
     public static String getTargetFilterQuery(final Rollout rollout) {
@@ -113,12 +104,9 @@ public final class RolloutHelper {
     }
 
     /**
-     * @param targetFilter
+     * @param targetFilter the target filter tp be extended
-     *            the target filter tp be extended
+     * @param createdAt timestamp
-     * @param createdAt
+     * @return a target filter query that only matches targets that were created after the provided timestamp.
-     *            timestamp
-     * @return a target filter query that only matches targets that were created
-     *         after the provided timestamp.
      */
     public static String getTargetFilterQuery(final String targetFilter, final Long createdAt) {
         if (createdAt != null) {
@@ -130,10 +118,8 @@ public final class RolloutHelper {
     /**
      * Verifies that the Rollout is in the required status.
      * 
-     * @param rollout
+     * @param rollout the Rollout
-     *            the Rollout
+     * @param status the Status
-     * @param status
-     *            the Status
      */
     public static void verifyRolloutInStatus(final Rollout rollout, final Rollout.RolloutStatus status) {
         if (rollout.getStatus() != status) {
@@ -145,10 +131,8 @@ public final class RolloutHelper {
      * Filters the groups of a Rollout to match a specific status and adds a group
      * to the result.
      * 
-     * @param status
+     * @param status the required status for the groups
-     *            the required status for the groups
+     * @param group the group to add
-     * @param group
-     *            the group to add
      * @return list of groups
      */
     public static List<Long> getGroupsByStatusIncludingGroup(final List<RolloutGroup> groups,
@@ -161,10 +145,8 @@ public final class RolloutHelper {
      * Creates an RSQL expression that matches all targets in the provided groups.
      * Links all target filter queries with OR.
      *
-     * @param groups
+     * @param groups the rollout groups
-     *            the rollout groups
+     * @return RSQL string without base filter of the Rollout. Can be an empty string.
-     * @return RSQL string without base filter of the Rollout. Can be an empty
-     *         string.
      */
     public static String getAllGroupsTargetFilter(final List<RolloutGroup> groups) {
         if (groups.stream().anyMatch(group -> StringUtils.isEmpty(group.getTargetFilterQuery()))) {
@@ -179,14 +161,10 @@ public final class RolloutHelper {
      * Creates an RSQL Filter that matches all targets that are in the provided
      * group and in the provided groups.
      *
-     * @param baseFilter
+     * @param baseFilter the base filter from the rollout
-     *            the base filter from the rollout
+     * @param groups the rollout groups
-     * @param groups
+     * @param group the target group
-     *            the rollout groups
+     * @return RSQL string without base filter of the Rollout. Can be an empty string.
-     * @param group
-     *            the target group
-     * @return RSQL string without base filter of the Rollout. Can be an empty
-     *         string.
      */
     public static String getOverlappingWithGroupsTargetFilter(final String baseFilter, final List<RolloutGroup> groups,
             final RolloutGroup group) {
@@ -220,10 +198,8 @@ public final class RolloutHelper {
     }
 
     /**
-     * @param baseFilter
+     * @param baseFilter the base filter from the rollout
-     *            the base filter from the rollout
+     * @param group group for which the filter string should be created
-     * @param group
-     *            group for which the filter string should be created
      * @return the final target filter query for a rollout group
      */
     public static String getGroupTargetFilter(final String baseFilter, final RolloutGroup group) {
@@ -236,8 +212,7 @@ public final class RolloutHelper {
     /**
      * Verifies that no targets are left
      * 
-     * @param targetCount
+     * @param targetCount the count of left targets
-     *            the count of left targets
      */
     public static void verifyRemainingTargets(final long targetCount) {
         if (targetCount > 0) {

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

@@ -742,7 +742,7 @@ public class JpaRolloutExecutor implements RolloutExecutor {
 
     private void createDynamicGroup(final JpaRollout rollout, final RolloutGroup lastGroup, final int groupCount, final RolloutGroupStatus status) {
         try {
-            RolloutHelper.verifyRolloutGroupParameter(groupCount + 1, quotaManagement);
+            RolloutHelper.verifyRolloutGroupAmount(groupCount + 1, quotaManagement);
         } catch (final AssignmentQuotaExceededException e) {
             log.warn("Quota exceeded for dynamic rollout group creation: {}. Stop it", e.getMessage());
             if (isRolloutComplete(rollout)) {

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaRolloutManagement.java

@@ -199,13 +199,15 @@ public class JpaRolloutManagement implements RolloutManagement {
             ConcurrencyFailureException.class }, maxAttempts = Constants.TX_RT_MAX, backoff = @Backoff(delay = Constants.TX_RT_DELAY))
     public Rollout create(final RolloutCreate rollout, final int amountGroup, final boolean confirmationRequired,
             final RolloutGroupConditions conditions, final DynamicRolloutGroupTemplate dynamicRolloutGroupTemplate) {
-        if (amountGroup == 0) {
+        if (amountGroup < 0) {
+            throw new ValidationException("The amount of groups cannot be lower than or equal to zero for static rollouts");
+        } else if (amountGroup == 0) {
             if (dynamicRolloutGroupTemplate == null) {
                 throw new ValidationException(
                     "When amount of groups is 0, the rollouts shall be dynamic and a dynamic group template must be provided");
             }
         } else {
-            RolloutHelper.verifyRolloutGroupParameter(amountGroup, quotaManagement);
+            RolloutHelper.verifyRolloutGroupAmount(amountGroup, quotaManagement);
         }
 
         final JpaRollout rolloutRequest = (JpaRollout) rollout.build();
@@ -223,7 +225,10 @@ public class JpaRolloutManagement implements RolloutManagement {
             ConcurrencyFailureException.class }, maxAttempts = Constants.TX_RT_MAX, backoff = @Backoff(delay = Constants.TX_RT_DELAY))
     public Rollout create(final RolloutCreate rollout, final List<RolloutGroupCreate> groups,
             final RolloutGroupConditions conditions) {
-        RolloutHelper.verifyRolloutGroupParameter(groups.size(), quotaManagement);
+        if (groups.isEmpty()) {
+            throw new ValidationException("The amount of groups cannot be 0");
+        }
+        RolloutHelper.verifyRolloutGroupAmount(groups.size(), quotaManagement);
         final JpaRollout rolloutRequest = (JpaRollout) rollout.build();
         final JpaRollout savedRollout = createRollout(rolloutRequest, false);
         return createRolloutGroups(groups, conditions, savedRollout);
@@ -329,6 +334,8 @@ public class JpaRolloutManagement implements RolloutManagement {
             publishRolloutGroupCreatedEventAfterCommit(lastSavedGroup, rollout);
         }
 
+        // lastSavedGroup is never null! amountOfGroups > 0 (and has static groups) or dynamicRolloutGroupTemplate is
+        // not null (validated) and (validated) the rollout is dynamic, so has dynamic group
         rollout.setRolloutGroupsCreated(lastSavedGroup.isDynamic() ? amountOfGroups + 1 : amountOfGroups);
         return rolloutRepository.save(rollout);
     }

hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/RolloutManagementFlowTest.java

@@ -154,7 +154,7 @@ class RolloutManagementFlowTest extends AbstractJpaIntegrationTest {
                 // filters for action of the last static group
                 .filter(action -> Integer.parseInt(action.getTarget().getControllerId().substring(targetPrefix.length())) < amountGroups * 3)
                 .forEach(this::finishAction);
-        executeWithoutOneTargetFromAGroup(rollout, dynamic1, 3);
+        executeWithoutOneTargetFromAGroup(dynamic1, rollout, 3);
         assertAndGetRunning(rollout, 1); // remains on in the first dynamic
 
         rolloutHandler.handleAll();
@@ -254,7 +254,7 @@ class RolloutManagementFlowTest extends AbstractJpaIntegrationTest {
                 // filters for action of the last static group
                 .filter(action -> Integer.parseInt(action.getTarget().getControllerId().substring(targetPrefix.length())) < amountGroups * 3)
                 .forEach(this::finishAction);
-        executeWithoutOneTargetFromAGroup(rollout, dynamic1, 6);
+        executeWithoutOneTargetFromAGroup(dynamic1, rollout, 6);
         assertAndGetRunning(rollout, 1); // remains on in the first dynamic
 
         rolloutHandler.handleAll();
@@ -339,7 +339,7 @@ class RolloutManagementFlowTest extends AbstractJpaIntegrationTest {
         assertScheduled(rollout, 0);
 
         // executes dynamic1 without 1 target
-        executeWithoutOneTargetFromAGroup(rollout, dynamic1, 6);
+        executeWithoutOneTargetFromAGroup(dynamic1, rollout, 6);
         assertAndGetRunning(rollout, 1); // remains on in the first dynamic
 
         rolloutHandler.handleAll();
@@ -402,8 +402,7 @@ class RolloutManagementFlowTest extends AbstractJpaIntegrationTest {
         }
     }
 
-    private void executeWithoutOneTargetFromAGroup(
+    private void executeWithoutOneTargetFromAGroup(final RolloutGroup group, final Rollout rollout, final int count) {
-            final Rollout rollout, final RolloutGroup group, final int count) {
         // execute groups (without on of the last)
         assertThat(refresh(group).getStatus()).isEqualTo(RolloutGroupStatus.RUNNING);
         // skip on from the last group only