HHH-8529 - AttributeConverter not applied to @ElementCollection

hibernate-core/src/main/java/org/hibernate/cfg/CollectionPropertyHolder.java

@@ -47,6 +47,7 @@ import org.hibernate.internal.CoreLogging;
 import org.hibernate.internal.CoreMessageLogger;
 import org.hibernate.internal.util.StringHelper;
 import org.hibernate.mapping.Collection;
+import org.hibernate.mapping.IndexedCollection;
 import org.hibernate.mapping.Join;
 import org.hibernate.mapping.KeyValue;
 import org.hibernate.mapping.PersistentClass;
@@ -196,80 +197,6 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder {
 		// todo : implement (and make sure it gets called - for handling collections of composites)
 	}
 
-	public AttributeConverterDefinition resolveElementAttributeConverterDefinition(XClass elementXClass) {
-		AttributeConversionInfo info = locateAttributeConversionInfo( "element" );
-		if ( info != null ) {
-			if ( info.isConversionDisabled() ) {
-				return null;
-			}
-			else {
-				try {
-					return makeAttributeConverterDefinition( info );
-				}
-				catch (Exception e) {
-					throw new IllegalStateException(
-							String.format( "Unable to instantiate AttributeConverter [%s", info.getConverterClass().getName() ),
-							e
-					);
-				}
-			}
-		}
-
-		log.debugf(
-				"Attempting to locate auto-apply AttributeConverter for collection element [%s]",
-				collection.getRole()
-		);
-
-		final Class elementClass = determineElementClass( elementXClass );
-		if ( elementClass != null ) {
-			for ( AttributeConverterDefinition attributeConverterDefinition : getMappings().getAttributeConverters() ) {
-				if ( ! attributeConverterDefinition.isAutoApply() ) {
-					continue;
-				}
-				log.debugf(
-						"Checking auto-apply AttributeConverter [%s] type [%s] for match [%s]",
-						attributeConverterDefinition.toString(),
-						attributeConverterDefinition.getEntityAttributeType().getSimpleName(),
-						elementClass.getSimpleName()
-				);
-				if ( areTypeMatch( attributeConverterDefinition.getEntityAttributeType(), elementClass ) ) {
-					return attributeConverterDefinition;
-				}
-			}
-		}
-
-		return null;
-	}
-
-	private Class determineElementClass(XClass elementXClass) {
-		if ( elementXClass != null ) {
-			try {
-				return getMappings().getReflectionManager().toClass( elementXClass );
-			}
-			catch (Exception e) {
-				log.debugf(
-						"Unable to resolve XClass [%s] to Class for collection elements [%s]",
-						elementXClass.getName(),
-						collection.getRole()
-				);
-			}
-		}
-
-		if ( collection.getElement() != null ) {
-			if ( collection.getElement().getType() != null ) {
-				return collection.getElement().getType().getReturnedClass();
-			}
-		}
-
-		// currently this is called from paths where the element type really should be known,
-		// so log the fact that we could not resolve the collection element info
-		log.debugf(
-				"Unable to resolve element information for collection [%s]",
-				collection.getRole()
-		);
-		return null;
-	}
-
 	@Override
 	protected AttributeConversionInfo locateAttributeConversionInfo(XProperty property) {
 		if ( canElementBeConverted && canKeyBeConverted ) {
@@ -403,4 +330,153 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder {
 			buildAttributeConversionInfoMaps( collectionProperty, elementAttributeConversionInfoMap, keyAttributeConversionInfoMap );
 		}
 	}
+
+	public AttributeConverterDefinition resolveElementAttributeConverterDefinition(XClass elementXClass) {
+		AttributeConversionInfo info = locateAttributeConversionInfo( "element" );
+		if ( info != null ) {
+			if ( info.isConversionDisabled() ) {
+				return null;
+			}
+			else {
+				try {
+					return makeAttributeConverterDefinition( info );
+				}
+				catch (Exception e) {
+					throw new IllegalStateException(
+							String.format( "Unable to instantiate AttributeConverter [%s", info.getConverterClass().getName() ),
+							e
+					);
+				}
+			}
+		}
+
+		log.debugf(
+				"Attempting to locate auto-apply AttributeConverter for collection element [%s]",
+				collection.getRole()
+		);
+
+		final Class elementClass = determineElementClass( elementXClass );
+		if ( elementClass != null ) {
+			for ( AttributeConverterDefinition attributeConverterDefinition : getMappings().getAttributeConverters() ) {
+				if ( ! attributeConverterDefinition.isAutoApply() ) {
+					continue;
+				}
+				log.debugf(
+						"Checking auto-apply AttributeConverter [%s] type [%s] for match [%s]",
+						attributeConverterDefinition.toString(),
+						attributeConverterDefinition.getEntityAttributeType().getSimpleName(),
+						elementClass.getSimpleName()
+				);
+				if ( areTypeMatch( attributeConverterDefinition.getEntityAttributeType(), elementClass ) ) {
+					return attributeConverterDefinition;
+				}
+			}
+		}
+
+		return null;
+	}
+
+	private Class determineElementClass(XClass elementXClass) {
+		if ( elementXClass != null ) {
+			try {
+				return getMappings().getReflectionManager().toClass( elementXClass );
+			}
+			catch (Exception e) {
+				log.debugf(
+						"Unable to resolve XClass [%s] to Class for collection elements [%s]",
+						elementXClass.getName(),
+						collection.getRole()
+				);
+			}
+		}
+
+		if ( collection.getElement() != null ) {
+			if ( collection.getElement().getType() != null ) {
+				return collection.getElement().getType().getReturnedClass();
+			}
+		}
+
+		// currently this is called from paths where the element type really should be known,
+		// so log the fact that we could not resolve the collection element info
+		log.debugf(
+				"Unable to resolve element information for collection [%s]",
+				collection.getRole()
+		);
+		return null;
+	}
+
+	public AttributeConverterDefinition keyElementAttributeConverterDefinition(XClass keyXClass) {
+		AttributeConversionInfo info = locateAttributeConversionInfo( "key" );
+		if ( info != null ) {
+			if ( info.isConversionDisabled() ) {
+				return null;
+			}
+			else {
+				try {
+					return makeAttributeConverterDefinition( info );
+				}
+				catch (Exception e) {
+					throw new IllegalStateException(
+							String.format( "Unable to instantiate AttributeConverter [%s", info.getConverterClass().getName() ),
+							e
+					);
+				}
+			}
+		}
+
+		log.debugf(
+				"Attempting to locate auto-apply AttributeConverter for collection key [%s]",
+				collection.getRole()
+		);
+
+		final Class elementClass = determineKeyClass( keyXClass );
+		if ( elementClass != null ) {
+			for ( AttributeConverterDefinition attributeConverterDefinition : getMappings().getAttributeConverters() ) {
+				if ( ! attributeConverterDefinition.isAutoApply() ) {
+					continue;
+				}
+				log.debugf(
+						"Checking auto-apply AttributeConverter [%s] type [%s] for match [%s]",
+						attributeConverterDefinition.toString(),
+						attributeConverterDefinition.getEntityAttributeType().getSimpleName(),
+						elementClass.getSimpleName()
+				);
+				if ( areTypeMatch( attributeConverterDefinition.getEntityAttributeType(), elementClass ) ) {
+					return attributeConverterDefinition;
+				}
+			}
+		}
+
+		return null;
+	}
+
+	private Class determineKeyClass(XClass keyXClass) {
+		if ( keyXClass != null ) {
+			try {
+				return getMappings().getReflectionManager().toClass( keyXClass );
+			}
+			catch (Exception e) {
+				log.debugf(
+						"Unable to resolve XClass [%s] to Class for collection key [%s]",
+						keyXClass.getName(),
+						collection.getRole()
+				);
+			}
+		}
+
+		final IndexedCollection indexedCollection = (IndexedCollection) collection;
+		if ( indexedCollection.getIndex() != null ) {
+			if ( indexedCollection.getIndex().getType() != null ) {
+				return indexedCollection.getIndex().getType().getReturnedClass();
+			}
+		}
+
+		// currently this is called from paths where the element type really should be known,
+		// so log the fact that we could not resolve the collection element info
+		log.debugf(
+				"Unable to resolve key information for collection [%s]",
+				collection.getRole()
+		);
+		return null;
+	}
 }

hibernate-core/src/main/java/org/hibernate/cfg/annotations/MapBinder.java

@@ -41,6 +41,7 @@ import org.hibernate.cfg.AccessType;
 import org.hibernate.cfg.AnnotatedClassType;
 import org.hibernate.cfg.AnnotationBinder;
 import org.hibernate.cfg.BinderHelper;
+import org.hibernate.cfg.CollectionPropertyHolder;
 import org.hibernate.cfg.CollectionSecondPass;
 import org.hibernate.cfg.Ejb3Column;
 import org.hibernate.cfg.Ejb3JoinColumn;
@@ -176,28 +177,20 @@ public class MapBinder extends CollectionBinder {
 				//does not make sense for a map key element.setIgnoreNotFound( ignoreNotFound );
 			}
 			else {
-				XClass elementClass;
+				XClass keyXClass;
 				AnnotatedClassType classType;
-				PropertyHolder holder = null;
 				if ( BinderHelper.PRIMITIVE_NAMES.contains( mapKeyType ) ) {
 					classType = AnnotatedClassType.NONE;
-					elementClass = null;
+					keyXClass = null;
 				}
 				else {
 					try {
-						elementClass = mappings.getReflectionManager().classForName( mapKeyType, MapBinder.class );
+						keyXClass = mappings.getReflectionManager().classForName( mapKeyType, MapBinder.class );
 					}
 					catch (ClassNotFoundException e) {
 						throw new AnnotationException( "Unable to find class: " + mapKeyType, e );
 					}
-					classType = mappings.getClassType( elementClass );
+					classType = mappings.getClassType( keyXClass );
-
-					holder = PropertyHolderBuilder.buildPropertyHolder(
-							mapValue,
-							StringHelper.qualify( mapValue.getRole(), "mapkey" ),
-							elementClass,
-							property, propertyHolder, mappings
-					);
 					//force in case of attribute override
 					boolean attributeOverride = property.isAnnotationPresent( AttributeOverride.class )
 							|| property.isAnnotationPresent( AttributeOverrides.class );
@@ -206,12 +199,29 @@ public class MapBinder extends CollectionBinder {
 					}
 				}
 
+				CollectionPropertyHolder holder = PropertyHolderBuilder.buildPropertyHolder(
+						mapValue,
+						StringHelper.qualify( mapValue.getRole(), "mapkey" ),
+						keyXClass,
+						property,
+						propertyHolder,
+						mappings
+				);
+
+
+				// 'propertyHolder' is the PropertyHolder for the owner of the collection
+				// 'holder' is the CollectionPropertyHolder.
+				// 'property' is the collection XProperty
+				propertyHolder.startingProperty( property );
+				holder.prepare( property );
+
 				PersistentClass owner = mapValue.getOwner();
 				AccessType accessType;
 				// FIXME support @Access for collection of elements
 				// String accessType = access != null ? access.value() : null;
 				if ( owner.getIdentifierProperty() != null ) {
-					accessType = owner.getIdentifierProperty().getPropertyAccessorName().equals( "property" ) ? AccessType.PROPERTY
+					accessType = owner.getIdentifierProperty().getPropertyAccessorName().equals( "property" )
+							? AccessType.PROPERTY
 							: AccessType.FIELD;
 				}
 				else if ( owner.getIdentifierMapper() != null && owner.getIdentifierMapper().getPropertySpan() > 0 ) {
@@ -228,18 +238,25 @@ public class MapBinder extends CollectionBinder {
 
 					PropertyData inferredData;
 					if ( isHibernateExtensionMapping() ) {
-						inferredData = new PropertyPreloadedData( AccessType.PROPERTY, "index", elementClass );
+						inferredData = new PropertyPreloadedData( AccessType.PROPERTY, "index", keyXClass );
 					}
 					else {
 						//"key" is the JPA 2 prefix for map keys
-						inferredData = new PropertyPreloadedData( AccessType.PROPERTY, "key", elementClass );
+						inferredData = new PropertyPreloadedData( AccessType.PROPERTY, "key", keyXClass );
 					}
 
 					//TODO be smart with isNullable
 					Component component = AnnotationBinder.fillComponent(
-							holder, inferredData, accessType, true,
+							holder,
-							entityBinder, false, false,
+							inferredData,
-							true, mappings, inheritanceStatePerClass
+							accessType,
+							true,
+							entityBinder,
+							false,
+							false,
+							true,
+							mappings,
+							inheritanceStatePerClass
 					);
 					mapValue.setIndex( component );
 				}
@@ -271,14 +288,17 @@ public class MapBinder extends CollectionBinder {
 					//the algorithm generally does not apply for map key anyway
 					elementBinder.setKey(true);
 					MapKeyType mapKeyTypeAnnotation = property.getAnnotation( MapKeyType.class );
-					if ( mapKeyTypeAnnotation != null && !BinderHelper.isEmptyAnnotationValue(
+					if ( mapKeyTypeAnnotation != null
-							mapKeyTypeAnnotation.value()
+							&& !BinderHelper.isEmptyAnnotationValue( mapKeyTypeAnnotation.value() .type() ) ) {
-									.type()
-					) ) {
 						elementBinder.setExplicitType( mapKeyTypeAnnotation.value() );
 					}
 					else {
-						elementBinder.setType( property, elementClass, this.collection.getOwnerEntityName(), null );
+						elementBinder.setType(
+								property,
+								keyXClass,
+								this.collection.getOwnerEntityName(),
+								holder.keyElementAttributeConverterDefinition( keyXClass )
+						);
 					}
 					elementBinder.setPersistentClassName( propertyHolder.getEntityName() );
 					elementBinder.setAccessType( accessType );

hibernate-entitymanager/src/test/java/org/hibernate/jpa/test/convert/CollectionCompositeElementConversionTest.java

@@ -0,0 +1,182 @@
+/*
+ * Hibernate, Relational Persistence for Idiomatic Java
+ *
+ * Copyright (c) 2013, Red Hat Inc. or third-party contributors as
+ * indicated by the @author tags or express copyright attribution
+ * statements applied by the authors.  All third-party contributions are
+ * distributed under license by Red Hat Inc.
+ *
+ * This copyrighted material is made available to anyone wishing to use, modify,
+ * copy, or redistribute it subject to the terms and conditions of the GNU
+ * Lesser General Public License, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this distribution; if not, write to:
+ * Free Software Foundation, Inc.
+ * 51 Franklin Street, Fifth Floor
+ * Boston, MA  02110-1301  USA
+ */
+package org.hibernate.jpa.test.convert;
+
+import javax.persistence.AttributeConverter;
+import javax.persistence.CollectionTable;
+import javax.persistence.Converter;
+import javax.persistence.ElementCollection;
+import javax.persistence.Embeddable;
+import javax.persistence.Entity;
+import javax.persistence.EntityManager;
+import javax.persistence.EntityManagerFactory;
+import javax.persistence.FetchType;
+import javax.persistence.Id;
+import javax.persistence.JoinColumn;
+import javax.persistence.Table;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.hibernate.cfg.AvailableSettings;
+import org.hibernate.jpa.boot.spi.Bootstrap;
+import org.hibernate.jpa.test.PersistenceUnitDescriptorAdapter;
+
+import org.junit.Test;
+
+import org.hibernate.testing.TestForIssue;
+import org.hibernate.testing.junit4.BaseUnitTestCase;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * @author Steve Ebersole
+ */
+@TestForIssue( jiraKey = "HHH-8529" )
+public class CollectionCompositeElementConversionTest extends BaseUnitTestCase {
+	@Test
+	public void testElementCollectionConversion() {
+		final PersistenceUnitDescriptorAdapter pu = new PersistenceUnitDescriptorAdapter() {
+			@Override
+			public List<String> getManagedClassNames() {
+				return Arrays.asList( Disguise.class.getName(), ColorTypeConverter.class.getName() );
+			}
+		};
+
+		final Map settings = new HashMap();
+		settings.put( AvailableSettings.HBM2DDL_AUTO, "create-drop" );
+
+		EntityManagerFactory emf = Bootstrap.getEntityManagerFactoryBuilder( pu, settings ).build();
+		try {
+			EntityManager em = emf.createEntityManager();
+			em.getTransaction().begin();
+			Disguise disguise = new Disguise( 1 );
+			disguise.traits.add( new Traits( ColorType.BLUE, ColorType.RED ) );
+			em.persist( disguise );
+			em.getTransaction().commit();
+			em.close();
+
+			em = emf.createEntityManager();
+			em.getTransaction().begin();
+			assertEquals( 1, em.find( Disguise.class, 1 ).traits.size() );
+			em.getTransaction().commit();
+			em.close();
+
+			em = emf.createEntityManager();
+			em.getTransaction().begin();
+			disguise = em.find( Disguise.class, 1 );
+			em.remove( disguise );
+			em.getTransaction().commit();
+			em.close();
+		}
+		finally {
+			emf.close();
+		}
+	}
+
+	@Entity( name = "Disguise" )
+	@Table( name = "DISGUISE" )
+	public static class Disguise {
+		@Id
+		private Integer id;
+
+		@ElementCollection(fetch = FetchType.EAGER)
+		@CollectionTable(
+				name = "DISGUISE_TRAIT",
+				joinColumns = @JoinColumn(name = "DISGUISE_FK", nullable = false)
+		)
+		private Set<Traits> traits = new HashSet<Traits>();
+
+		public Disguise() {
+		}
+
+		public Disguise(Integer id) {
+			this.id = id;
+		}
+	}
+
+	@Embeddable
+	public static class Traits {
+		public ColorType eyeColor;
+		public ColorType hairColor;
+
+		public Traits() {
+		}
+
+		public Traits(
+				ColorType eyeColor,
+				ColorType hairColor) {
+			this.eyeColor = eyeColor;
+			this.hairColor = hairColor;
+		}
+	}
+
+	public static class ColorType {
+		public static ColorType BLUE = new ColorType( "blue" );
+		public static ColorType RED = new ColorType( "red" );
+		public static ColorType YELLOW = new ColorType( "yellow" );
+
+		private final String color;
+
+		public ColorType(String color) {
+			this.color = color;
+		}
+
+		public String toExternalForm() {
+			return color;
+		}
+
+		public static ColorType fromExternalForm(String color) {
+			if ( BLUE.color.equals( color ) ) {
+				return BLUE;
+			}
+			else if ( RED.color.equals( color ) ) {
+				return RED;
+			}
+			else if ( YELLOW.color.equals( color ) ) {
+				return YELLOW;
+			}
+			else {
+				throw new RuntimeException( "Unknown color : " + color );
+			}
+		}
+	}
+
+	@Converter( autoApply = true )
+	public static class ColorTypeConverter implements AttributeConverter<ColorType, String> {
+
+		@Override
+		public String convertToDatabaseColumn(ColorType attribute) {
+			return attribute == null ? null : attribute.toExternalForm();
+		}
+
+		@Override
+		public ColorType convertToEntityAttribute(String dbData) {
+			return ColorType.fromExternalForm( dbData );
+		}
+	}
+}

hibernate-entitymanager/src/test/java/org/hibernate/jpa/test/convert/MapKeyConversionTest.java

@@ -45,7 +45,6 @@ import org.hibernate.jpa.test.PersistenceUnitDescriptorAdapter;
 
 import org.junit.Test;
 
-import org.hibernate.testing.FailureExpected;
 import org.hibernate.testing.TestForIssue;
 import org.hibernate.testing.junit4.BaseUnitTestCase;
 
@@ -55,7 +54,6 @@ import static org.junit.Assert.assertEquals;
  * @author Steve Ebersole
  */
 @TestForIssue( jiraKey = "HHH-8529" )
-@FailureExpected( jiraKey = "HHH-8529" )
 public class MapKeyConversionTest extends BaseUnitTestCase {
 	@Test
 	public void testElementCollectionConversion() {