Fix BPNL Group validation operators logic (#1673)

* Fix BPNL Group Validation Operator's Logic and update doc and tests. * Update method doc. * Fix failing test. * Change EQ and NEQ to deprecated. * Update EQ and NEQ to deprecated. * Changes from PR. * Changes from PR. * Changes from PR. * Revert EQ and NEQ test removals and other updates. * Update DEPENDENCIES file. * Update DEPENDENCIES file. * Update DEPENDENCIES file. * Update DEPENDENCIES file. * Update DEPENDENCIES file.
eclipse-tractusx · Nov 26, 2024 · ef8189a · ef8189a
1 parent 5765d00
commit ef8189a
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 34 deletions.
diff --git a/edc-extensions/bpn-validation/README.md b/edc-extensions/bpn-validation/README.md
@@ -28,14 +28,16 @@ Both previously mentioned evaluation functions are bound to the following scopes
 
 This policy states, that a certain BPN must, may or must not be member of a certain group. Groups may be represented as
 scalar, or as comma-separated lists. For semantic expression, the following ODRL operators are
-supported: 
+supported:
 - `eq`
 - `neq`
 - `isPartOf`
 - `isAllOf`
 - `isAnyOf`
 - `isNoneOf` 
 
+The `eq` and `neq` operators are **deprecated** in favor of `isAllOf`, `isAnyOf` and `isNoneOf` operators.
+
 The following example demonstrates a full JSON-LD structure in expanded form, containing such a constraint.
 
 ### Example

diff --git a/...g/eclipse/tractusx/edc/validation/businesspartner/BusinessPartnerValidationExtension.java b/...g/eclipse/tractusx/edc/validation/businesspartner/BusinessPartnerValidationExtension.java
@@ -75,10 +75,10 @@ public class BusinessPartnerValidationExtension implements ServiceExtension {
 
     @Override
     public void initialize(ServiceExtensionContext context) {
-
-        bindToScope(TRANSFER_SCOPE, TransferProcessPolicyContext.class, new BusinessPartnerGroupFunction<>(store));
-        bindToScope(NEGOTIATION_SCOPE, ContractNegotiationPolicyContext.class, new BusinessPartnerGroupFunction<>(store));
-        bindToScope(CATALOG_SCOPE, CatalogPolicyContext.class, new BusinessPartnerGroupFunction<>(store));
+        var monitor = context.getMonitor().withPrefix("BusinessPartnerGroupFunction");
+        bindToScope(TRANSFER_SCOPE, TransferProcessPolicyContext.class, new BusinessPartnerGroupFunction<>(store, monitor));
+        bindToScope(NEGOTIATION_SCOPE, ContractNegotiationPolicyContext.class, new BusinessPartnerGroupFunction<>(store, monitor));
+        bindToScope(CATALOG_SCOPE, CatalogPolicyContext.class, new BusinessPartnerGroupFunction<>(store, monitor));
     }
 
     private <C extends PolicyContext> void bindToScope(String scope, Class<C> contextType, AtomicConstraintRuleFunction<Permission, C> function) {

diff --git a/...lipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunction.java b/...lipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunction.java
@@ -25,14 +25,15 @@
 import org.eclipse.edc.policy.engine.spi.PolicyContext;
 import org.eclipse.edc.policy.model.Operator;
 import org.eclipse.edc.policy.model.Permission;
+import org.eclipse.edc.spi.monitor.Monitor;
 import org.eclipse.tractusx.edc.validation.businesspartner.spi.BusinessPartnerStore;
 
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.Function;
 import java.util.stream.Collectors;
 
@@ -82,13 +83,15 @@ public class BusinessPartnerGroupFunction<C extends ParticipantAgentPolicyContex
     private static final List<Operator> ALLOWED_OPERATORS = List.of(EQ, NEQ, IN, IS_ALL_OF, IS_ANY_OF, IS_NONE_OF);
     private static final Map<Operator, Function<BpnGroupHolder, Boolean>> OPERATOR_EVALUATOR_MAP = new HashMap<>();
     private final BusinessPartnerStore store;
+    private final Monitor monitor;
 
-    public BusinessPartnerGroupFunction(BusinessPartnerStore store) {
+    public BusinessPartnerGroupFunction(BusinessPartnerStore store, Monitor monitor) {
         this.store = store;
+        this.monitor = monitor;
         OPERATOR_EVALUATOR_MAP.put(EQ, this::evaluateEquals);
         OPERATOR_EVALUATOR_MAP.put(NEQ, this::evaluateNotEquals);
-        OPERATOR_EVALUATOR_MAP.put(IN, this::evaluateIn);
-        OPERATOR_EVALUATOR_MAP.put(IS_ALL_OF, this::evaluateIn);
+        OPERATOR_EVALUATOR_MAP.put(IN, this::evaluateIsAnyOf);
+        OPERATOR_EVALUATOR_MAP.put(IS_ALL_OF, this::evaluateIsAllOf);
         OPERATOR_EVALUATOR_MAP.put(IS_ANY_OF, this::evaluateIsAnyOf);
         OPERATOR_EVALUATOR_MAP.put(IS_NONE_OF, this::evaluateIsNoneOf);
     }
@@ -115,42 +118,46 @@ public boolean evaluate(Operator operator, Object rightOperand, Permission permi
         }
 
         // right-operand is anything other than String or Collection
-        var rightOperand1 = parseRightOperand(rightOperand, context);
-        if (rightOperand1 == null) {
+        var parsedRightOperand = parseRightOperand(rightOperand, context);
+        if (parsedRightOperand == null) {
             return false;
         }
 
         //call evaluator function
-        return OPERATOR_EVALUATOR_MAP.get(operator).apply(new BpnGroupHolder(assignedGroups, rightOperand1));
+        return OPERATOR_EVALUATOR_MAP.get(operator).apply(new BpnGroupHolder(new HashSet<>(assignedGroups), parsedRightOperand));
     }
 
-    private List<String> parseRightOperand(Object rightValue, PolicyContext context) {
+    private Set<String> parseRightOperand(Object rightValue, PolicyContext context) {
         if (rightValue instanceof String value) {
             var tokens = value.split(",");
-            return Arrays.asList(tokens);
+            return Set.of(tokens);
         }
         if (rightValue instanceof Collection<?>) {
-            return ((Collection<?>) rightValue).stream().map(Object::toString).toList();
+            return ((Collection<?>) rightValue).stream().map(Object::toString).collect(Collectors.toSet());
         }
 
         context.reportProblem(format("Right operand expected to be either String or a Collection, but was %s", rightValue.getClass()));
         return null;
     }
 
-    private Boolean evaluateIn(BpnGroupHolder bpnGroupHolder) {
-        var assigned = bpnGroupHolder.assignedGroups;
-        // checks whether both lists overlap
-        return new HashSet<>(bpnGroupHolder.allowedGroups).containsAll(assigned);
-    }
-
+    @Deprecated(since = "0.9.0")
     private Boolean evaluateNotEquals(BpnGroupHolder bpnGroupHolder) {
-        return !evaluateEquals(bpnGroupHolder);
+        monitor.warning("%s is a deprecated operator, in future please use %s operator.".formatted(NEQ, IS_NONE_OF));
+        return !bpnGroupHolder.allowedGroups.equals(bpnGroupHolder.assignedGroups);
     }
 
+    @Deprecated(since = "0.9.0")
     private Boolean evaluateEquals(BpnGroupHolder bpnGroupHolder) {
+        monitor.warning("%s is a deprecated operator, in future please use %s operator.".formatted(EQ, IS_ALL_OF));
         return bpnGroupHolder.allowedGroups.equals(bpnGroupHolder.assignedGroups);
     }
 
+    private Boolean evaluateIsAllOf(BpnGroupHolder bpnGroupHolder) {
+        var assigned = bpnGroupHolder.assignedGroups;
+        var allowed = bpnGroupHolder.allowedGroups;
+        return (assigned.isEmpty() || !allowed.isEmpty()) && assigned.containsAll(allowed);
+    }
+
     private boolean evaluateIsAnyOf(BpnGroupHolder bpnGroupHolder) {
         if (bpnGroupHolder.allowedGroups.isEmpty() && bpnGroupHolder.assignedGroups.isEmpty()) {
             return true;
@@ -159,7 +166,6 @@ private boolean evaluateIsAnyOf(BpnGroupHolder bpnGroupHolder) {
         var allowedGroups = bpnGroupHolder.allowedGroups;
         return bpnGroupHolder.assignedGroups
                 .stream()
-                .distinct()
                 .anyMatch(allowedGroups::contains);
     }
 
@@ -168,9 +174,9 @@ private boolean evaluateIsNoneOf(BpnGroupHolder bpnGroupHolder) {
     }
 
     /**
-     * Internal utility class to hold the list of assigned groups for a BPN, and the list of groups specified in the policy ("allowed groups").
+     * Internal utility class to hold the collection of assigned groups for a BPN, and the collection of groups specified in the policy ("allowed groups").
      */
-    private record BpnGroupHolder(List<String> assignedGroups, List<String> allowedGroups) {
+    private record BpnGroupHolder(Set<String> assignedGroups, Set<String> allowedGroups) {
     }
 
 }
diff --git a/...e/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunctionTest.java b/...e/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunctionTest.java
@@ -23,6 +23,7 @@
 import org.eclipse.edc.participant.spi.ParticipantAgentPolicyContext;
 import org.eclipse.edc.policy.model.Operator;
 import org.eclipse.edc.policy.model.Permission;
+import org.eclipse.edc.spi.monitor.Monitor;
 import org.eclipse.edc.spi.result.StoreResult;
 import org.eclipse.tractusx.edc.validation.businesspartner.spi.BusinessPartnerStore;
 import org.junit.jupiter.api.DisplayName;
@@ -34,7 +35,6 @@
 import org.junit.jupiter.params.provider.ArgumentsSource;
 
 import java.util.List;
-import java.util.Map;
 import java.util.stream.Stream;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -61,15 +61,15 @@ class BusinessPartnerGroupFunctionTest {
     private static final String TEST_BPN = "BPN000TEST";
     private final BusinessPartnerStore store = mock();
     private final ParticipantAgent agent = mock();
+    private final Monitor monitor = mock();
     private final Permission unusedPermission = Permission.Builder.newInstance().build();
     private final ParticipantAgentPolicyContext context = new TestParticipantAgentPolicyContext(agent);
-    private final BusinessPartnerGroupFunction<ParticipantAgentPolicyContext> function = new BusinessPartnerGroupFunction<>(store);
+    private final BusinessPartnerGroupFunction<ParticipantAgentPolicyContext> function = new BusinessPartnerGroupFunction<>(store, monitor);
 
     @ParameterizedTest(name = "Invalid operator {0}")
     @ArgumentsSource(InvalidOperatorProvider.class)
     @DisplayName("Invalid operators, expect report in policy context")
     void evaluate_invalidOperator(Operator invalidOperator) {
-        var agent = new ParticipantAgent(Map.of(), Map.of());
 
         var result = function.evaluate(invalidOperator, "test-group", unusedPermission, context);
 
@@ -84,7 +84,7 @@ void evaluate_rightOperandNotStringOrCollection(Object rightValue) {
         when(store.resolveForBpn(TEST_BPN)).thenReturn(StoreResult.success(List.of("test-group")));
         when(agent.getIdentity()).thenReturn(TEST_BPN);
 
-        var result = function.evaluate(EQ, rightValue, unusedPermission, context);
+        var result = function.evaluate(IS_ANY_OF, rightValue, unusedPermission, context);
 
         assertThat(result).isFalse();
         assertThat(context.getProblems()).containsOnly("Right operand expected to be either String or a Collection, but was " + rightValue.getClass());
@@ -109,7 +109,7 @@ void evaluate_failedResolveForBpn_shouldBeFalse() {
         when(agent.getIdentity()).thenReturn(TEST_BPN);
         when(store.resolveForBpn(TEST_BPN)).thenReturn(StoreResult.notFound("foobar"));
 
-        var result = function.evaluate(EQ, allowedGroups, unusedPermission, context);
+        var result = function.evaluate(IS_ANY_OF, allowedGroups, unusedPermission, context);
 
         assertThat(result).isFalse();
         assertThat(context.getProblems()).containsOnly("foobar");
@@ -156,14 +156,14 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext extensionCo
                     Arguments.of("Empty groups", NEQ, List.of(), true),
 
                     Arguments.of("Matching groups", IN, List.of(TEST_GROUP_1, TEST_GROUP_2), true),
-                    Arguments.of("Overlapping groups", IN, List.of(TEST_GROUP_1, "different-group"), false),
+                    Arguments.of("Overlapping groups", IN, List.of(TEST_GROUP_1, "different-group"), true),
                     Arguments.of("Disjoint groups", IN, List.of("different-group", "another-different-group"), false),
 
                     Arguments.of("Disjoint groups", IS_ALL_OF, List.of("different-group", "another-different-group"), false),
                     Arguments.of("Matching groups", IS_ALL_OF, List.of(TEST_GROUP_1, TEST_GROUP_2), true),
-                    Arguments.of("Overlapping groups", IS_ALL_OF, List.of(TEST_GROUP_1, TEST_GROUP_2, "different-group", "another-different-group"), false),
+                    Arguments.of("Overlapping groups", IS_ALL_OF, List.of(TEST_GROUP_1, TEST_GROUP_2, "different-group", "another-different-group"), true),
                     Arguments.of("Overlapping groups (1 overlap)", IS_ALL_OF, List.of(TEST_GROUP_1, "different-group"), false),
-                    Arguments.of("Overlapping groups (1 overlap)", IS_ALL_OF, List.of(TEST_GROUP_1), true),
+                    Arguments.of("Overlapping groups (1 overlap)", IS_ALL_OF, List.of(TEST_GROUP_1), false),
 
                     Arguments.of("Disjoint groups", IS_ANY_OF, List.of("different-group", "another-different-group"), false),
                     Arguments.of("Matching groups", IS_ANY_OF, List.of(TEST_GROUP_1, TEST_GROUP_2), true),

diff --git a/...rc/test/java/org/eclipse/tractusx/edc/tests/agreement/retirement/RetireAgreementTest.java b/...rc/test/java/org/eclipse/tractusx/edc/tests/agreement/retirement/RetireAgreementTest.java
@@ -91,7 +91,7 @@ void retireAgreement_shouldCloseTransferProcesses() {
             PROVIDER.createAsset(assetId, Map.of(), dataAddress);
 
             PROVIDER.storeBusinessPartner(CONSUMER.getBpn(), "test-group1");
-            var accessPolicy = PROVIDER.createPolicyDefinition(PolicyHelperFunctions.bpnGroupPolicy(Operator.EQ, "test-group1"));
+            var accessPolicy = PROVIDER.createPolicyDefinition(PolicyHelperFunctions.bpnGroupPolicy(Operator.IS_ALL_OF, "test-group1"));
             var policy = PolicyHelperFunctions.frameworkPolicy(Map.of());
             var contractPolicy = PROVIDER.createPolicyDefinition(policy);
             PROVIDER.createContractDefinition(assetId, "def-1", accessPolicy, contractPolicy);