-
Notifications
You must be signed in to change notification settings - Fork 233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: enforce contract negotiation request and transfer request consistency #4260
Conversation
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. |
d2c2a9b
to
3c70e81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
target
field of thePolicy
.EndpointDataReferenceEntry
being 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.