Skip to content

Commit

Permalink
HHH-18833 Introduce EnhancementContext#getUnsupportedEnhancementStrategy
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
yrodiere committed Nov 13, 2024
1 parent 0ceada1 commit abfe6b9
Show file tree
Hide file tree
Showing 5 changed files with 225 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
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;

Expand Down Expand Up @@ -106,6 +107,10 @@ public void registerDiscoveredType(TypeDescription typeDescription, Type.Persist
enhancementContext.registerDiscoveredType( new UnloadedTypeDescription( typeDescription ), type );
}

public UnsupportedEnhancementStrategy getUnsupportedEnhancementStrategy() {
return enhancementContext.getUnsupportedEnhancementStrategy();
}

public void discoverCompositeTypes(TypeDescription managedCtClass, TypePool typePool) {
if ( isDiscoveredType( managedCtClass ) ) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
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;
Expand All @@ -35,6 +36,7 @@
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;
Expand Down Expand Up @@ -173,7 +175,7 @@ private DynamicType.Builder<?> doEnhance(Supplier<DynamicType.Builder<?>> builde
}

if ( enhancementContext.isEntityClass( managedCtClass ) ) {
if ( hasUnsupportedAttributeNaming( managedCtClass ) ) {
if ( checkUnsupportedAttributeNaming( managedCtClass ) ) {
// do not enhance classes with mismatched names for PROPERTY-access persistent attributes
return null;
}
Expand Down Expand Up @@ -337,7 +339,7 @@ private DynamicType.Builder<?> doEnhance(Supplier<DynamicType.Builder<?>> builde
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;
}
Expand Down Expand Up @@ -377,7 +379,7 @@ else if ( enhancementContext.isCompositeClass( managedCtClass ) ) {
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;
}

Expand All @@ -401,8 +403,22 @@ else if ( enhancementContext.doExtendedEnhancement( managedCtClass ) ) {
* 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
//
Expand Down Expand Up @@ -464,10 +480,23 @@ else if (methodName.startsWith("get") ||
}
}
}
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,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>
Expand Down Expand Up @@ -146,4 +147,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;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Hibernate, Relational Persistence for Idiomatic Java
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later.
* See the lgpl.txt file in the root directory or <http://www.gnu.org/licenses/lgpl-2.1.html>.
*/
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

}
Original file line number Diff line number Diff line change
@@ -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;
}
}
}

0 comments on commit abfe6b9

Please sign in to comment.