refactor: clean up ids-api-multipart-endpoint-v1
#1715
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ronjaquensel
force-pushed
the
chore/1610-clean-up-multipart-endpoint
branch
from
19be7aa to
189ede4
Compare
ndr-brt
reviewed
Jul 20, 2022
...in/java/org/eclipse/dataspaceconnector/ids/api/multipart/controller/MultipartController.java
Outdated
Show resolved
Hide resolved
...in/java/org/eclipse/dataspaceconnector/ids/api/multipart/controller/MultipartController.java
Outdated
Show resolved
Hide resolved
...in/java/org/eclipse/dataspaceconnector/ids/api/multipart/controller/MultipartController.java
Outdated
Show resolved
Hide resolved
...in/java/org/eclipse/dataspaceconnector/ids/api/multipart/handler/ArtifactRequestHandler.java
Outdated
Show resolved
Hide resolved
| .connectorId(connectorId) | ||
| .assetId(contractAgreement.getAssetId()) | ||
| .contractId(contractAgreement.getId()) | ||
| .properties(props) | ||
| .connectorAddress(idsWebhookAddress) | ||
| .build(); | ||
|
|
||
| var result = transferProcessManager.initiateProviderRequest(dataRequest); | ||
| // Initiate a transfer process for the request | ||
| var transferInitiateResult = transferProcessManager.initiateProviderRequest(dataRequest); |
There was a problem hiding this comment.
note for the future (after project structure review), here the TransferProcessService should be used instead of the manager
There was a problem hiding this comment.
Hey, @ronjaquensel, this aligns with our discussion to encapsulate some of the policy evaluation as well.
There was a problem hiding this comment.
@ronjaquensel Can we either create an issue or add a TODO for this comment and the one regarding the ContractNegotiationService?
There was a problem hiding this comment.
good point, I added TODOs.
| } | ||
|
|
||
| // TODO get hash from message | ||
| var output = result.getContent(); | ||
| var processId = message.getTransferContract(); | ||
| var negotiationResponse = negotiationManager.confirmed(claimToken, | ||
| var negotiationConfirmResult = negotiationManager.confirmed(claimToken, |
There was a problem hiding this comment.
note for the future (after project structure review), here the ContractNegotiationService should be used instead of the manager
...main/java/org/eclipse/dataspaceconnector/ids/api/multipart/handler/ContractOfferHandler.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| var transformedEdr = transformationResult.getContent(); | ||
|
|
||
| // Apply all endpoint data reference receivers to the endpoint data reference | ||
| var receiveResult = receiverRegistry.receiveAll(transformedEdr).join(); |
There was a problem hiding this comment.
not for this PR, but joins should be avoided, we should think about making the handlers async to avoid blocking code
ndr-brt
approved these changes
Jul 20, 2022
juliapampus
reviewed
Jul 22, 2022
...n/java/org/eclipse/dataspaceconnector/ids/api/multipart/IdsMultipartApiServiceExtension.java
Outdated
Show resolved
Hide resolved
| * | ||
| * @param header the message. | ||
| */ | ||
| private DynamicAttributeToken getToken(Message header) { |
There was a problem hiding this comment.
Is this a method that may be also needed by the MultipartSender classes? As the sender needs a DAT, too. Maybe this could be in a utils oder MessageFunctions class in ids-core.message?
There was a problem hiding this comment.
I think so. I stumbled over other duplications as well, e.g. the resolveConnectorId method in both extension classes. Probably moving all these to ids-core would be something for another PR, to restrict this one to the endpoint module, but I'll open an issue.
juliapampus
approved these changes
Jul 22, 2022
diegogomez-zf
pushed a commit
to diegogomez-zf/DataSpaceConnector
that referenced
this pull request
Aug 3, 2022
* chore: remove IdsResponseMessageFactory * chore: remove unused exceptions * chore: merge response and rejection util classes * chore: remove class IdsClientCredentialsScope * chore: do not use description sub-handlers * chore: remove sub-handler classes * chore: remove DescriptionRequestHandler interface * chore: remove DescriptionResponseMessageUtil * chore: move util classes to util package * chore: add token to all response headers * chore: add Javadoc to MultipartController * chore: move method for building description response to util * chore: return RequestInProcessMessage for artifact requests * chore: make response depend on status result from state machine * chore: do not build message in ArtifactRequestHandler * chore: rename DescriptionRequestHandler * chore: align parameter order in ResponseMessageUtil * docs: add Javadoc for ResponseMessageUtil * chore: remove unused method * docs: add Javadoc for MultipartResponseUtil * refactor: remove duplicated methods from handlers * refactor: remove MultipartResponseUtil * refactor: process status result in contract handlers * refactor: remove notification sub-handlers * chore: remove check for connector payload in MultipartController * refactor: remove superfluous claim token parameter * refactor: remove internal handle method from DescriptionRequestHandler * docs: add comments and Javadoc * refactor: make handleRequest in Handler @NotNull * chore: change rejection reason in EDR handler * chore: add required fields to test messages * test: update handler tests * test: replace tests for DescriptionRequestHandler * refactor: rename RequestUtil * chore: only store secret if TP initialized successfully * docs: update CHANGELOG.md * test: update ResponseUtilTest * chore: fix import order * chore: fix import order * refactor: return FormDataMultiPart from MultipartController * refactor: return token from method in MultipartController * chore: remove unused contract offer handler * chore: remove superfluous requireNonNull checks * refactor: inject monitor into IdsMultipartApiServiceExtension * chore: add TODOs for after project structure review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR changes/adds
Cleans-up the
ids-api-multipart-endpoint-v1module.Further notes
Changes the message type returned by the
ArtifactRequestHandlerfromResponseMessagetoRequestInProcessMessage.Linked Issue(s)
Closes #1610
Checklist
no-changelog)