refactor handling of NaturalId unique keys

Signed-off-by: Gavin King <gavin@hibernate.org>

hibernate-core/src/main/java/org/hibernate/boot/internal/InFlightMetadataCollectorImpl.java

@@ -1300,9 +1300,8 @@ public class InFlightMetadataCollectorImpl implements InFlightMetadataCollector,
 		}
 
 		if ( logicalName == null ) {
-			throw new MappingException(
+			throw new MappingException( "Unable to find column with physical name '"
-					"Unable to find column with physical name " + physicalNameString + " in table " + table.getName()
+					+ physicalNameString + "' in table '" + table.getName() + "'" );
-			);
 		}
 		return logicalName.render();
 	}

hibernate-core/src/main/java/org/hibernate/boot/model/internal/AnnotatedColumn.java

@@ -1011,17 +1011,6 @@ public class AnnotatedColumn {
 		return columns;
 	}
 
-	void addUniqueKey(String uniqueKeyName, boolean inSecondPass) {
-		final UniqueKeySecondPass secondPass =
-				new UniqueKeySecondPass( uniqueKeyName, this, getBuildingContext() );
-		if ( inSecondPass ) {
-			secondPass.doSecondPass( getBuildingContext().getMetadataCollector().getEntityBindingMap() );
-		}
-		else {
-			getBuildingContext().getMetadataCollector().addSecondPass( secondPass );
-		}
-	}
-
 	@Override
 	public String toString() {
 		StringBuilder string = new StringBuilder();

hibernate-core/src/main/java/org/hibernate/boot/model/internal/AnnotatedColumns.java

@@ -14,6 +14,7 @@ import java.util.Map;
 import org.hibernate.AnnotationException;
 import org.hibernate.boot.spi.MetadataBuildingContext;
 import org.hibernate.mapping.Join;
+import org.hibernate.mapping.Property;
 import org.hibernate.mapping.Table;
 
 import static java.util.Collections.unmodifiableList;
@@ -65,6 +66,12 @@ public class AnnotatedColumns {
 		this.propertyName = propertyName;
 	}
 
+	Property resolveProperty() {
+		return buildingContext.getMetadataCollector().getEntityBindingMap()
+				.get( propertyHolder.getPersistentClass().getEntityName() )
+				.getReferencedProperty( propertyName );
+	}
+
 	public void setBuildingContext(MetadataBuildingContext buildingContext) {
 		this.buildingContext = buildingContext;
 	}
@@ -106,7 +113,7 @@ public class AnnotatedColumns {
 		final String explicitTableName = firstColumn.getExplicitTableName();
 		//note: checkPropertyConsistency() is responsible for ensuring they all have the same table name
 		return isNotEmpty( explicitTableName )
-				&& !getPropertyHolder().getTable().getName().equals( explicitTableName );
+			&& !getPropertyHolder().getTable().getName().equals( explicitTableName );
 	}
 
 	/**

hibernate-core/src/main/java/org/hibernate/boot/model/internal/AnnotatedJoinColumns.java

@@ -66,7 +66,6 @@ import static org.hibernate.internal.util.StringHelper.qualify;
 public class AnnotatedJoinColumns extends AnnotatedColumns {
 
 	private final List<AnnotatedJoinColumn> columns = new ArrayList<>();
-	private String propertyName; // this is really a .-separated property path
 
 	private String referencedProperty;
 
@@ -105,15 +104,12 @@ public class AnnotatedJoinColumns extends AnnotatedColumns {
 			}
 		}
 
-		handlePropertyRef( inferredData.getAttributeMember(), parent, context );
+		handlePropertyRef( inferredData.getAttributeMember(), parent );
 
 		return parent;
 	}
 
-	private static void handlePropertyRef(
+	private static void handlePropertyRef(MemberDetails attributeMember, AnnotatedJoinColumns parent) {
-			MemberDetails attributeMember,
-			AnnotatedJoinColumns parent,
-			MetadataBuildingContext context) {
 		final PropertyRef propertyRefUsage = attributeMember.getDirectAnnotationUsage( PropertyRef.class );
 		if ( propertyRefUsage == null ) {
 			return;
@@ -138,7 +134,7 @@ public class AnnotatedJoinColumns extends AnnotatedColumns {
 		joinColumns.setPropertyHolder( propertyHolder );
 		joinColumns.setPropertyName( getRelativePath( propertyHolder, inferredData.getPropertyName() ) );
 		AnnotatedJoinColumn.buildJoinFormula( joinFormula, joinColumns );
-		handlePropertyRef( inferredData.getAttributeMember(), joinColumns, context );
+		handlePropertyRef( inferredData.getAttributeMember(), joinColumns );
 		return joinColumns;
 	}
 
@@ -208,7 +204,7 @@ public class AnnotatedJoinColumns extends AnnotatedColumns {
 				);
 			}
 		}
-		handlePropertyRef( memberDetails, parent, context );
+		handlePropertyRef( memberDetails, parent );
 		return parent;
 	}
 
@@ -236,7 +232,7 @@ public class AnnotatedJoinColumns extends AnnotatedColumns {
 				AnnotatedJoinColumn.buildExplicitJoinTableJoinColumn( parent, propertyHolder, inferredData, joinColumn );
 			}
 		}
-		handlePropertyRef( inferredData.getAttributeMember(), parent, context );
+		handlePropertyRef( inferredData.getAttributeMember(), parent );
 		return parent;
 	}
 
@@ -508,19 +504,6 @@ public class AnnotatedJoinColumns extends AnnotatedColumns {
 			|| getMappedByPropertyName() != null;
 	}
 
-	/**
-	 * A property path relative to the {@link #getPropertyHolder() PropertyHolder}.
-	 */
-	@Override
-	public String getPropertyName() {
-		return propertyName;
-	}
-
-	@Override
-	public void setPropertyName(String propertyName) {
-		this.propertyName = propertyName;
-	}
-
 	private ImplicitJoinColumnNameSource.Nature getImplicitNature() {
 		if ( getPropertyHolder().isEntity() ) {
 			return ImplicitJoinColumnNameSource.Nature.ENTITY;

hibernate-core/src/main/java/org/hibernate/boot/model/internal/NaturalIdBinder.java

@@ -0,0 +1,119 @@
+/*
+ * Hibernate, Relational Persistence for Idiomatic Java
+ *
+ * License: GNU Lesser General Public License (LGPL), version 2.1 or later.
+ * See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html.
+ */
+package org.hibernate.boot.model.internal;
+
+import org.hibernate.AnnotationException;
+import org.hibernate.annotations.NaturalId;
+import org.hibernate.boot.model.naming.Identifier;
+import org.hibernate.boot.model.naming.ImplicitUniqueKeyNameSource;
+import org.hibernate.boot.spi.InFlightMetadataCollector;
+import org.hibernate.boot.spi.MetadataBuildingContext;
+import org.hibernate.mapping.Property;
+import org.hibernate.mapping.Selectable;
+import org.hibernate.mapping.Table;
+import org.hibernate.mapping.UniqueKey;
+import org.hibernate.models.spi.MemberDetails;
+
+import java.util.List;
+
+import static java.util.Collections.singletonList;
+import static org.hibernate.boot.model.naming.Identifier.toIdentifier;
+
+/**
+ * @author Gavin King
+ */
+class NaturalIdBinder {
+
+	static void addNaturalIds(
+			boolean inSecondPass,
+			MemberDetails property,
+			AnnotatedColumns columns,
+			AnnotatedJoinColumns joinColumns,
+			MetadataBuildingContext context) {
+		// Natural ID columns must reside in one single UniqueKey within the Table.
+		// For now, simply ensure consistent naming.
+		final NaturalId naturalId = property.getDirectAnnotationUsage( NaturalId.class );
+		if ( naturalId != null ) {
+			final AnnotatedColumns annotatedColumns = joinColumns != null ? joinColumns : columns;
+			final Identifier name = uniqueKeyName( context, annotatedColumns );
+			if ( inSecondPass ) {
+				addColumnsToUniqueKey( annotatedColumns, name );
+			}
+			else {
+				context.getMetadataCollector()
+						.addSecondPass( persistentClasses -> addColumnsToUniqueKey( annotatedColumns, name ) );
+			}
+		}
+	}
+
+	private static Identifier uniqueKeyName(MetadataBuildingContext context, AnnotatedColumns annotatedColumns) {
+		return context.getBuildingOptions().getImplicitNamingStrategy()
+				.determineUniqueKeyName( new NaturalIdNameSource( annotatedColumns.getTable(), context) );
+	}
+
+	private static void addColumnsToUniqueKey(AnnotatedColumns columns, Identifier name) {
+		final InFlightMetadataCollector collector = columns.getBuildingContext().getMetadataCollector();
+		final Table table = columns.getTable();
+		final UniqueKey uniqueKey = table.getOrCreateUniqueKey( name.render( collector.getDatabase().getDialect() ) );
+		final Property property = columns.resolveProperty();
+		if ( property.isComposite() ) {
+			for ( Selectable selectable : property.getValue().getSelectables() ) {
+				if ( selectable instanceof org.hibernate.mapping.Column) {
+					uniqueKey.addColumn( tableColumn( (org.hibernate.mapping.Column) selectable, table, collector ) );
+				}
+			}
+		}
+		else {
+			for ( AnnotatedColumn column : columns.getColumns() ) {
+				uniqueKey.addColumn( tableColumn( column.getMappingColumn(), table, collector ) );
+			}
+		}
+	}
+
+	private static org.hibernate.mapping.Column tableColumn(
+			org.hibernate.mapping.Column column, Table table, InFlightMetadataCollector collector) {
+		final String columnName = collector.getLogicalColumnName( table, column.getQuotedName() );
+		final org.hibernate.mapping.Column tableColumn = table.getColumn( collector, columnName );
+		if ( tableColumn == null ) {
+			throw new AnnotationException(
+					"Table '" + table.getName() + "' has no column named '" + columnName
+							+ "' matching the column specified in '@Index'"
+			);
+		}
+		return tableColumn;
+	}
+
+	private static class NaturalIdNameSource implements ImplicitUniqueKeyNameSource {
+		private final Table table;
+		private final MetadataBuildingContext context;
+
+		NaturalIdNameSource(Table table, MetadataBuildingContext context) {
+			this.table = table;
+			this.context = context;
+		}
+
+		@Override
+		public Identifier getTableName() {
+			return table.getNameIdentifier();
+		}
+
+		@Override
+		public List<Identifier> getColumnNames() {
+			return singletonList( toIdentifier("_NaturalID") );
+		}
+
+		@Override
+		public Identifier getUserProvidedIdentifier() {
+			return null;
+		}
+
+		@Override
+		public MetadataBuildingContext getBuildingContext() {
+			return context;
+		}
+	}
+}

hibernate-core/src/main/java/org/hibernate/boot/model/internal/PropertyBinder.java

@@ -25,10 +25,6 @@ import org.hibernate.annotations.OptimisticLock;
 import org.hibernate.annotations.Parent;
 import org.hibernate.binder.AttributeBinder;
 import org.hibernate.boot.model.IdentifierGeneratorDefinition;
-import org.hibernate.boot.model.naming.Identifier;
-import org.hibernate.boot.model.naming.ImplicitNamingStrategy;
-import org.hibernate.boot.model.naming.ImplicitUniqueKeyNameSource;
-import org.hibernate.boot.model.relational.Database;
 import org.hibernate.boot.models.JpaAnnotations;
 import org.hibernate.boot.spi.AccessType;
 import org.hibernate.boot.spi.InFlightMetadataCollector;
@@ -79,7 +75,6 @@ import jakarta.persistence.OneToOne;
 import jakarta.persistence.Version;
 
 import static jakarta.persistence.FetchType.LAZY;
-import static java.util.Collections.singletonList;
 import static org.hibernate.boot.model.internal.AnyBinder.bindAny;
 import static org.hibernate.boot.model.internal.BinderHelper.getMappedSuperclassOrNull;
 import static org.hibernate.boot.model.internal.BinderHelper.getPath;
@@ -90,13 +85,14 @@ import static org.hibernate.boot.model.internal.ClassPropertyHolder.prepareActua
 import static org.hibernate.boot.model.internal.CollectionBinder.bindCollection;
 import static org.hibernate.boot.model.internal.EmbeddableBinder.createCompositeBinder;
 import static org.hibernate.boot.model.internal.EmbeddableBinder.createEmbeddable;
+import static org.hibernate.boot.model.internal.EmbeddableBinder.determineCustomInstantiator;
 import static org.hibernate.boot.model.internal.EmbeddableBinder.isEmbedded;
 import static org.hibernate.boot.model.internal.GeneratorBinder.createIdGeneratorsFromGeneratorAnnotations;
 import static org.hibernate.boot.model.internal.GeneratorBinder.createValueGeneratorFromAnnotations;
+import static org.hibernate.boot.model.internal.NaturalIdBinder.addNaturalIds;
 import static org.hibernate.boot.model.internal.TimeZoneStorageHelper.resolveTimeZoneStorageCompositeUserType;
 import static org.hibernate.boot.model.internal.ToOneBinder.bindManyToOne;
 import static org.hibernate.boot.model.internal.ToOneBinder.bindOneToOne;
-import static org.hibernate.boot.model.naming.Identifier.toIdentifier;
 import static org.hibernate.id.IdentifierGeneratorHelper.getForeignId;
 import static org.hibernate.internal.util.StringHelper.qualify;
 
@@ -764,18 +760,14 @@ public class PropertyBinder {
 			MetadataBuildingContext context,
 			Map<ClassDetails, InheritanceState> inheritanceStatePerClass,
 			MemberDetails property) {
-		final TypeDetails attributeTypeDetails = inferredData.getAttributeMember().isPlural()
+		final TypeDetails attributeTypeDetails =
-				? inferredData.getAttributeMember().getType()
+				inferredData.getAttributeMember().isPlural()
-				: inferredData.getClassOrElementType();
+						? inferredData.getAttributeMember().getType()
+						: inferredData.getClassOrElementType();
 		final ClassDetails attributeClassDetails = attributeTypeDetails.determineRawClass();
-		final ColumnsBuilder columnsBuilder = new ColumnsBuilder(
+		final ColumnsBuilder columnsBuilder =
-				propertyHolder,
+				new ColumnsBuilder( propertyHolder, nullability, property, inferredData, entityBinder, context )
-				nullability,
+						.extractMetadata();
-				property,
-				inferredData,
-				entityBinder,
-				context
-		).extractMetadata();
 
 		final PropertyBinder propertyBinder = new PropertyBinder();
 		propertyBinder.setName( inferredData.getPropertyName() );
@@ -941,11 +933,6 @@ public class PropertyBinder {
 			|| property.hasDirectAnnotationUsage( ManyToAny.class );
 	}
 
-	private static boolean isForcePersist(MemberDetails property) {
-		return property.hasDirectAnnotationUsage( MapsId.class )
-			|| property.hasDirectAnnotationUsage( Id.class );
-	}
-
 	private static void bindVersionProperty(
 			PropertyHolder propertyHolder,
 			PropertyData inferredData,
@@ -1073,7 +1060,7 @@ public class PropertyBinder {
 								inheritanceStatePerClass,
 								null,
 								null,
-								EmbeddableBinder.determineCustomInstantiator( property, returnedClass, context ),
+								determineCustomInstantiator( property, returnedClass, context ),
 								compositeUserType,
 								null,
 								columns
@@ -1117,7 +1104,7 @@ public class PropertyBinder {
 							inheritanceStatePerClass,
 							null,
 							null,
-							EmbeddableBinder.determineCustomInstantiator( property, property.getElementType().determineRawClass(), context ),
+							determineCustomInstantiator( property, property.getElementType().determineRawClass(), context ),
 							compositeUserType,
 							null,
 							columns
@@ -1304,91 +1291,23 @@ public class PropertyBinder {
 		return annotationUsage != null && annotationUsage.fetch() == LAZY;
 	}
 
-	private static void addNaturalIds(
-			boolean inSecondPass,
-			MemberDetails property,
-			AnnotatedColumns columns,
-			AnnotatedJoinColumns joinColumns,
-			MetadataBuildingContext context) {
-		// Natural ID columns must reside in one single UniqueKey within the Table.
-		// For now, simply ensure consistent naming.
-		// TODO: AFAIK, there really isn't a reason for these UKs to be created
-		//       on the SecondPass. This whole area should go away...
-		final NaturalId naturalId = property.getDirectAnnotationUsage( NaturalId.class );
-		if ( naturalId != null ) {
-			final Database database = context.getMetadataCollector().getDatabase();
-			final ImplicitNamingStrategy implicitNamingStrategy = context.getBuildingOptions().getImplicitNamingStrategy();
-			if ( joinColumns != null ) {
-				final Identifier name = implicitNamingStrategy.determineUniqueKeyName( new ImplicitUniqueKeyNameSource() {
-					@Override
-					public Identifier getTableName() {
-						return joinColumns.getTable().getNameIdentifier();
-					}
-
-					@Override
-					public List<Identifier> getColumnNames() {
-						return singletonList(toIdentifier("_NaturalID"));
-					}
-
-					@Override
-					public Identifier getUserProvidedIdentifier() {
-						return null;
-					}
-
-					@Override
-					public MetadataBuildingContext getBuildingContext() {
-						return context;
-					}
-				});
-				final String keyName = name.render( database.getDialect() );
-				for ( AnnotatedColumn column : joinColumns.getColumns() ) {
-					column.addUniqueKey( keyName, inSecondPass );
-				}
-			}
-			else {
-				final Identifier name = implicitNamingStrategy.determineUniqueKeyName(new ImplicitUniqueKeyNameSource() {
-					@Override
-					public Identifier getTableName() {
-						return columns.getTable().getNameIdentifier();
-					}
-
-					@Override
-					public List<Identifier> getColumnNames() {
-						return singletonList(toIdentifier("_NaturalID"));
-					}
-
-					@Override
-					public Identifier getUserProvidedIdentifier() {
-						return null;
-					}
-
-					@Override
-					public MetadataBuildingContext getBuildingContext() {
-						return context;
-					}
-				});
-				final String keyName = name.render( database.getDialect() );
-				for ( AnnotatedColumn column : columns.getColumns() ) {
-					column.addUniqueKey( keyName, inSecondPass );
-				}
-			}
-		}
-	}
-
 	private static Class<? extends CompositeUserType<?>> resolveCompositeUserType(
 			PropertyData inferredData,
 			MetadataBuildingContext context) {
-		final SourceModelBuildingContext sourceModelContext = context.getMetadataCollector().getSourceModelBuildingContext();
+		final SourceModelBuildingContext sourceModelContext =
+				context.getMetadataCollector().getSourceModelBuildingContext();
 		final MemberDetails attributeMember = inferredData.getAttributeMember();
 		final TypeDetails classOrElementType = inferredData.getClassOrElementType();
 		final ClassDetails returnedClass = classOrElementType.determineRawClass();
 
 		if ( attributeMember != null ) {
-			final CompositeType compositeType = attributeMember.locateAnnotationUsage( CompositeType.class, sourceModelContext );
+			final CompositeType compositeType =
+					attributeMember.locateAnnotationUsage( CompositeType.class, sourceModelContext );
 			if ( compositeType != null ) {
 				return compositeType.value();
 			}
-			final Class<? extends CompositeUserType<?>> compositeUserType = resolveTimeZoneStorageCompositeUserType( attributeMember, returnedClass, context );
+			final Class<? extends CompositeUserType<?>> compositeUserType =
+					resolveTimeZoneStorageCompositeUserType( attributeMember, returnedClass, context );
 			if ( compositeUserType != null ) {
 				return compositeUserType;
 			}

hibernate-core/src/main/java/org/hibernate/boot/model/internal/UniqueKeySecondPass.java

@@ -1,94 +0,0 @@
-/*
- * Hibernate, Relational Persistence for Idiomatic Java
- *
- * License: GNU Lesser General Public License (LGPL), version 2.1 or later.
- * See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html.
- */
-package org.hibernate.boot.model.internal;
-
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Map;
-
-import org.hibernate.AnnotationException;
-import org.hibernate.MappingException;
-import org.hibernate.boot.spi.MetadataBuildingContext;
-import org.hibernate.boot.spi.SecondPass;
-import org.hibernate.mapping.Column;
-import org.hibernate.mapping.Component;
-import org.hibernate.mapping.PersistentClass;
-import org.hibernate.mapping.Property;
-import org.hibernate.mapping.Selectable;
-import org.hibernate.mapping.Table;
-import org.hibernate.mapping.UniqueKey;
-
-/**
- * @author Emmanuel Bernard
- */
-public class UniqueKeySecondPass implements SecondPass {
-	private final String indexName;
-	private final MetadataBuildingContext buildingContext;
-	private final AnnotatedColumn column;
-
-	/**
-	 * Build an index if unique is false or a Unique Key if unique is true
-	 */
-	public UniqueKeySecondPass(String indexName, AnnotatedColumn column, MetadataBuildingContext buildingContext) {
-		this.indexName = indexName;
-		this.column = column;
-		this.buildingContext = buildingContext;
-	}
-
-	@Override
-	public void doSecondPass(Map<String, PersistentClass> persistentClasses) throws MappingException {
-		if ( column != null ) {
-			final AnnotatedColumns annotatedColumns = column.getParent();
-			final Table table = annotatedColumns.getTable();
-			final PropertyHolder propertyHolder = annotatedColumns.getPropertyHolder();
-			final String entityName =
-					propertyHolder.isComponent()
-							? propertyHolder.getPersistentClass().getEntityName()
-							: propertyHolder.getEntityName();
-			final String propertyName = annotatedColumns.getPropertyName();
-			final Property property = persistentClasses.get( entityName ).getProperty( propertyName );
-			addConstraintToProperty( property, table );
-		}
-	}
-
-	private void addConstraintToProperty(Property property, Table table) {
-		if ( property.getValue() instanceof Component ) {
-			final Component component = (Component) property.getValue();
-			final List<Column> columns = new ArrayList<>();
-			for ( Selectable selectable: component.getSelectables() ) {
-				if ( selectable instanceof Column ) {
-					columns.add( (Column) selectable );
-				}
-			}
-			addConstraintToColumns( columns, table );
-		}
-		else {
-			addConstraintToColumn( column.getMappingColumn(), table );
-		}
-	}
-
-	private void addConstraintToColumn(Column mappingColumn, Table table) {
-		final String columnName =
-				buildingContext.getMetadataCollector()
-						.getLogicalColumnName( table, mappingColumn.getQuotedName() );
-		final Column column = table.getColumn( buildingContext.getMetadataCollector(), columnName );
-		if ( column == null ) {
-			throw new AnnotationException(
-					"Table '" + table.getName() + "' has no column named '" + columnName
-							+ "' matching the column specified in '@Index'"
-			);
-		}
-		table.getOrCreateUniqueKey( indexName ).addColumn( column );
-	}
-
-	private void addConstraintToColumns(List<Column> columns, Table table) {
-		final UniqueKey uniqueKey = table.getOrCreateUniqueKey( indexName );
-		for ( Column column : columns ) {
-			uniqueKey.addColumn( column );
-		}
-	}
-}

hibernate-core/src/test/java/org/hibernate/orm/test/annotations/naturalid/NaturalIdTest.java

@@ -20,7 +20,6 @@ import org.hibernate.query.Query;
 import org.hibernate.stat.Statistics;
 
 import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
-import org.hibernate.testing.orm.junit.SessionFactoryScope;
 import org.junit.After;
 import org.junit.Test;
 import org.junit.jupiter.api.Assertions;
@@ -37,7 +36,6 @@ import static org.junit.jupiter.api.Assertions.assertNull;
  * @author Emmanuel Bernard
  * @author Hardy Ferentschik
  */
-@SuppressWarnings("unchecked")
 public class NaturalIdTest extends BaseCoreFunctionalTestCase {
 	@After
 	public void cleanupData() {