From b5221e2ec65d0118fb04fcbc496e599066014b7a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= <yoann@hibernate.org>
Date: Fri, 8 Nov 2024 15:11:58 +0100
Subject: [PATCH] HHH-18833 Introduce
 EnhancementContext#getUnsupportedEnhancementStrategy

This method allows custom contexts to pick the behavior they want when
a class contains getters/setters that do not have a matching field,
making enhancement impossible.

Three behaviors are available:

* SKIP (the default), which will skip enhancement of such classes.
* FAIL, which will throw an exception upon encountering such classes.
* LEGACY, which will restore the pre-HHH-16572 behavior.

I do not think LEGACY is useful at the moment, but I wanted to have
that option in case it turns out HHH-16572 does more harm than good in
Quarkus 3.15.
---
 .../ByteBuddyEnhancementContext.java          |   5 +
 .../internal/bytebuddy/EnhancerImpl.java      |  45 ++++--
 .../enhance/spi/EnhancementContext.java       |  12 ++
 .../spi/UnsupportedEnhancementStrategy.java   |  38 +++++
 .../UnsupportedEnhancementStrategyTest.java   | 131 ++++++++++++++++++
 5 files changed, 223 insertions(+), 8 deletions(-)
 create mode 100644 hibernate-core/src/main/java/org/hibernate/bytecode/enhance/spi/UnsupportedEnhancementStrategy.java
 create mode 100644 hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/UnsupportedEnhancementStrategyTest.java

diff --git a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/ByteBuddyEnhancementContext.java b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/ByteBuddyEnhancementContext.java
index 46c3acca09..99288dcdc4 100644
--- a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/ByteBuddyEnhancementContext.java
+++ b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/ByteBuddyEnhancementContext.java
@@ -22,6 +22,7 @@ import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.dynamic.scaffold.MethodGraph;
 import net.bytebuddy.matcher.ElementMatcher;
 import net.bytebuddy.pool.TypePool;
+import org.hibernate.bytecode.enhance.spi.UnsupportedEnhancementStrategy;
 
 import static net.bytebuddy.matcher.ElementMatchers.isGetter;
 
@@ -94,6 +95,10 @@ class ByteBuddyEnhancementContext {
 		enhancementContext.registerDiscoveredType( new UnloadedTypeDescription( typeDescription ), type );
 	}
 
+	public UnsupportedEnhancementStrategy getUnsupportedEnhancementStrategy() {
+		return enhancementContext.getUnsupportedEnhancementStrategy();
+	}
+
 	public void discoverCompositeTypes(TypeDescription managedCtClass, TypePool typePool) {
 		if ( isDiscoveredType( managedCtClass ) ) {
 			return;
diff --git a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java
index 547b1895ab..36310ab84e 100644
--- a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java
+++ b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java
@@ -23,6 +23,7 @@ import net.bytebuddy.implementation.FieldAccessor;
 import net.bytebuddy.implementation.FixedValue;
 import net.bytebuddy.implementation.Implementation;
 import net.bytebuddy.implementation.StubMethod;
+import org.hibernate.AssertionFailure;
 import org.hibernate.Version;
 import org.hibernate.bytecode.enhance.VersionMismatchException;
 import org.hibernate.bytecode.enhance.internal.tracker.CompositeOwnerTracker;
@@ -33,6 +34,7 @@ import org.hibernate.bytecode.enhance.spi.EnhancementInfo;
 import org.hibernate.bytecode.enhance.spi.Enhancer;
 import org.hibernate.bytecode.enhance.spi.EnhancerConstants;
 import org.hibernate.bytecode.enhance.spi.UnloadedField;
+import org.hibernate.bytecode.enhance.spi.UnsupportedEnhancementStrategy;
 import org.hibernate.bytecode.enhance.spi.interceptor.LazyAttributeLoadingInterceptor;
 import org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState;
 import org.hibernate.engine.spi.CompositeOwner;
@@ -171,7 +173,7 @@ public class EnhancerImpl implements Enhancer {
 		}
 
 		if ( enhancementContext.isEntityClass( managedCtClass ) ) {
-			if ( hasUnsupportedAttributeNaming( managedCtClass ) ) {
+			if ( checkUnsupportedAttributeNaming( managedCtClass ) ) {
 				// do not enhance classes with mismatched names for PROPERTY-access persistent attributes
 				return null;
 			}
@@ -335,7 +337,7 @@ public class EnhancerImpl implements Enhancer {
 			return createTransformer( managedCtClass ).applyTo( builder );
 		}
 		else if ( enhancementContext.isCompositeClass( managedCtClass ) ) {
-			if ( hasUnsupportedAttributeNaming( managedCtClass ) ) {
+			if ( checkUnsupportedAttributeNaming( managedCtClass ) ) {
 				// do not enhance classes with mismatched names for PROPERTY-access persistent attributes
 				return null;
 			}
@@ -375,7 +377,7 @@ public class EnhancerImpl implements Enhancer {
 		else if ( enhancementContext.isMappedSuperclassClass( managedCtClass ) ) {
 
 			// Check for HHH-16572 (PROPERTY attributes with mismatched field and method names)
-			if ( hasUnsupportedAttributeNaming( managedCtClass ) ) {
+			if ( checkUnsupportedAttributeNaming( managedCtClass ) ) {
 				return null;
 			}
 
@@ -399,8 +401,22 @@ public class EnhancerImpl implements Enhancer {
 	 * Check whether an entity class ({@code managedCtClass}) has mismatched names between a persistent field and its
 	 * getter/setter when using {@link AccessType#PROPERTY}, which Hibernate does not currently support for enhancement.
 	 * See https://hibernate.atlassian.net/browse/HHH-16572
+	 *
+	 * @return {@code true} if enhancement of the class must be {@link org.hibernate.bytecode.enhance.spi.UnsupportedEnhancementStrategy#SKIP skipped}
+	 * because it has mismatched names.
+	 * {@code false} if enhancement of the class must proceed, either because it doesn't have any mismatched names,
+	 * or because {@link org.hibernate.bytecode.enhance.spi.UnsupportedEnhancementStrategy#LEGACY legacy mode} was opted into.
+	 * @throws EnhancementException if enhancement of the class must {@link org.hibernate.bytecode.enhance.spi.UnsupportedEnhancementStrategy#FAIL abort} because it has mismatched names.
 	 */
-	private boolean hasUnsupportedAttributeNaming(TypeDescription managedCtClass) {
+	@SuppressWarnings("deprecation")
+	private boolean checkUnsupportedAttributeNaming(TypeDescription managedCtClass) {
+		var strategy = enhancementContext.getUnsupportedEnhancementStrategy();
+		if ( UnsupportedEnhancementStrategy.LEGACY.equals( strategy ) ) {
+			// Don't check anything and act as if there was nothing unsupported in the class.
+			// This is unsafe but that's what LEGACY is about.
+			return false;
+		}
+
 		// For process access rules, See https://jakarta.ee/specifications/persistence/3.2/jakarta-persistence-spec-3.2#default-access-type
 		// and https://jakarta.ee/specifications/persistence/3.2/jakarta-persistence-spec-3.2#a122
 		//
@@ -462,10 +478,23 @@ public class EnhancerImpl implements Enhancer {
 					}
 				}
 			}
-			if (propertyHasAnnotation && !propertyNameMatchesFieldName) {
-				log.debugf("Skipping enhancement of [%s]: due to class [%s] not having a property accessor method name matching field name [%s]",
-						managedCtClass, methodDescription.getDeclaringType().getActualName(), methodFieldName);
-				return true;
+			if ( propertyHasAnnotation && !propertyNameMatchesFieldName ) {
+				switch ( strategy ) {
+					case SKIP:
+						log.debugf(
+								"Skipping enhancement of [%s] because no field named [%s] could be found for property accessor method [%s]."
+										+ " To fix this, make sure all property accessor methods have a matching field.",
+								managedCtClass.getName(), methodFieldName, methodDescription.getName() );
+						return true;
+					case FAIL:
+						throw new EnhancementException( String.format(
+								"Enhancement of [%s] failed because no field named [%s] could be found for property accessor method [%s]."
+										+ " To fix this, make sure all property accessor methods have a matching field.",
+								managedCtClass.getName(), methodFieldName, methodDescription.getName() ) );
+					default:
+						// We shouldn't even be in this method if using LEGACY, see top of this method.
+						throw new AssertionFailure( "Unexpected strategy at this point: " + strategy );
+				}
 			}
 		}
 		return false;
diff --git a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/spi/EnhancementContext.java b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/spi/EnhancementContext.java
index 7a68e2bebc..97bef960b2 100644
--- a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/spi/EnhancementContext.java
+++ b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/spi/EnhancementContext.java
@@ -5,6 +5,7 @@
 package org.hibernate.bytecode.enhance.spi;
 
 import jakarta.persistence.metamodel.Type;
+import org.hibernate.Incubating;
 
 /**
  * The context for performing an enhancement.  Enhancement can happen in any number of ways:<ul>
@@ -144,4 +145,15 @@ public interface EnhancementContext {
 	boolean isDiscoveredType(UnloadedClass classDescriptor);
 
 	void registerDiscoveredType(UnloadedClass classDescriptor, Type.PersistenceType type);
+
+	/**
+	 * @return The expected behavior when encountering a class that cannot be enhanced,
+	 * in particular when attribute names don't match field names.
+	 * @see <a href="https://hibernate.atlassian.net/browse/HHH-16572">HHH-16572</a>
+	 * @see <a href="https://hibernate.atlassian.net/browse/HHH-18833">HHH-18833</a>
+	 */
+	@Incubating
+	default UnsupportedEnhancementStrategy getUnsupportedEnhancementStrategy() {
+		return UnsupportedEnhancementStrategy.SKIP;
+	}
 }
diff --git a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/spi/UnsupportedEnhancementStrategy.java b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/spi/UnsupportedEnhancementStrategy.java
new file mode 100644
index 0000000000..e27a065593
--- /dev/null
+++ b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/spi/UnsupportedEnhancementStrategy.java
@@ -0,0 +1,38 @@
+/*
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ * Copyright Red Hat Inc. and Hibernate Authors
+ */
+package org.hibernate.bytecode.enhance.spi;
+
+import org.hibernate.Incubating;
+
+/**
+ * The expected behavior when encountering a class that cannot be enhanced,
+ * in particular when attribute names don't match field names.
+ *
+ * @see org.hibernate.bytecode.enhance.spi.EnhancementContext#getUnsupportedEnhancementStrategy
+ */
+@Incubating
+public enum UnsupportedEnhancementStrategy {
+
+	/**
+	 * When a class cannot be enhanced, skip enhancement for that class only.
+	 */
+	SKIP,
+	/**
+	 * When a class cannot be enhanced, throw an exception with an actionable message.
+	 */
+	FAIL,
+	/**
+	 * Legacy behavior: when a class cannot be enhanced, ignore that fact and try to enhance it anyway.
+	 * <p>
+	 * <strong>This is utterly unsafe and may cause errors, unpredictable behavior, and data loss.</strong>
+	 * <p>
+	 * Intended only for internal use in contexts with rigid backwards compatibility requirements.
+	 *
+	 * @deprecated Use {@link #SKIP} or {@link #FAIL} instead.
+	 */
+	@Deprecated
+	LEGACY
+
+}
diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/UnsupportedEnhancementStrategyTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/UnsupportedEnhancementStrategyTest.java
new file mode 100644
index 0000000000..902b19c959
--- /dev/null
+++ b/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/UnsupportedEnhancementStrategyTest.java
@@ -0,0 +1,131 @@
+/*
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ * Copyright Red Hat Inc. and Hibernate Authors
+ */
+package org.hibernate.orm.test.bytecode.enhancement.access;
+
+import jakarta.persistence.Access;
+import jakarta.persistence.AccessType;
+import jakarta.persistence.Basic;
+import jakarta.persistence.Entity;
+import jakarta.persistence.Id;
+import jakarta.persistence.Table;
+import org.hibernate.bytecode.enhance.internal.bytebuddy.EnhancerImpl;
+import org.hibernate.bytecode.enhance.spi.EnhancementContext;
+import org.hibernate.bytecode.enhance.spi.EnhancementException;
+import org.hibernate.bytecode.enhance.spi.Enhancer;
+import org.hibernate.bytecode.enhance.spi.UnsupportedEnhancementStrategy;
+import org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState;
+import org.hibernate.bytecode.spi.ByteCodeHelper;
+import org.hibernate.testing.bytecode.enhancement.EnhancerTestContext;
+import org.hibernate.testing.orm.junit.JiraKey;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+@JiraKey("HHH-18833")
+public class UnsupportedEnhancementStrategyTest {
+
+	@Test
+	public void skip() throws IOException {
+		var context = new EnhancerTestContext() {
+			@Override
+			public UnsupportedEnhancementStrategy getUnsupportedEnhancementStrategy() {
+				// This is currently the default, but we don't care about what's the default here
+				return UnsupportedEnhancementStrategy.SKIP;
+			}
+		};
+		byte[] originalBytes = getAsBytes( SomeEntity.class );
+		byte[] enhancedBytes = doEnhance( SomeEntity.class, originalBytes, context );
+		assertThat( enhancedBytes ).isNull(); // null means "not enhanced"
+	}
+
+	@Test
+	public void fail() throws IOException {
+		var context = new EnhancerTestContext() {
+			@Override
+			public UnsupportedEnhancementStrategy getUnsupportedEnhancementStrategy() {
+				return UnsupportedEnhancementStrategy.FAIL;
+			}
+		};
+		byte[] originalBytes = getAsBytes( SomeEntity.class );
+		assertThatThrownBy( () -> doEnhance( SomeEntity.class, originalBytes, context ) )
+				.isInstanceOf( EnhancementException.class )
+				.hasMessageContainingAll(
+						String.format(
+								"Enhancement of [%s] failed because no field named [%s] could be found for property accessor method [%s].",
+								SomeEntity.class.getName(), "propertyMethod", "getPropertyMethod" ),
+						"To fix this, make sure all property accessor methods have a matching field."
+				);
+	}
+
+	@Test
+	@SuppressWarnings("deprecation")
+	public void legacy() throws IOException {
+		var context = new EnhancerTestContext() {
+			@Override
+			public UnsupportedEnhancementStrategy getUnsupportedEnhancementStrategy() {
+				// This is currently the default, but we don't care about what's the default here
+				return UnsupportedEnhancementStrategy.LEGACY;
+			}
+		};
+		byte[] originalBytes = getAsBytes( SomeEntity.class );
+		byte[] enhancedBytes = doEnhance( SomeEntity.class, originalBytes, context );
+		assertThat( enhancedBytes ).isNotNull(); // non-null means enhancement _was_ performed
+	}
+
+	private byte[] doEnhance(Class<SomeEntity> someEntityClass, byte[] originalBytes, EnhancementContext context) {
+		final ByteBuddyState byteBuddyState = new ByteBuddyState();
+		final Enhancer enhancer = new EnhancerImpl( context, byteBuddyState );
+		return enhancer.enhance( someEntityClass.getName(), originalBytes );
+	}
+
+	private byte[] getAsBytes(Class<?> clazz) throws IOException {
+		final String classFile = clazz.getName().replace( '.', '/' ) + ".class";
+		try (InputStream classFileStream = clazz.getClassLoader().getResourceAsStream( classFile )) {
+			return ByteCodeHelper.readByteCode( classFileStream );
+		}
+	}
+
+	@Entity
+	@Table(name = "SOME_ENTITY")
+	static class SomeEntity {
+		@Id
+		Long id;
+
+		@Basic
+		String field;
+
+		String property;
+
+		public SomeEntity() {
+		}
+
+		public SomeEntity(Long id, String field, String property) {
+			this.id = id;
+			this.field = field;
+			this.property = property;
+		}
+
+		/**
+		 * The following property accessor methods are purposely named incorrectly to
+		 * not match the "property" field.  The HHH-16572 change ensures that
+		 * this entity is not (bytecode) enhanced.  Eventually further changes will be made
+		 * such that this entity is enhanced in which case the FailureExpected can be removed
+		 * and the cleanup() uncommented.
+		 */
+		@Basic
+		@Access(AccessType.PROPERTY)
+		public String getPropertyMethod() {
+			return "from getter: " + property;
+		}
+
+		public void setPropertyMethod(String property) {
+			this.property = property;
+		}
+	}
+}