diff --git a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/FieldAccessEnhancer.java b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/FieldAccessEnhancer.java index 47a92f4db964..096e985102de 100644 --- a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/FieldAccessEnhancer.java +++ b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/FieldAccessEnhancer.java @@ -91,6 +91,11 @@ public void visitFieldInsn(int opcode, String owner, String name, String desc) { ); return; case Opcodes.PUTFIELD: + if ( field.getFieldDescription().isFinal() ) { + // Final fields will only be written to from the constructor, + // so there's no point trying to replace final field writes with a method call. + break; + } methodVisitor.visitMethodInsn( Opcodes.INVOKEVIRTUAL, owner, @@ -103,9 +108,7 @@ public void visitFieldInsn(int opcode, String owner, String name, String desc) { throw new EnhancementException( "Unexpected opcode: " + opcode ); } } - else { - super.visitFieldInsn( opcode, owner, name, desc ); - } + super.visitFieldInsn( opcode, owner, name, desc ); } }; } diff --git a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/PersistentAttributeTransformer.java b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/PersistentAttributeTransformer.java index 1e106de35f90..287ce6b793e4 100644 --- a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/PersistentAttributeTransformer.java +++ b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/PersistentAttributeTransformer.java @@ -6,6 +6,7 @@ */ package org.hibernate.bytecode.enhance.internal.bytebuddy; +import static net.bytebuddy.matcher.ElementMatchers.anyOf; import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; import static net.bytebuddy.matcher.ElementMatchers.not; @@ -15,10 +16,8 @@ import java.util.Collections; import java.util.List; import java.util.Objects; - import javax.persistence.Embedded; -import net.bytebuddy.utility.OpenedClassReader; import org.hibernate.bytecode.enhance.internal.bytebuddy.EnhancerImpl.AnnotatedFieldDescription; import org.hibernate.bytecode.enhance.spi.EnhancerConstants; import org.hibernate.engine.spi.CompositeOwner; @@ -27,8 +26,10 @@ import net.bytebuddy.asm.Advice; import net.bytebuddy.asm.AsmVisitorWrapper; +import net.bytebuddy.asm.ModifierAdjustment; import net.bytebuddy.description.field.FieldDescription; import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.modifier.ModifierContributor; import net.bytebuddy.description.modifier.Visibility; import net.bytebuddy.description.type.TypeDefinition; import net.bytebuddy.description.type.TypeDescription; @@ -42,12 +43,29 @@ import net.bytebuddy.jar.asm.Type; import net.bytebuddy.matcher.ElementMatcher.Junction; import net.bytebuddy.pool.TypePool; +import net.bytebuddy.utility.OpenedClassReader; final class PersistentAttributeTransformer implements AsmVisitorWrapper.ForDeclaredMethods.MethodVisitorWrapper { private static final CoreMessageLogger log = CoreLogging.messageLogger( PersistentAttributeTransformer.class ); private static final Junction NOT_HIBERNATE_GENERATED = not( nameStartsWith( "$$_hibernate_" ) ); + private static final ModifierContributor.ForField REMOVE_FINAL_MODIFIER = new ModifierContributor.ForField() { + @Override + public int getMask() { + return EMPTY_MASK; // Do not add any modifier + } + + @Override + public int getRange() { + return Opcodes.ACC_FINAL; // Remove the "final" modifier + } + + @Override + public boolean isDefault() { + return false; + } + }; private final TypeDescription managedCtClass; @@ -138,7 +156,8 @@ public MethodVisitor wrap( return new MethodVisitor( OpenedClassReader.ASM_API, methodVisitor ) { @Override public void visitFieldInsn(int opcode, String owner, String name, String desc) { - if ( isEnhanced( owner, name, desc ) ) { + AnnotatedFieldDescription enhancedField = getEnhancedField( owner, name, desc ); + if ( enhancedField != null ) { switch ( opcode ) { case Opcodes.GETFIELD: methodVisitor.visitMethodInsn( @@ -150,6 +169,11 @@ public void visitFieldInsn(int opcode, String owner, String name, String desc) { ); return; case Opcodes.PUTFIELD: + if ( enhancedField.getFieldDescription().isFinal() ) { + // Final fields will only be written to from the constructor, + // so there's no point trying to replace final field writes with a method call. + break; + } methodVisitor.visitMethodInsn( Opcodes.INVOKEVIRTUAL, owner, @@ -165,21 +189,31 @@ public void visitFieldInsn(int opcode, String owner, String name, String desc) { }; } - private boolean isEnhanced(String owner, String name, String desc) { + private AnnotatedFieldDescription getEnhancedField(String owner, String name, String desc) { for ( AnnotatedFieldDescription enhancedField : enhancedFields ) { if ( enhancedField.getName().equals( name ) && enhancedField.getDescriptor().equals( desc ) && enhancedField.getDeclaringType().asErasure().getInternalName().equals( owner ) ) { - return true; + return enhancedField; } } - return false; + return null; } DynamicType.Builder applyTo(DynamicType.Builder builder) { boolean compositeOwner = false; builder = builder.visit( new AsmVisitorWrapper.ForDeclaredMethods().invokable( NOT_HIBERNATE_GENERATED, this ) ); + // Remove the final modifier from all enhanced fields, because: + // 1. We sometimes need to write to final fields when they are lazy. + // 2. Those fields are already written to by Hibernate ORM through reflection anyway. + // 3. The compiler already makes sure that final fields are not written to from the user's source code. + List enhancedFieldsAsDefined = new ArrayList<>(); + for ( AnnotatedFieldDescription f : enhancedFields ) { + enhancedFieldsAsDefined.add( f.asDefined() ); + } + builder = builder.visit( new ModifierAdjustment().withFieldModifiers( anyOf( enhancedFieldsAsDefined ), + REMOVE_FINAL_MODIFIER ) ); for ( AnnotatedFieldDescription enhancedField : enhancedFields ) { builder = builder .defineMethod( @@ -187,15 +221,19 @@ DynamicType.Builder applyTo(DynamicType.Builder builder) { enhancedField.getType().asErasure(), Visibility.PUBLIC ) - .intercept( fieldReader( enhancedField ) - ) - .defineMethod( - EnhancerConstants.PERSISTENT_FIELD_WRITER_PREFIX + enhancedField.getName(), - TypeDescription.VOID, - Visibility.PUBLIC - ) - .withParameters( enhancedField.getType().asErasure() ) - .intercept( fieldWriter( enhancedField ) ); + .intercept( fieldReader( enhancedField ) ); + // Final fields will only be written to from the constructor, + // so there's no point trying to replace final field writes with a method call. + if ( !enhancedField.getFieldDescription().isFinal() ) { + builder = builder + .defineMethod( + EnhancerConstants.PERSISTENT_FIELD_WRITER_PREFIX + enhancedField.getName(), + TypeDescription.VOID, + Visibility.PUBLIC + ) + .withParameters( enhancedField.getType().asErasure() ) + .intercept( fieldWriter( enhancedField ) ); + } if ( !compositeOwner && !enhancementContext.isMappedSuperclassClass( managedCtClass ) diff --git a/hibernate-core/src/test/java/org/hibernate/test/bytecode/enhancement/basic/FinalFieldEnhancementTest.java b/hibernate-core/src/test/java/org/hibernate/test/bytecode/enhancement/basic/FinalFieldEnhancementTest.java new file mode 100644 index 000000000000..2bb38af83917 --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/test/bytecode/enhancement/basic/FinalFieldEnhancementTest.java @@ -0,0 +1,297 @@ +/* + * 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 . + */ +package org.hibernate.test.bytecode.enhancement.basic; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.Serializable; +import java.util.Objects; +import java.util.UUID; +import javax.persistence.Embeddable; +import javax.persistence.Embedded; +import javax.persistence.EmbeddedId; +import javax.persistence.Entity; +import javax.persistence.GeneratedValue; +import javax.persistence.Id; + +import org.hibernate.annotations.Immutable; + +import org.hibernate.testing.bytecode.enhancement.BytecodeEnhancerRunner; +import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase; +import org.junit.Test; +import org.junit.runner.RunWith; + +@RunWith(BytecodeEnhancerRunner.class) +public class FinalFieldEnhancementTest extends BaseCoreFunctionalTestCase { + + @Override + protected Class[] getAnnotatedClasses() { + return new Class[] { + EntityWithFinalField.class, + EntityWithEmbeddedIdWithFinalField.class, EntityWithEmbeddedIdWithFinalField.EmbeddableId.class, + EntityWithEmbeddedNonIdWithFinalField.class, EntityWithEmbeddedNonIdWithFinalField.EmbeddableNonId.class + }; + } + + @Test + public void entityWithFinalField_constructor() { + EntityWithFinalField entity = new EntityWithFinalField( "foo" ); + assertThat( entity.immutableProperty ).isEqualTo( "foo" ); + } + + // Just test that the embedded non-ID works correctly over a persist/retrieve cycle + @Test + public void entityWithFinalField_smokeTest() { + EntityWithFinalField persistedEntity = new EntityWithFinalField( "foo" ); + persistedEntity.setName( "Some name" ); + inTransaction( s -> { + s.persist( persistedEntity ); + } ); + + inTransaction( s -> { + EntityWithFinalField entity = s.find( EntityWithFinalField.class, persistedEntity.getId() ); + assertThat( entity ).extracting( EntityWithFinalField::getImmutableProperty ) + .isEqualTo( persistedEntity.getImmutableProperty() ); + } ); + } + + // Just test that the embedded ID works correctly over a persist/retrieve cycle + @Test + public void embeddableIdWithFinalField_smokeTest() { + EntityWithEmbeddedIdWithFinalField persistedEntity = new EntityWithEmbeddedIdWithFinalField(); + persistedEntity.setName( "Some name" ); + inTransaction( s -> { + s.persist( persistedEntity ); + } ); + + // Read with the same ID instance + inTransaction( s -> { + EntityWithEmbeddedIdWithFinalField entity = s.find( EntityWithEmbeddedIdWithFinalField.class, persistedEntity.getId() ); + assertThat( entity ).extracting( EntityWithEmbeddedIdWithFinalField::getId ).extracting( i -> i.id ) + .isEqualTo( persistedEntity.getId().id ); + } ); + + // Read with a new ID instance + inTransaction( s -> { + EntityWithEmbeddedIdWithFinalField entity = s.find( EntityWithEmbeddedIdWithFinalField.class, EntityWithEmbeddedIdWithFinalField.EmbeddableId.of( persistedEntity.getId().id ) ); + assertThat( entity ).extracting( EntityWithEmbeddedIdWithFinalField::getId ).extracting( i -> i.id ) + .isEqualTo( persistedEntity.getId().id ); + } ); + + // Read with a query + // This is special because in this particular test, + // we know Hibernate ORM *has to* instantiate the EmbeddableIdType itself: + // it cannot reuse the ID we passed. + // And since the EmbeddableIdType has a final field, instantiation will not be able to use a no-arg constructor... + inTransaction( s -> { + EntityWithEmbeddedIdWithFinalField entity = + s.createQuery( "from embidwithfinal e where e.name = :name", EntityWithEmbeddedIdWithFinalField.class ) + .setParameter( "name", persistedEntity.getName() ) + .uniqueResult(); + assertThat( entity ).extracting( EntityWithEmbeddedIdWithFinalField::getId ).extracting( i -> i.id ) + .isEqualTo( persistedEntity.getId().id ); + } ); + } + + @Test + public void embeddableNonIdWithFinalField_constructor() { + EntityWithEmbeddedNonIdWithFinalField.EmbeddableNonId embeddable = + new EntityWithEmbeddedNonIdWithFinalField.EmbeddableNonId( "foo" ); + assertThat( embeddable.immutableProperty ).isEqualTo( "foo" ); + } + + // Just test that the embedded non-ID works correctly over a persist/retrieve cycle + @Test + public void embeddableNonIdWithFinalField_smokeTest() { + EntityWithEmbeddedNonIdWithFinalField persistedEntity = new EntityWithEmbeddedNonIdWithFinalField(); + persistedEntity.setName( "Some name" ); + persistedEntity.setEmbedded( new EntityWithEmbeddedNonIdWithFinalField.EmbeddableNonId( "foo" ) ); + inTransaction( s -> { + s.persist( persistedEntity ); + } ); + + inTransaction( s -> { + EntityWithEmbeddedNonIdWithFinalField entity = s.find( EntityWithEmbeddedNonIdWithFinalField.class, persistedEntity.getId() ); + assertThat( entity ).extracting( EntityWithEmbeddedNonIdWithFinalField::getEmbedded ) + .extracting( EntityWithEmbeddedNonIdWithFinalField.EmbeddableNonId::getImmutableProperty ) + .isEqualTo( persistedEntity.getEmbedded().getImmutableProperty() ); + } ); + } + + @Entity(name = "withfinal") + public static class EntityWithFinalField { + + @Id + @GeneratedValue + private Long id; + + private final String immutableProperty; + + private String name; + + // For Hibernate ORM + protected EntityWithFinalField() { + this.immutableProperty = null; + } + + private EntityWithFinalField(String id) { + this.immutableProperty = id; + } + + public Long getId() { + return id; + } + + public void setId(Long id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public String getImmutableProperty() { + return immutableProperty; + } + } + + @Entity(name = "embidwithfinal") + public static class EntityWithEmbeddedIdWithFinalField { + + @EmbeddedId + private EmbeddableId id; + + private String name; + + public EntityWithEmbeddedIdWithFinalField() { + this.id = EmbeddableId.of( UUID.randomUUID().toString() ); + } + + public EmbeddableId getId() { + return id; + } + + public void setId(EmbeddableId id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + @Immutable + @Embeddable + public static class EmbeddableId implements Serializable { + private final String id; + + // For Hibernate ORM + protected EmbeddableId() { + this.id = null; + } + + private EmbeddableId(String id) { + this.id = id; + } + + public static EmbeddableId of(String string) { + return new EmbeddableId( string ); + } + + @Override + public boolean equals(Object o) { + if ( this == o ) { + return true; + } + if ( !( o instanceof EmbeddableId ) ) { + return false; + } + EmbeddableId embeddableIdType = (EmbeddableId) o; + return Objects.equals( id, embeddableIdType.id ); + } + + @Override + public int hashCode() { + return Objects.hash( id ); + } + } + } + + @Entity(name = "embwithfinal") + public static class EntityWithEmbeddedNonIdWithFinalField { + + @Id + @GeneratedValue + private Long id; + + private String name; + + @Embedded + private EmbeddableNonId embedded; + + public Long getId() { + return id; + } + + public void setId(Long id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public EmbeddableNonId getEmbedded() { + return embedded; + } + + public void setEmbedded( + EmbeddableNonId embedded) { + this.embedded = embedded; + } + + @Embeddable + public static class EmbeddableNonId { + private final String immutableProperty; + + private String mutableProperty; + + protected EmbeddableNonId() { + // For Hibernate ORM only - it will change the property value through reflection + this.immutableProperty = null; + } + + private EmbeddableNonId(String immutableProperty) { + this.immutableProperty = immutableProperty; + } + + public String getImmutableProperty() { + return immutableProperty; + } + + public String getMutableProperty() { + return mutableProperty; + } + + public void setMutableProperty(String mutableProperty) { + this.mutableProperty = mutableProperty; + } + } + } + +}