From 2f6f17912f2f94dfa103dbabc766a9d05749afab Mon Sep 17 00:00:00 2001 From: Marco Belladelli Date: Mon, 28 Nov 2022 19:06:14 +0100 Subject: [PATCH] HHH-15733 Change convert logic to default to value for Map collections of basic types --- .../boot/model/internal/CollectionBinder.java | 8 ++-- .../internal/CollectionPropertyHolder.java | 43 +++++++++++++------ .../boot/model/internal/MapBinder.java | 3 +- .../map/MapElementBaseTypeConversionTest.java | 9 +--- 4 files changed, 39 insertions(+), 24 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionBinder.java b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionBinder.java index 1cab0abd7e..27d9d6a438 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionBinder.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionBinder.java @@ -2131,15 +2131,17 @@ public abstract class CollectionBinder { propertyHolder, buildingContext ); - holder.prepare( property ); final Class> compositeUserType = resolveCompositeUserType( property, elementClass, buildingContext ); - if ( classType == EMBEDDABLE || compositeUserType != null ) { + boolean isComposite = classType == EMBEDDABLE || compositeUserType != null; + holder.prepare( property, isComposite ); + + if ( isComposite ) { handleCompositeCollectionElement( hqlOrderBy, elementClass, holder, compositeUserType ); } else { - handleCollectionElement( elementType, hqlOrderBy, elementClass, holder ); + handleCollectionElement( elementType, hqlOrderBy, elementClass, holder ); } } diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionPropertyHolder.java b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionPropertyHolder.java index 8e743d228d..5a1bccefb7 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionPropertyHolder.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionPropertyHolder.java @@ -78,6 +78,7 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder { private void buildAttributeConversionInfoMaps( XProperty collectionProperty, + boolean isComposite, Map elementAttributeConversionInfoMap, Map keyAttributeConversionInfoMap) { if ( collectionProperty == null ) { @@ -88,7 +89,13 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder { { final Convert convertAnnotation = collectionProperty.getAnnotation( Convert.class ); if ( convertAnnotation != null ) { - applyLocalConvert( convertAnnotation, collectionProperty, elementAttributeConversionInfoMap, keyAttributeConversionInfoMap ); + applyLocalConvert( + convertAnnotation, + collectionProperty, + isComposite, + elementAttributeConversionInfoMap, + keyAttributeConversionInfoMap + ); } } @@ -99,6 +106,7 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder { applyLocalConvert( convertAnnotation, collectionProperty, + isComposite, elementAttributeConversionInfoMap, keyAttributeConversionInfoMap ); @@ -110,14 +118,15 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder { private void applyLocalConvert( Convert convertAnnotation, XProperty collectionProperty, + boolean isComposite, Map elementAttributeConversionInfoMap, Map keyAttributeConversionInfoMap) { - // IMPL NOTE : the rules here are quite more lenient than what JPA says. For example, JPA says - // that @Convert on a Map always needs to specify attributeName of key/value (or prefixed with - // key./value. for embedded paths). However, we try to see if conversion of either is disabled - // for whatever reason. For example, if the Map is annotated with @Enumerated the elements cannot - // be converted so any @Convert likely meant the key, so we apply it to the key + // IMPL NOTE : the rules here are quite more lenient than what JPA says. For example, JPA says that @Convert + // on a Map of basic types should default to "value" but it should explicitly specify attributeName of "key" + // (or prefixed with "key." for embedded paths) to be applied on the key. However, we try to see if conversion + // of either is disabled for whatever reason. For example, if the Map is annotated with @Enumerated the + // elements cannot be converted so any @Convert likely meant the key, so we apply it to the key final AttributeConversionInfo info = new AttributeConversionInfo( convertAnnotation, collectionProperty ); if ( collection.isMap() ) { @@ -132,10 +141,15 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder { if ( isEmpty( info.getAttributeName() ) ) { // the @Convert did not name an attribute... if ( canElementBeConverted && canKeyBeConverted ) { - throw new IllegalStateException( - "@Convert placed on Map attribute [" + collection.getRole() - + "] must define attributeName of 'key' or 'value'" - ); + if ( !isComposite ) { + // if element is of basic type default to "value" + elementAttributeConversionInfoMap.put( "", info ); + } + else { + throw new IllegalStateException( + "@Convert placed on Map attribute [" + collection.getRole() + + "] of non-basic types must define attributeName of 'key' or 'value'" ); + } } else if ( canKeyBeConverted ) { keyAttributeConversionInfoMap.put( "", info ); @@ -325,7 +339,7 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder { boolean prepared; - public void prepare(XProperty collectionProperty) { + public void prepare(XProperty collectionProperty, boolean isComposite) { // fugly if ( prepared ) { return; @@ -377,7 +391,12 @@ public class CollectionPropertyHolder extends AbstractPropertyHolder { // Is it valid to reference a collection attribute in a @Convert attached to the owner (entity) by path? // if so we should pass in 'clazzToProcess' also if ( canKeyBeConverted || canElementBeConverted ) { - buildAttributeConversionInfoMaps( collectionProperty, elementAttributeConversionInfoMap, keyAttributeConversionInfoMap ); + buildAttributeConversionInfoMaps( + collectionProperty, + isComposite, + elementAttributeConversionInfoMap, + keyAttributeConversionInfoMap + ); } } diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/MapBinder.java b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/MapBinder.java index 20f3ce78c7..aa12028a69 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/MapBinder.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/MapBinder.java @@ -37,6 +37,7 @@ import org.hibernate.mapping.SimpleValue; import org.hibernate.mapping.Table; import org.hibernate.mapping.Value; import org.hibernate.resource.beans.spi.ManagedBean; +import org.hibernate.type.BasicType; import org.hibernate.usertype.CompositeUserType; import org.hibernate.usertype.UserCollectionType; @@ -278,7 +279,7 @@ public class MapBinder extends CollectionBinder { // 'holder' is the CollectionPropertyHolder. // 'property' is the collection XProperty propertyHolder.startingProperty( property ); - holder.prepare(property); + holder.prepare( property, !( collection.getKey().getType() instanceof BasicType ) ); return holder; } diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/mapping/converted/converter/map/MapElementBaseTypeConversionTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/mapping/converted/converter/map/MapElementBaseTypeConversionTest.java index 7e9d1fc714..18f150b787 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/mapping/converted/converter/map/MapElementBaseTypeConversionTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/mapping/converted/converter/map/MapElementBaseTypeConversionTest.java @@ -67,16 +67,9 @@ public class MapElementBaseTypeConversionTest { private Integer id; @ElementCollection(fetch = FetchType.EAGER) - @Convert(converter = MyStringConverter.class) + @Convert(converter = MyStringConverter.class) // note omitted attributeName private final Map colors = new HashMap<>(); - public Customer() { - } - - public Customer(Integer id) { - this.id = id; - } - public Map getColors() { return colors; }