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: consume offering #491

Closed
wants to merge 38 commits into from
Closed

feat: consume offering #491

wants to merge 38 commits into from

Conversation

M-Fitzke
Copy link

Pull Request

Implements the workflow for consuming an offering (asset, policy, contract definition) as stated in #182. Adds an API endpoint for consuming an offering with a single API call.
Will start a contract negotiation and once that is finisehd it will start a data transfer.

How Has This Been Tested?

All new classes are tested via Unit Testing, in addition to this there is an integration test covering the whole consumption process.

Linked Issue(s)

Part of #182 .

Checklist

  • I have formatted the title correctly and precisely
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas and public classes/methods
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings (performed checkstyle check locally)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have added/updated copyright headers

timdah and others added 30 commits June 28, 2023 17:16
* Add missing annotations
* Align class names
* Add null checks
…ensions into feat/182-consume-offering

feat: merge latest upstream changes
# Conflicts:
#	extensions/wrapper/wrapper-common-api/src/main/java/de/sovity/edc/ext/wrapper/api/common/model/CriterionDto.java
#	extensions/wrapper/wrapper/build.gradle.kts
#	extensions/wrapper/wrapper/src/main/java/de/sovity/edc/ext/wrapper/WrapperExtension.java
#	extensions/wrapper/wrapper/src/main/java/de/sovity/edc/ext/wrapper/WrapperExtensionContextBuilder.java
#	extensions/wrapper/wrapper/src/main/java/de/sovity/edc/ext/wrapper/api/usecase/model/ContractDefinitionRequestDto.java
#	extensions/wrapper/wrapper/src/main/java/de/sovity/edc/ext/wrapper/api/usecase/services/OfferingService.java
#	extensions/wrapper/wrapper/src/test/java/de/sovity/edc/ext/wrapper/api/usecase/services/OfferingServiceTest.java
#	extensions/wrapper/wrapper/src/test/resources/usecase/contract-offer-valid.json
#	gradle.properties
#	settings.gradle.kts
import lombok.RequiredArgsConstructor;
import lombok.Setter;
import lombok.ToString;
import lombok.*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

convention: no asterisk imports (requires fix in intellij settings)

@@ -15,6 +15,7 @@

import io.swagger.v3.oas.annotations.media.Schema;
import lombok.AllArgsConstructor;
import lombok.Builder;
Copy link
Collaborator

Choose a reason for hiding this comment

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

unused import?

doLast {
println(sourceSets["main"].runtimeClasspath.asPath)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be for local debugging only, right?

private String id;
private String providerId;
private String consumerId;
private long contractSigningDate;
Copy link
Collaborator

Choose a reason for hiding this comment

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

convention: API should use real date classes, e.g. OffsetDateTime

Comment on lines +169 to +178
transformerRegistry.register(new PermissionToPermissionDtoTransformer());
transformerRegistry.register(new PolicyToPolicyDtoTransformer());
transformerRegistry.register(new ContractAgreementToContractAgreementDtoTransformer());
transformerRegistry.register(new DataRequestToDataRequestDtoTransformer());
transformerRegistry.register(new ContractNegotiationToContractNegotiationOutputDtoTransformer());
transformerRegistry.register(new TransferProcessToTransferProcessOutputDtoTransformer());
var consumptionService = new ConsumptionService(contractNegotiationService, transferProcessService,
contractNegotiationStore, transferProcessStore, transformerRegistry, policyMappingService);

negotiationObservable.registerListener(new ContractNegotiationConsumptionListener(consumptionService));
Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as we can we should try to avoid using the transformer registry to allow to see the code flow, calling functions depending on what's necessary, rather than registering global resources / mappings.

violates clean code / YAGNI

private String id;

/** The input that was used to start this process. */
private ConsumptionInputDto input;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this required?

@Getter
public class ContractNegotiationOutputDto {
private String id;
private String state;
Copy link
Collaborator

Choose a reason for hiding this comment

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

String state, what information does this encode?

private String counterPartyId;
private String counterPartyAddress;
private String protocol;
private ContractAgreementDto contractAgreement;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be unnested?

* A DTO Class for DataRequests
*
* @author Haydar Qarawlus
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this class do?

private String state;
private DataRequestDto dataRequest;
private Map<String, String> contentDataAddress;
private Map<String, String> privateProperties = new HashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we really output the data address and private properties here?

@richardtreier
Copy link
Collaborator

richardtreier commented Sep 12, 2023

In general, I'd recommend taking a look once more at these topics:

  • Consistent documentation of API models with @Schema
  • Use of the transformer registry only on Core EDC to Core EDC type mappings.
  • Another general look at the API model itself regarding nesting. 1-to-1 cardinality object shouldn't be nested unless there's a very good reason for it.
  • Our mapping for the UiPolicyDto (PolicyMapper) and the UiAsset (AssetMapper, type-safe UiAsset #512, WIP) will be implemented soon. Maybe we can re-use the UI models for the Use Case API as the mapping really was non-trivial, and we could profit both from the simplified policy creation and the type-safe asset creation

@richardtreier
Copy link
Collaborator

closed the PR, the general API model of the Use Case API is still on a MS8-State. Using EDC 0 we'll create a new simplified Use Case API that also re-uses the classes UiAsset, UiPolicy.

@tmberthold tmberthold deleted the feat/182-consume-offering branch May 16, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants