HHH-19993 Introduce AnnotationBasedUserType and allow UserType constructor accept MemberDetails as parameter

[quaff]

[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]

@gavinking Please review.

[gavinking]

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.

[quaff]

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.

[quaff]

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?

[yrodiere]

@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.

[gavinking]

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.

[quaff]

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.

hibernate-orm/hibernate-core/src/main/java/org/hibernate/mapping/MappingHelper.java

Lines 74 to 82 in 8ca24d9

	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" );
	}
	}

[gavinking]

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.

[quaff]

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" ); 

[gavinking]

Yes.

[quaff]

Done.

[gavinking]

Suggested change

	throw new AnnotationException( String.format( "'UserType' implementation '%s' implements '%s' but no custom annotation present,"
	+ " please refer to the Javadoc of '%s'.",
	typeInstance.getClass().getName(), AnnotationBasedUserType.class.getName(), UserType.class.getName() ) );
	throw new AnnotationException( String.format( "Custom type '%s' implements 'AnnotationBasedUserType' but no custom type annotation is present",
	typeInstance.getClass().getName() ) );

[gavinking]

Suggested change

	else if ( parameters != null && !parameters.isEmpty() && !( type instanceof AnnotationBasedUserType<?,?> ) ) {
	else if ( parameters != null && !parameters.isEmpty()
	&& !( type instanceof AnnotationBasedUserType<?,?> ) ) {

[gavinking]

Is this an unchecked cast?

[gavinking]

It is. OK, here's my suggestion of how we should write this code, to get much better error messages instead of cryptic CCEs from weird undebuggable byte code. This approach uses wildcard capture and reflection. Since the reflection happens at start time, it should not cause a problem.

	private <T extends Annotation> void initializeType(Properties properties,
			Annotation typeAnnotation,
			MemberDetails memberDetails,
			AnnotationBasedUserType<T, ?> annotationBased) {
		annotationBased.initialize( castAnnotationType( typeAnnotation, annotationBased ),
				new UserTypeCreationContext() {
					@Override
					public MetadataBuildingContext getBuildingContext() {
						return BasicValue.this.getBuildingContext();
					}

					@Override
					public ServiceRegistry getServiceRegistry() {
						return BasicValue.this.getServiceRegistry();
					}

					@Override
					public MemberDetails getMemberDetails() {
						return memberDetails;
					}

					@Override
					public Properties getParameters() {
						return properties;
					}
				} );
	}

	private <T extends Annotation> T castAnnotationType(
			Annotation typeAnnotation,
			AnnotationBasedUserType<T, ?> annotationBased) {
		final var annotationType = annotationBased.getClass();
		for ( var iface: annotationType.getGenericInterfaces() ) {
			if ( iface instanceof ParameterizedType parameterizedType
					&& parameterizedType.getRawType() == AnnotationBasedUserType.class ) {
				final var typeArguments = parameterizedType.getActualTypeArguments();
				if ( typeArguments.length > 0
						&& typeArguments[0] instanceof Class<?> annotationClass ) {
					if ( !annotationClass.isInstance( typeAnnotation ) ) {
						throw new AnnotationException( String.format( "Annotation '%s' is not assignable to '%s'",
								annotationType.getName(), iface.getTypeName() ) );
					}
					@SuppressWarnings("unchecked") // safe, we just checked it
					final var castAnnotation = (T) typeAnnotation;
					return castAnnotation;
				}
			}
		}
		throw new AssertionFailure( "Could not find implementing interface" );
	}

[gavinking]

See the analogous change in this PR:

#11477