HHH-19662 - Define granularity of EnhancementContext options matching reality

HHH-19168 - Disallow re-enhancement of entities with different configuration

# Conflicts:
#	hibernate-core/src/main/java/org/hibernate/bytecode/enhance/spi/EnhancementContext.java

Author: sebersole

hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/BiDirectionalAssociationHandler.java

@@ -47,7 +47,7 @@ static Implementation wrap(
 			ByteBuddyEnhancementContext enhancementContext,
 			AnnotatedFieldDescription persistentField,
 			Implementation implementation) {
-		if ( !enhancementContext.doBiDirectionalAssociationManagement( persistentField ) ) {
+		if ( !enhancementContext.doBiDirectionalAssociationManagement() ) {
 			return implementation;
 		}
 

hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/ByteBuddyEnhancementContext.java

@@ -51,12 +51,12 @@ public boolean isMappedSuperclassClass(TypeDescription classDescriptor) {
 		return enhancementContext.isMappedSuperclassClass( new UnloadedTypeDescription( classDescriptor ) );
 	}
 
-	public boolean doDirtyCheckingInline(TypeDescription classDescriptor) {
-		return enhancementContext.doDirtyCheckingInline( new UnloadedTypeDescription( classDescriptor ) );
+	public boolean doDirtyCheckingInline() {
+		return enhancementContext.doDirtyCheckingInline();
 	}
 
-	public boolean doExtendedEnhancement(TypeDescription classDescriptor) {
-		return enhancementContext.doExtendedEnhancement( new UnloadedTypeDescription( classDescriptor ) );
+	public boolean doExtendedEnhancement() {
+		return enhancementContext.doExtendedEnhancement();
 	}
 
 	public boolean hasLazyLoadableAttributes(TypeDescription classDescriptor) {
@@ -83,8 +83,8 @@ public boolean isMappedCollection(AnnotatedFieldDescription field) {
 		return enhancementContext.isMappedCollection( field );
 	}
 
-	public boolean doBiDirectionalAssociationManagement(AnnotatedFieldDescription field) {
-		return enhancementContext.doBiDirectionalAssociationManagement( field );
+	public boolean doBiDirectionalAssociationManagement() {
+		return enhancementContext.doBiDirectionalAssociationManagement();
 	}
 
 	public boolean isDiscoveredType(TypeDescription typeDescription) {

hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java

@@ -30,7 +30,6 @@
 import net.bytebuddy.implementation.FixedValue;
 import net.bytebuddy.implementation.Implementation;
 import net.bytebuddy.implementation.StubMethod;
-
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.hibernate.AssertionFailure;
 import org.hibernate.Version;
@@ -76,15 +75,17 @@
 import static net.bytebuddy.matcher.ElementMatchers.isStatic;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.not;
+import static org.hibernate.bytecode.enhance.internal.bytebuddy.FeatureMismatchException.Feature.ASSOCIATION_MANAGEMENT;
+import static org.hibernate.bytecode.enhance.internal.bytebuddy.FeatureMismatchException.Feature.DIRTY_CHECK;
 
 public class EnhancerImpl implements Enhancer {
-
 	private static final CoreMessageLogger log = CoreLogging.messageLogger( Enhancer.class );
 
 	protected final ByteBuddyEnhancementContext enhancementContext;
 	private final ByteBuddyState byteBuddyState;
 	private final EnhancerClassLocator typePool;
 	private final EnhancerImplConstants constants;
+	private final List<? extends Annotation> infoAnnotationList;
 
 	/**
 	 * Constructs the Enhancer, using the given context.
@@ -109,8 +110,11 @@ public EnhancerImpl(final EnhancementContext enhancementContext, final ByteBuddy
 		this.byteBuddyState = Objects.requireNonNull( byteBuddyState );
 		this.typePool = Objects.requireNonNull( classLocator );
 		this.constants = byteBuddyState.getEnhancerConstants();
+
+		this.infoAnnotationList = List.of( createInfoAnnotation( enhancementContext ) );
 	}
 
+
 	/**
 	 * Performs the enhancement.
 	 *
@@ -133,7 +137,7 @@ public byte[] enhance(String className, byte[] originalBytes) throws Enhancement
 			return byteBuddyState.rewrite( typePool, safeClassName, byteBuddy -> doEnhance(
 					() -> byteBuddy.ignore( isDefaultFinalizer() )
 							.redefine( typeDescription, typePool.asClassFileLocator() )
-							.annotateType( constants.HIBERNATE_VERSION_ANNOTATION ),
+							.annotateType( infoAnnotationList ),
 					typeDescription
 			) );
 		}
@@ -167,14 +171,17 @@ public void discoverTypes(String className, byte[] originalBytes) {
 	}
 
 	private DynamicType.Builder<?> doEnhance(Supplier<DynamicType.Builder<?>> builderSupplier, TypeDescription managedCtClass) {
-		// skip if the class was already enhanced. This is very common in WildFly as classloading is highly concurrent.
-		// We need to ensure that no class is instrumented multiple times as that might result in incorrect bytecode.
-		// N.B. there is a second check below using a different approach: checking for the marker interfaces,
-		// which does not address the case of extended bytecode enhancement
-		// (because it enhances classes that do not end up with these marker interfaces).
-		// I'm currently inclined to keep both checks, as one is safer and the other has better backwards compatibility.
-		if ( managedCtClass.getDeclaredAnnotations().isAnnotationPresent( EnhancementInfo.class ) ) {
-			verifyVersions( managedCtClass, enhancementContext );
+		if ( alreadyEnhanced( managedCtClass ) ) {
+			// the class already implements `Managed`.  there are 2 broad cases -
+			//		1. the user manually implemented `Managed`
+			//		2. the class was previously enhanced
+			// in either case, look for `@EnhancementInfo` and, if found, verify we can "re-enhance" the class
+			final AnnotationDescription.Loadable<EnhancementInfo> infoAnnotation = managedCtClass.getDeclaredAnnotations().ofType( EnhancementInfo.class );
+			if ( infoAnnotation != null ) {
+				// throws an exception if there is a mismatch...
+				verifyReEnhancement( managedCtClass, infoAnnotation.load(), enhancementContext );
+			}
+			// verification succeeded (or not done) - we can simply skip the enhancement
 			log.tracef( "Skipping enhancement of [%s]: it's already annotated with @EnhancementInfo", managedCtClass.getName() );
 			return null;
 		}
@@ -191,14 +198,6 @@ private DynamicType.Builder<?> doEnhance(Supplier<DynamicType.Builder<?>> builde
 			return null;
 		}
 
-		// handle already enhanced classes
-		if ( alreadyEnhanced( managedCtClass ) ) {
-			verifyVersions( managedCtClass, enhancementContext );
-
-			log.tracef( "Skipping enhancement of [%s]: it's already implementing 'Managed'", managedCtClass.getName() );
-			return null;
-		}
-
 		if ( enhancementContext.isEntityClass( managedCtClass ) ) {
 			if ( checkUnsupportedAttributeNaming( managedCtClass, enhancementContext ) ) {
 				// do not enhance classes with mismatched names for PROPERTY-access persistent attributes
@@ -258,7 +257,7 @@ private DynamicType.Builder<?> doEnhance(Supplier<DynamicType.Builder<?>> builde
 
 			builder = addInterceptorHandling( builder, managedCtClass );
 
-			if ( enhancementContext.doDirtyCheckingInline( managedCtClass ) ) {
+			if ( enhancementContext.doDirtyCheckingInline() ) {
 				List<AnnotatedFieldDescription> collectionFields = collectCollectionFields( managedCtClass );
 
 				if ( collectionFields.isEmpty() ) {
@@ -390,7 +389,7 @@ else if ( enhancementContext.isCompositeClass( managedCtClass ) ) {
 			builder = builder.implement( ManagedComposite.class );
 			builder = addInterceptorHandling( builder, managedCtClass );
 
-			if ( enhancementContext.doDirtyCheckingInline( managedCtClass ) ) {
+			if ( enhancementContext.doDirtyCheckingInline() ) {
 				builder = builder.implement( CompositeTracker.class )
 						.defineField(
 								EnhancerConstants.TRACKER_COMPOSITE_FIELD_NAME,
@@ -428,7 +427,7 @@ else if ( enhancementContext.isMappedSuperclassClass( managedCtClass ) ) {
 			builder = builder.implement( ManagedMappedSuperclass.class );
 			return createTransformer( managedCtClass ).applyTo( builder );
 		}
-		else if ( enhancementContext.doExtendedEnhancement( managedCtClass ) ) {
+		else if ( enhancementContext.doExtendedEnhancement() ) {
 			log.tracef( "Extended enhancement of [%s]", managedCtClass.getName() );
 			return createTransformer( managedCtClass ).applyExtended( builderSupplier.get() );
 		}
@@ -438,6 +437,36 @@ else if ( enhancementContext.doExtendedEnhancement( managedCtClass ) ) {
 		}
 	}
 
+	private void verifyReEnhancement(
+			TypeDescription managedCtClass,
+			EnhancementInfo existingInfo,
+			ByteBuddyEnhancementContext enhancementContext) {
+		// first, make sure versions match
+		final String enhancementVersion = existingInfo.version();
+		if ( "ignore".equals( enhancementVersion ) ) {
+			// for testing
+			log.debugf( "Skipping re-enhancement version check for %s due to `ignore`", managedCtClass.getName() );
+		}
+		else if ( !Version.getVersionString().equals( enhancementVersion ) ) {
+			throw new VersionMismatchException( managedCtClass, enhancementVersion, Version.getVersionString() );
+		}
+
+		FeatureMismatchException.checkFeatureEnablement(
+				managedCtClass,
+				DIRTY_CHECK,
+				enhancementContext.doDirtyCheckingInline(),
+				existingInfo.includesDirtyChecking()
+		);
+
+		FeatureMismatchException.checkFeatureEnablement(
+				managedCtClass,
+				ASSOCIATION_MANAGEMENT,
+				enhancementContext.doBiDirectionalAssociationManagement(),
+				existingInfo.includesAssociationManagement()
+		);
+	}
+
+
 	/**
 	 * Utility that determines the access-type of a mapped class based on an explicit annotation
 	 * or guessing it from the placement of its identifier property. Implementation should be
@@ -629,31 +658,6 @@ private static boolean checkUnsupportedAttributeNaming(TypeDescription managedCt
 		return new String( chars );
 	}
 
-	private static void verifyVersions(TypeDescription managedCtClass, ByteBuddyEnhancementContext enhancementContext) {
-		final AnnotationDescription.Loadable<EnhancementInfo> existingInfo = managedCtClass
-				.getDeclaredAnnotations()
-				.ofType( EnhancementInfo.class );
-		if ( existingInfo == null ) {
-			// There is an edge case here where a user manually adds `implement Managed` to
-			// their domain class, in which case there will most likely not be a
-			// `EnhancementInfo` annotation.  Such cases should simply not do version checking.
-			//
-			// However, there is also ambiguity in this case with classes that were enhanced
-			// with old versions of Hibernate which did not add that annotation as part of
-			// enhancement.  But overall we consider this condition to be acceptable
-			return;
-		}
-
-		final String enhancementVersion = extractVersion( existingInfo );
-		if ( !Version.getVersionString().equals( enhancementVersion ) ) {
-			throw new VersionMismatchException( managedCtClass, enhancementVersion, Version.getVersionString() );
-		}
-	}
-
-	private static String extractVersion(AnnotationDescription.Loadable<EnhancementInfo> annotation) {
-		return annotation.load().version();
-	}
-
 	private PersistentAttributeTransformer createTransformer(TypeDescription typeDescription) {
 		return PersistentAttributeTransformer.collectPersistentFields( typeDescription, enhancementContext, typePool );
 	}
@@ -872,4 +876,42 @@ else if ( access != null && access.load().value() == AccessType.FIELD ) {
 		}
 	}
 
+
+	private static EnhancementInfo createInfoAnnotation(EnhancementContext enhancementContext) {
+		return new EnhancementInfoImpl( enhancementContext.doDirtyCheckingInline(), enhancementContext.doBiDirectionalAssociationManagement() );
+	}
+
+	private static class EnhancementInfoImpl implements EnhancementInfo {
+		private final String version;
+		private final boolean includesDirtyChecking;
+		private final boolean includesAssociationManagement;
+
+		public EnhancementInfoImpl(boolean includesDirtyChecking, boolean includesAssociationManagement) {
+			this.version = Version.getVersionString();
+			this.includesDirtyChecking = includesDirtyChecking;
+			this.includesAssociationManagement = includesAssociationManagement;
+		}
+
+		@Override
+		public String version() {
+			return version;
+		}
+
+		@Override
+		public boolean includesDirtyChecking() {
+			return includesDirtyChecking;
+		}
+
+		@Override
+		public boolean includesAssociationManagement() {
+			return includesAssociationManagement;
+		}
+
+		@Override
+		public Class<? extends Annotation> annotationType() {
+			return EnhancementInfo.class;
+		}
+	}
+
+
 }

hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImplConstants.java

@@ -16,13 +16,10 @@
 import net.bytebuddy.implementation.Implementation;
 import net.bytebuddy.implementation.StubMethod;
 
-import java.lang.annotation.Annotation;
 import java.util.Collection;
 import java.util.List;
 
-import org.hibernate.Version;
 import org.hibernate.bytecode.enhance.spi.CollectionTracker;
-import org.hibernate.bytecode.enhance.spi.EnhancementInfo;
 import org.hibernate.engine.spi.EntityEntry;
 import org.hibernate.engine.spi.ManagedEntity;
 import org.hibernate.engine.spi.PersistentAttributeInterceptor;
@@ -60,17 +57,6 @@ public final class EnhancerImplConstants {
 	//Frequently used annotations, declared as collections as otherwise they get wrapped into them over and over again:
 	final Collection<? extends AnnotationDescription> TRANSIENT_ANNOTATION = List.of(
 			AnnotationDescription.Builder.ofType( Transient.class ).build() );
-	final List<Annotation> HIBERNATE_VERSION_ANNOTATION = List.of( new EnhancementInfo() {
-		@Override
-		public String version() {
-			return Version.getVersionString();
-		}
-
-		@Override
-		public Class<? extends Annotation> annotationType() {
-			return EnhancementInfo.class;
-		}
-	} );
 
 	//Frequently used Types for method signatures:
 	final TypeDefinition TypeVoid = TypeDescription.ForLoadedType.of( void.class );

hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/FeatureMismatchException.java

@@ -0,0 +1,75 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ * Copyright Red Hat Inc. and Hibernate Authors
+ */
+package org.hibernate.bytecode.enhance.internal.bytebuddy;
+
+import net.bytebuddy.description.type.TypeDescription;
+import org.hibernate.bytecode.enhance.spi.EnhancementException;
+import org.hibernate.bytecode.enhance.spi.EnhancementInfo;
+
+import java.util.Locale;
+
+/**
+ * Indicates a mismatch in either {@linkplain EnhancementInfo#includesDirtyChecking() dirty tracking}
+ * or {@linkplain EnhancementInfo#includesAssociationManagement() association management}
+ * between consecutive attempts to enhance a class.
+ *
+ * @author Steve Ebersole
+ */
+public class FeatureMismatchException extends EnhancementException {
+	public enum Feature { DIRTY_CHECK, ASSOCIATION_MANAGEMENT }
+
+	private final String className;
+	private final Feature mismatchedFeature;
+	private final boolean previousValue;
+
+	public FeatureMismatchException(
+			String className,
+			Feature mismatchedFeature,
+			boolean previousValue) {
+		super( String.format(
+				Locale.ROOT,
+				"Support for %s was enabled during enhancement, but `%s` was previously enhanced with that support %s.",
+				featureText( mismatchedFeature ),
+				className,
+				decode( previousValue )
+		) );
+		this.className = className;
+		this.mismatchedFeature = mismatchedFeature;
+		this.previousValue = previousValue;
+	}
+
+	public String getClassName() {
+		return className;
+	}
+
+	public Feature getMismatchedFeature() {
+		return mismatchedFeature;
+	}
+
+	public boolean wasPreviouslyEnabled() {
+		return previousValue;
+	}
+
+	public static void checkFeatureEnablement(
+			TypeDescription managedCtClass,
+			Feature feature,
+			boolean currentlyEnabled,
+			boolean previouslyEnabled) {
+		if ( currentlyEnabled != previouslyEnabled ) {
+			throw new FeatureMismatchException( managedCtClass.getName(), feature, previouslyEnabled );
+		}
+	}
+
+	private static String featureText(Feature mismatchedFeature) {
+		return switch ( mismatchedFeature ) {
+			case DIRTY_CHECK -> "inline dirty checking";
+			case ASSOCIATION_MANAGEMENT -> "bidirectional association management";
+		};
+	}
+
+	private static String decode(boolean previousValue) {
+		return previousValue ? "enabled" : "disabled";
+	}
+}

hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/InlineDirtyCheckingHandler.java

@@ -57,7 +57,7 @@ static Implementation wrap(
 			ByteBuddyEnhancementContext enhancementContext,
 			AnnotatedFieldDescription persistentField,
 			Implementation implementation) {
-		if ( enhancementContext.doDirtyCheckingInline( managedCtClass ) ) {
+		if ( enhancementContext.doDirtyCheckingInline() ) {
 
 			if ( enhancementContext.isCompositeClass( managedCtClass ) ) {
 				implementation = Advice.to( CodeTemplates.CompositeDirtyCheckingHandler.class ).wrap( implementation );

hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/PersistentAttributeTransformer.java

@@ -281,7 +281,7 @@ DynamicType.Builder<?> applyTo(DynamicType.Builder<?> builder) {
 			if ( !compositeOwner
 					&& !enhancementContext.isMappedSuperclassClass( managedCtClass )
 					&& enhancementContext.isCompositeField( enhancedField )
-					&& enhancementContext.doDirtyCheckingInline( managedCtClass ) ) {
+					&& enhancementContext.doDirtyCheckingInline() ) {
 				compositeOwner = true;
 			}
 		}
@@ -296,7 +296,7 @@ DynamicType.Builder<?> applyTo(DynamicType.Builder<?> builder) {
 			}
 		}
 
-		if ( enhancementContext.doExtendedEnhancement( managedCtClass ) ) {
+		if ( enhancementContext.doExtendedEnhancement() ) {
 			builder = applyExtended( builder );
 		}