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: dataspace protocol negotiation dispatcher #2780

Conversation

juliapampus
Copy link
Contributor

@juliapampus juliapampus commented Apr 20, 2023

What this PR changes/adds

Adds delegates for dsp negotiation messages.

Why it does that

Implement dataspace protocol

Further notes

Adds dependencies:

Linked Issue(s)

Relates #2474

Checklist

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

@juliapampus juliapampus added enhancement New feature or request dataspace-protocol related to the dataspace protocol labels Apr 20, 2023
@juliapampus juliapampus temporarily deployed to Azure-dev April 20, 2023 14:25 — with GitHub Actions Inactive
@juliapampus juliapampus marked this pull request as ready for review April 20, 2023 14:49
@juliapampus juliapampus force-pushed the feat/dsp_contract_negotiation_dispatcher branch from 36e2fb1 to deafc6f Compare April 20, 2023 15:33
@juliapampus juliapampus temporarily deployed to Azure-dev April 20, 2023 15:34 — with GitHub Actions Inactive
Copy link
Contributor

@jimmarino jimmarino left a comment

Choose a reason for hiding this comment

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

Couldn't some of the boilerplate code around creating a JSON structure, HTTP request and dispatching it be pulled up into an abstract class?

@juliapampus
Copy link
Contributor Author

juliapampus commented Apr 21, 2023

Couldn't some of the boilerplate code around creating a JSON structure, HTTP request and dispatching it be pulled up into an abstract class?

Agree. When you are okay with it, I would open an issue for this refactoring. I guess we could also merge this with the transformer delegates and add the abstract class to the dsp-http-core module, as soon as both PRs are merged.

@jimmarino
Copy link
Contributor

I think we should refactor this PR to include the abstract class because it will simplify the code, which contains a lot of duplication. Later we can move the abstract class out in a subsequent PR.

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.

Couldn't some of the boilerplate code around creating a JSON structure, HTTP request and dispatching it be pulled up into an abstract class?

I would prefer a collaborator, its behavior is pretty clear, serialize a RemoteMessage, it could be an interface called RemoteMessageSerializer with the JsonLdRemoteMessageSerializer implementation.

I agree that this should be done in this PR, it won't take much and it will simplify tests a lot

@juliapampus juliapampus force-pushed the feat/dsp_contract_negotiation_dispatcher branch from deafc6f to 753bf09 Compare April 24, 2023 11:18
@juliapampus juliapampus temporarily deployed to Azure-dev April 24, 2023 11:19 — with GitHub Actions Inactive
@juliapampus
Copy link
Contributor Author

juliapampus commented Apr 24, 2023

PR is rebased. @ronjaquensel will take care of refactoring. The refactored structure will then also be applied to #2760.

@juliapampus juliapampus removed the request for review from ronjaquensel April 24, 2023 11:20
@ronjaquensel
Copy link
Contributor

@ndr-brt @jimmarino I refactored the delegates as follows:

  • Introcuded JsonLdRemoteMessageSerializer interface and implementation in dsp-http-spi and dsp-http-core respectively (for now, can be moved further up when required). This serializer is now used by the delegates to transform, compact and serialize a message.
  • Made DspHttpDispatcherDelegate an abstract class and moved the code for creating the HTTP request there. As this is specific to HTTP dispatchers, I think it makes more sense in the abstract class than as an additional helper class. But as this is just a single method, it should not make the code harder to read/understand.
  • Introduced a testFixture for delegates, as pretty much the same code was duplicated between many of the delegate tests. The testFixture itself does not contain @Test annotated methods, but instead provides methods for testing the common behaviour that can be used in the delegate tests. First, I wanted to move the actual tests to the testFixture class, but as the delegate tests use these methods in different combinations (e.g. some parse the response body, others don't), that quickly became rather confusing and unreadable.

@ronjaquensel ronjaquensel force-pushed the feat/dsp_contract_negotiation_dispatcher branch from e20e5b4 to 5ba30e2 Compare April 25, 2023 12:58
@ronjaquensel ronjaquensel temporarily deployed to Azure-dev April 25, 2023 12:59 — with GitHub Actions Inactive
var requestBody = RequestBody.create(body, MediaType.get(APPLICATION_JSON));

return new Request.Builder()
.url(message.getCallbackAddress() + path)

Check failure

Code scanning / CodeQL

Server-side request forgery

Potential server-side request forgery due to a [user-provided value](1). Potential server-side request forgery due to a [user-provided value](2).
@ronjaquensel ronjaquensel force-pushed the feat/dsp_contract_negotiation_dispatcher branch from 5ba30e2 to cb4648d Compare April 25, 2023 13:24
@ronjaquensel ronjaquensel temporarily deployed to Azure-dev April 25, 2023 13:28 — with GitHub Actions Inactive
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 87.23% and project coverage change: +0.48 🎉

Comparison is base (da9d50c) 65.50% compared to head (cb4648d) 65.99%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2780      +/-   ##
==========================================
+ Coverage   65.50%   65.99%   +0.48%     
==========================================
  Files         978      979       +1     
  Lines       19896    19806      -90     
  Branches     1177     1163      -14     
==========================================
+ Hits        13032    13070      +38     
+ Misses       6398     6271     -127     
+ Partials      466      465       -1     
Impacted Files Coverage Δ
.../dispatcher/DspCatalogHttpDispatcherExtension.java 0.00% <0.00%> (ø)
...eclipse/edc/protocol/dsp/DspHttpCoreExtension.java 0.00% <0.00%> (ø)
...patcher/DspNegotiationHttpDispatcherExtension.java 0.00% <0.00%> (ø)
...r/delegate/ContractRequestMessageHttpDelegate.java 90.00% <90.00%> (ø)
...or/service/catalog/CatalogProtocolServiceImpl.java 100.00% <100.00%> (ø)
.../dsp/catalog/api/controller/CatalogController.java 100.00% <100.00%> (+6.89%) ⬆️
...ispatcher/delegate/CatalogRequestHttpDelegate.java 100.00% <100.00%> (+5.71%) ⬆️
...sonObjectFromCatalogRequestMessageTransformer.java 100.00% <100.00%> (ø)
.../JsonObjectToCatalogRequestMessageTransformer.java 60.00% <100.00%> (-8.43%) ⬇️
...rialization/JsonLdRemoteMessageSerializerImpl.java 100.00% <100.00%> (ø)
... and 4 more

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@ronjaquensel ronjaquensel temporarily deployed to Azure-dev April 25, 2023 14:45 — with GitHub Actions Inactive
@ronjaquensel ronjaquensel force-pushed the feat/dsp_contract_negotiation_dispatcher branch from fe37043 to 4408ffa Compare April 26, 2023 08:27
@ronjaquensel ronjaquensel temporarily deployed to Azure-dev April 26, 2023 08:28 — with GitHub Actions Inactive
@ronjaquensel ronjaquensel merged commit ffe3268 into eclipse-edc:main Apr 26, 2023
@ronjaquensel ronjaquensel deleted the feat/dsp_contract_negotiation_dispatcher branch April 26, 2023 08:51
majadlymhmd pushed a commit to FraunhoferISST/edc-connector that referenced this pull request May 10, 2023
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.

5 participants