From e8e8538228d8746113243ccefc8192527e90a761 Mon Sep 17 00:00:00 2001 From: Steve Ebersole Date: Thu, 16 Jun 2016 11:09:13 -0500 Subject: [PATCH] HHH-10277 - AttributeConverter not applied to attributes of an embeddable used as collection element (cherry picked from commit b7f17ce898916968b79f043757810828ae17cea1) --- .../cfg/CollectionPropertyHolder.java | 49 +++++++++-- .../cfg/ComponentPropertyHolder.java | 82 +++++++++++-------- .../cfg/annotations/CollectionBinder.java | 2 + .../hibernate/internal/util/StringHelper.java | 7 ++ ...ompositeElementExplicitConversionTest.java | 2 - 5 files changed, 98 insertions(+), 44 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/cfg/CollectionPropertyHolder.java b/hibernate-core/src/main/java/org/hibernate/cfg/CollectionPropertyHolder.java index 842f4974b8..97ea9e41cc 100644 --- a/hibernate-core/src/main/java/org/hibernate/cfg/CollectionPropertyHolder.java +++ b/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,15 +145,30 @@ 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 ) { - // 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'" - ); + // 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 ) { @@ -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 diff --git a/hibernate-core/src/main/java/org/hibernate/cfg/ComponentPropertyHolder.java b/hibernate-core/src/main/java/org/hibernate/cfg/ComponentPropertyHolder.java index feb27bd65c..b2c60e681b 100644 --- a/hibernate-core/src/main/java/org/hibernate/cfg/ComponentPropertyHolder.java +++ b/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 attributeConversionInfoMap; @@ -84,14 +84,20 @@ public class ComponentPropertyHolder extends AbstractPropertyHolder { ( embeddedXProperty.isAnnotationPresent( Id.class ) || embeddedXProperty.isAnnotationPresent( EmbeddedId.class ) ) ); - this.virtual = embeddedXProperty == null; - if ( !virtual ) { + if ( embeddedXProperty != null ) { +// this.virtual = false; this.embeddedAttributeName = embeddedXProperty.getName(); this.attributeConversionInfoMap = processAttributeConversions( embeddedXProperty ); } else { - embeddedAttributeName = ""; - this.attributeConversionInfoMap = Collections.emptyMap(); + // could be either: + // 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... - { - // @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 ); - } - } - } + processAttributeConversions( embeddableXClass, infoMap ); // 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 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 processAttributeConversions(XClass embeddableXClass) { + final Map infoMap = new HashMap(); + 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 ) { - return; - } +// if ( virtual ) { +// 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; diff --git a/hibernate-core/src/main/java/org/hibernate/cfg/annotations/CollectionBinder.java b/hibernate-core/src/main/java/org/hibernate/cfg/annotations/CollectionBinder.java index 0560ab852a..ae945cbdd4 100644 --- a/hibernate-core/src/main/java/org/hibernate/cfg/annotations/CollectionBinder.java +++ b/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; diff --git a/hibernate-core/src/main/java/org/hibernate/internal/util/StringHelper.java b/hibernate-core/src/main/java/org/hibernate/internal/util/StringHelper.java index 6dd32297ac..86801ef224 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/util/StringHelper.java +++ b/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; diff --git a/hibernate-core/src/test/java/org/hibernate/test/converter/elementCollection/CollectionCompositeElementExplicitConversionTest.java b/hibernate-core/src/test/java/org/hibernate/test/converter/elementCollection/CollectionCompositeElementExplicitConversionTest.java index 71da3c8769..1a7db1e747 100644 --- a/hibernate-core/src/test/java/org/hibernate/test/converter/elementCollection/CollectionCompositeElementExplicitConversionTest.java +++ b/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 )