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

fix: permit query Asset json properties #3559

Merged
merged 1 commit into from
Oct 29, 2023

Conversation

ndr-brt
Copy link
Member

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

What this PR changes/adds

Permits to query over nested properties in Asset.
To do that, the notation should be, e.g.:

{
  "edc:operandLeft": "'https://w3id.org/edc/v0.0.1/ns/nested'.'https://w3id.org/edc/v0.0.1/ns/key'",
  "edc:operator": "=",
  "edc:operandRight": "value"
}

to navigate through the property tree, the wrapping ' are needed to clearly separate path items (because keys could contain the .).
Current behavior without wrapping ' for a not nested property is still valid.

Why it does that

fix bug

Further notes

  • refactored the asset related sql tables, now collapsed into a single table with JSON fields. THIS IS A BREAKING CHANGE. please refer to the asset-index-sql extension README to get migration instructions.
  • removed the unused updateDataAddress method from AssetService

Linked Issue(s)

Closes #3423

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

@ndr-brt ndr-brt added bug Something isn't working api Feature related to the (REST) api labels Oct 23, 2023
@ndr-brt ndr-brt requested review from wolf4ood and jimmarino October 23, 2023 12:34
@ndr-brt ndr-brt force-pushed the 3423-asset-query-json branch from 4c48e4f to efcea29 Compare October 23, 2023 12:35
var openingBracketIx = first.toString().indexOf(OPENING_BRACKET);
var closingBracketIx = first.toString().indexOf(CLOSING_BRACKET);
var propName = first.toString().substring(0, openingBracketIx);
var arrayIndex = Integer.parseInt(first.toString().substring(openingBracketIx + 1, closingBracketIx));

Check notice

Code scanning / CodeQL

Missing catch of NumberFormatException Note

Potential uncaught 'java.lang.NumberFormatException'.
@ndr-brt ndr-brt force-pushed the 3423-asset-query-json branch 2 times, most recently from 832f658 to 4169f68 Compare October 23, 2023 13:57
@codecov-commenter
Copy link

Codecov Report

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

Comparison is base (cba9558) 72.49% compared to head (4169f68) 17.55%.
Report is 1 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    #3559       +/-   ##
===========================================
- Coverage   72.49%   17.55%   -54.94%     
===========================================
  Files         877      879        +2     
  Lines       17474    17410       -64     
  Branches      994      988        -6     
===========================================
- Hits        12667     3056     -9611     
- Misses       4391    14267     +9876     
+ Partials      416       87      -329     
Files Coverage Δ
.../edc/connector/service/asset/AssetServiceImpl.java 0.00% <ø> (-97.78%) ⬇️
...efaults/storage/assetindex/InMemoryAssetIndex.java 0.00% <ø> (-94.60%) ⬇️
...r/store/sql/assetindex/schema/AssetStatements.java 40.00% <100.00%> (-60.00%) ⬇️
...ql/assetindex/schema/BaseSqlDialectStatements.java 100.00% <100.00%> (+4.61%) ⬆️
...e/sql/assetindex/schema/postgres/AssetMapping.java 100.00% <100.00%> (ø)
.../connector/store/sql/assetindex/SqlAssetIndex.java 84.21% <92.30%> (-1.30%) ⬇️
.../eclipse/edc/sql/translation/JsonFieldMapping.java 0.00% <0.00%> (ø)
...clipse/edc/sql/translation/TranslationMapping.java 0.00% <0.00%> (-86.67%) ⬇️
.../asset/CriterionToAssetPredicateConverterImpl.java 0.00% <0.00%> (-75.00%) ⬇️
...rg/eclipse/edc/util/reflection/ReflectionUtil.java 0.00% <0.00%> (-80.71%) ⬇️
... and 1 more

... and 569 files with indirect coverage changes

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

@ndr-brt ndr-brt force-pushed the 3423-asset-query-json branch from 4169f68 to f5b05ac Compare October 23, 2023 14:24
@ndr-brt ndr-brt force-pushed the 3423-asset-query-json branch from f5b05ac to 7be0a83 Compare October 24, 2023 10:09
@ndr-brt ndr-brt merged commit 26a6f4e into eclipse-edc:main Oct 29, 2023
16 checks passed
@ndr-brt ndr-brt deleted the 3423-asset-query-json branch October 29, 2023 06:56
@ndr-brt ndr-brt added the breaking-change Will require manual intervention for version update label Nov 14, 2023
@arnoweiss
Copy link

arnoweiss commented Feb 22, 2024

The segments of operandLeft must be in extended form, right? I observed that nothing in there resolves against a @context. This isn't necessarily a problem, but I'll document it that way if you confirm.

@ndr-brt
Copy link
Member Author

ndr-brt commented Feb 23, 2024

The segments of operandLeft must be in extended form, right? I observed that nothing in there resolves against a @context. This isn't necessarily a problem, but I'll document it that way if you confirm.

yes, it must be extended, because it is a simple "value", not an "id"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Feature related to the (REST) api breaking-change Will require manual intervention for version update bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asset query service - JSON
5 participants