From 01d608ca849c48d488557a7887e2edef46420f2c Mon Sep 17 00:00:00 2001 From: Gavin Date: Mon, 26 Dec 2022 01:02:39 +0100 Subject: [PATCH] HHH-14526 validate table repetition in JOINED hierarchies and automatically force the discriminator when necessary --- .../org/hibernate/mapping/JoinedSubclass.java | 4 +- .../hibernate/mapping/PersistentClass.java | 87 +++++++++---------- .../java/org/hibernate/mapping/RootClass.java | 36 +++++++- .../java/org/hibernate/mapping/Subclass.java | 18 ++-- .../org/hibernate/mapping/TableOwner.java | 8 +- .../org/hibernate/mapping/UnionSubclass.java | 2 +- .../RepeatedSubclassTableTest.java | 2 - .../repeatedtable/RepeatedTableTest.java | 2 - 8 files changed, 93 insertions(+), 66 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/mapping/JoinedSubclass.java b/hibernate-core/src/main/java/org/hibernate/mapping/JoinedSubclass.java index c3c627438a..51b6c036ec 100644 --- a/hibernate-core/src/main/java/org/hibernate/mapping/JoinedSubclass.java +++ b/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; - getSuperclass().addSubclassTable(table); + this.table = table; + getSuperclass().addSubclassTable( table ); } public KeyValue getKey() { diff --git a/hibernate-core/src/main/java/org/hibernate/mapping/PersistentClass.java b/hibernate-core/src/main/java/org/hibernate/mapping/PersistentClass.java index 5a1015a877..938d7e75f4 100644 --- a/hibernate-core/src/main/java/org/hibernate/mapping/PersistentClass.java +++ b/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[] subclassLists = new List[subclasses.size() + 1]; int j; for (j = 0; j < subclasses.size(); j++) { - Subclass subclass = subclasses.get(j); - subclassLists[j] = subclass.getSubclasses(); + subclassLists[j] = subclasses.get(j).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 getSubclassIterator() { @SuppressWarnings("unchecked") - final Iterator[] iters = new Iterator[subclasses.size() + 1]; - final Iterator iter = subclasses.iterator(); + final Iterator[] iterators = new Iterator[subclasses.size() + 1]; + final Iterator iterator = subclasses.iterator(); int i = 0; - while ( iter.hasNext() ) { - iters[i++] = iter.next().getSubclassIterator(); + while ( iterator.hasNext() ) { + iterators[i++] = iterator.next().getSubclassIterator(); } - iters[i] = subclasses.iterator(); - return new JoinedIterator<>( iters ); + iterators[i] = subclasses.iterator(); + return new JoinedIterator<>( iterators ); } public List getSubclassClosure() { @@ -275,12 +278,12 @@ public abstract class PersistentClass implements AttributeContainer, Serializabl @Deprecated(since = "6.0") @Remove public Iterator getSubclassClosureIterator() { - final ArrayList> iters = new ArrayList<>(); - iters.add( new SingletonIterator<>( this ) ); + final ArrayList> iterators = new ArrayList<>(); + 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> 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 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 closure = getSubclassPropertyClosure(); - Component identifierMapper = getIdentifierMapper(); - if ( identifierMapper != null ) { - closure = new JoinedList<>( identifierMapper.getProperties(), closure ); - } + final Component identifierMapper = getIdentifierMapper(); + final List closure = identifierMapper != null + ? new JoinedList<>( identifierMapper.getProperties(), getSubclassPropertyClosure() ) + : 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(); - int actualColumns = prop.getColumnSpan(); - int requiredColumns = type.getColumnSpan(mapping); + final Type type = prop.getType(); + final int actualColumns = prop.getColumnSpan(); + 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 getPropertyIterator() { final ArrayList> 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 getProperties() { final ArrayList> 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 distinctColumns, List properties) throws MappingException { for ( Property prop : properties ) { - Value value = prop.getValue(); + final Value value = prop.getValue(); if ( value instanceof Component ) { - Component component = (Component) value; - AggregateColumn aggregateColumn = component.getAggregateColumn(); + final Component component = (Component) value; + 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 getCallbackDefinitions() { - if ( callbackDefinitions == null ) { - return Collections.emptyList(); - } - return Collections.unmodifiableList( callbackDefinitions ); + return callbackDefinitions == null ? emptyList() : 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 getDeclaredProperties() { - ArrayList> lists = new ArrayList<>(); + final ArrayList> 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 getDeclaredPropertyIterator() { - ArrayList> iterators = new ArrayList<>(); + final ArrayList> iterators = new ArrayList<>(); iterators.add( declaredProperties.iterator() ); - for (Join join : joins) { + for ( Join join : joins ) { iterators.add( join.getDeclaredPropertyIterator() ); } return new JoinedIterator<>( iterators ); diff --git a/hibernate-core/src/main/java/org/hibernate/mapping/RootClass.java b/hibernate-core/src/main/java/org/hibernate/mapping/RootClass.java index 000e8bab59..cbd3ffcc44 100644 --- a/hibernate-core/src/main/java/org/hibernate/mapping/RootClass.java +++ b/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 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
getIdentityTables() { - Set
tables = new HashSet<>(); + final Set
tables = new HashSet<>(); for ( PersistentClass clazz : getSubclassClosure() ) { if ( clazz.isAbstract() == null || !clazz.isAbstract() ) { tables.add( clazz.getIdentityTable() ); diff --git a/hibernate-core/src/main/java/org/hibernate/mapping/Subclass.java b/hibernate-core/src/main/java/org/hibernate/mapping/Subclass.java index d1f7757f1d..f473516444 100644 --- a/hibernate-core/src/main/java/org/hibernate/mapping/Subclass.java +++ b/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 getSynchronizedTables() { - HashSet result = new HashSet<>(); - result.addAll(synchronizedTables); + final HashSet result = new HashSet<>(); + result.addAll( synchronizedTables ); result.addAll( getSuperclass().getSynchronizedTables() ); return result; } @@ -320,15 +322,15 @@ public class Subclass extends PersistentClass { @Override public java.util.List getFilters() { - java.util.List filters = new ArrayList<>(super.getFilters()); - filters.addAll(getSuperclass().getFilters()); + final ArrayList filters = new ArrayList<>( super.getFilters() ); + filters.addAll( getSuperclass().getFilters() ); return filters; } @Override public boolean hasSubselectLoadableCollections() { - return super.hasSubselectLoadableCollections() || - getSuperclass().hasSubselectLoadableCollections(); + return super.hasSubselectLoadableCollections() + || getSuperclass().hasSubselectLoadableCollections(); } @Override diff --git a/hibernate-core/src/main/java/org/hibernate/mapping/TableOwner.java b/hibernate-core/src/main/java/org/hibernate/mapping/TableOwner.java index 1563fd63c6..227d3e3db0 100644 --- a/hibernate-core/src/main/java/org/hibernate/mapping/TableOwner.java +++ b/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} - * hierarchy used to differentiate entity bindings for entities that map to their own table - * (root, union-subclass, joined-subclass) versus those that do not (discriminator-subclass). + * Optional contract implemented by some subtypes of {@link PersistentClass}. + *

+ * 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 diff --git a/hibernate-core/src/main/java/org/hibernate/mapping/UnionSubclass.java b/hibernate-core/src/main/java/org/hibernate/mapping/UnionSubclass.java index 08967e874d..ab2146e5ae 100644 --- a/hibernate-core/src/main/java/org/hibernate/mapping/UnionSubclass.java +++ b/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 getSynchronizedTables() { diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/repeatedtable/RepeatedSubclassTableTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/repeatedtable/RepeatedSubclassTableTest.java index 55d31b2a77..aedc5b2b26 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/repeatedtable/RepeatedSubclassTableTest.java +++ b/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; diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/repeatedtable/RepeatedTableTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/repeatedtable/RepeatedTableTest.java index 49407dab37..07f6686308 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/repeatedtable/RepeatedTableTest.java +++ b/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;