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: PolicyMonitorStore SQL implementation #3507

Merged

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Oct 4, 2023

What this PR changes/adds

Provide sql implementation for PolicyManagerStore

Why it does that

policy-manager

Further notes

  • refactored in-memory stores and statements to avoid duplication

Linked Issue(s)

Closes #3447

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

@ndr-brt ndr-brt added the enhancement New feature or request label Oct 4, 2023
@ndr-brt ndr-brt requested a review from wolf4ood October 4, 2023 07:59
return "edc_policy_monitor";
}

default String getIdColumn() {

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
StatefulEntityStatements.getIdColumn
; it is advisable to add an Override annotation.

default String getErrorDetailColumn() {
return "error_detail";
default String getIdColumn() {

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
StatefulEntityStatements.getIdColumn
; it is advisable to add an Override annotation.
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2023

Codecov Report

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

Comparison is base (00e50d2) 72.08% compared to head (287f3ed) 72.07%.
Report is 12 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3507      +/-   ##
==========================================
- Coverage   72.08%   72.07%   -0.01%     
==========================================
  Files         841      841              
  Lines       16996    17030      +34     
  Branches      951      956       +5     
==========================================
+ Hits        12251    12275      +24     
- Misses       4341     4351      +10     
  Partials      404      404              
Files Coverage Δ
...eclipse/edc/sql/translation/SqlQueryStatement.java 91.83% <100.00%> (-6.21%) ⬇️
...ql/assetindex/schema/BaseSqlDialectStatements.java 95.38% <100.00%> (-4.62%) ⬇️
...negotiation/store/SqlContractNegotiationStore.java 86.43% <100.00%> (-0.07%) ⬇️
...on/store/schema/ContractNegotiationStatements.java 100.00% <ø> (ø)
...re/schema/postgres/ContractNegotiationMapping.java 100.00% <100.00%> (ø)
...transferprocess/store/SqlTransferProcessStore.java 88.10% <100.00%> (-0.07%) ⬇️
...s/store/schema/TransferProcessStoreStatements.java 100.00% <100.00%> (ø)
.../store/schema/postgres/TransferProcessMapping.java 100.00% <100.00%> (ø)
...nnector/dataplane/store/sql/SqlDataPlaneStore.java 90.72% <100.00%> (-0.10%) ⬇️
...e/store/sql/schema/BaseSqlDataPlaneStatements.java 100.00% <ø> (+2.27%) ⬆️
... and 20 more

... and 57 files with indirect coverage changes

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

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.

LGTM, but quite a substantial amount of the changeset comes from seemingly unrelated changes, i.e. refactoring of the in-mem stuff.

I think it would have been easier to review, if that had been in a separate PR. No need to revert now, just something to keep in mind for the future.

@ndr-brt
Copy link
Member Author

ndr-brt commented Oct 5, 2023

@paullatzelsperger yes, you're right, I noted a lot of related things on my todo list and this PR seemed to be the right place to implement them, with hindsight, a different PR would had made sense.

@paullatzelsperger
Copy link
Member

@paullatzelsperger yes, you're right, I noted a lot of related things on my todo list and this PR seemed to be the right place to implement them, with hindsight, a different PR would had made sense.

understandable.

Copy link
Contributor

@wolf4ood wolf4ood left a comment

Choose a reason for hiding this comment

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

LGTM just a minor comment

try (var connection = getConnection()) {
var existing = findByIdInternal(connection, entity.getId());
if (existing != null) {
leaseContext.by(leaseHolderName).withConnection(connection).breakLease(entity.getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

minor

leaseContext.by(leaseHolderName) could be a noop since the leaseHolder should not change right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we do this in all the Stores, I guess this is something coming from the past, because as we decided, all the entity fetched to be modified should be leased. I would keep it for consistency but it could a good refactor to do on all the stores.

The more I think about it, the more I think it does not make any sense 🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

🤣

@ndr-brt ndr-brt merged commit 40216e3 into eclipse-edc:main Oct 6, 2023
16 checks passed
@ndr-brt ndr-brt deleted the 3447-policymonitor-postgres-1 branch October 6, 2023 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

policy-monitor: sql storage
4 participants