Skip to content

Conversation

@quaff
Copy link
Contributor

@quaff quaff commented Dec 16, 2025

[Please describe here what your change is about]


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-19993

@quaff
Copy link
Contributor Author

quaff commented Dec 16, 2025

@gavinking Please review.

* @throws org.hibernate.HibernateException in case an error occurred during initialization, e.g. if
* an implementation can't create a value for the given property type.
*/
void initialize(A annotation, Member member, Properties properties);
Copy link
Member

Choose a reason for hiding this comment

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

Please don't pass Properties here. We're nearing the end of a massive mulityear effort to wean everyone off this terrible addiction to passing strings around everywhere.

Using Properties here makes this new interface almost no better than the ParameterizedType interface it's supposed to be replacing. You're just sorta reintroducing ParameterizedType with a different name and much the same lack of type safety.

What makes most sense is a new Context interface analogous to GeneratorCreationContext.

@yrodiere @sebersole How much of a problem is it for Quarkus to have a Member here? We do pass a Member to AnnotationBasedGenerator.initialize() but perhaps it's a problem there too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please don't pass Properties here. We're nearing the end of a massive mulityear effort to wean everyone off this terrible addiction to passing strings around everywhere.

Removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How much of a problem is it for Quarkus to have a Member here? We do pass a Member to AnnotationBasedGenerator.initialize() but perhaps it's a problem there too.

Currently, this PR accepts Member as constructor's parameter for custom UserType implementation, should I change it to accept MemberDetails instead?

Copy link
Member

Choose a reason for hiding this comment

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

@yrodiere @sebersole How much of a problem is it for Quarkus to have a Member here? We do pass a Member to AnnotationBasedGenerator.initialize() but perhaps it's a problem there too.

Passing a Member should be fine as far as GraalVM is concerned.
The only problems would be:

  1. Retrieving the Member from its Class in the first place. If done early (~Metadata building) it should work, but anything done later could (will) break.
  2. Member just isn't an appropriate representation of attributes on dynamic-Map entities.

I think something like MemberDetails would make more sense indeed, but there may be a better fit for user-facing APIs -- @sebersole would know.

Also... I don't know if it's relevant here, but keep in mind retrieving values using a Member can perform badly. If this Member is intended for retrieving values from the entity, we'd need some other abstraction so that we can hope to leverage org.hibernate.bytecode.spi.ReflectionOptimizer.AccessOptimizer one day. If it's just about looking up the attribute's type, though, we don't need that.

Copy link
Member

@gavinking gavinking Dec 16, 2025

Choose a reason for hiding this comment

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

If this Member is intended for retrieving values from the entity

No, that would definitely be an abuse. It would be used only to reflect on the member and configure the UserType at startup.

I think something like MemberDetails would make more sense indeed

Damn. Because I'm an incredible fucking dumbass, I apparently did not think to declare AnnotationBasedGenerator as @Incubating. 🤬

It would be nice to switch to MemberDetails indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly seems like it would have been better to just have had a getMember() method on GeneratorCreationContext than the current signature of initialize().

getMemberDetails() added.

Copy link
Member

Choose a reason for hiding this comment

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

That looks good.

If you really want your strings, you could also add a getParameters() method to UserTypeCreationContext returning that horrible Properties object, as long as it comes with a big flashing @apiNote that that's no longer the best way to do things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you really want your strings, you could also add a getParameters() method to UserTypeCreationContext returning that horrible Properties object, as long as it comes with a big flashing @apiNote that that's no longer the best way to do things.

Parameters is specific to ParameterizedType, I don't think it's applicable for UserTypeCreationContext.

public static void injectParameters(Object type, Properties parameters) {
if ( type instanceof ParameterizedType parameterizedType ) {
parameterizedType.setParameterValues( parameters == null ? EMPTY_PROPERTIES : parameters );
}
else if ( parameters != null && !parameters.isEmpty() ) {
throw new MappingException( "'UserType' implementation '" + type.getClass().getName()
+ "' does not implement 'ParameterizedType' but parameters were provided" );
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Parameters is specific to ParameterizedType

I don't know what you mean by that.

The @Type annotations and friends all have a parameters member. If we want to keep supporting those, instead of just deprecating them, then we should pass the parameters via the new context object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parameters is specific to ParameterizedType

I don't know what you mean by that.

The @Type annotations and friends all have a parameters member. If we want to keep supporting those, instead of just deprecating them, then we should pass the parameters via the new context object.

Do you mean I need to relax the restriction for AnnotationBasedUserType?

throw new MappingException( "'UserType' implementation '" + type.getClass().getName()	+ "' does not implement 'ParameterizedType' but parameters were provided" ); 
@quaff quaff changed the title HHH-19993 Introduce AnnotationBasedUserType and allow UserType constructor accept Member as parameter HHH-19993 Introduce AnnotationBasedUserType and allow UserType constructor accept MemberDetails as parameter Dec 16, 2025
@quaff quaff force-pushed the HHH-19993 branch 3 times, most recently from f0fbaa4 to 5024e01 Compare December 16, 2025 10:13
…parameter Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants