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

chore: handle missing destination in transfer protocol service #3316

Conversation

ronjaquensel
Copy link
Contributor

What this PR changes/adds

Adds a null check on the data destination to TransferProcessProtocolServiceImpl::notifyRequested before validating the destination. In requestedAction creates a destination DataAddress of type HTTP_PROXY (= pull) for the DataRequest, if no destination was specified.

Why it does that

The destination is optional according to the protocol spec, as it is only required for push transfers.

Linked Issue(s)

Closes #2728

@ronjaquensel ronjaquensel added enhancement New feature or request dataspace-protocol related to the dataspace protocol labels Jul 20, 2023
@ronjaquensel ronjaquensel force-pushed the chore/2728-handle-missing-destination branch from b5bf5b2 to 8883e21 Compare July 20, 2023 08:15
@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: -53.43 ⚠️

Comparison is base (b6c5f4e) 71.91% compared to head (b5bf5b2) 18.48%.

❗ Current head b5bf5b2 differs from pull request most recent head 8883e21. Consider uploading reports for the commit 8883e21 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3316       +/-   ##
===========================================
- Coverage   71.91%   18.48%   -53.43%     
===========================================
  Files         810      810               
  Lines       16509    16513        +4     
  Branches      962      964        +2     
===========================================
- Hits        11872     3052     -8820     
- Misses       4236    13372     +9136     
+ Partials      401       89      -312     
Impacted Files Coverage Δ
...ferprocess/TransferProcessProtocolServiceImpl.java 0.00% <0.00%> (-98.86%) ⬇️

... and 520 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

I'd tend to avoid comments unless they are strictly necessary because they tend to rot quickly, in my opinion the unit test already describes the need for this condition.
all good otherwise.

@ronjaquensel ronjaquensel merged commit e96e047 into eclipse-edc:main Jul 20, 2023
15 checks passed
@ronjaquensel ronjaquensel deleted the chore/2728-handle-missing-destination branch July 20, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dataspace-protocol related to the dataspace protocol enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dataspace protocol: transfer process should handle missing data destinations
4 participants