feat(DataManagementApi): returns id in Assest, Policy and Contract de… #2012

git-masoud · 2022-09-25T15:43:04Z

What this PR changes/adds

Made all the DataManagementApi POST resource return id of the created resource

Added wrapper class for PolicyDefinition#getId, ContractDefinition#getId and AssetIdPolicyDefinition#getId.

Modified Asset, PolicyDefinition and ContractDefinition classes to return their wrapperId in the POST functions

Modified and added the unit tests and integration tests

Why it does that

Improving the DataManagementApi

Linked Issue(s)

Closes #1984

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 styleguide for details)

…finition Post calls. Modify and added the unit tests and integration tests

github-actions

We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contribution guidelines, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.

codecov-commenter · 2022-09-25T15:54:44Z

Codecov Report

Base: 63.64% // Head: 5.39% // Decreases project coverage by -58.24% ⚠️

Coverage data is based on head (5eb5003) compared to base (740c100).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head 5eb5003 differs from pull request most recent head 75d5a32. Consider uploading reports for the commit 75d5a32 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #2012       +/-   ##
============================================
- Coverage     63.64%   5.39%   -58.25%     
- Complexity        0     166      +166     
============================================
  Files           772     780        +8     
  Lines         16370   16521      +151     
  Branches       1055    1074       +19     
============================================
- Hits          10418     892     -9526     
- Misses         5508   15563    +10055     
+ Partials        444      66      -378

Impacted Files	Coverage Δ
...se/dataspaceconnector/api/model/IdResponseDto.java	`0.00% <0.00%> (ø)`
...r/api/datamanagement/asset/AssetApiController.java	`0.00% <0.00%> (-97.73%)`	⬇️
...actdefinition/ContractDefinitionApiController.java	`0.00% <0.00%> (-95.24%)`	⬇️
...tnegotiation/ContractNegotiationApiController.java	`0.00% <0.00%> (-96.37%)`	⬇️
...nagement/policy/PolicyDefinitionApiController.java	`0.00% <0.00%> (-95.00%)`	⬇️
.../transferprocess/TransferProcessApiController.java	`0.00% <0.00%> (-95.92%)`	⬇️
...eclipse/dataspaceconnector/spi/result/Failure.java	`0.00% <0.00%> (-100.00%)`	⬇️
...clipse/dataspaceconnector/spi/query/SortOrder.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../eclipse/dataspaceconnector/policy/model/Rule.java	`0.00% <0.00%> (-100.00%)`	⬇️
...pse/dataspaceconnector/spi/event/EventPayload.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 548 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…pi definitions

paullatzelsperger · 2022-09-26T07:19:43Z

...sset-api/src/main/java/org/eclipse/dataspaceconnector/api/datamanagement/asset/AssetApi.java

                    @ApiResponse(responseCode = "400", description = "Request body was malformed",
                            content = @Content(array = @ArraySchema(schema = @Schema(implementation = ApiErrorDetail.class)))),
                    @ApiResponse(responseCode = "409", description = "Could not create asset, because an asset with that ID already exists",
                            content = @Content(array = @ArraySchema(schema = @Schema(implementation = ApiErrorDetail.class)))) }
    )
-    void createAsset(@Valid AssetEntryDto assetEntryDto);
+    AssetId createAsset(@Valid AssetEntryDto assetEntryDto);


Please do not use these unnecessary wrappers. Just return a String.

yes, I agree. I just tried to implement the same thing as contractnegotiation-api ,
org.eclipse.dataspaceconnector.api.datamanagement.contractnegotiation.model.NegotiationId

looking at the other endpoints responses, using String will breaks the consistency of the api responses, since all the other endpoints either return nothing or a json. Furthermore, as I mentioned contractnegotiation-api and transferprocess-api are already implemented with the same approach(Using a wrapper for Id).
if this argument is convincing, then I can use the subclasses of BaseResponseDto not instead of wrappers. However my question is, if I should use the ones which are already implemented such as AssetResponseDto or ContractDefinitionResponseDto, which returns the whole object or a new one to return only the Id?

.../eclipse/dataspaceconnector/api/datamanagement/contractdefinition/ContractDefinitionApi.java

.../main/java/org/eclipse/dataspaceconnector/api/datamanagement/policy/PolicyDefinitionApi.java

...c/testFixtures/java/org/eclipse/dataspaceconnector/system/tests/local/BlobTransferUtils.java

...-transfer-test/runner/src/test/java/org/eclipse/dataspaceconnector/test/e2e/Participant.java

paullatzelsperger · 2022-09-26T07:26:37Z

There is really no point in wrapping a String in an object, much less in different objects that do the same thing. Please don't do that.
If the response absolutely cannot be a String for some reason (which you would have to argue well), you could look at the BaseResponseDto and subclass it.

juliapampus

@git-masoud You need to create an Eclipse account and sign the ECA.

ndr-brt · 2022-09-26T14:06:40Z

There is really no point in wrapping a String in an object, much less in different objects that do the same thing. Please don't do that. If the response absolutely cannot be a String for some reason (which you would have to argue well), you could look at the BaseResponseDto and subclass it.

@paullatzelsperger if I remember correctly we decided for the wrapping (in the TransferProcess and in the ContractNegotiation controllers) to always have a json object as response type. I would keep this convention.

paullatzelsperger · 2022-09-26T14:09:13Z

@paullatzelsperger if I remember correctly we decided for the wrapping (in the TransferProcess and in the ContractNegotiation controllers) to always have a json object as response type. I would keep this convention.

That is the reason I suggested the use of the BaseResponseDto (e.g. could be something like a StringResponseDto), in case we absolutely want the wrapping. That could even contain the creation timestamp. We do absolutely not need three different classes for wrapping a single String, all of which circumvent our DTO hierarchy.

ndr-brt

Looks neat, apart from some duplicated contentType(JSON) for me it's good to go

ndr-brt · 2022-09-26T14:13:38Z

@paullatzelsperger if I remember correctly we decided for the wrapping (in the TransferProcess and in the ContractNegotiation controllers) to always have a json object as response type. I would keep this convention.

That is the reason I suggested the use of the BaseResponseDto, in case we absolutely want the wrapping. We do absolutely not need three different classes for wrapping a single String, all of which circumvent our DTO hierarchy.

Oh, yeah, you're right, for some reasons I lost that part :D
Anyway it could be a subsequent refactoring.

paullatzelsperger · 2022-09-26T14:14:17Z

@paullatzelsperger if I remember correctly we decided for the wrapping (in the TransferProcess and in the ContractNegotiation controllers) to always have a json object as response type. I would keep this convention.

That is the reason I suggested the use of the BaseResponseDto, in case we absolutely want the wrapping. We do absolutely not need three different classes for wrapping a single String, all of which circumvent our DTO hierarchy.

Oh, yeah, you're right, for some reasons I lost that part :D

@ndr_brt no worries :) However, this seems like an easy-enough thing to do, so I'd rather have this done properly now instead of introducing technical debt on purpose. I mean, we're talking about a trivial change, that would even reduce the surface of the current PR.

…StringResponseDto to be used instead of the object's wrappers. Remove extra contentType tests in the integration tests. Change all the Data Management Api post return to StringResponseDto. Change openapi.yaml

git-masoud · 2022-09-27T07:04:21Z

Thanks for the tips. I implemented StringResponseDto to be used instead of the object's wrappers, although I am not sure about it's location(currently in extensions/common/api/api-core/src/main/java/org/eclipse/dataspaceconnector/api/model/StringResponseDto.java).

...n/api/api-core/src/main/java/org/eclipse/dataspaceconnector/api/model/StringResponseDto.java

ndr-brt · 2022-09-27T07:52:13Z

...rc/main/java/org/eclipse/dataspaceconnector/api/datamanagement/asset/AssetApiController.java

-            return new AssetId(result.getContent().getId());
+            return StringResponseDto.Builder.newInstance()
+                    .id(result.getContent().getId())
+                    .createdAt(result.getContent().getCreatedAt())


Please extract the "content" in a variable

ndr-brt · 2022-09-27T07:52:29Z

...ataspaceconnector/api/datamanagement/contractdefinition/ContractDefinitionApiController.java

@@ -111,7 +111,10 @@ public ContractDefinitionId createContractDefinition(@Valid ContractDefinitionRe
        var result = service.create(contractDefinition);
        if (result.succeeded()) {
            monitor.debug(format("Contract definition created %s", result.getContent().getId()));
-            return new ContractDefinitionId(result.getContent().getId());
+            return StringResponseDto.Builder.newInstance()
+                    .id(result.getContent().getId())


Please extract the "content" in a variable

ndr-brt · 2022-09-27T07:53:11Z

.../org/eclipse/dataspaceconnector/api/datamanagement/policy/PolicyDefinitionApiController.java

-            return new PolicyDefinitionId(result.getContent().getId());
+            return StringResponseDto.Builder.newInstance()
+                    .id(result.getContent().getId())
+                    .createdAt(result.getContent().getCreatedAt())


Please extract the "content" in a variable

…to. Refactor and optimise the related classes

paullatzelsperger · 2022-09-27T14:36:35Z

re-requesting my own review, so that I have it in my to-review list.

paullatzelsperger · 2022-09-27T15:15:58Z

...ommon/api/api-core/src/main/java/org/eclipse/dataspaceconnector/api/model/IdResponseDto.java

+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
+
+@JsonDeserialize(builder = String.class)


that seems incorrect. should be IdResponseDto.Builder. Also, there is no test for this class. There should at least be a test to verify correct serialization and deserialization, which would have surfaced the incorrect builder.

paullatzelsperger · 2022-09-27T15:33:58Z

@git-masoud the Openapi test is failing. don't forget to regenerate the open api spec and the swagger ui:

./gradlew resolve
./gradlew mergeOpenApiFiles
./gradlew generateSwaggerUI

Execute these three tasks in separate gradle calls (=do NOT join them together) and also commit that changeset.

…seDto test. Update openApi files

paullatzelsperger · 2022-09-29T11:19:24Z

@git-masoud please note that I updated the PR title for merging so that it complies with our standards regarding conventional commits and contributing guidelines, specifically regarding the imperative mood.

feat(DataManagementApi): returns id in Assest, Policy and Contract de…

718621b

…finition Post calls. Modify and added the unit tests and integration tests

git-masoud requested a review from paullatzelsperger as a code owner September 25, 2022 15:43

git-masoud temporarily deployed to Azure-dev September 25, 2022 15:43 Inactive

github-actions bot reviewed Sep 25, 2022

View reviewed changes

git-masoud requested a review from ndr-brt September 25, 2022 15:44

feat(DataManagementApi): Fix checkstyle error, end to end test, OpenA…

a661a14

…pi definitions

git-masoud temporarily deployed to Azure-dev September 25, 2022 16:46 Inactive

feat(DataManagementApi): Fix end to end tests errors

07b9e13

git-masoud temporarily deployed to Azure-dev September 25, 2022 17:03 Inactive

feat(DataManagementApi): remove unused import

f25b37a

git-masoud temporarily deployed to Azure-dev September 25, 2022 17:10 Inactive

git-masoud added enhancement New feature or request api Feature related to the (REST) api labels Sep 25, 2022

paullatzelsperger requested changes Sep 26, 2022

View reviewed changes

juliapampus reviewed Sep 26, 2022

View reviewed changes

ndr-brt approved these changes Sep 26, 2022

View reviewed changes

feat(DataManagementApi): Remove all the objectId wrappers. Implement …

abc2e24

…StringResponseDto to be used instead of the object's wrappers. Remove extra contentType tests in the integration tests. Change all the Data Management Api post return to StringResponseDto. Change openapi.yaml

git-masoud temporarily deployed to Azure-dev September 27, 2022 06:55 Inactive

ndr-brt reviewed Sep 27, 2022

View reviewed changes

feat(DataManagementApi): Change StringResponseDto name to IdResponseD…

4a904bb

…to. Refactor and optimise the related classes

git-masoud temporarily deployed to Azure-dev September 27, 2022 08:41 Inactive

paullatzelsperger self-requested a review September 27, 2022 14:36

paullatzelsperger requested changes Sep 27, 2022

View reviewed changes

git-masoud added 3 commits September 28, 2022 08:21

feat(DataManagementApi): Fix the error in IdResponseDto. Add IdRespon…

2274c77

…seDto test. Update openApi files

feat(DataManagementApi): Fix the error in IdResponseDto. Add IdRespon…

e4ad3b1

…seDto test. Update openApi files

feat(DataManagementApi): add signature to IdResponseDtoTest

5eb5003

git-masoud temporarily deployed to Azure-dev September 28, 2022 06:23 Inactive

feat(DataManagementApi): fix check style issue IdResponseDtoTest

75d5a32

git-masoud temporarily deployed to Azure-dev September 28, 2022 06:33 Inactive

git-masoud requested a review from paullatzelsperger September 29, 2022 06:31

paullatzelsperger approved these changes Sep 29, 2022

View reviewed changes

paullatzelsperger merged commit 5bc741c into eclipse-edc:main Sep 29, 2022

git-masoud deleted the feature/1984_DataManagement_CreateController_Return_IdWrapper branch September 29, 2022 11:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(DataManagementApi): returns id in Assest, Policy and Contract de… #2012

feat(DataManagementApi): returns id in Assest, Policy and Contract de… #2012

git-masoud commented Sep 25, 2022

github-actions bot left a comment

codecov-commenter commented Sep 25, 2022 •

edited

Loading

paullatzelsperger Sep 26, 2022

git-masoud Sep 26, 2022

git-masoud Sep 26, 2022

paullatzelsperger commented Sep 26, 2022 •

edited

Loading

juliapampus left a comment

ndr-brt commented Sep 26, 2022

paullatzelsperger commented Sep 26, 2022 •

edited

Loading

ndr-brt left a comment

ndr-brt commented Sep 26, 2022 •

edited

Loading

paullatzelsperger commented Sep 26, 2022 •

edited

Loading

git-masoud commented Sep 27, 2022

ndr-brt Sep 27, 2022

ndr-brt Sep 27, 2022

ndr-brt Sep 27, 2022

paullatzelsperger commented Sep 27, 2022

paullatzelsperger Sep 27, 2022

paullatzelsperger commented Sep 27, 2022

paullatzelsperger commented Sep 29, 2022

feat(DataManagementApi): returns id in Assest, Policy and Contract de… #2012

feat(DataManagementApi): returns id in Assest, Policy and Contract de… #2012

Conversation

git-masoud commented Sep 25, 2022

What this PR changes/adds

Why it does that

Linked Issue(s)

Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 25, 2022 • edited Loading

Codecov Report

paullatzelsperger Sep 26, 2022

Choose a reason for hiding this comment

git-masoud Sep 26, 2022

Choose a reason for hiding this comment

git-masoud Sep 26, 2022

Choose a reason for hiding this comment

paullatzelsperger commented Sep 26, 2022 • edited Loading

juliapampus left a comment

Choose a reason for hiding this comment

ndr-brt commented Sep 26, 2022

paullatzelsperger commented Sep 26, 2022 • edited Loading

ndr-brt left a comment

Choose a reason for hiding this comment

ndr-brt commented Sep 26, 2022 • edited Loading

paullatzelsperger commented Sep 26, 2022 • edited Loading

git-masoud commented Sep 27, 2022

ndr-brt Sep 27, 2022

Choose a reason for hiding this comment

ndr-brt Sep 27, 2022

Choose a reason for hiding this comment

ndr-brt Sep 27, 2022

Choose a reason for hiding this comment

paullatzelsperger commented Sep 27, 2022

paullatzelsperger Sep 27, 2022

Choose a reason for hiding this comment

paullatzelsperger commented Sep 27, 2022

paullatzelsperger commented Sep 29, 2022

codecov-commenter commented Sep 25, 2022 •

edited

Loading

paullatzelsperger commented Sep 26, 2022 •

edited

Loading

paullatzelsperger commented Sep 26, 2022 •

edited

Loading

ndr-brt commented Sep 26, 2022 •

edited

Loading

paullatzelsperger commented Sep 26, 2022 •

edited

Loading