IRS Policy Validation accepts subset of AND constraint

ds-jhartmann commented 2 months ago

Description

Current behaviour

When IRS accepted policy has a AND constraint with 2 constraints and the EDC offer policy has a AND constraint with 1 constraint, IRS accepts the Policy if one of the IRS accepted policies matches the EDC offer policy AND constraint.

Expected behavior

[ ] IRS accepted Policies have to match exactly the EDC offer policy.

Steps to reproduce the Bug

IRS accepted Policy with AND constraint A1 and A2
EDC offer with AND constraint A1
IRS job with contract negotiation.
-> actual: Negotiation is started with IRS accepting the policy
-> expected: Negotiation is aborted with IRS not accepting the policy because constraint A2 is missing in EDC offer

ds-jhartmann commented 1 week ago

Planning 2

Check for policies is done here https://github.com/eclipse-tractusx/item-relationship-service/blob/72b03963cce3895d75e6b651b0b405195dd716d3/irs-edc-client/src/main/java/org/eclipse/tractusx/irs/edc/client/policy/ConstraintCheckerService.java#L46-L58

Make sure that and constraints match for all constraints

This Test should validate that the check is working as expected: https://github.com/eclipse-tractusx/item-relationship-service/blob/a45960ba21905403ef6d55883709ad0e817221ac/irs-edc-client/src/test/java/org/eclipse/tractusx/irs/edc/client/policy/ConstraintCheckerServiceTest.java

    @Test
    void shouldNotAcceptAndConstraintWithOneLessElement1() {
        final AndConstraint andConstraint = createAndConstraint(
                List.of(createAtomicConstraint(TestConstants.FRAMEWORK_AGREEMENT_TRACEABILITY,
                                TestConstants.STATUS_ACTIVE),
                        createAtomicConstraint(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

        final Policy acceptedPolicy = createPolicyWithAndConstraint(
                List.of(new Operand(TestConstants.FRAMEWORK_AGREEMENT_TRACEABILITY, TestConstants.STATUS_ACTIVE),
                        new Operand(TestConstants.MEMBERSHIP, TestConstants.STATUS_ACTIVE),
                        new Operand(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

        boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(andConstraint));

        assertThat(result).isFalse();
    }

dsmf commented 6 days ago

@ds-jhartmann: shouldn't it be the other way around: andConstraint one constraint more than acceptedPolicy?

        final AndConstraint andConstraint = createAndConstraint(
                List.of(createAtomicConstraint(TestConstants.FRAMEWORK_AGREEMENT_TRACEABILITY,
                                TestConstants.STATUS_ACTIVE),
                        createAtomicConstraint(TestConstants.MEMBERSHIP, TestConstants.STATUS_ACTIVE),
                        createAtomicConstraint(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

        final Policy acceptedPolicy = createPolicyWithAndConstraint(
                List.of(new Operand(TestConstants.FRAMEWORK_AGREEMENT_TRACEABILITY, TestConstants.STATUS_ACTIVE),
                        new Operand(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

        boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(andConstraint));

        assertThat(result).isFalse();

ds-jhartmann commented 6 days ago

@ds-jhartmann: shouldn't it be the other way around: andConstraint one constraint more than acceptedPolicy?

Oh, you're right. The bug describes your behavior. However, the Test example I provided should also fail, since the Policy does not match exactly

dsmf commented 5 days ago

Outcome

AND does not allow subsets in any direction anymore (list of constraints must be same size and all must match) -> see corrections in ConstraintCheckerService.isSameAs and ConstraintCheckerServiceTest.SubsetAndTests
fixed tests in PolicyCheckerServiceTest and removed one obsolete test concerning XOne (which we do not support)
improved readability of tests

PR

https://github.com/eclipse-tractusx/item-relationship-service/pull/797

ds-kgassner commented 2 days ago

Successfully tested: Starting Point: EDC has 2 constraints

IRS accepted policy has 3 constraints: syntax and value match EDC policy IRS accepted policy has 2 constraints: syntax and value match EDC policy IRS accepted policy has 2 constraints: syntax does not match EDC policy IRS accepted policy has 2 constraints: value does not match EDC policy

eclipse-tractusx / item-relationship-service