Skip to content
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: transform policy to contract offer #3626

Merged
merged 2 commits into from Nov 20, 2023

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Nov 17, 2023

What this PR changes/adds

Map the new policy field to ContractOffer

Why it does that

avoid null pointer exceptions, because ContractOffer cannot be missing in a ContractRequest object.

Further notes

  • make all the test use the new fields defined (policy, counterPartyAddress) instead of the deprecated ones
  • policy transformer now sets the target as an @id, as requested by the ODRL information model

Linked Issue(s)

Closes #3624

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@ndr-brt ndr-brt added bug Something isn't working api Feature related to the (REST) api labels Nov 17, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (fa978df) 71.78% compared to head (7dd0f71) 71.87%.
Report is 6 commits behind head on main.

Files Patch % Lines
...nsform/JsonObjectToContractRequestTransformer.java 90.00% 1 Missing ⚠️
...ontract/spi/types/negotiation/ContractRequest.java 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3626      +/-   ##
==========================================
+ Coverage   71.78%   71.87%   +0.08%     
==========================================
  Files         909      914       +5     
  Lines       18134    18283     +149     
  Branches     1033     1037       +4     
==========================================
+ Hits        13018    13140     +122     
- Misses       4672     4691      +19     
- Partials      444      452       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some suggestions inline

if (callbackAddress != null) {
var addresses = new ArrayList<CallbackAddress>();
transformArrayOrObject(callbackAddress, CallbackAddress.class, addresses::add, context);
contractRequestBuilder.callbackAddresses(addresses);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be more elegant to add a callbackAddress(String) method to ContractRequest.Builder, like we do in several other builders. that would avoid having to create a new addresses list here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was out of my jurisdiction here, anyway I used a cleaner approach. That transformArrayOrObject method in my opinion does not make any sense... jsonld objects are always expanded as arrays, the decision on deserialization is to go with a single object (the first) or with the full array. This could be something do be addressed in another issue

contractRequestBuilder.policy(policy);
if (policyJson != null && !policyJson.asJsonArray().isEmpty()) {
var policy = transformObject(policyJson, Policy.class, context);
var offerId = transformString(policyJson.asJsonArray().getJsonObject(0).get(ID), context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use transformArrayOrObject?

Copy link
Member Author

@ndr-brt ndr-brt Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately our Policy object does not have an id field, because in fact the ContractOffer could be seen as a Policy extension, I think extracting a dedicate json-to-contractoffer transformer will make everything cleared

@ndr-brt ndr-brt force-pushed the 3624-fix-contract-request branch 6 times, most recently from 32e236f to f747a1e Compare November 20, 2023 11:20
@ndr-brt ndr-brt merged commit 6012c7a into eclipse-edc:main Nov 20, 2023
17 checks passed
@ndr-brt ndr-brt deleted the 3624-fix-contract-request branch November 20, 2023 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Feature related to the (REST) api bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using new policy attribute on ContractRequest causes null pointer exception
4 participants