HHH-14526 validate table repetition in JOINED hierarchies

and automatically force the discriminator when necessary

hibernate-core/src/main/java/org/hibernate/mapping/JoinedSubclass.java

@@ -32,8 +32,8 @@ public class JoinedSubclass extends Subclass implements TableOwner {
 	}
 
 	public void setTable(Table table) {
-		this.table=table;
+		this.table = table;
-		getSuperclass().addSubclassTable(table);
+		getSuperclass().addSubclassTable( table );
 	}
 
 	public KeyValue getKey() {

hibernate-core/src/main/java/org/hibernate/mapping/PersistentClass.java

@@ -37,6 +37,10 @@ import org.hibernate.service.ServiceRegistry;
 import org.hibernate.sql.Alias;
 import org.hibernate.type.Type;
 
+import static java.util.Collections.emptyList;
+import static java.util.Collections.unmodifiableList;
+import static org.hibernate.internal.util.StringHelper.root;
+
 /**
  * A mapping model object that represents an {@linkplain jakarta.persistence.Entity entity class}.
  *
@@ -239,8 +243,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 		List<Subclass>[] subclassLists = new List[subclasses.size() + 1];
 		int j;
 		for (j = 0; j < subclasses.size(); j++) {
-			Subclass subclass = subclasses.get(j);
+			subclassLists[j] = subclasses.get(j).getSubclasses();
-			subclassLists[j] = subclass.getSubclasses();
 		}
 		subclassLists[j] = subclasses;
 		return new JoinedList<>( subclassLists );
@@ -254,14 +257,14 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 	@Deprecated(since = "6.0") @Remove
 	public Iterator<Subclass> getSubclassIterator() {
 		@SuppressWarnings("unchecked")
-		final Iterator<Subclass>[] iters = new Iterator[subclasses.size() + 1];
+		final Iterator<Subclass>[] iterators = new Iterator[subclasses.size() + 1];
-		final Iterator<Subclass> iter = subclasses.iterator();
+		final Iterator<Subclass> iterator = subclasses.iterator();
 		int i = 0;
-		while ( iter.hasNext() ) {
+		while ( iterator.hasNext() ) {
-			iters[i++] = iter.next().getSubclassIterator();
+			iterators[i++] = iterator.next().getSubclassIterator();
 		}
-		iters[i] = subclasses.iterator();
+		iterators[i] = subclasses.iterator();
-		return new JoinedIterator<>( iters );
+		return new JoinedIterator<>( iterators );
 	}
 
 	public List<PersistentClass> getSubclassClosure() {
@@ -275,12 +278,12 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 
 	@Deprecated(since = "6.0") @Remove
 	public Iterator<PersistentClass> getSubclassClosureIterator() {
-		final ArrayList<Iterator<PersistentClass>> iters = new ArrayList<>();
+		final ArrayList<Iterator<PersistentClass>> iterators = new ArrayList<>();
-		iters.add( new SingletonIterator<>( this ) );
+		iterators.add( new SingletonIterator<>( this ) );
 		for ( Subclass subclass : getSubclasses() ) {
-			iters.add( subclass.getSubclassClosureIterator() );
+			iterators.add( subclass.getSubclassClosureIterator() );
 		}
-		return new JoinedIterator<>( iters );
+		return new JoinedIterator<>( iterators );
 	}
 
 	public Table getIdentityTable() {
@@ -403,7 +406,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 		final ArrayList<List<Property>> lists = new ArrayList<>();
 		lists.add( getPropertyClosure() );
 		lists.add( subclassProperties );
-		for (Join join : subclassJoins) {
+		for ( Join join : subclassJoins ) {
 			lists.add( join.getProperties() );
 		}
 		return new JoinedList<>( lists );
@@ -537,7 +540,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 		try {
 			return getRecursiveProperty( propertyPath, getReferenceableProperties() );
 		}
-		catch (MappingException e) {
+		catch ( MappingException e ) {
 			throw new MappingException(
 					"property-ref [" + propertyPath + "] not found on entity [" + getEntityName() + "]", e
 			);
@@ -548,7 +551,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 		try {
 			return getRecursiveProperty( propertyPath, getPropertyClosure() );
 		}
-		catch (MappingException e) {
+		catch ( MappingException e ) {
 			throw new MappingException(
 					"property [" + propertyPath + "] not found on entity [" + getEntityName() + "]", e
 			);
@@ -577,7 +580,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 							// of the embedded composite identifier properties
 							property = identifierProperty;
 						}
-						catch (MappingException ignore) {
+						catch ( MappingException ignore ) {
 							// ignore it...
 						}
 					}
@@ -592,7 +595,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 				}
 			}
 		}
-		catch (MappingException e) {
+		catch ( MappingException e ) {
 			throw new MappingException( "property [" + propertyPath + "] not found on entity [" + getEntityName() + "]" );
 		}
 
@@ -600,7 +603,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 	}
 
 	private Property getProperty(String propertyName, List<Property> properties) throws MappingException {
-		String root = StringHelper.root( propertyName );
+		String root = root( propertyName );
 		for ( Property prop : properties ) {
 			if ( prop.getName().equals( root )
 					|| ( prop instanceof Backref || prop instanceof IndexBackref )
@@ -614,7 +617,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 	public Property getProperty(String propertyName) throws MappingException {
 		Property identifierProperty = getIdentifierProperty();
 		if ( identifierProperty != null
-				&& identifierProperty.getName().equals( StringHelper.root( propertyName ) ) ) {
+				&& identifierProperty.getName().equals( root( propertyName ) ) ) {
 			return identifierProperty;
 		}
 		else {
@@ -628,17 +631,16 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 	}
 
 	public Property getSubclassProperty(String propertyName) throws MappingException {
-		Property identifierProperty = getIdentifierProperty();
+		final Property identifierProperty = getIdentifierProperty();
 		if ( identifierProperty != null
-				&& identifierProperty.getName().equals( StringHelper.root( propertyName ) ) ) {
+				&& identifierProperty.getName().equals( root( propertyName ) ) ) {
 			return identifierProperty;
 		}
 		else {
-			List<Property> closure = getSubclassPropertyClosure();
+			final Component identifierMapper = getIdentifierMapper();
-			Component identifierMapper = getIdentifierMapper();
+			final List<Property> closure = identifierMapper != null
-			if ( identifierMapper != null ) {
+					? new JoinedList<>( identifierMapper.getProperties(), getSubclassPropertyClosure() )
-				closure = new JoinedList<>( identifierMapper.getProperties(), closure );
+					: getSubclassPropertyClosure();
-			}
 			return getProperty( propertyName, closure );
 		}
 	}
@@ -657,7 +659,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 		}
 
 		for ( Property property : getPropertyClosure() ) {
-			if (property.getName().equals(name)) {
+			if ( property.getName().equals(name) ) {
 				return true;
 			}
 		}
@@ -691,7 +693,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 	public boolean isPropertyDefinedInHierarchy(String name) {
 		return hasProperty( name )
 			|| getSuperMappedSuperclass() != null && getSuperMappedSuperclass().isPropertyDefinedInHierarchy( name )
-			||getSuperclass() != null && getSuperclass().isPropertyDefinedInHierarchy(name);
+			|| getSuperclass() != null && getSuperclass().isPropertyDefinedInHierarchy( name );
 	}
 
 	/**
@@ -721,9 +723,9 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 	public void validate(Metadata mapping) throws MappingException {
 		for ( Property prop : getProperties() ) {
 			if ( !prop.isValid( mapping ) ) {
-				Type type = prop.getType();
+				final Type type = prop.getType();
-				int actualColumns = prop.getColumnSpan();
+				final int actualColumns = prop.getColumnSpan();
-				int requiredColumns = type.getColumnSpan(mapping);
+				final int requiredColumns = type.getColumnSpan(mapping);
 				throw new MappingException(
 						"Property '" + StringHelper.qualify( getEntityName(), prop.getName() )
 								+ "' maps to " + actualColumns + " columns but " + requiredColumns
@@ -835,7 +837,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 	public Iterator<Property> getPropertyIterator() {
 		final ArrayList<Iterator<Property>> iterators = new ArrayList<>();
 		iterators.add( properties.iterator() );
-		for (Join join : joins) {
+		for ( Join join : joins ) {
 			iterators.add( join.getPropertyIterator() );
 		}
 		return new JoinedIterator<>( iterators );
@@ -857,7 +859,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 	public List<Property> getProperties() {
 		final ArrayList<List<Property>> list = new ArrayList<>();
 		list.add( properties );
-		for (Join join : joins) {
+		for ( Join join : joins ) {
 			list.add( join.getProperties() );
 		}
 		return new JoinedList<>( list );
@@ -1043,10 +1045,10 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 	protected void checkPropertyColumnDuplication(Set<String> distinctColumns, List<Property> properties)
 			throws MappingException {
 		for ( Property prop : properties ) {
-			Value value = prop.getValue();
+			final Value value = prop.getValue();
 			if ( value instanceof Component ) {
-				Component component = (Component) value;
+				final Component component = (Component) value;
-				AggregateColumn aggregateColumn = component.getAggregateColumn();
+				final AggregateColumn aggregateColumn = component.getAggregateColumn();
 				if ( aggregateColumn == null ) {
 					checkPropertyColumnDuplication( distinctColumns, component.getProperties() );
 				}
@@ -1155,10 +1157,7 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 	}
 
 	public java.util.List<CallbackDefinition> getCallbackDefinitions() {
-		if ( callbackDefinitions == null ) {
+		return callbackDefinitions == null ? emptyList() : unmodifiableList( callbackDefinitions );
-			return Collections.emptyList();
-		}
-		return Collections.unmodifiableList( callbackDefinitions );
 	}
 
 	public void setIdentifierMapper(Component handle) {
@@ -1186,9 +1185,9 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 
 	// The following methods are added to support @MappedSuperclass in the metamodel
 	public List<Property> getDeclaredProperties() {
-		ArrayList<List<Property>> lists = new ArrayList<>();
+		final ArrayList<List<Property>> lists = new ArrayList<>();
 		lists.add( declaredProperties );
-		for (Join join : joins) {
+		for ( Join join : joins ) {
 			lists.add( join.getDeclaredProperties() );
 		}
 		return new JoinedList<>( lists );
@@ -1196,9 +1195,9 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl
 
 	@Deprecated(since = "6.0")
 	public Iterator<Property> getDeclaredPropertyIterator() {
-		ArrayList<Iterator<Property>> iterators = new ArrayList<>();
+		final ArrayList<Iterator<Property>> iterators = new ArrayList<>();
 		iterators.add( declaredProperties.iterator() );
-		for (Join join : joins) {
+		for ( Join join : joins ) {
 			iterators.add( join.getDeclaredPropertyIterator() );
 		}
 		return new JoinedIterator<>( iterators );

hibernate-core/src/main/java/org/hibernate/mapping/RootClass.java

@@ -18,10 +18,11 @@ import org.hibernate.boot.spi.MetadataBuildingContext;
 import org.hibernate.internal.CoreLogging;
 import org.hibernate.internal.CoreMessageLogger;
 import org.hibernate.internal.util.ReflectHelper;
-import org.hibernate.internal.util.StringHelper;
 import org.hibernate.internal.util.collections.SingletonIterator;
 import org.hibernate.persister.entity.EntityPersister;
 
+import static org.hibernate.internal.util.StringHelper.nullIfEmpty;
+
 /**
  * A mapping model object that represents the root class in an entity class
  * {@linkplain jakarta.persistence.Inheritance inheritance} hierarchy.
@@ -294,11 +295,38 @@ public class RootClass extends PersistentClass implements TableOwner {
 			);
 		}
 		checkCompositeIdentifier();
+		checkTableDuplication();
+	}
+
+	private void checkTableDuplication() {
+		if ( hasSubclasses() ) {
+			final Set<Table> tables = new HashSet<>();
+			tables.add( getTable() );
+			for ( Subclass subclass : getSubclasses() ) {
+				if ( !(subclass instanceof SingleTableSubclass) ) {
+					final Table table = subclass.getTable();
+					if ( !tables.add( table ) ) {
+						if ( getDiscriminator() == null ) {
+							throw new MappingException( "Two different subclasses of '" + getEntityName()
+									+ "' map to the table '" + table.getName()
+									+ "' and the hierarchy has no discriminator column" );
+						}
+						else {
+							// This is arguably not the right place to do this.
+							// Perhaps it's an issue better dealt with later on
+							// by the persisters.
+							forceDiscriminator = true;
+						}
+						break;
+					}
+				}
+			}
+		}
 	}
 
 	private void checkCompositeIdentifier() {
 		if ( getIdentifier() instanceof Component ) {
-			Component id = (Component) getIdentifier();
+			final Component id = (Component) getIdentifier();
 			if ( !id.isDynamic() ) {
 				final Class<?> idClass = id.getComponentClass();
 				if ( idClass != null ) {
@@ -328,7 +356,7 @@ public class RootClass extends PersistentClass implements TableOwner {
 	}
 
 	public void setCacheRegionName(String cacheRegionName) {
-		this.cacheRegionName = StringHelper.nullIfEmpty( cacheRegionName );
+		this.cacheRegionName = nullIfEmpty( cacheRegionName );
 	}
 
 	public boolean isLazyPropertiesCacheable() {
@@ -359,7 +387,7 @@ public class RootClass extends PersistentClass implements TableOwner {
 	}
 
 	public Set<Table> getIdentityTables() {
-		Set<Table> tables = new HashSet<>();
+		final Set<Table> tables = new HashSet<>();
 		for ( PersistentClass clazz : getSubclassClosure() ) {
 			if ( clazz.isAbstract() == null || !clazz.isAbstract() ) {
 				tables.add( clazz.getIdentityTable() );

hibernate-core/src/main/java/org/hibernate/mapping/Subclass.java

@@ -282,12 +282,14 @@ public class Subclass extends PersistentClass {
 
 	@Override
 	public boolean isClassOrSuperclassJoin(Join join) {
-		return super.isClassOrSuperclassJoin(join) || getSuperclass().isClassOrSuperclassJoin(join);
+		return super.isClassOrSuperclassJoin( join )
+			|| getSuperclass().isClassOrSuperclassJoin( join );
 	}
 
 	@Override
 	public boolean isClassOrSuperclassTable(Table table) {
-		return super.isClassOrSuperclassTable(table) || getSuperclass().isClassOrSuperclassTable(table);
+		return super.isClassOrSuperclassTable( table )
+			|| getSuperclass().isClassOrSuperclassTable( table );
 	}
 
 	@Override
@@ -307,8 +309,8 @@ public class Subclass extends PersistentClass {
 
 	@Override
 	public java.util.Set<String> getSynchronizedTables() {
-		HashSet<String> result = new HashSet<>();
+		final HashSet<String> result = new HashSet<>();
-		result.addAll(synchronizedTables);
+		result.addAll( synchronizedTables );
 		result.addAll( getSuperclass().getSynchronizedTables() );
 		return result;
 	}
@@ -320,15 +322,15 @@ public class Subclass extends PersistentClass {
 
 	@Override
 	public java.util.List<FilterConfiguration> getFilters() {
-		java.util.List<FilterConfiguration> filters = new ArrayList<>(super.getFilters());
+		final ArrayList<FilterConfiguration> filters = new ArrayList<>( super.getFilters() );
-		filters.addAll(getSuperclass().getFilters());
+		filters.addAll( getSuperclass().getFilters() );
 		return filters;
 	}
 
 	@Override
 	public boolean hasSubselectLoadableCollections() {
-		return super.hasSubselectLoadableCollections() ||
+		return super.hasSubselectLoadableCollections()
-			getSuperclass().hasSubselectLoadableCollections();
+			|| getSuperclass().hasSubselectLoadableCollections();
 	}
 
 	@Override

hibernate-core/src/main/java/org/hibernate/mapping/TableOwner.java

@@ -7,9 +7,11 @@
 package org.hibernate.mapping;
 
 /**
- * Additional, optional contract as part pf the {@link PersistentClass}
+ * Optional contract implemented by some subtypes of {@link PersistentClass}.
- * hierarchy used to differentiate entity bindings for entities that map to their own table
+ * <p>
- * (root, union-subclass, joined-subclass) versus those that do not (discriminator-subclass).
+ * Differentiates entity types that map to their own table ({@link RootClass},
+ * {@link UnionSubclass}, and {@link JoinedSubclass}) from those which do not
+ * ({@link SingleTableSubclass}).
  * 
  * @author Emmanuel Bernard
  * @author Steve Ebersole

hibernate-core/src/main/java/org/hibernate/mapping/UnionSubclass.java

@@ -30,7 +30,7 @@ public class UnionSubclass extends Subclass implements TableOwner {
 
 	public void setTable(Table table) {
 		this.table = table;
-		getSuperclass().addSubclassTable(table);
+		getSuperclass().addSubclassTable( table );
 	}
 
 	public java.util.Set<String> getSynchronizedTables() {

hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/repeatedtable/RepeatedSubclassTableTest.java

@@ -14,7 +14,6 @@ import jakarta.persistence.PrimaryKeyJoinColumn;
 import jakarta.persistence.Table;
 import org.hibernate.Session;
 import org.hibernate.Transaction;
-import org.hibernate.annotations.DiscriminatorOptions;
 import org.hibernate.cfg.Configuration;
 import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
 import org.junit.Test;
@@ -110,7 +109,6 @@ public class RepeatedSubclassTableTest extends BaseCoreFunctionalTestCase {
 	@Table(name = "DATA_TYPE")
 	@Inheritance(strategy = JOINED)
 	@DiscriminatorColumn(name = "supertype_id")
-	@DiscriminatorOptions(force = true)
 	public static abstract class DataType {
 
 		private Long id;

hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/repeatedtable/RepeatedTableTest.java

@@ -2,7 +2,6 @@ package org.hibernate.orm.test.inheritance.repeatedtable;
 
 import org.hibernate.Session;
 import org.hibernate.Transaction;
-import org.hibernate.annotations.DiscriminatorOptions;
 import org.hibernate.cfg.Configuration;
 import org.hibernate.testing.TestForIssue;
 import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
@@ -101,7 +100,6 @@ public class RepeatedTableTest extends BaseCoreFunctionalTestCase {
 	@Table(name = "DATA_TYPE")
 	@Inheritance(strategy = JOINED)
 	@DiscriminatorColumn(name = "supertype_id")
-	@DiscriminatorOptions(force = true)
 	public static abstract class DataType {
 
 		private Long id;