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.

Sign up for GitHub

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

Jump to bottom

feat: add contract definition empty asset selector validator #1786

Merged
merged 17 commits into from
Feb 13, 2025
Merged

feat: add contract definition empty asset selector validator #1786

merged 17 commits into from
Feb 13, 2025

Conversation

rafaelmag110
Copy link
Contributor

@rafaelmag110 rafaelmag110 commented Feb 5, 2025 •
edited
Loading

WHAT

Adds a validator that replaces the existing contract definition validator for the data management API.

WHY

Implementation of #1774

Closes #1773

@rafaelmag110 rafaelmag110 changed the title Empty asset selector validator feat: add contract definition empty asset selector validator Feb 5, 2025
@rafaelmag110 rafaelmag110 added the enhancement New feature or request label Feb 5, 2025
@gerbigf
Copy link
Contributor

gerbigf commented Feb 6, 2025 •
edited
Loading

Have we already discussed to enable that per default for Tractus-X? I would heavily support that...

@bmg13
Copy link
Contributor

bmg13 commented Feb 7, 2025 •
edited
Loading

Have we already discussed to enable that per default for Tractus-X? I would heavily support that...

Believe during technical discussion it was set as default disabled to avoid adding breaking changes. Additionally as default it is seen as a "querying" approach, i. e., a filtering view in each no value in asset selector means no filter. Changing this would be adopter's choice.

@ndr-brt
Copy link
Contributor

ndr-brt commented Feb 7, 2025

Have we already discussed to enable that per default for Tractus-X? I would heavily support that...

In the DR we agreed on "off by default": https://github.com/eclipse-tractusx/tractusx-edc/tree/main/docs/development/decision-records/2025-01-30-empty-asset-selector-validator

BTW I don't have strong feelings here

lgblaumeiser
lgblaumeiser approved these changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@lgblaumeiser lgblaumeiser left a comment

Choose a reason for hiding this comment

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

I support Felix remark and have another documentation issue. In general, I think, this PR looks good to me, so I approve it already, although I would appreciate if you could improve the documentation remarks!

...sset-selector/src/main/java/org/eclipse/tractusx/edc/validators/emptyassetselector/README.md Outdated Show resolved Hide resolved
...sset-selector/src/main/java/org/eclipse/tractusx/edc/validators/emptyassetselector/README.md Outdated Show resolved Hide resolved
@rafaelmag110
Copy link
Contributor Author

rafaelmag110 commented Feb 11, 2025 •
edited
Loading

@gerbigf I added your remarks.
@lgblaumeiser Thanks for approving! I took the effort of reformulating the docs as you suggested. Please give it a second look in the last commit and let me know your thoughts.
@ndr-brt I added your suggestions too and left the ongoing discussion open.

@rafaelmag110 rafaelmag110 force-pushed the empty_asset_selector_validator branch from 434346c to 4c8cdc0 Compare February 12, 2025 16:43
@rafaelmag110
Copy link
Contributor Author

@ndr-brt
I have removed the bypass. Please have a second look and merge it if it's good to go.

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@rafaelmag110 rafaelmag110 merged commit 9b8d662 into eclipse-tractusx:main Feb 13, 2025
33 checks passed
@rafaelmag110 rafaelmag110 deleted the empty_asset_selector_validator branch February 13, 2025 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gerbigf gerbigf gerbigf left review comments

@ndr-brt ndr-brt ndr-brt approved these changes

@lgblaumeiser lgblaumeiser lgblaumeiser approved these changes

@bmg13 bmg13 bmg13 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
EDC PR Board
Status: Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Block contract definitions with empty assetSelector
5 participants
@rafaelmag110 @gerbigf @bmg13 @ndr-brt @lgblaumeiser