HHH-10277 - AttributeConverter not applied to attributes of an embeddable used as collection element

(cherry picked from commit b7f17ce898)

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

@@ -7,6 +7,7 @@
 package org.hibernate.cfg;
 
 import java.util.HashMap;
+import java.util.Locale;
 import java.util.Map;
 import javax.persistence.Convert;
 import javax.persistence.Converts;
@@ -144,16 +145,31 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder {
 		}
 		else {
 			// the @Convert named an attribute...
-			final String keyPath = removePrefix( info.getAttributeName(), "key" );
-			final String elementPath = removePrefix( info.getAttributeName(), "value" );
 
-			if ( canElementBeConverted && canKeyBeConverted && keyPath == null && elementPath == null ) {
+			// we have different "resolution rules" based on whether element and key can be converted
+			final String keyPath;
+			final String elementPath;
+
+			if ( canElementBeConverted && canKeyBeConverted ) {
+				keyPath = removePrefix( info.getAttributeName(), "key" );
+				elementPath = removePrefix( info.getAttributeName(), "value" );
+
+				if ( keyPath == null && elementPath == null ) {
 					// specified attributeName needs to have 'key.' or 'value.' prefix
 					throw new IllegalStateException(
 							"@Convert placed on Map attribute [" + collection.getRole()
 									+ "] must define attributeName of 'key' or 'value'"
 					);
 				}
+			}
+			else if ( canKeyBeConverted ) {
+				keyPath = removePrefix( info.getAttributeName(), "key", info.getAttributeName() );
+				elementPath = null;
+			}
+			else {
+				keyPath = null;
+				elementPath = removePrefix( info.getAttributeName(), "value", info.getAttributeName() );
+			}
 
 			if ( keyPath != null ) {
 				keyAttributeConversionInfoMap.put( keyPath, info );
@@ -161,6 +177,17 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder {
 			else if ( elementPath != null ) {
 				elementAttributeConversionInfoMap.put( elementPath, info );
 			}
+			else {
+				// specified attributeName needs to have 'key.' or 'value.' prefix
+				throw new IllegalStateException(
+						String.format(
+								Locale.ROOT,
+								"Could not determine how to apply @Convert(attributeName='%s') to collection [%s]",
+								info.getAttributeName(),
+								collection.getRole()
+						)
+				);
+			}
 		}
 	}
 
@@ -172,6 +199,10 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder {
 	 * @return Path without prefix, or null, if path did not have the prefix.
 	 */
 	private String removePrefix(String path, String prefix) {
+		return removePrefix( path, prefix, null );
+	}
+
+	private String removePrefix(String path, String prefix, String defaultValue) {
 		if ( path.equals(prefix) ) {
 			return "";
 		}
@@ -180,7 +211,7 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder {
 			return path.substring( prefix.length() + 1 );
 		}
 
-		return null;
+		return defaultValue;
 	}
 
 	@Override

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

@@ -64,7 +64,7 @@ public class ComponentPropertyHolder extends AbstractPropertyHolder {
 	private Component component;
 	private boolean isOrWithinEmbeddedId;
 
-	private boolean virtual;
+//	private boolean virtual;
 	private String embeddedAttributeName;
 	private Map<String,AttributeConversionInfo> attributeConversionInfoMap;
 
@@ -84,14 +84,20 @@ public class ComponentPropertyHolder extends AbstractPropertyHolder {
 						( embeddedXProperty.isAnnotationPresent( Id.class )
 								|| embeddedXProperty.isAnnotationPresent( EmbeddedId.class ) ) );
 
-		this.virtual = embeddedXProperty == null;
+		if ( embeddedXProperty != null ) {
-		if ( !virtual ) {
+//			this.virtual = false;
 			this.embeddedAttributeName = embeddedXProperty.getName();
 			this.attributeConversionInfoMap = processAttributeConversions( embeddedXProperty );
 		}
 		else {
-			embeddedAttributeName = "";
+			// could be either:
-			this.attributeConversionInfoMap = Collections.emptyMap();
+			// 		1) virtual/dynamic component
+			// 		2) collection element/key
+
+			// temp
+//			this.virtual = true;
+			this.embeddedAttributeName = "";
+			this.attributeConversionInfoMap = processAttributeConversions( inferredData.getClassOrElement() );
 		}
 	}
 
@@ -119,30 +125,7 @@ public class ComponentPropertyHolder extends AbstractPropertyHolder {
 		// from the Embedded
 
 		// first apply conversions from the Embeddable...
-		{
+		processAttributeConversions( embeddableXClass, infoMap );
-			// @Convert annotation on the Embeddable class level
-			final Convert convertAnnotation = embeddableXClass.getAnnotation( Convert.class );
-			if ( convertAnnotation != null ) {
-				final AttributeConversionInfo info = new AttributeConversionInfo( convertAnnotation, embeddableXClass );
-				if ( StringHelper.isEmpty( info.getAttributeName() ) ) {
-					throw new IllegalStateException( "@Convert placed on @Embeddable must define attributeName" );
-				}
-				infoMap.put( info.getAttributeName(), info );
-			}
-		}
-		{
-			// @Converts annotation on the Embeddable class level
-			final Converts convertsAnnotation = embeddableXClass.getAnnotation( Converts.class );
-			if ( convertsAnnotation != null ) {
-				for ( Convert convertAnnotation : convertsAnnotation.value() ) {
-					final AttributeConversionInfo info = new AttributeConversionInfo( convertAnnotation, embeddableXClass );
-					if ( StringHelper.isEmpty( info.getAttributeName() ) ) {
-						throw new IllegalStateException( "@Converts placed on @Embeddable must define attributeName" );
-					}
-					infoMap.put( info.getAttributeName(), info );
-				}
-			}
-		}
 
 		// then we can overlay any conversions from the Embedded attribute
 		{
@@ -173,6 +156,39 @@ public class ComponentPropertyHolder extends AbstractPropertyHolder {
 		return infoMap;
 	}
 
+	private void processAttributeConversions(XClass embeddableXClass, Map<String, AttributeConversionInfo> infoMap) {
+		{
+			// @Convert annotation on the Embeddable class level
+			final Convert convertAnnotation = embeddableXClass.getAnnotation( Convert.class );
+			if ( convertAnnotation != null ) {
+				final AttributeConversionInfo info = new AttributeConversionInfo( convertAnnotation, embeddableXClass );
+				if ( StringHelper.isEmpty( info.getAttributeName() ) ) {
+					throw new IllegalStateException( "@Convert placed on @Embeddable must define attributeName" );
+				}
+				infoMap.put( info.getAttributeName(), info );
+			}
+		}
+		{
+			// @Converts annotation on the Embeddable class level
+			final Converts convertsAnnotation = embeddableXClass.getAnnotation( Converts.class );
+			if ( convertsAnnotation != null ) {
+				for ( Convert convertAnnotation : convertsAnnotation.value() ) {
+					final AttributeConversionInfo info = new AttributeConversionInfo( convertAnnotation, embeddableXClass );
+					if ( StringHelper.isEmpty( info.getAttributeName() ) ) {
+						throw new IllegalStateException( "@Converts placed on @Embeddable must define attributeName" );
+					}
+					infoMap.put( info.getAttributeName(), info );
+				}
+			}
+		}
+	}
+
+	private Map<String,AttributeConversionInfo> processAttributeConversions(XClass embeddableXClass) {
+		final Map<String,AttributeConversionInfo> infoMap = new HashMap<String, AttributeConversionInfo>();
+		processAttributeConversions( embeddableXClass, infoMap );
+		return infoMap;
+	}
+
 	@Override
 	protected String normalizeCompositePath(String attributeName) {
 		return embeddedAttributeName + '.' + attributeName;
@@ -189,9 +205,9 @@ public class ComponentPropertyHolder extends AbstractPropertyHolder {
 			return;
 		}
 
-		if ( virtual ) {
+//		if ( virtual ) {
-			return;
+//			return;
-		}
+//		}
 
 		// again : the property coming in here *should* be the property on the embeddable (Address#city in the example),
 		// so we just ignore it if there is already an existing conversion info for that path since they would have
@@ -239,7 +255,7 @@ public class ComponentPropertyHolder extends AbstractPropertyHolder {
 
 	@Override
 	protected AttributeConversionInfo locateAttributeConversionInfo(String path) {
-		final String embeddedPath = embeddedAttributeName + '.' + path;
+		final String embeddedPath = StringHelper.qualifyConditionally( embeddedAttributeName, path );
 		AttributeConversionInfo fromParent = parent.locateAttributeConversionInfo( embeddedPath );
 		if ( fromParent != null ) {
 			return fromParent;

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

@@ -1402,6 +1402,8 @@ public abstract class CollectionBinder {
 			}
 
 			if ( AnnotatedClassType.EMBEDDABLE.equals( classType ) ) {
+				holder.prepare( property );
+
 				EntityBinder entityBinder = new EntityBinder();
 				PersistentClass owner = collValue.getOwner();
 				boolean isPropertyAnnotated;

hibernate-core/src/main/java/org/hibernate/internal/util/StringHelper.java

@@ -497,6 +497,13 @@ public final class StringHelper {
 		return prefix + '.' + name;
 	}
 
+	public static String qualifyConditionally(String prefix, String name) {
+		if ( name == null ) {
+			throw new NullPointerException( "name was null attempting to build qualified name" );
+		}
+		return isEmpty( prefix ) ? name : prefix + '.' + name;
+	}
+
 	public static String[] qualify(String prefix, String[] names) {
 		if ( prefix == null ) {
 			return names;

hibernate-core/src/test/java/org/hibernate/test/converter/elementCollection/CollectionCompositeElementExplicitConversionTest.java

@@ -32,7 +32,6 @@ import org.hibernate.mapping.PersistentClass;
 import org.hibernate.mapping.Property;
 import org.hibernate.mapping.SimpleValue;
 
-import org.hibernate.testing.FailureExpected;
 import org.hibernate.testing.TestForIssue;
 import org.hibernate.testing.junit4.BaseUnitTestCase;
 import org.junit.After;
@@ -68,7 +67,6 @@ public class CollectionCompositeElementExplicitConversionTest extends BaseUnitTe
 	}
 
 	@Test
-	@FailureExpected( jiraKey = "HHH-10277" )
 	public void testCollectionOfEmbeddablesWithConvertedAttributes() throws Exception {
 		final MetadataImplementor metadata = (MetadataImplementor) new MetadataSources( ssr )
 				.addAnnotatedClass( Disguise.class )