-
Notifications
You must be signed in to change notification settings - Fork 240
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
feat: encode ContractId
parts
#3235
Conversation
Codecov ReportPatch coverage:
❗ 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
☔ View full report in Codecov by Sentry. |
There was a problem hiding this 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
...trol-plane/contract-spi/src/main/java/org/eclipse/edc/connector/contract/spi/ContractId.java
Outdated
Show resolved
Hide resolved
...trol-plane/contract-spi/src/main/java/org/eclipse/edc/connector/contract/spi/ContractId.java
Outdated
Show resolved
Hide resolved
@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. |
@jimmarino now the |
What this PR changes/adds
Encodes the three parts of a
ContractId
with base64Why it does that
avoid issues with id containing
:
.Further notes
this is a BREAKING CHANGE, so already stored ongoing contracts could stop workingit will work also with not encoded old idsContractId
class to adhere our convention, adding theparseId
method returning aResult<ContractId>
and adding acreate
that returns aContractId
Linked Issue(s)
Closes #3211
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.