Skip to content

Commit

Permalink
HHH-16081 - Converted collection-as-basic values are considered immut…
Browse files Browse the repository at this point in the history
…able

HHH-16132 - Dirty checking broken for collection-as-basic mappings (test)
  • Loading branch information
sebersole committed Feb 6, 2023
1 parent f4e95d9 commit ae238d3
Show file tree
Hide file tree
Showing 7 changed files with 613 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,21 @@
*/
package org.hibernate.boot.model.process.internal;

import java.util.Map;
import java.util.function.Function;

import org.hibernate.annotations.Immutable;
import org.hibernate.boot.model.convert.internal.ClassBasedConverterDescriptor;
import org.hibernate.boot.model.convert.spi.ConverterDescriptor;
import org.hibernate.boot.model.convert.spi.JpaAttributeConverterCreationContext;
import org.hibernate.boot.registry.classloading.spi.ClassLoaderService;
import org.hibernate.boot.spi.MetadataBuildingContext;
import org.hibernate.mapping.BasicValue;
import org.hibernate.mapping.Collection;
import org.hibernate.metamodel.mapping.JdbcMapping;
import org.hibernate.type.descriptor.converter.spi.JpaAttributeConverter;
import org.hibernate.type.BasicType;
import org.hibernate.type.descriptor.converter.internal.AttributeConverterMutabilityPlanImpl;
import org.hibernate.type.descriptor.converter.spi.JpaAttributeConverter;
import org.hibernate.type.descriptor.java.BasicJavaType;
import org.hibernate.type.descriptor.java.ImmutableMutabilityPlan;
import org.hibernate.type.descriptor.java.JavaType;
Expand Down Expand Up @@ -112,23 +115,14 @@ private static <T> NamedConverterResolution<T> fromInternal(
? explicitJdbcType
: relationalJtd.getRecommendedJdbcType( sqlTypeIndicators );

final MutabilityPlan<T> explicitMutabilityPlan = explicitMutabilityPlanAccess != null
? explicitMutabilityPlanAccess.apply( typeConfiguration )
: null;


final MutabilityPlan<T> mutabilityPlan;
if ( explicitMutabilityPlan != null ) {
mutabilityPlan = explicitMutabilityPlan;
}
else if ( ! domainJtd.getMutabilityPlan().isMutable() ) {
mutabilityPlan = ImmutableMutabilityPlan.instance();
}
else {
mutabilityPlan = new AttributeConverterMutabilityPlanImpl<>( converter, true );
}
final MutabilityPlan<T> mutabilityPlan = determineMutabilityPlan(
explicitMutabilityPlanAccess,
typeConfiguration,
converter,
domainJtd
);

return new NamedConverterResolution<T>(
return new NamedConverterResolution<>(
domainJtd,
relationalJtd,
jdbcType,
Expand All @@ -138,6 +132,36 @@ else if ( ! domainJtd.getMutabilityPlan().isMutable() ) {
);
}

private static <T> MutabilityPlan<T> determineMutabilityPlan(
Function<TypeConfiguration, MutabilityPlan> explicitMutabilityPlanAccess,
TypeConfiguration typeConfiguration,
JpaAttributeConverter<T, ?> converter,
JavaType<T> domainJtd) {
//noinspection unchecked
final MutabilityPlan<T> explicitMutabilityPlan = explicitMutabilityPlanAccess != null
? explicitMutabilityPlanAccess.apply( typeConfiguration )
: null;
if ( explicitMutabilityPlan != null ) {
return explicitMutabilityPlan;
}

if ( converter.getConverterJavaType().getJavaTypeClass().isAnnotationPresent( Immutable.class ) ) {
return ImmutableMutabilityPlan.instance();
}

if ( !domainJtd.getMutabilityPlan().isMutable()
&& !isCollection( domainJtd.getJavaTypeClass() ) ) {
return ImmutableMutabilityPlan.instance();
}

return new AttributeConverterMutabilityPlanImpl<>( converter, true );
}

private static boolean isCollection(Class<?> javaType) {
return Collection.class.isAssignableFrom( javaType )
|| Map.class.isAssignableFrom( javaType );
}


private final JavaType<J> domainJtd;
private final JavaType<?> relationalJtd;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,16 @@ private static void applyToMap(Map<String,?> map, Object... pairs) {
}
}

public static String[] asPairs(Map<String,String> map) {
final String[] pairs = new String[ map.size() * 2 ];
int i = 0;
for ( Map.Entry<String,String> entry : map.entrySet() ) {
pairs[i++] = entry.getKey();
pairs[i++] = entry.getValue();
}
return pairs;
}

public static Properties toProperties(Object... pairs) {
final Properties properties = new Properties();
if ( pairs.length > 0 ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
package org.hibernate.type.descriptor.java.spi;

import java.lang.reflect.ParameterizedType;
import java.util.Objects;

import org.hibernate.collection.spi.CollectionSemantics;
import org.hibernate.collection.spi.PersistentCollection;
Expand Down Expand Up @@ -89,17 +90,34 @@ public <X> C wrap(X value, WrapperOptions options) {

@Override
public boolean areEqual(C one, C another) {
return one == another ||
(
one instanceof PersistentCollection &&
( (PersistentCollection<?>) one ).wasInitialized() &&
( (PersistentCollection<?>) one ).isWrapper( another )
) ||
(
another instanceof PersistentCollection &&
( (PersistentCollection<?>) another ).wasInitialized() &&
( (PersistentCollection<?>) another ).isWrapper( one )
);
// return one == another ||
// (
// one instanceof PersistentCollection &&
// ( (PersistentCollection<?>) one ).wasInitialized() &&
// ( (PersistentCollection<?>) one ).isWrapper( another )
// ) ||
// (
// another instanceof PersistentCollection &&
// ( (PersistentCollection<?>) another ).wasInitialized() &&
// ( (PersistentCollection<?>) another ).isWrapper( one )
// );


if ( one == another ) {
return true;
}

if ( one instanceof PersistentCollection ) {
final PersistentCollection pc = (PersistentCollection) one;
return pc.wasInitialized() && ( pc.isWrapper( another ) || pc.isDirectlyProvidedCollection( another ) );
}

if ( another instanceof PersistentCollection ) {
final PersistentCollection pc = (PersistentCollection) another;
return pc.wasInitialized() && ( pc.isWrapper( one ) || pc.isDirectlyProvidedCollection( one ) );
}

return Objects.equals( one, another );
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public <J> JavaType<J> resolveDescriptor(Type javaType) {
() -> {
if ( javaType instanceof ParameterizedType ) {
final ParameterizedType parameterizedType = (ParameterizedType) javaType;
final JavaType<J> rawType = findDescriptor( ( parameterizedType ).getRawType() );
final JavaType<J> rawType = findDescriptor( parameterizedType.getRawType() );
if ( rawType != null ) {
return rawType.createJavaType( parameterizedType, typeConfiguration );
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
/*
* 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.orm.test.mapping.converted.converter.mutabiity;

import java.util.Map;

import org.hibernate.annotations.Immutable;
import org.hibernate.internal.util.collections.CollectionHelper;

import org.hibernate.testing.jdbc.SQLStatementInspector;
import org.hibernate.testing.orm.junit.DomainModel;
import org.hibernate.testing.orm.junit.FailureExpected;
import org.hibernate.testing.orm.junit.JiraKey;
import org.hibernate.testing.orm.junit.SessionFactory;
import org.hibernate.testing.orm.junit.SessionFactoryScope;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import jakarta.persistence.Column;
import jakarta.persistence.Convert;
import jakarta.persistence.Entity;
import jakarta.persistence.Id;
import jakarta.persistence.Table;

import static org.assertj.core.api.Assertions.assertThat;

/**
* @author Steve Ebersole
*/
@DomainModel( annotatedClasses = ConvertedMapImmutableTests.TestEntity.class )
@SessionFactory( useCollectingStatementInspector = true )
public class ConvertedMapImmutableTests {

@Test
@JiraKey( "HHH-16081" )
void testManagedUpdate(SessionFactoryScope scope) {
final SQLStatementInspector statementInspector = scope.getCollectingStatementInspector();

scope.inTransaction( (session) -> {
final TestEntity loaded = session.get( TestEntity.class, 1 );
loaded.values.put( "ghi", "789" );
statementInspector.clear();
} );

final TestEntity after = scope.fromTransaction( (session) -> session.get( TestEntity.class, 1 ) );
assertThat( after.values ).hasSize( 2 );
}

@Test
@JiraKey( "HHH-16081" )
@FailureExpected( reason = "Fails due to HHH-16132 - Hibernate believes the attribute is dirty, even though it is immutable." )
void testMerge(SessionFactoryScope scope) {
final SQLStatementInspector statementInspector = scope.getCollectingStatementInspector();

final TestEntity loaded = scope.fromTransaction( (session) -> session.get( TestEntity.class, 1 ) );
assertThat( loaded.values ).hasSize( 2 );

loaded.values.put( "ghi", "789" );
statementInspector.clear();
scope.inTransaction( (session) -> session.merge( loaded ) );

final TestEntity merged = scope.fromTransaction( (session) -> session.get( TestEntity.class, 1 ) );
assertThat( merged.values ).hasSize( 2 );
}

@Test
@JiraKey( "HHH-16132" )
@FailureExpected( reason = "Fails due to HHH-16132 - Hibernate believes the attribute is dirty, even though it is immutable." )
void testDirtyChecking(SessionFactoryScope scope) {
final SQLStatementInspector statementInspector = scope.getCollectingStatementInspector();

// make changes to a managed entity - should not trigger update since it is immutable
scope.inTransaction( (session) -> {
final TestEntity managed = session.get( TestEntity.class, 1 );
statementInspector.clear();
assertThat( managed.values ).hasSize( 2 );
// make the change
managed.values.put( "ghi", "789" );
} );
assertThat( statementInspector.getSqlQueries() ).isEmpty();

// make no changes to a detached entity and merge it - should not trigger update
final TestEntity loaded = scope.fromTransaction( (session) -> session.get( TestEntity.class, 1 ) );
assertThat( loaded.values ).hasSize( 2 );
// make the change
loaded.values.put( "ghi", "789" );
statementInspector.clear();
scope.inTransaction( (session) -> session.merge( loaded ) );
// the SELECT
assertThat( statementInspector.getSqlQueries() ).hasSize( 1 );
}

@Test
@JiraKey( "HHH-16132" )
void testNotDirtyChecking(SessionFactoryScope scope) {
final SQLStatementInspector statementInspector = scope.getCollectingStatementInspector();

// make changes to a managed entity - should not trigger update
scope.inTransaction( (session) -> {
final TestEntity managed = session.get( TestEntity.class, 1 );
statementInspector.clear();
assertThat( managed.values ).hasSize( 2 );
} );
assertThat( statementInspector.getSqlQueries() ).isEmpty();

// make no changes to a detached entity and merge it - should not trigger update
final TestEntity loaded = scope.fromTransaction( (session) -> session.get( TestEntity.class, 1 ) );
assertThat( loaded.values ).hasSize( 2 );
statementInspector.clear();
scope.inTransaction( (session) -> session.merge( loaded ) );
// the SELECT
assertThat( statementInspector.getSqlQueries() ).hasSize( 1 );
}

@BeforeEach
void createTestData(SessionFactoryScope scope) {
scope.inTransaction( (session) -> {
session.persist( new TestEntity(
1,
CollectionHelper.toMap(
"abc", "123",
"def", "456"
)
) );
} );
}

@AfterEach
void dropTestData(SessionFactoryScope scope) {
scope.inTransaction( (session) -> {
session.createMutationQuery( "delete TestEntity" ).executeUpdate();
} );
}

@Immutable
public static class ImmutableMapConverter extends ConvertedMapMutableTests.MapConverter {
}

@Entity( name = "TestEntity" )
@Table( name = "entity_immutable_map" )
public static class TestEntity {
@Id
private Integer id;

@Convert( converter = ImmutableMapConverter.class )
@Column( name="vals" )
private Map<String,String> values;

private TestEntity() {
// for use by Hibernate
}

public TestEntity(
Integer id,
Map<String,String> values) {
this.id = id;
this.values = values;
}
}
}

0 comments on commit ae238d3

Please sign in to comment.