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(dsp): Add missing notify methods on TransferProcessService #2661

Merged

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Mar 30, 2023

What this PR changes/adds

Adds missing methods on TransferProcessService that will permit the DSP controller to ingest an incoming RemoteMessage to the service

Why it does that

refactoring

Further notes

  • added processId to the TransferProcess*Messages through a TransferRemoteMessage interface
  • applied an huge refactoring to the service, where every action creates a command than it gets validated first and applied later. the validation logic is duplicated in the service and in the command handler, this is needed because in the service we want to eventually give fast feedback to the client/counter part, and in the handler we want to ensure the validity of the command (as it is executed asynchronously). These logic could be refactored to a single one, a related issue will come.

Linked Issue(s)

Closes #2651

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)

@ndr-brt ndr-brt added enhancement New feature or request dataspace-protocol related to the dataspace protocol labels Mar 30, 2023
@ndr-brt ndr-brt requested a review from jimmarino March 30, 2023 15:26
@ndr-brt ndr-brt temporarily deployed to Azure-dev March 30, 2023 15:26 — with GitHub Actions Inactive
* @param claimToken the counter-party claim token
* @return a succeeded result if the operation was successful, a failed one otherwise
*/
ServiceResult<TransferProcess> notifyStarted(TransferStartMessage message, ClaimToken claimToken);

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'claimToken' is never used.
@@ -40,6 +40,10 @@
return connectorAddress;
}

public String getProcessId() {

Check notice

Code scanning / CodeQL

Missing Override annotation

This method overrides [TransferRemoteMessage.getProcessId](1); it is advisable to add an Override annotation.
* @param claimToken the counter-party claim token
* @return a succeeded result if the operation was successful, a failed one otherwise
*/
ServiceResult<TransferProcess> notifyTerminated(TransferTerminationMessage message, ClaimToken claimToken);

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'claimToken' is never used.
* @param claimToken the counter-party claim token
* @return a succeeded result if the operation was successful, a failed one otherwise
*/
ServiceResult<TransferProcess> notifyCompleted(TransferCompletionMessage message, ClaimToken claimToken);

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'claimToken' is never used.
@codecov-commenter
Copy link

Codecov Report

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

Comparison is base (30faa3f) 64.27% compared to head (4e0c371) 3.35%.

📣 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   #2661       +/-   ##
============================================
- Coverage     64.27%   3.35%   -60.93%     
- Complexity        0     119      +119     
============================================
  Files           897     901        +4     
  Lines         18370   18432       +62     
  Branches       1094    1095        +1     
============================================
- Hits          11808     618    -11190     
- Misses         6126   17771    +11645     
+ Partials        436      43      -393     
Impacted Files Coverage Δ
...ce/transferprocess/TransferProcessServiceImpl.java 0.00% <0.00%> (-83.12%) ⬇️
...ctor/transfer/TransferProcessCommandExtension.java 0.00% <0.00%> (ø)
...andlers/NotifyCompletedTransferCommandHandler.java 0.00% <0.00%> (ø)
...ndlers/NotifyTerminatedTransferCommandHandler.java 0.00% <0.00%> (ø)
...r/transfer/process/TransferProcessManagerImpl.java 23.75% <0.00%> (-63.95%) ⬇️
.../api/multipart/handler/ArtifactRequestHandler.java 13.84% <0.00%> (-49.80%) ⬇️
.../types/command/NotifyCompletedTransferCommand.java 0.00% <0.00%> (ø)
...pi/types/command/NotifyStartedTransferCommand.java 0.00% <ø> (ø)
...types/command/NotifyTerminatedTransferCommand.java 0.00% <0.00%> (ø)
.../spi/types/protocol/TransferCompletionMessage.java 0.00% <0.00%> (ø)
... and 3 more

... and 615 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.

@ndr-brt ndr-brt changed the title feat(dsp) Add missing notify methods on TransferProcessService feat(dsp): Add missing notify methods on TransferProcessService Mar 31, 2023
@ndr-brt ndr-brt force-pushed the 2651-add-missing-tp-notify-methods branch from 4e0c371 to 0394189 Compare March 31, 2023 08:00
@ndr-brt ndr-brt temporarily deployed to Azure-dev March 31, 2023 08:00 — with GitHub Actions Inactive
@ndr-brt ndr-brt force-pushed the 2651-add-missing-tp-notify-methods branch from 0394189 to 0aa6a01 Compare March 31, 2023 08:25
@ndr-brt ndr-brt temporarily deployed to Azure-dev March 31, 2023 08:26 — with GitHub Actions Inactive
@ndr-brt ndr-brt temporarily deployed to Azure-dev March 31, 2023 09:30 — with GitHub Actions Inactive
@ndr-brt ndr-brt force-pushed the 2651-add-missing-tp-notify-methods branch from 0aa6a01 to 706a74a Compare March 31, 2023 10:29
@ndr-brt ndr-brt temporarily deployed to Azure-dev March 31, 2023 10:30 — with GitHub Actions Inactive
@ndr-brt ndr-brt force-pushed the 2651-add-missing-tp-notify-methods branch from 706a74a to d92af19 Compare March 31, 2023 11:30
@ndr-brt ndr-brt temporarily deployed to Azure-dev March 31, 2023 11:30 — with GitHub Actions Inactive
@ndr-brt ndr-brt force-pushed the 2651-add-missing-tp-notify-methods branch from d92af19 to 3b3bb3d Compare April 3, 2023 11:39
@ndr-brt ndr-brt temporarily deployed to Azure-dev April 3, 2023 11:40 — with GitHub Actions Inactive
@ndr-brt ndr-brt force-pushed the 2651-add-missing-tp-notify-methods branch from 3b3bb3d to fa97d47 Compare April 4, 2023 09:31
@ndr-brt ndr-brt temporarily deployed to Azure-dev April 4, 2023 09:33 — with GitHub Actions Inactive
@ndr-brt ndr-brt merged commit 3c7c154 into eclipse-edc:main Apr 4, 2023
@ndr-brt ndr-brt deleted the 2651-add-missing-tp-notify-methods branch April 4, 2023 09:55
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: Add missing notify methods on TransferProcess services
3 participants