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

HV-1946 Add an empty path node for iterable/multivalued container elements #1316

Merged
merged 1 commit into from
Jul 21, 2024

Conversation

marko-bekhta
Copy link
Member

@gsmet
Copy link
Member

gsmet commented Jun 16, 2023

We probably should discuss this patch together as the primary issue was pushing a null value. I'm not sure if we should support that but would need you to refresh my memories :).

Let's try to organize a call soon to discuss that.

@marko-bekhta
Copy link
Member Author

Returning to this one ... since having a null node name passed by a value extractor leads to problems with paths, we could either add a "fake" null node to the path which will serve similarly as, e.g. <iterable element> (used for some built-in ones), or we don't allow nulls and force a value to be passed...
but looking at the spec https://jakarta.ee/specifications/bean-validation/3.1/jakarta-validation-spec-3.1#valueextractordefinition or JavaDocs to be more precise:

  • @param nodeName the name of the node representing the container element. If not
    * {@code null}, the name will be used when adding a container element node to the
    * {@link Path}

It means we have to accept the nulls 😕 🫤. Hence we are only left with an option to somehow fix the null case.
I've looked at the patch once more and it seems to make sense. We add a "non-name" node to the path to represent the container element. If we don't have any container element node but are working with container elements, we end up modifying the container node itself, which means that the path becomes incorrect.

Sooooo, I'd suggest merging this in for 9.0 and maybe adding a note to the migration guide stating that the path behaviour now may be different. But maybe you have some other suggestions or ideas for trying to address this case @gsmet @yrodiere ?

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

It's tested, it's compliant with the spec -- that's enough for me.

+1 to merge.

@marko-bekhta marko-bekhta merged commit 58006f5 into hibernate:main Jul 21, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants