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

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
+ * @implNote Sure, this could be handled with {@code Boolean}, but
- * you run into potential problems with unwanted auto-unboxing
+ *           that option is vulnerable to unwanted auto-unboxing
- * and {@linkplain NullPointerException NPE}.
+ *           and {@link NullPointerException}s.
  *
  * @author Steve Ebersole
  */

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) {
+		switch ( sharedCacheMode ) {
-			case ALL: {
+			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;
-			}
 		}
 	}
 

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.UNKNOWN );
-			return new Caching( TruthValue.FALSE );
+		}
+		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 );
 		}
-
+		else {
-		return new Caching(
+			return new Caching(
-				StringHelper.nullIfEmpty( cacheElement.getRegion() ),
+					nullIfEmpty( cacheElement.getRegion() ),
-				null,
+					null,
-				false,
+					false,
-				TruthValue.TRUE
+					TruthValue.TRUE
-		);
+			);
+		}
 	}
 
 	public static String getPropertyAccessorName(String access, boolean isEmbedded, String defaultAccess) {

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 ) {
+		if ( isCacheEnabled( mappingDocument, caching ) ) {
-			// see if JPA's SharedCacheMode indicates we should implicitly apply caching
+			rootEntityDescriptor.setCacheConcurrencyStrategy( getConcurrencyStrategy( mappingDocument, caching ).getExternalName() );
-			//
+			rootEntityDescriptor.setCacheRegionName( caching == null ? null : caching.getRegion() );
-			// here we only really look for ALL.  Ideally we could look at NONE too as a means
+			rootEntityDescriptor.setLazyPropertiesCacheable( caching == null || caching.isCacheLazyProperties() );
-			// to selectively disable all caching, but historically hbm.xml mappings were processed
+			rootEntityDescriptor.setCached( true );
-			// 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 ( caching == null || caching.getRequested() == TruthValue.FALSE ) {
+	private static AccessType getConcurrencyStrategy(MappingDocument mappingDocument, Caching caching) {
-			return;
+		return caching == null || caching.getAccessType() == null
-		}
+				? mappingDocument.getBuildingOptions().getImplicitCacheAccessType()
+				: caching.getAccessType();
+	}
 
-		if ( caching.getAccessType() != null ) {
+	private static boolean isCacheEnabled(MappingDocument mappingDocument, Caching caching) {
-			rootEntityDescriptor.setCacheConcurrencyStrategy( caching.getAccessType().getExternalName() );
+		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 ) {
+		if ( isCacheEnabled( mappingDocument, caching ) ) {
-			// see if JPA's SharedCacheMode indicates we should implicitly apply caching
+			collection.setCacheConcurrencyStrategy( getConcurrencyStrategy( mappingDocument, caching ).getExternalName() );
-			switch ( mappingDocument.getBuildingOptions().getSharedCacheMode() ) {
+			collection.setCacheRegionName( caching == null ? null : caching.getRegion() );
-				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 ( 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(