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

feat: Introduce transfer type #3727

Merged
merged 4 commits into from
Dec 22, 2023

Conversation

wolf4ood
Copy link
Contributor

@wolf4ood wolf4ood commented Dec 20, 2023

What this PR changes/adds

Introduce the transfer type concept as outlined in the DR #3637.

Adds the transfer type field in:

  • TransferRequest
  • DataRequest
  • TransferRequestMessage
  • DataFlowRequest

and their transformers when necessary.

The TransferRequest#dataTestination (DataAddress) is still mandatory, even in the case where the transferType could be enough for the transfer, like in the PULLs scenarios (Backward compatibiliy).

The EDC DataFlowControllers :

  • ConsumerPullTransferDataFlowController
  • ProviderPushTransferDataFlowController

have been slightly modified to be backward compatible in case they receive the transferType field.

Why it does that

Decouple the type of transfer from the DataAddress#type of the destination in a transfer request

Linked Issue(s)

Closes #3724

@wolf4ood wolf4ood changed the title Feat/3724 transfer type Introduce transfer type Dec 20, 2023
@wolf4ood wolf4ood self-assigned this Dec 20, 2023
@wolf4ood wolf4ood added the enhancement New feature or request label Dec 20, 2023
@wolf4ood wolf4ood changed the title Introduce transfer type feat: Introduce transfer type Dec 20, 2023
@wolf4ood wolf4ood force-pushed the feat/3724_transfer_type branch 2 times, most recently from 5c32702 to fb8b409 Compare December 20, 2023 14:21
@wolf4ood wolf4ood force-pushed the feat/3724_transfer_type branch from fb8b409 to ff0d1a0 Compare December 20, 2023 14:36
@wolf4ood wolf4ood marked this pull request as ready for review December 21, 2023 09:21
@wolf4ood wolf4ood requested a review from jimmarino December 21, 2023 15:15
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

why is transferType added on DataRequest? This value object has no particular use in the current implementation so I would add transferType directly on the TransferProcess

@wolf4ood
Copy link
Contributor Author

wolf4ood commented Dec 21, 2023

@ndr-brt no particular reason, all the DSP/ protocol related informations are stored in the DataRequest that's why I put that there. If i remember correctly we wanted also the merge those two. For me it's fine to put that in TransferProcess

@ndr-brt
Copy link
Member

ndr-brt commented Dec 21, 2023

@ndr-brt no particular reason, all the DSP/ protocol related informations are stored in the DataRequest that's why I put that there. If i remember correctly we wanted also the merge those two. For me it's fine to put that in TransferProcess

yes, the merging process proceeds really slowly 😁 I think we should not add new stuff on the DataRequest but that should be done directly in the TransferProcess.
To inline the DataRequest we'll need to change some apis first, like the ResourceManifestGenerator.
I created the issue so that we won't forget: #3732

@wolf4ood
Copy link
Contributor Author

Cool make sense :)

Verified

This commit was signed with the committer’s verified signature.
gustavste Gustav
@wolf4ood wolf4ood force-pushed the feat/3724_transfer_type branch from 3bb626d to 994af8e Compare December 21, 2023 16:05
@wolf4ood wolf4ood requested a review from ndr-brt December 21, 2023 16:28
@wolf4ood wolf4ood merged commit 470cb4c into eclipse-edc:main Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introducing transfer type
4 participants