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

DPF: add TransferRequestDto id #1188

Merged

Conversation

algattik
Copy link
Contributor

@algattik algattik commented Apr 21, 2022

What this PR changes/adds

Add id to TransferRequestDto and support it in converter to TransferRequest

Why it does that

If different data transfers are created (POST /api/v1/data/transferprocess) for the same asset, the same ID is returned back. The reason is that the DataTransfer.id field is not populated (as it is not present in DataTransferDto), and the idempotency mechanism in TransferProcessManagerImpl#initiateRequest is based on it.

Further notes

Also deleted a (unused) duplicate TransferRequestDto class

Linked Issue(s)

Closes #1182

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • added relevant details to the changelog? (skip with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@algattik algattik marked this pull request as ready for review April 21, 2022 08:59
@algattik
Copy link
Contributor Author

algattik commented Apr 21, 2022

We could also allow the client optionally not to generate an ID, and have the server generate one in that case. Personally I'd rather not add code paths that are not necessary, as this creates more testing and validation work - the client can just generate a random UUID. Agree @ndr-brt @bscholtes1A @paullatzelsperger ?

As discussed, I am fine with the proposed approach. Even if I think that putting it as optional would be even better, i.e. if no id provided in the Dto, then a random id is generated internally.

@florianrusch-zf
Copy link
Contributor

florianrusch-zf commented Apr 21, 2022

@ndr-brt this PR reflects what we were talking about this morning, doesn't it? nevermind, saw the other issue

@ndr-brt
Copy link
Member

ndr-brt commented Apr 21, 2022

@algattik I agree on generating a random UUID in the case the DataRequestDto.id is null

@bscholtes1A
Copy link
Contributor

bscholtes1A commented Apr 21, 2022

@algattik I agree on generating a random UUID in the case the DataRequestDto.id is null

As discussed, I am also in favor of generating a random id if none is provided in the request, i.e. make id optional.

@paullatzelsperger WDYT?

@algattik algattik force-pushed the bug/1182-transfer-process-dto-id branch from 8a2dcf0 to 2bcadf4 Compare April 25, 2022 04:13
@codecov-commenter
Copy link

Codecov Report

Merging #1188 (2bcadf4) into main (d056af2) will decrease coverage by 4.18%.
The diff coverage is 86.48%.

@@             Coverage Diff              @@
##               main    #1188      +/-   ##
============================================
- Coverage     62.65%   58.46%   -4.19%     
+ Complexity     2981     2728     -253     
============================================
  Files           694      698       +4     
  Lines         15428    15469      +41     
  Branches       1048     1047       -1     
============================================
- Hits           9666     9044     -622     
- Misses         5331     6002     +671     
+ Partials        431      423       -8     
Impacted Files Coverage Δ
...tractagreement/ContractAgreementApiController.java 100.00% <ø> (ø)
...tnegotiation/ContractNegotiationApiController.java 90.32% <ø> (ø)
...ionInitiateRequestDtoToDataRequestTransformer.java 90.47% <60.00%> (-9.53%) ⬇️
...r/api/datamanagement/asset/AssetApiController.java 84.44% <66.66%> (ø)
...actdefinition/ContractDefinitionApiController.java 90.90% <66.66%> (ø)
.../transferprocess/TransferProcessApiController.java 90.00% <94.11%> (ø)
...aceconnector/common/statemachine/StateMachine.java 94.11% <100.00%> (-0.12%) ⬇️
...api/datamanagement/policy/PolicyApiController.java 80.64% <100.00%> (-0.61%) ⬇️
...ment/transferprocess/model/TransferRequestDto.java 100.00% <100.00%> (ø)
...rm/TransferRequestDtoToDataRequestTransformer.java 90.47% <100.00%> (+1.00%) ⬆️
... and 72 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 994cdd7...2bcadf4. Read the comment docs.

@algattik
Copy link
Contributor Author

@ndr-brt @bscholtes1A thanks for the feedback, I've implemented "generating a random id if none is provided in the request" per this discussion.

@algattik algattik force-pushed the bug/1182-transfer-process-dto-id branch from 2bcadf4 to 326d859 Compare April 25, 2022 04:38
@bscholtes1A bscholtes1A merged commit 60da937 into eclipse-edc:main Apr 25, 2022
@algattik algattik deleted the bug/1182-transfer-process-dto-id branch June 22, 2022 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data management API: Transfer process: can't create more than one transfer
5 participants