From 327342b39e238c46af9655904f2388319cdd91d9 Mon Sep 17 00:00:00 2001 From: Gavin Date: Sat, 31 Dec 2022 11:44:32 +0100 Subject: [PATCH] correct implementation of JPA SharedCacheMode for .hbm.xml - even though I hate most of its members, I think NONE is pretty useful - anyway we may as well make it work, since it's trivial --- .../org/hibernate/boot/model/TruthValue.java | 6 +- .../boot/model/internal/EntityBinder.java | 23 ++-- .../model/source/internal/hbm/Helper.java | 39 +++--- .../source/internal/hbm/ModelBinder.java | 128 ++++++------------ 4 files changed, 69 insertions(+), 127 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/TruthValue.java b/hibernate-core/src/main/java/org/hibernate/boot/model/TruthValue.java index b698d6727e..bf16b90b4a 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/TruthValue.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/model/TruthValue.java @@ -9,9 +9,9 @@ package org.hibernate.boot.model; /** * An enumeration of truth values. * - * @implNote Yes this *could* be handled with Boolean, but then - * you run into potential problems with unwanted auto-unboxing - * and {@linkplain NullPointerException NPE}. + * @implNote Sure, this could be handled with {@code Boolean}, but + * that option is vulnerable to unwanted auto-unboxing + * and {@link NullPointerException}s. * * @author Steve Ebersole */ diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/EntityBinder.java b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/EntityBinder.java index 70943827ad..1254e45a28 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/EntityBinder.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/EntityBinder.java @@ -1470,37 +1470,30 @@ public class EntityBinder { return false; } switch ( effectiveCache.include().toLowerCase( Locale.ROOT ) ) { - case "all": { + case "all": return true; - } - case "non-lazy": { + case "non-lazy": return false; - } - default: { + default: throw new AnnotationException( "Class '" + annotatedClass.getName() + "' has a '@Cache' with undefined option 'include=\"" + effectiveCache.include() + "\"'" ); - } } } private static boolean isCacheable(SharedCacheMode sharedCacheMode, Cacheable explicitCacheableAnn) { - switch (sharedCacheMode) { - case ALL: { + switch ( sharedCacheMode ) { + case ALL: // all entities should be cached return true; - } - case ENABLE_SELECTIVE: { + case ENABLE_SELECTIVE: // only entities with @Cacheable(true) should be cached return explicitCacheableAnn != null && explicitCacheableAnn.value(); - } - case DISABLE_SELECTIVE: { + case DISABLE_SELECTIVE: // only entities with @Cacheable(false) should not be cached return explicitCacheableAnn == null || explicitCacheableAnn.value(); - } - default: { + default: // treat both NONE and UNSPECIFIED the same return false; - } } } diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/source/internal/hbm/Helper.java b/hibernate-core/src/main/java/org/hibernate/boot/model/source/internal/hbm/Helper.java index 84d603fe0c..c06d74882d 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/source/internal/hbm/Helper.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/model/source/internal/hbm/Helper.java @@ -35,6 +35,8 @@ import org.hibernate.engine.spi.ExecuteUpdateResultCheckStyle; import org.hibernate.internal.util.ReflectHelper; import org.hibernate.internal.util.StringHelper; +import static org.hibernate.internal.util.StringHelper.nullIfEmpty; + /** * @author Steve Ebersole * @author Gail Badner @@ -76,32 +78,31 @@ public class Helper { public static Caching createCaching(JaxbHbmCacheType cacheElement) { if ( cacheElement == null ) { - // I'd really rather this be UNKNOWN, but the annotation version resolves this to TRUE/FALSE - return new Caching( TruthValue.FALSE ); + return new Caching( TruthValue.UNKNOWN ); + } + else { + return new Caching( + cacheElement.getRegion(), + cacheElement.getUsage(), + cacheElement.getInclude() == null + || !"non-lazy".equals( cacheElement.getInclude().value() ), + TruthValue.TRUE + ); } - - final boolean cacheLazyProps = cacheElement.getInclude() == null - || !"non-lazy".equals( cacheElement.getInclude().value() ); - - return new Caching( - cacheElement.getRegion(), - cacheElement.getUsage(), - cacheLazyProps, - TruthValue.TRUE - ); } public static Caching createNaturalIdCaching(JaxbHbmNaturalIdCacheType cacheElement) { if ( cacheElement == null ) { return new Caching( TruthValue.UNKNOWN ); } - - return new Caching( - StringHelper.nullIfEmpty( cacheElement.getRegion() ), - null, - false, - TruthValue.TRUE - ); + else { + return new Caching( + nullIfEmpty( cacheElement.getRegion() ), + null, + false, + TruthValue.TRUE + ); + } } public static String getPropertyAccessorName(String access, boolean isEmbedded, String defaultAccess) { diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/source/internal/hbm/ModelBinder.java b/hibernate-core/src/main/java/org/hibernate/boot/model/source/internal/hbm/ModelBinder.java index d5b9137624..963d90a654 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/source/internal/hbm/ModelBinder.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/model/source/internal/hbm/ModelBinder.java @@ -96,6 +96,7 @@ import org.hibernate.boot.spi.InFlightMetadataCollector.EntityTableXref; import org.hibernate.boot.spi.MetadataBuildingContext; import org.hibernate.boot.spi.NaturalIdUniqueKeyBinder; import org.hibernate.boot.spi.SecondPass; +import org.hibernate.cache.spi.access.AccessType; import org.hibernate.cfg.AvailableSettings; import org.hibernate.engine.FetchStyle; import org.hibernate.engine.FetchTiming; @@ -203,22 +204,18 @@ public class ModelBinder { .addEntityBinding( rootEntityDescriptor ); switch ( hierarchySource.getHierarchyInheritanceType() ) { - case NO_INHERITANCE: { + case NO_INHERITANCE: // nothing to do break; - } - case DISCRIMINATED: { + case DISCRIMINATED: bindDiscriminatorSubclassEntities( hierarchySource.getRoot(), rootEntityDescriptor ); break; - } - case JOINED: { + case JOINED: bindJoinedSubclassEntities( hierarchySource.getRoot(), rootEntityDescriptor ); break; - } - case UNION: { + case UNION: bindUnionSubclassEntities( hierarchySource.getRoot(), rootEntityDescriptor ); break; - } } } @@ -293,49 +290,40 @@ public class ModelBinder { } private void applyCaching(MappingDocument mappingDocument, Caching caching, RootClass rootEntityDescriptor) { - if ( caching == null || caching.getRequested() == TruthValue.UNKNOWN ) { - // see if JPA's SharedCacheMode indicates we should implicitly apply caching - // - // here we only really look for ALL. Ideally we could look at NONE too as a means - // to selectively disable all caching, but historically hbm.xml mappings were processed - // outside this concept and whether to cache or not was defined wholly by what - // is defined in the mapping document. So for backwards compatibility we - // do not consider ENABLE_SELECTIVE nor DISABLE_SELECTIVE here. - // - // Granted, ALL was not historically considered either, but I have a practical - // reason for wanting to support this... our legacy tests built using - // Configuration applied a similar logic but that capability is no longer - // accessible from Configuration - switch ( mappingDocument.getBuildingOptions().getSharedCacheMode() ) { - case ALL: - caching = new Caching( - null, - mappingDocument.getBuildingOptions().getImplicitCacheAccessType(), - false, - TruthValue.UNKNOWN - ); - break; - case NONE: // Ideally we'd disable all caching... - case ENABLE_SELECTIVE: // this is default behavior for hbm.xml - case DISABLE_SELECTIVE: // really makes no sense for hbm.xml - default: // null or UNSPECIFIED, nothing to do. IMO for hbm.xml this is equivalent to ENABLE_SELECTIVE - // do nothing - } + if ( isCacheEnabled( mappingDocument, caching ) ) { + rootEntityDescriptor.setCacheConcurrencyStrategy( getConcurrencyStrategy( mappingDocument, caching ).getExternalName() ); + rootEntityDescriptor.setCacheRegionName( caching == null ? null : caching.getRegion() ); + rootEntityDescriptor.setLazyPropertiesCacheable( caching == null || caching.isCacheLazyProperties() ); + rootEntityDescriptor.setCached( true ); } + } - if ( caching == null || caching.getRequested() == TruthValue.FALSE ) { - return; - } + private static AccessType getConcurrencyStrategy(MappingDocument mappingDocument, Caching caching) { + return caching == null || caching.getAccessType() == null + ? mappingDocument.getBuildingOptions().getImplicitCacheAccessType() + : caching.getAccessType(); + } - if ( caching.getAccessType() != null ) { - rootEntityDescriptor.setCacheConcurrencyStrategy( caching.getAccessType().getExternalName() ); + private static boolean isCacheEnabled(MappingDocument mappingDocument, Caching caching) { + switch ( mappingDocument.getBuildingOptions().getSharedCacheMode() ) { + case UNSPECIFIED: + case ENABLE_SELECTIVE: + // this is default behavior for hbm.xml + return caching != null && caching.getRequested().toBoolean(false); + case NONE: + // this option is actually really useful + return false; + case ALL: + // goes completely against the whole ideology we have for + // caching, and so it hurts me to support it here + return true; + case DISABLE_SELECTIVE: + // makes no sense for hbm.xml, and also goes against our + // ideology, and so it hurts me to support it here + return caching == null || caching.getRequested().toBoolean(true); + default: + throw new AssertionFailure( "unknown SharedCacheMode" ); } - else { - rootEntityDescriptor.setCacheConcurrencyStrategy( mappingDocument.getBuildingOptions().getImplicitCacheAccessType().getExternalName() ); - } - rootEntityDescriptor.setCacheRegionName( caching.getRegion() ); - rootEntityDescriptor.setLazyPropertiesCacheable( caching.isCacheLazyProperties() ); - rootEntityDescriptor.setCached( caching.getRequested() != TruthValue.UNKNOWN ); } private void bindEntityIdentifier( @@ -1553,50 +1541,10 @@ public class ModelBinder { } private void applyCaching(MappingDocument mappingDocument, Caching caching, Collection collection) { - if ( caching == null || caching.getRequested() == TruthValue.UNKNOWN ) { - // see if JPA's SharedCacheMode indicates we should implicitly apply caching - switch ( mappingDocument.getBuildingOptions().getSharedCacheMode() ) { - case ALL: { - caching = new Caching( - null, - mappingDocument.getBuildingOptions().getImplicitCacheAccessType(), - false, - TruthValue.UNKNOWN - ); - break; - } - case NONE: { - // Ideally we'd disable all caching... - break; - } - case ENABLE_SELECTIVE: { - // this is default behavior for hbm.xml - break; - } - case DISABLE_SELECTIVE: { - // really makes no sense for hbm.xml - break; - } - default: { - // null or UNSPECIFIED, nothing to do. IMO for hbm.xml this is equivalent - // to ENABLE_SELECTIVE - break; - } - } + if ( isCacheEnabled( mappingDocument, caching ) ) { + collection.setCacheConcurrencyStrategy( getConcurrencyStrategy( mappingDocument, caching ).getExternalName() ); + collection.setCacheRegionName( caching == null ? null : caching.getRegion() ); } - - if ( caching == null || caching.getRequested() == TruthValue.FALSE ) { - return; - } - - if ( caching.getAccessType() != null ) { - collection.setCacheConcurrencyStrategy( caching.getAccessType().getExternalName() ); - } - else { - collection.setCacheConcurrencyStrategy( mappingDocument.getBuildingOptions().getImplicitCacheAccessType().getExternalName() ); - } - collection.setCacheRegionName( caching.getRegion() ); -// collection.setCachingExplicitlyRequested( caching.getRequested() != TruthValue.UNKNOWN ); } private Identifier determineTable(