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: add private properties to policy definition entity #3576

Conversation

suh-rao
Copy link
Contributor

@suh-rao suh-rao commented Oct 30, 2023

What this PR changes/adds

Add private properties to policy 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), policyType (Access, usage). We assume that the private properties can be used to store such a requirement after registering a listener on the policy definition.
More information is in this discussion:
#2592 (comment)

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 #3567

@jimmarino
Copy link
Contributor

@suh-rao Can you please not open-and-close PRs like this as it puts an administrative burden on the reviewers? Stick to one PR per topic.

@suh-rao
Copy link
Contributor Author

suh-rao commented Oct 30, 2023

@suh-rao Can you please not open-and-close PRs like this as it puts an administrative burden on the reviewers? Stick to one PR per topic.

@jimmarino : ECA was not updated with old PR even after retrigger. Hence need to close and reopen it again. Will stick to this PR now for the feature

@suh-rao suh-rao changed the title feat: add private properties to contract definition entity feat: add private properties to policy definition entity Oct 30, 2023
@suh-rao suh-rao marked this pull request as ready for review October 30, 2023 09:15
@paullatzelsperger paullatzelsperger removed their request for review October 30, 2023 09:20
@paullatzelsperger
Copy link
Member

Removing my review as I am out of office

@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a3ef0f1) 71.81% compared to head (9e787fc) 71.84%.

Files Patch % Lines
...pse/edc/connector/policy/spi/PolicyDefinition.java 66.66% 3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3576      +/-   ##
==========================================
+ Coverage   71.81%   71.84%   +0.03%     
==========================================
  Files         917      917              
  Lines       18395    18422      +27     
  Branches     1044     1045       +1     
==========================================
+ Hits        13210    13235      +25     
- Misses       4726     4729       +3     
+ Partials      459      458       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jimmarino
Copy link
Contributor

Add @paullatzelsperger to review when he is back since this is an area he has worked on extensively.

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.

still missing:

  • a test where a Policy is queried by a private property
  • E2E tests in the PolicyDefinitionApiEndToEndTest. It's not needed to re-test all test cases, tests for creating policies with private props, querying with private props, one update scenario and a delete scenario would suffice.

@suh-rao
Copy link
Contributor Author

suh-rao commented Nov 6, 2023

still missing:

  • a test where a Policy is queried by a private property
  • E2E tests in the PolicyDefinitionApiEndToEndTest. It's not needed to re-test all test cases, tests for creating policies with private props, querying with private props, one update scenario and a delete scenario would suffice.

a test where a Policy is queried by a private property

I followed approach done for contractdefinition. In this PR can I only do changes regarding storing privateProperties as JSON and query part later in next PR?

@suh-rao
Copy link
Contributor Author

suh-rao commented Nov 7, 2023

still missing:

  • a test where a Policy is queried by a private property
  • E2E tests in the PolicyDefinitionApiEndToEndTest. It's not needed to re-test all test cases, tests for creating policies with private props, querying with private props, one update scenario and a delete scenario would suffice.

a test where a Policy is queried by a private property

I followed approach done for contractdefinition. In this PR can I only do changes regarding storing privateProperties as JSON and query part later in next PR?

Hello, I am unable to add label to pill request.
Also is it ok to have query part later? (As mentioned in above point)

@ndr-brt ndr-brt added the enhancement New feature or request label Nov 8, 2023
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'm ok with tackling the query in a subsequent pr, please open an issue related to it

@ndr-brt ndr-brt added the breaking-change Will require manual intervention for version update label Nov 14, 2023
@suh-rao
Copy link
Contributor Author

suh-rao commented Nov 17, 2023

I'm ok with tackling the query in a subsequent pr, please open an issue related to it

#3623. This is new issue created for search

@suh-rao suh-rao closed this Nov 17, 2023
@suh-rao suh-rao reopened this Nov 17, 2023
@paullatzelsperger
Copy link
Member

I'm ok with tackling the query in a subsequent pr, please open an issue related to it

#3623. This is new issue created for search

Hey @suh-rao thanks for creating the issue, however it's not an adoption request, just a feature request. Adoption requests are for bigger features, for example "Support XYZ Database in EDC". Please adapt the description of #3623 thus.

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.

I already said it in my previous review: please extend PolicyDefinitionApiEndToEndTest, so that it uses private properties, and add corresponding test assertions.

Also, Checkstyle is failing.

@suh-rao
Copy link
Contributor Author

suh-rao commented Nov 20, 2023

I'm ok with tackling the query in a subsequent pr, please open an issue related to it

#3623. This is new issue created for search

Hey @suh-rao thanks for creating the issue, however it's not an adoption request, just a feature request. Adoption requests are for bigger features, for example "Support XYZ Database in EDC". Please adapt the description of #3623 thus.

Changed issue description as "feature request"

@suh-rao
Copy link
Contributor Author

suh-rao commented Nov 23, 2023

I'm ok with tackling the query in a subsequent pr, please open an issue related to it

#3623. This is new issue created for search

Hey @suh-rao thanks for creating the issue, however it's not an adoption request, just a feature request. Adoption requests are for bigger features, for example "Support XYZ Database in EDC". Please adapt the description of #3623 thus.

Hey @paullatzelsperger I updated issue description

I already said it in my previous review: please extend PolicyDefinitionApiEndToEndTest, so that it uses private properties, and add corresponding test assertions.

Also, Checkstyle is failing.

Hey @paullatzelsperger I updated endtoend tests also checkstyle issues. All workflows should be green now. Please have a look at into it.

@paullatzelsperger
Copy link
Member

I'm ok with tackling the query in a subsequent pr, please open an issue related to it

#3623. This is new issue created for search

Hey @suh-rao thanks for creating the issue, however it's not an adoption request, just a feature request. Adoption requests are for bigger features, for example "Support XYZ Database in EDC". Please adapt the description of #3623 thus.

Hey @paullatzelsperger I updated issue description

I already said it in my previous review: please extend PolicyDefinitionApiEndToEndTest, so that it uses private properties, and add corresponding test assertions.
Also, Checkstyle is failing.

Hey @paullatzelsperger I updated endtoend tests also checkstyle issues. All workflows should be green now. Please have a look at into it.

dependency check still failing.

@suh-rao
Copy link
Contributor Author

suh-rao commented Nov 26, 2023

I'm ok with tackling the query in a subsequent pr, please open an issue related to it

#3623. This is new issue created for search

Hey @suh-rao thanks for creating the issue, however it's not an adoption request, just a feature request. Adoption requests are for bigger features, for example "Support XYZ Database in EDC". Please adapt the description of #3623 thus.

Hey @paullatzelsperger I updated issue description

I already said it in my previous review: please extend PolicyDefinitionApiEndToEndTest, so that it uses private properties, and add corresponding test assertions.
Also, Checkstyle is failing.

Hey @paullatzelsperger I updated endtoend tests also checkstyle issues. All workflows should be green now. Please have a look at into it.

dependency check still failing.

In this PR I havent changed any dependencies. So not sure what can be the issue. I synched the branch to main now.

@paullatzelsperger
Copy link
Member

In this PR I havent changed any dependencies. So not sure what can be the issue. I synched the branch to main now.

@suh-rao just take a look at the failing check, you need to replace the contents of your DEPENDENCIES file with whatever the job output says. unfortunately, this can happen without any of your (or our, for that matter) doing.

@suh-rao
Copy link
Contributor Author

suh-rao commented Nov 27, 2023

In this PR I havent changed any dependencies. So not sure what can be the issue. I synched the branch to main now.

@suh-rao just take a look at the failing check, you need to replace the contents of your DEPENDENCIES file with whatever the job output says. unfortunately, this can happen without any of your (or our, for that matter) doing.

@paullatzelsperger All 17 checks passed now. Sync to main branch resolved the issue with dependency.

@suh-rao
Copy link
Contributor Author

suh-rao commented Nov 28, 2023

In this PR I havent changed any dependencies. So not sure what can be the issue. I synched the branch to main now.

@suh-rao just take a look at the failing check, you need to replace the contents of your DEPENDENCIES file with whatever the job output says. unfortunately, this can happen without any of your (or our, for that matter) doing.

@paullatzelsperger All 17 checks passed now. Sync to main branch resolved the issue with dependency.

Hello @paullatzelsperger : can it be merged now? I can work on search by properties feature after that.

@paullatzelsperger paullatzelsperger merged commit 9bde3e9 into eclipse-edc:main Nov 28, 2023
16 checks passed
@suh-rao suh-rao deleted the feat/policy-definition-json-private-properties branch December 5, 2023 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Will require manual intervention for version update enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adoption Request: Extend Policy Definition entity with Private Properties
5 participants