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.

Sign up for GitHub

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

Jump to bottom

Feature: add validation for contract definition id #1372

Closed
wants to merge 4 commits into from
Closed

Feature: add validation for contract definition id #1372

wants to merge 4 commits into from

Conversation

tuncaytunc-zf
Copy link
Contributor

What this PR changes/adds

Add validation for contract definition id

Why it does that

Validation of contract definition id by creation solves the problem in negotiation process when the id is not valid.

Further notes

Marking jersey-bean-validation helps for the correct validation of parameters.

Linked Issue(s)

Closes #1347

Checklist

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

@tuncaytunc-zf tuncaytunc-zf requested review from florianrusch-zf and removed request for paullatzelsperger May 30, 2022 13:58
@tuncaytunc-zf tuncaytunc-zf changed the title Add validation for contract definition id Feature: add validation for contract definition id May 30, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1372 (d59ed1d) into main (e4735bc) will decrease coverage by 56.20%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##               main    #1372       +/-   ##
=============================================
- Coverage     67.60%   11.40%   -56.21%     
- Complexity        0      488      +488     
=============================================
  Files           716      717        +1     
  Lines         15858    15889       +31     
  Branches       1041     1041               
=============================================
- Hits          10721     1812     -8909     
- Misses         4663    13972     +9309     
+ Partials        474      105      -369
Impacted Files Coverage Δ
...spaceconnector/common/validator/UuidValidator.java 0.00% <0.00%> (ø)
...ontractdefinition/model/ContractDefinitionDto.java 0.00% <ø> (-100.00%) ⬇️
.../eclipse/dataspaceconnector/spi/result/Result.java 0.00% <0.00%> (-100.00%) ⬇️
...eclipse/dataspaceconnector/spi/result/Failure.java 0.00% <0.00%> (-100.00%) ⬇️
...clipse/dataspaceconnector/spi/query/SortOrder.java 0.00% <0.00%> (-100.00%) ⬇️
...dataspaceconnector/spi/observe/ObservableImpl.java 0.00% <0.00%> (-100.00%) ⬇️
...paceconnector/common/stream/PartitionIterator.java 0.00% <0.00%> (-100.00%) ⬇️
...paceconnector/spi/command/BoundedCommandQueue.java 0.00% <0.00%> (-100.00%) ⬇️
...connector/boot/util/CyclicDependencyException.java 0.00% <0.00%> (-100.00%) ⬇️
.../dataspaceconnector/iam/daps/DapsJwtDecorator.java 0.00% <0.00%> (-100.00%) ⬇️
... and 476 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4735bc...d59ed1d. Read the comment docs.

paullatzelsperger
paullatzelsperger requested changes May 31, 2022
View reviewed changes
Copy link
Member

@paullatzelsperger paullatzelsperger left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We cannot and will not force people to use the UUID format for ContractDefinitions.

As per the linked issue, the problem should be solved in a different way, specifically to verify that the ID does not contain the ":" (colon) character, which is a one-liner.

Please either fix the code or withdraw the PR.

@paullatzelsperger
Copy link
Member

in addition, please re-generate the OpenApi documents and commit them, otherwise the build will fail.

@tuncaytunc-zf
Copy link
Contributor Author

Hi @paullatzelsperger the idea to validate the contractDefinitionId against the UUID was coming from @jimmarino please see the comments in issue #1347, maybe he has a reason why to use UUID. Otherwise I can revert the PR and check only if the ID contains any ":".

@paullatzelsperger
Copy link
Member

paullatzelsperger commented May 31, 2022 •
edited
Loading

Hi @paullatzelsperger the idea to validate the contractDefinitionId against the UUID was coming from @jimmarino please see the comments in issue #1347, maybe he has a reason why to use UUID. Otherwise I can revert the PR and check only if the ID contains any ":".

@jimmarino never said to validate against a UUID, or a GUID. Using a UUID is what we recommend, to ensure a high level of uniqueness, but APIs should never be opinionated like that unless there is a technical reason.

@tuncaytunc-zf
Copy link
Contributor Author

Hi @paullatzelsperger the idea to validate the contractDefinitionId against the UUID was coming from @jimmarino please see the comments in issue #1347, maybe he has a reason why to use UUID. Otherwise I can revert the PR and check only if the ID contains any ":".

@jimmarino never said to validate against a UUID, or a GUID. Using a UUID is what we recommend, to ensure a high level of uniqueness, but APIs should never be opinionated like that unless there is a technical reason.

My understanding from his comment "The id should be a GUID (not URN) so +1 on adding validation in the API. ..." was that the id should be a GUID and should be validated in the API :-) But as I said if there are no special reason to use UUID of course I will remove the UUID validation and only check if id contains a ":"

@florianrusch-zf
Copy link
Contributor

But as I said if there are no special reason to use UUID of course I will remove the UUID validation and only check if id contains a ":"

@paullatzelsperger @jimmarino @tuncaytunc-zf Do we have to look also for other chars which should't be part of the ID?

@paullatzelsperger
Copy link
Member

But as I said if there are no special reason to use UUID of course I will remove the UUID validation and only check if id contains a ":"

@paullatzelsperger @jimmarino @tuncaytunc-zf Do we have to look also for other chars which should't be part of the ID?

none that I can think of, from a technical perspective.

@jimmarino
Copy link
Contributor

Hi @paullatzelsperger the idea to validate the contractDefinitionId against the UUID was coming from @jimmarino please see the comments in issue #1347, maybe he has a reason why to use UUID. Otherwise I can revert the PR and check only if the ID contains any ":".

@jimmarino never said to validate against a UUID, or a GUID. Using a UUID is what we recommend, to ensure a high level of uniqueness, but APIs should never be opinionated like that unless there is a technical reason.

My understanding from his comment "The id should be a GUID (not URN) so +1 on adding validation in the API. ..." was that the id should be a GUID and should be validated in the API :-) But as I said if there are no special reason to use UUID of course I will remove the UUID validation and only check if id contains a ":"

I think this PR should be about 1-3 lines of code. We don't need to drag in validation dependencies into the core.

@tuncaytunc-zf
Copy link
Contributor Author

Will close this PR and implement just to check if the id contains any ":".

@tuncaytunc-zf tuncaytunc-zf deleted the feature/contract-definition-id-validation branch June 2, 2022 16:28
ndr-brt pushed a commit that referenced this pull request Jun 8, 2022
@tuncaytunc-zf
Feature: add validation for contract definition id #1372 (#1407)
5938d26 
* Add validation for contract definition id

* Remove "jersey-bean-validation" from extensions.

* Remove NotNull constraint for id in TransferRequestDto.

* Rebuild OpenApi definitions

* Rebuild OpenApi definitions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paullatzelsperger paullatzelsperger paullatzelsperger requested changes

@ndr-brt ndr-brt Awaiting requested review from ndr-brt

@florianrusch-zf florianrusch-zf Awaiting requested review from florianrusch-zf

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add validation to avoid ContractDefinition id to contain :
5 participants
@tuncaytunc-zf @codecov-commenter @paullatzelsperger @florianrusch-zf @jimmarino