Skip to content

feat: make data address handle complex properties #5012

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
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat: make data address handle complex properties #5012

wants to merge 12 commits into from

Conversation

rafaelmag110
Copy link
Contributor

@rafaelmag110 rafaelmag110 commented May 25, 2025 •
edited
Loading

What this PR changes/adds

Changes DataAddress TO and FROM transformers to allow complex attributes such as json objects or json arrays to be used.

Who will sponsor this feature?

@jimmarino
@ndr-brt

Linked Issue(s)

Closes #4986

@bmg13 bmg13 added the enhancement New feature or request label May 26, 2025
bmg13
bmg13 reviewed May 26, 2025
View reviewed changes
Copy link
Contributor

@bmg13 bmg13 left a comment

Choose a reason for hiding this comment

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

just very small things, looks good

ndr-brt
ndr-brt requested changes May 27, 2025
View reviewed changes
Copy link
Member

@ndr-brt ndr-brt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'd also add an e2e track on this, like in the AssetApiEndToEndTest, no need for an explicit test, just adding a complex property in the data address there and verifying that the output is consistent would be enough

@rafaelmag110 rafaelmag110 requested review from bmg13 and ndr-brt May 28, 2025 16:47
@rafaelmag110
Copy link
Contributor Author

@ndr-brt @bmg13
Thanks for the initial reviews. I've changed the PR according your suggestions.

ndr-brt
ndr-brt reviewed May 29, 2025
View reviewed changes
...in/java/org/eclipse/edc/transform/transformer/edc/to/JsonObjectToDataAddressTransformer.java Outdated
Comment on lines 55 to 57
LinkedHashMap<String, Object> complex = new LinkedHashMap<>();
visitProperties(props, (k, v) -> complex.put(k, transformGenericProperty(v, context)));
builder.property(key, complex);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this logic will support a second level of nested properties.
Could this be handled like it has been done in the JsonObjectToAssetTransformer? That one should already handle such case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure how to handle it like it's done in the JsonObjectToAssetTransformer, so didn't do it that way.
I gave it a try nevertheless. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

in the other transformer it just uses transformGenericProperty, that deserialize the whole json object into a map, that should be enough right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. What I was trying to achieve is a deserialization where the values wouldn't be wrapped into lists of value maps (as it happens with the deserialization using transformGenericProperty). But since it is done on another places too, i'll leave it be.

Copy link
Contributor Author

@rafaelmag110 rafaelmag110 May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Another thing that's overlooked in the JsonObjectToAssetTransformer is that JsonLD keywords in nested maps will not be ignored as in the root map, since the transformGenericProperty will eat everything as is. If this is not intended it should be fixed...

Copy link
Member

Choose a reason for hiding this comment

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

@rafaelmag110 what do you mean with "ignored"?
the workaround to transform a single value array into a string is indeed a workaround, so is ok if it's applied only on top level.

@rafaelmag110 rafaelmag110 requested a review from ndr-brt May 29, 2025 14:53
@rafaelmag110 rafaelmag110 requested a review from jimmarino as a code owner May 29, 2025 17:10
@rafaelmag110 rafaelmag110 requested a review from jimmarino June 2, 2025 18:06
ndr-brt
ndr-brt approved these changes Jun 9, 2025
View reviewed changes
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.

checkstyle has been updated, if you rebase the branch it should execute correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@paullatzelsperger paullatzelsperger Awaiting requested review from paullatzelsperger paullatzelsperger is a code owner

@ronjaquensel ronjaquensel Awaiting requested review from ronjaquensel ronjaquensel is a code owner

@juliapampus juliapampus Awaiting requested review from juliapampus juliapampus is a code owner

@bmg13 bmg13 Awaiting requested review from bmg13

@jimmarino jimmarino Awaiting requested review from jimmarino

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DataAddress should support complex attributes
4 participants
@rafaelmag110 @jimmarino @ndr-brt @bmg13