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

Feature: add validation for contract definition id #1372 #1407

Merged

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)

@florianrusch-zf
Copy link
Contributor

If we now would specify an incorrect ID, what would the error message look like?

@tuncaytunc-zf
Copy link
Contributor Author

tuncaytunc-zf commented Jun 3, 2022

If we now would specify an incorrect ID, what would the error message look like?

You would get "400 Bad Request" like other validation fails.

@florianrusch-zf
Copy link
Contributor

You would get "400 Bad Request" like other validation fails.

Without any kind of error message?

@tuncaytunc-zf
Copy link
Contributor Author

You would get "400 Bad Request" like other validation fails.

Without any kind of error message?

Unfortunately not like all other fields but as I can remember there were some discussions ongoing to improve the error handling or logging, maybe it exists already an issue for it. I think to improve the error handling was not part of this issue.

@florianrusch-zf
Copy link
Contributor

@ndr-brt what do you think about this? In my opinion we should always consider to improve the error handling - also if we just fix a bug.

@tuncaytunc-zf we have since a few days the Logging Guide. But I think that this special case is currently not be considered within the logging guide.

In addition we also discussed this kind of error handling here: #1332

Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

I think this goes beyond the logging guide, as this information (failed input validation) is interesting for the client, but not for the server.
We should add some details in the response body when validation fails to make the client aware of what's needed to fix.

Talking about the PR, this detail (the interdiction of : in contract definition id) should be described in the openapi specification

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

Merging #1407 (699d9d0) into main (d701dc3) will decrease coverage by 56.12%.
The diff coverage is 0.00%.

❗ Current head 699d9d0 differs from pull request most recent head 610f738. Consider uploading reports for the commit 610f738 to get more accurate results

@@              Coverage Diff              @@
##               main    #1407       +/-   ##
=============================================
- Coverage     67.61%   11.48%   -56.13%     
- Complexity        0      494      +494     
=============================================
  Files           719      719               
  Lines         15935    15936        +1     
  Branches       1043     1043               
=============================================
- Hits          10774     1830     -8944     
- Misses         4687    14001     +9314     
+ Partials        474      105      -369     
Impacted Files Coverage Δ
...ontractdefinition/model/ContractDefinitionDto.java 0.00% <0.00%> (-100.00%) ⬇️
...ment/transferprocess/model/TransferRequestDto.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 475 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 d701dc3...610f738. Read the comment docs.

@ndr-brt ndr-brt merged commit 5938d26 into eclipse-edc:main Jun 8, 2022
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.

Add validation to avoid ContractDefinition id to contain :
5 participants