fix: enforce contract negotiation request and transfer request consistency #4260
Conversation
bscholtes1A
requested review from
ndr-brt,
paullatzelsperger,
ronjaquensel and
juliapampus
as code owners
bscholtes1A
force-pushed
the
fix/4239-4240_verify_contract_nego_and_transfer_process_requests_consistency
branch
2 times, most recently
from
4a22187
to
d2c2a9b
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #4260 +/- ##
============================================
- Coverage 71.74% 62.44% -9.31%
- Complexity 0 4444 +4444
============================================
Files 919 1046 +127
Lines 18457 20989 +2532
Branches 1037 1172 +135
============================================
- Hits 13242 13106 -136
- Misses 4756 7423 +2667
- Partials 459 460 +1 ☔ View full report in Codecov by Sentry. |
bscholtes1A
force-pushed
the
fix/4239-4240_verify_contract_nego_and_transfer_process_requests_consistency
branch
from
d2c2a9b
to
3c70e81
Compare
There was a problem hiding this comment.
I think that the point 1 reported in the PR does not need any work, because the contract id "structure" is EDC dependant, adding a validation on that one will raise interoperability issues.
About the point 2, a validation in the TransferRequestValidator would do the job.
An improval that could be made is to remove the assetId from that TransferRequest as it could be obtained from the ContractAgreement instead of having the client to specify it
| @@ -371,9 +370,10 @@ private CompletableFuture<?> toFuture(ServiceResult<ContractNegotiation> result, | |||
| */ | |||
| private ContractOffer getContractOffer() { | |||
| return ContractOffer.Builder.newInstance() | |||
| .id(ContractOfferId.create("1", "test-asset-id").toString()) | |||
| .assetId(randomUUID().toString()) | |||
| .id("%s:%s:%s".formatted("MQ==", "YXNzZXRJZA==", "MQ==")) | |||
There was a problem hiding this comment.
This way we are exposing unnecessary details to the test (how the id is structured), I'd stick to the static factory method used before
| @@ -192,6 +192,11 @@ private boolean processInitial(TransferProcess process) { | |||
| return true; | |||
| } | |||
|
|
|||
| if (!process.getAssetId().equals(policy.getTarget())) { | |||
There was a problem hiding this comment.
maybe this validation would fit better in the TransferRequestValidator so the client on consumer side will receive an immediate feedback.
For the provider side this validation is not needed at all, because the assetId is extracted by the stored ContractAgreement
| if (!offerId.assetIdPart().equals(contractOffer.assetId)) { | ||
| throw new IllegalArgumentException("Asset id %s must be equal to asset id part of offer id %s".formatted(contractOffer.assetId, offerId.assetIdPart())); | ||
| } |
There was a problem hiding this comment.
this check will cause issues in the communication with connectors that are not EDC (the contract offer id structure is not standardized, it is EDC specific)
| var offerId = ContractOfferId.parseId(contractOffer.id) | ||
| .orElseThrow(failure -> new IllegalArgumentException(failure.getFailureDetail())); |
There was a problem hiding this comment.
same here, this validation would break for every contract offer created by a connector that's not an EDC.
| if (!contractOffer.assetId.equals(contractOffer.policy.getTarget())) { | ||
| throw new IllegalArgumentException("Policy target %s must be equal to asset id %s".formatted(contractOffer.policy.getTarget(), contractOffer.assetId)); | ||
| } |
There was a problem hiding this comment.
generally speaking these builder validations are somewhat dangerous e.g. if in the database we have entities that won't pass this validation there will be un-manageable extensions thrown, plus they make testing really annoying (as demonstrated by the changelog of this PR)
We have 4 ContractOffer new instance creations in the codebase, and 3 of them already take the policy asset id to set the assetId, it's just a matter to add a validation before the remaining call (that, looking at the code, it's not really needed).
What this PR changes/adds
This PR fixes two issues of the current implementation:
targetfield of thePolicy.EndpointDataReferenceEntrybeing generated.Why it does that
Fix.
Linked Issue(s)
Closes #4239
Closes #4240
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.