-
Notifications
You must be signed in to change notification settings - Fork 245
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: add private properties to contract definition entity #3534
feat: add private properties to contract definition entity #3534
Conversation
…ohannad-ezzo/Connector into feat-contract-def-private-prop
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.
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.
...r/api/management/contractdefinition/transform/JsonObjectToContractDefinitionTransformer.java
Outdated
Show resolved
Hide resolved
property_name VARCHAR(255) NOT NULL, | ||
property_value TEXT NOT NULL, | ||
property_type VARCHAR(255) NOT NULL, | ||
property_is_private BOOLEAN, |
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.
not needed, since contract definitions can only have private properties.
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.
I think this was copied by the sql asset index implementation, but to be fair I'd prefer to follow the transfer process one, where the private properties are serialized into a json field.
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.
I followed the same pattern which is used for Asset. Therefore, I followed this approach. In addition, I intended to add property_is_private
as a preparation for adding non-private properties for the contract definition. To be honest, our usecase for contract definition is just to have private properties, but not for non-private properties. So, as requested, the property property_is_private
is removed and the corresponding code is adjusted.
.../java/org/eclipse/edc/connector/store/sql/contractdefinition/SqlContractDefinitionStore.java
Outdated
Show resolved
Hide resolved
.../java/org/eclipse/edc/connector/store/sql/contractdefinition/SqlContractDefinitionStore.java
Outdated
Show resolved
Hide resolved
...pse/edc/connector/contract/spi/testfixtures/offer/store/ContractDefinitionStoreTestBase.java
Show resolved
Hide resolved
.../java/org/eclipse/edc/connector/store/sql/contractdefinition/SqlContractDefinitionStore.java
Outdated
Show resolved
Hide resolved
.../java/org/eclipse/edc/connector/store/sql/contractdefinition/SqlContractDefinitionStore.java
Outdated
Show resolved
Hide resolved
.../java/org/eclipse/edc/connector/store/sql/contractdefinition/SqlContractDefinitionStore.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/eclipse/edc/connector/contract/spi/types/offer/ContractDefinitionTest.java
Outdated
Show resolved
Hide resolved
@mohannad-ezzo two more things:
|
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3534 +/- ##
==========================================
- Coverage 72.49% 72.32% -0.17%
==========================================
Files 877 880 +3
Lines 17474 17729 +255
Branches 994 1021 +27
==========================================
+ Hits 12667 12822 +155
- Misses 4391 4471 +80
- Partials 416 436 +20
☔ 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.
generally, I'd follow the SqlTransferProcessStore
approach, putting the properties into a JSON column into the same entity table, it will save a lot of code lines and unneeded overcomplication.
.../eclipse/edc/connector/api/management/contractdefinition/ContractDefinitionApiExtension.java
Outdated
Show resolved
Hide resolved
...api/management/contractdefinition/transform/JsonObjectFromContractDefinitionTransformer.java
Outdated
Show resolved
Hide resolved
...r/api/management/contractdefinition/transform/JsonObjectToContractDefinitionTransformer.java
Outdated
Show resolved
Hide resolved
...management/contractdefinition/transform/JsonObjectFromContractDefinitionTransformerTest.java
Outdated
Show resolved
Hide resolved
...i/management/contractdefinition/transform/JsonObjectToContractDefinitionTransformerTest.java
Outdated
Show resolved
Hide resolved
property_name VARCHAR(255) NOT NULL, | ||
property_value TEXT NOT NULL, | ||
property_type VARCHAR(255) NOT NULL, | ||
property_is_private BOOLEAN, |
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.
I think this was copied by the sql asset index implementation, but to be fair I'd prefer to follow the transfer process one, where the private properties are serialized into a json field.
.../java/org/eclipse/edc/connector/store/sql/contractdefinition/SqlContractDefinitionStore.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/eclipse/edc/connector/contract/spi/types/offer/ContractDefinitionTest.java
Outdated
Show resolved
Hide resolved
I agree, that is an even easier way way to do it. @mohannad-ezzo can you adapt your PR accordingly? |
@paullatzelsperger @ndr-brt By following the JSON approach, we may loose the flexibility to support search within the private properties across all contract definitions. For sure, for a small set of contract definitions, JSON approach would still work. However, whenever the set becomes bigger and bigger, cross search would be difficult. I still see that the current approach may fit better our current and future requirement. What do you think ? |
@mohannad-ezzo Postgres supports JSON operators, and EDC has a way of converting a |
@paullatzelsperger @ndr-brt ok, I am going to follow the JSON approach of storing the private properties (instead of a having a separate table) and provide you with an update asap. |
I agree with paul, plus, using the "separate table" approach will lead to issues in trying to query nested properties (see #3424 ) |
As discussed I adapted the PR so that JSON is used to store private properties of contract definition instead of storing them in a separate table. |
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.
Just avoid those two not necessary changes to the sql store and it's ready to be merged
.../java/org/eclipse/edc/connector/store/sql/contractdefinition/SqlContractDefinitionStore.java
Outdated
Show resolved
Hide resolved
.../java/org/eclipse/edc/connector/store/sql/contractdefinition/SqlContractDefinitionStore.java
Outdated
Show resolved
Hide resolved
Thank you for mentioning this. This actually was inherited from the old logic when I started to have another table for private properties. Old logic is back now :) |
End-To-End-Tests and Component-Tests failed now. They were ok before the last commit. |
@mohannad-ezzo you need to fix the broken tests and the checkstyle as well |
I just fixed the checksytle issues. Regarding the broken tests, they ran fine this time without any change from me. |
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.
LGTM!
What this PR changes/adds
Add private properties to contract definition entity. The implementation supports in-memory and postgres as a store.
Why it does that
The private properties are required to provide the owner with the possibility to add extra properties which should not be exposed outside. This requirement was initiated when we want to add administrative properties like (createdBy, createdAt, chnagedBy, chnagedAt). We assume that the private properties can be used to store such a requirement after registering a listener on the contract definition.
More information is in this discussion:
#2592 (reply in thread)
Further notes
List other areas of code that have changed but are not necessarily linked to the main feature. This could be method
signature changes, package declarations, bugs that were encountered and were fixed inline, etc.
Linked Issue(s)
Closes #3490