From f6c39a358903d68c59ab9945d42301996a6c5515 Mon Sep 17 00:00:00 2001 From: Robert Sing <50371841+singrob@users.noreply.github.com> Date: Fri, 29 Oct 2021 14:15:16 +0200 Subject: [PATCH] fixed compatibility check bug (#1198) * fixed compatibility check bug & improved testing Signed-off-by: Robert Sing * fixed review finding Signed-off-by: Robert Sing * reduced amount of targets in test Signed-off-by: Robert Sing --- .../specifications/TargetSpecifications.java | 16 ++++--- .../jpa/autoassign/AutoAssignCheckerTest.java | 47 +++++++++++++++---- 2 files changed, 47 insertions(+), 16 deletions(-) diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/specifications/TargetSpecifications.java b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/specifications/TargetSpecifications.java index de8c79e7b..e4cdc5459 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/specifications/TargetSpecifications.java +++ b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/specifications/TargetSpecifications.java @@ -22,6 +22,7 @@ import javax.persistence.criteria.Path; import javax.persistence.criteria.Predicate; import javax.persistence.criteria.Root; import javax.persistence.criteria.SetJoin; +import javax.persistence.criteria.Subquery; import javax.validation.constraints.NotNull; import org.eclipse.hawkbit.repository.jpa.model.JpaAction; @@ -418,11 +419,14 @@ public final class TargetSpecifications { // isNotNull predicate first final Predicate targetTypeNotNull = targetRoot.get(JpaTarget_.targetType).isNotNull(); - // We need to check for isNull(...) and notEqual(...) since we allow - // target types that don't have any compatible distribution set type - return cb.and(targetTypeNotNull, - cb.or(cb.isNull(getDsTypeIdPath(targetRoot)), - cb.notEqual(getDsTypeIdPath(targetRoot), distributionSetTypeId))); + final Subquery compatibilitySubQuery = query.subquery(Long.class); + final Root subQueryTargetRoot = compatibilitySubQuery.from(JpaTarget.class); + + compatibilitySubQuery.select(subQueryTargetRoot.get(JpaTarget_.id)) + .where(cb.and(cb.equal(targetRoot.get(JpaTarget_.id), subQueryTargetRoot.get(JpaTarget_.id)), + cb.equal(getDsTypeIdPath(subQueryTargetRoot), distributionSetTypeId))); + + return cb.and(targetTypeNotNull, cb.not(cb.exists(compatibilitySubQuery))); }; } @@ -586,7 +590,7 @@ public final class TargetSpecifications { * distribution set to consider * @return specification that applies order by ds, may be overwritten */ - public static Specification orderedByLinkedDistributionSet(long distributionSetIdForOrder) { + public static Specification orderedByLinkedDistributionSet(final long distributionSetIdForOrder) { return (targetRoot, query, cb) -> { // Enhance query with custom select based sort final Expression selectCase = cb.selectCase() diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/autoassign/AutoAssignCheckerTest.java b/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/autoassign/AutoAssignCheckerTest.java index 1afee2112..44c2cb766 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/autoassign/AutoAssignCheckerTest.java +++ b/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/autoassign/AutoAssignCheckerTest.java @@ -11,10 +11,13 @@ package org.eclipse.hawkbit.repository.jpa.autoassign; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Set; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.eclipse.hawkbit.repository.exception.IncompleteDistributionSetException; import org.eclipse.hawkbit.repository.jpa.AbstractJpaIntegrationTest; @@ -24,6 +27,7 @@ import org.eclipse.hawkbit.repository.model.Action.ActionType; import org.eclipse.hawkbit.repository.model.Action.Status; import org.eclipse.hawkbit.repository.model.DistributionSet; import org.eclipse.hawkbit.repository.model.DistributionSetAssignmentResult; +import org.eclipse.hawkbit.repository.model.DistributionSetType; import org.eclipse.hawkbit.repository.model.Target; import org.eclipse.hawkbit.repository.model.TargetFilterQuery; import org.eclipse.hawkbit.repository.model.TargetType; @@ -319,28 +323,51 @@ class AutoAssignCheckerTest extends AbstractJpaIntegrationTest { @Test @Description("Verifies an auto assignment only creates actions for compatible targets") - void checkAutoAssignmentWithIncompatibleTargets() throws Exception { + void checkAutoAssignmentWithIncompatibleTargets() { + final int TARGET_COUNT = 5; + final DistributionSet testDs = testdataFactory.createDistributionSet(); + final DistributionSetType incompatibleDsType1 = testdataFactory + .findOrCreateDistributionSetType("incompatibleDsType1", "incompDsType1"); + final DistributionSetType incompatibleDsType2 = testdataFactory + .findOrCreateDistributionSetType("incompatibleDsType2", "incompDsType2"); final TargetFilterQuery testFilter = targetFilterQueryManagement.create(entityFactory.targetFilterQuery() .create().name("test-filter").query("name==*").autoAssignDistributionSet(testDs)); - final TargetType incompatibleType = testdataFactory.createTargetType("incompatibleType", + final TargetType incompatibleEmptyType = testdataFactory.createTargetType("incompatibleEmptyType", Collections.emptyList()); - final TargetType compatibleType = testdataFactory.createTargetType("compatibleType", + final TargetType incompatibleSingleType = testdataFactory.createTargetType("incompatibleSingleType", + Collections.singletonList(incompatibleDsType1)); + final TargetType incompatibleMultiType = testdataFactory.createTargetType("incompatibleMultiType", + Arrays.asList(incompatibleDsType1, incompatibleDsType2)); + final TargetType compatibleSingleType = testdataFactory.createTargetType("compatibleSingleType", Collections.singletonList(testDs.getType())); - testdataFactory.createTargetsWithType(10, "incompatible", incompatibleType); - final Target compatibleTarget = testdataFactory.createTargetsWithType(1, "compatible", compatibleType).get(0); - final Target targetWithoutType = testdataFactory.createTarget(); + final TargetType compatibleMultiType = testdataFactory.createTargetType("compatibleMultiType", + Arrays.asList(testDs.getType(), incompatibleDsType1)); + testdataFactory.createTargetsWithType(TARGET_COUNT, "incompatibleEmpty", incompatibleEmptyType); + testdataFactory.createTargetsWithType(TARGET_COUNT, "incompatibleSingle", incompatibleSingleType); + testdataFactory.createTargetsWithType(TARGET_COUNT, "incompatibleMulti", incompatibleMultiType); + + final List compatibleTargetsSingleType = testdataFactory.createTargetsWithType(TARGET_COUNT, + "compatibleSingle", compatibleSingleType); + final List compatibleTargetsMultiType = testdataFactory.createTargetsWithType(TARGET_COUNT, + "compatibleMulti", compatibleMultiType); + final List compatibleTargetsWithoutType = testdataFactory.createTargets(TARGET_COUNT, + "compatibleSingleWithoutType"); + + final List compatibleTargets = Stream + .of(compatibleTargetsSingleType, compatibleTargetsMultiType, compatibleTargetsWithoutType) + .flatMap(Collection::stream).map(Target::getId).collect(Collectors.toList()); final long compatibleCount = targetManagement.countByRsqlAndNonDSAndCompatible(testDs.getId(), testFilter.getQuery()); - assertThat(compatibleCount).isEqualTo(2); + assertThat(compatibleCount).isEqualTo(compatibleTargets.size()); autoAssignChecker.check(); - final List actions = deploymentManagement.findActionsAll(PAGE).getContent(); - assertThat(actions).hasSize(2); + final List actions = deploymentManagement.findActionsAll(Pageable.unpaged()).getContent(); + assertThat(actions).hasSize(compatibleTargets.size()); final List actionTargets = actions.stream().map(a -> a.getTarget().getId()).collect(Collectors.toList()); - assertThat(actionTargets).containsExactlyInAnyOrder(compatibleTarget.getId(), targetWithoutType.getId()); + assertThat(actionTargets).containsExactlyInAnyOrderElementsOf(compatibleTargets); } }