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: encode ContractId parts #3235

Merged
merged 3 commits into from
Jun 28, 2023

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Jun 26, 2023

What this PR changes/adds

Encodes the three parts of a ContractId with base64

Why it does that

avoid issues with id containing :.

Further notes

  • this is a BREAKING CHANGE, so already stored ongoing contracts could stop working it will work also with not encoded old ids
  • refactored the ContractId class to adhere our convention, adding the parseId method returning a Result<ContractId> and adding a create that returns a ContractId
  • deprecating old methods to avoid compilation errors in downstream projects.

Linked Issue(s)

Closes #3211

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2023

Codecov Report

Patch coverage: 89.15% and project coverage change: +0.24 🎉

Comparison is base (3172ed9) 65.68% compared to head (34df404) 65.93%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3235      +/-   ##
==========================================
+ Coverage   65.68%   65.93%   +0.24%     
==========================================
  Files         840      842       +2     
  Lines       16787    16875      +88     
  Branches      917      926       +9     
==========================================
+ Hits        11027    11126      +99     
+ Misses       5393     5380      -13     
- Partials      367      369       +2     
Impacted Files Coverage Δ
...iation/ConsumerContractNegotiationManagerImpl.java 96.61% <60.00%> (ø)
...iation/ProviderContractNegotiationManagerImpl.java 96.19% <60.00%> (ø)
...eclipse/edc/connector/contract/spi/ContractId.java 88.23% <88.23%> (+3.61%) ⬆️
...ract/validation/ContractValidationServiceImpl.java 87.34% <96.55%> (+2.07%) ⬆️
...pse/edc/connector/catalog/DatasetResolverImpl.java 100.00% <100.00%> (ø)
...ctor/contract/offer/ContractOfferResolverImpl.java 100.00% <100.00%> (ø)
...ferprocess/TransferProcessProtocolServiceImpl.java 98.86% <100.00%> (+0.09%) ⬆️

... and 21 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

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

minor nits, all good otherwise

@jimmarino
Copy link
Contributor

@ndr-brt Can we discuss tomorrow during the committer's call? I'd like to see if we can avoid the breaking change.

@ndr-brt
Copy link
Member Author

ndr-brt commented Jun 27, 2023

@ndr-brt Can we discuss tomorrow during the committer's call? I'd like to see if we can avoid the breaking change.

sure, just for the record I also tried to use a "try to decode and if it does not work just use the plain string", but it showed up that strings that aren't base64 representation can be detected as base64 representation of weird characters, e.g. "definitionId" decodes correctly into a bunch of unreadable data.

We can, though state that is practically impossible to have an UUID that can result as a decodable base64, so if not of all the three parts decode correctly then we can assume that's an "old fashioned" id and we can build a valid object as it is.

@ndr-brt
Copy link
Member Author

ndr-brt commented Jun 28, 2023

@jimmarino now the ContractId will also being able to handle not encoded ids, so this PR does not introduce any breaking change

@ndr-brt ndr-brt merged commit 33a22bc into eclipse-edc:main Jun 28, 2023
@ndr-brt ndr-brt deleted the 3211-encode-contractid-parts branch June 28, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core feature enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem when constructing ContractIds containing colons ":"
5 participants