Skip to content

HHH-18198 - Remove @org.hibernate.annotations.Target #10040

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

Merged
merged 1 commit into from
Apr 22, 2025

Conversation

jrenaat
Copy link
Member

@jrenaat jrenaat commented Apr 16, 2025

Removed org.hibernate.annotations.Target
Created new org.hibernate.annotations.TargetEmbeddable


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


https://hibernate.atlassian.net/browse/HHH-18198

@jrenaat jrenaat force-pushed the HHH-18198_removeTarget branch from c651880 to 22ce27a Compare April 16, 2025 16:37
Comment on lines +20 to +26
public @interface TargetEmbeddable {
/**
* The target type for a {@link jakarta.persistence.Embedded} entity property
*/
Class<?> value();
}
Copy link
Member

Choose a reason for hiding this comment

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

I thought we had decided we didn't need this and we were going to just discover the implementing embeddable class automatically like what we do with polymorphic embeddables?

Copy link
Member

Choose a reason for hiding this comment

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

We decided to do both. If there is just one, we should just pick it.

Copy link
Member

Choose a reason for hiding this comment

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

OK

Copy link
Member Author

Choose a reason for hiding this comment

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

We decided to do both. If there is just one, we should just pick it.

Tbh, I cannot find where or how to do this

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, navigating around all this *Binder stuff is a nightmare. I feel ya.

I'd try this (I believe it will work, but not certain):

  1. In PropertyInferredData, just return the declared type (this is the part specifically I am not certain will work).
  2. Move this handling for "concrete type" into EmbeddableBinder.
    1. If there is an explicit TargetEmbeddable annotation, use that
    2. If the declared type is a class, use that
    3. If the declared type is an interface and there is a single concrete implementation of the interface and use that
    4. throw an exception

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, navigating around all this *Binder stuff is a nightmare. I feel ya.

After a number of hours of debugging this stuff, I was ready to tender my resignation ... 😫

Copy link
Member

Choose a reason for hiding this comment

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

Definitely understand :D

Its one of the main things I want to address with the phased approach I want to do in 8 (needs hbm.xml support dropped first).

Copy link
Member

Choose a reason for hiding this comment

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

discover the implementing embeddable class automatically

So it turns out this won't work without a change to hibernate-models. Currently it tracks {class -> subclasses}, but that does not account for interfaces. We'd need to add that capability for this to work fully.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

So let's break this into phases...

  • 7.0 - @TargetEmbeddable
  • 7.1 - implicit determination (hibernate-models change)

private Class<?> value;

/**
* Used in creating dynamic annotation instances (e.g. from XML)
*/
public TargetLegacyAnnotation(ModelsContext modelContext) {
public TargetEmbeddableAnnotation(ModelsContext modelContext) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'modelContext' is never used.
}

/**
* Used in creating annotation instances from JDK variant
*/
public TargetLegacyAnnotation(Target annotation, ModelsContext modelContext) {
public TargetEmbeddableAnnotation(TargetEmbeddable annotation, ModelsContext modelContext) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'modelContext' is never used.
this.value = annotation.value();
}

/**
* Used in creating annotation instances from Jandex variant
*/
public TargetLegacyAnnotation(Map<String, Object> attributeValues, ModelsContext modelContext) {
public TargetEmbeddableAnnotation(Map<String, Object> attributeValues, ModelsContext modelContext) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'modelContext' is never used.
@jrenaat jrenaat force-pushed the HHH-18198_removeTarget branch 2 times, most recently from aff7bd4 to 1fa01d0 Compare April 17, 2025 17:05
@jrenaat jrenaat force-pushed the HHH-18198_removeTarget branch from 1fa01d0 to ab3b6e1 Compare April 17, 2025 20:20
Removed org.hibernate.annotations.Target
Created new org.hibernate.annotations.TargetEmbeddable

Signed-off-by: Jan Schatteman <[email protected]>
@jrenaat jrenaat force-pushed the HHH-18198_removeTarget branch from ab3b6e1 to f09f0ed Compare April 18, 2025 20:16
@sebersole sebersole merged commit 50a4444 into hibernate:main Apr 22, 2025
25 checks passed
@jrenaat jrenaat deleted the HHH-18198_removeTarget branch April 22, 2025 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants