HHH-18830 remove workaround after fix

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

@@ -21,6 +21,7 @@
 import org.hibernate.resource.beans.spi.ManagedBean;
 import org.hibernate.usertype.UserCollectionType;
 
+import static org.hibernate.boot.model.internal.PropertyHolderBuilder.buildPropertyHolder;
 import static org.hibernate.internal.util.StringHelper.qualify;
 
 /**
@@ -65,14 +66,8 @@ public void secondPass(Map<String, PersistentClass> persistentClasses) throws Ma
 	}
 
 	private void bindIndex() {
-		final PropertyHolder valueHolder = PropertyHolderBuilder.buildPropertyHolder(
-				collection,
-				qualify( collection.getRole(), "key" ),
-				null,
-				null,
-				propertyHolder,
-				getBuildingContext()
-		);
+		final PropertyHolder valueHolder =
+				buildPropertyHolder( collection, getPath(), null, null, propertyHolder, buildingContext );
 
 		if ( !collection.isOneToMany() ) {
 			indexColumn.forceNotNull();
@@ -94,6 +89,10 @@ private void bindIndex() {
 		createBackref();
 	}
 
+	private String getPath() {
+		return qualify( collection.getRole(), "key" );
+	}
+
 	private void createBackref() {
 		if ( collection.isOneToMany()
 				&& !collection.getKey().isNullable()

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

@@ -88,9 +88,10 @@ protected Collection createCollection(PersistentClass owner) {
 
 	@Override
 	SecondPass getSecondPass() {
-		return new CollectionSecondPass( MapBinder.this.collection ) {
+		return new CollectionSecondPass( collection ) {
 			public void secondPass(java.util.Map<String, PersistentClass> persistentClasses)
 					throws MappingException {
+				getMap().setHasMapKeyProperty( hasMapKeyProperty );
 				bindStarToManySecondPass( persistentClasses );
 				bindKeyFromAssociationTable(
 						getElementType(),
@@ -290,14 +291,8 @@ private CollectionPropertyHolder buildCollectionPropertyHolder(
 	private CollectionPropertyHolder buildCollectionPropertyHolder(
 			MemberDetails property,
 			ClassDetails keyClass) {
-		final CollectionPropertyHolder holder = buildPropertyHolder(
-				collection,
-				qualify( collection.getRole(), "mapkey" ),
-				keyClass,
-				property,
-				propertyHolder,
-				buildingContext
-		);
+		final CollectionPropertyHolder holder =
+				buildPropertyHolder( collection, getPath(), keyClass, property, propertyHolder, buildingContext );
 		// 'propertyHolder' is the PropertyHolder for the owner of the collection
 		// 'holder' is the CollectionPropertyHolder.
 		// 'property' is the collection XProperty
@@ -306,6 +301,10 @@ private CollectionPropertyHolder buildCollectionPropertyHolder(
 		return holder;
 	}
 
+	private String getPath() {
+		return qualify( collection.getRole(), "mapkey" );
+	}
+
 	private void handleForeignKey(MemberDetails property, ManyToOne element) {
 		final ForeignKey foreignKey = getMapKeyForeignKey( property );
 		if ( foreignKey != null ) {
@@ -471,20 +470,20 @@ private Value createFormulatedValue(
 			Collection collection,
 			PersistentClass associatedClass,
 			PersistentClass targetPropertyPersistentClass) {
-		if ( value instanceof Component ) {
-			return createIndexComponent( collection, associatedClass, (Component) value );
+		if ( value instanceof Component component ) {
+			return createIndexComponent( collection, associatedClass, component );
 		}
 		else {
 			// HHH-11005 - only if we are @OneToMany and location of map key property is
 			// at a different level, need to add a select
 			final Table mapKeyTable = !associatedClass.equals( targetPropertyPersistentClass )
 					? targetPropertyPersistentClass.getTable()
 					: associatedClass.getTable();
-			if ( value instanceof BasicValue ) {
-				return createDependantBasicValue( mapKeyTable, (BasicValue) value );
+			if ( value instanceof BasicValue basicValue ) {
+				return createDependantBasicValue( mapKeyTable, basicValue );
 			}
-			else if ( value instanceof SimpleValue ) {
-				return createTargetValue( mapKeyTable, (SimpleValue) value );
+			else if ( value instanceof SimpleValue simpleValue ) {
+				return createTargetValue( mapKeyTable, simpleValue );
 			}
 			else {
 				throw new AssertionFailure( "Unknown type encountered for map key: " + value.getClass() );
@@ -525,11 +524,11 @@ private DependantBasicValue createDependantBasicValue(Table mapKeyTable, BasicVa
 	}
 
 	private static void addSelectable(SimpleValue targetValue, Selectable selectable) {
-		if ( selectable instanceof Column ) {
-			targetValue.addColumn( ( (Column) selectable).clone(), false, false  );
+		if ( selectable instanceof Column column ) {
+			targetValue.addColumn( column.clone(), false, false  );
 		}
-		else if ( selectable instanceof Formula ) {
-			targetValue.addFormula( new Formula( ( (Formula) selectable).getFormula() ) );
+		else if ( selectable instanceof Formula formula ) {
+			targetValue.addFormula( new Formula( formula.getFormula() ) );
 		}
 		else {
 			throw new AssertionFailure( "Unknown element in column iterator: " + selectable.getClass() );

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

@@ -60,8 +60,8 @@ private static void addColumnsToUniqueKey(AnnotatedColumns columns, Identifier n
 		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 ) );
+				if ( selectable instanceof org.hibernate.mapping.Column column) {
+					uniqueKey.addColumn( tableColumn( column, table, collector ) );
 				}
 			}
 		}

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

@@ -442,23 +442,23 @@ private void handleValueGeneration(Property property) {
 	}
 
 	private void handleLob(Property property) {
-		if ( this.memberDetails != null ) {
+		if ( memberDetails != null ) {
 			// HHH-4635 -- needed for dialect-specific property ordering
-			property.setLob( this.memberDetails.hasDirectAnnotationUsage( Lob.class ) );
+			property.setLob( memberDetails.hasDirectAnnotationUsage( Lob.class ) );
 		}
 	}
 
 	private void handleMutability(Property property) {
-		if ( this.memberDetails != null && this.memberDetails.hasDirectAnnotationUsage( Immutable.class ) ) {
+		if ( memberDetails != null && memberDetails.hasDirectAnnotationUsage( Immutable.class ) ) {
 			updatable = false;
 		}
 		property.setInsertable( insertable );
 		property.setUpdateable( updatable );
 	}
 
 	private void handleOptional(Property property) {
-		if ( this.memberDetails != null ) {
-			property.setOptional( !isId && isOptional( this.memberDetails, this.holder ) );
+		if ( memberDetails != null ) {
+			property.setOptional( !isId && isOptional( memberDetails, holder ) );
 			if ( property.isOptional() ) {
 				final OptionalDeterminationSecondPass secondPass = persistentClasses -> {
 					// Defer determining whether a property and its columns are nullable,
@@ -487,8 +487,8 @@ private void handleOptional(Property property) {
 	}
 
 	private void handleNaturalId(Property property) {
-		if ( this.memberDetails != null && entityBinder != null ) {
-			final NaturalId naturalId = this.memberDetails.getDirectAnnotationUsage( NaturalId.class );
+		if ( memberDetails != null && entityBinder != null ) {
+			final NaturalId naturalId = memberDetails.getDirectAnnotationUsage( NaturalId.class );
 			if ( naturalId != null ) {
 				if ( !entityBinder.isRootEntity() ) {
 					throw new AnnotationException( "Property '" + qualify( holder.getPath(), name )
@@ -505,11 +505,11 @@ private void handleNaturalId(Property property) {
 
 	private void inferOptimisticLocking(Property property) {
 		// this is already handled for collections in CollectionBinder...
-		if ( value instanceof Collection ) {
-			property.setOptimisticLocked( ((Collection) value).isOptimisticLocked() );
+		if ( value instanceof Collection collection ) {
+			property.setOptimisticLocked( collection.isOptimisticLocked() );
 		}
-		else if ( this.memberDetails != null && this.memberDetails.hasDirectAnnotationUsage( OptimisticLock.class ) ) {
-			final OptimisticLock optimisticLock = this.memberDetails.getDirectAnnotationUsage( OptimisticLock.class );
+		else if ( memberDetails != null && memberDetails.hasDirectAnnotationUsage( OptimisticLock.class ) ) {
+			final OptimisticLock optimisticLock = memberDetails.getDirectAnnotationUsage( OptimisticLock.class );
 			final boolean excluded = optimisticLock.excluded();
 			validateOptimisticLock( excluded );
 			property.setOptimisticLocked( !excluded );

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

@@ -533,7 +533,7 @@ public boolean isValid(MappingContext mappingContext) {
 	@Override
 	public boolean isSame(Value other) {
 		return this == other
-			|| other instanceof Collection && isSame( (Collection) other );
+			|| other instanceof Collection collection && isSame( collection );
 	}
 
 	protected static boolean isSame(Value v1, Value v2) {
@@ -770,8 +770,8 @@ public void setTypeParameters(Properties parameterMap) {
 
 	@SuppressWarnings("rawtypes")
 	public void setTypeParameters(java.util.Map typeParameters) {
-		if ( typeParameters instanceof Properties ) {
-			this.typeParameters = (Properties) typeParameters;
+		if ( typeParameters instanceof Properties properties ) {
+			this.typeParameters = properties;
 		}
 		else {
 			this.typeParameters = new Properties();

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

@@ -40,27 +40,33 @@ protected IndexedCollection(IndexedCollection original) {
 	public Value getIndex() {
 		return index;
 	}
+
 	public void setIndex(Value index) {
 		this.index = index;
 	}
+
 	public final boolean isIndexed() {
 		return true;
 	}
 
+	public boolean hasMapKeyProperty() {
+		return false;
+	}
+
 	@Override
 	public boolean isSame(Collection other) {
-		return other instanceof IndexedCollection
-				&& isSame( (IndexedCollection) other );
+		return other instanceof IndexedCollection indexedCollection
+			&& isSame( indexedCollection );
 	}
 
 	public boolean isSame(IndexedCollection other) {
 		return super.isSame( other )
-				&& isSame( index, other.index );
+			&& isSame( index, other.index );
 	}
 
 	void createPrimaryKey() {
 		if ( !isOneToMany() ) {
-			PrimaryKey pk = new PrimaryKey( getCollectionTable() );
+			final PrimaryKey pk = new PrimaryKey( getCollectionTable() );
 			pk.addColumns( getKey() );
 
 			// index should be last column listed
@@ -90,6 +96,7 @@ void createPrimaryKey() {
 //		}
 	}
 
+	@Deprecated
 	public void validate(Mapping mapping) throws MappingException {
 		validate( (MappingContext) mapping);
 	}

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

@@ -22,6 +22,7 @@
 public class Map extends IndexedCollection {
 
 	private String mapKeyPropertyName;
+	private boolean hasMapKeyProperty;
 
 	public Map(MetadataBuildingContext buildingContext, PersistentClass owner) {
 		super( buildingContext, owner );
@@ -75,4 +76,13 @@ public void createAllKeys() throws MappingException {
 	public Object accept(ValueVisitor visitor) {
 		return visitor.accept(this);
 	}
+
+	@Override
+	public boolean hasMapKeyProperty() {
+		return hasMapKeyProperty;
+	}
+
+	public void setHasMapKeyProperty(boolean hasMapKeyProperty) {
+		this.hasMapKeyProperty = hasMapKeyProperty;
+	}
 }

hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java

@@ -400,14 +400,14 @@ public AbstractCollectionPersister(
 
 		// INDEX AND ROW SELECT
 
-		final boolean hasIndex = collectionBootDescriptor.isIndexed();
-		if ( hasIndex ) {
+		if ( collectionBootDescriptor instanceof IndexedCollection indexedCollection ) {
+			assert collectionBootDescriptor.isIndexed();
 			// NativeSQL: collect index column and auto-aliases
-			final IndexedCollection indexedCollection = (IndexedCollection) collectionBootDescriptor;
-			indexType = indexedCollection.getIndex().getType();
-			final int indexSpan = indexedCollection.getIndex().getColumnSpan();
-			final boolean[] indexColumnInsertability = indexedCollection.getIndex().getColumnInsertability();
-			final boolean[] indexColumnUpdatability = indexedCollection.getIndex().getColumnUpdateability();
+			final Value index = indexedCollection.getIndex();
+			indexType = index.getType();
+			final int indexSpan = index.getColumnSpan();
+			final boolean[] indexColumnInsertability = index.getColumnInsertability();
+			final boolean[] indexColumnUpdatability = index.getColumnUpdateability();
 			indexColumnNames = new String[indexSpan];
 			indexFormulaTemplates = new String[indexSpan];
 			indexFormulas = new String[indexSpan];
@@ -416,7 +416,7 @@ public AbstractCollectionPersister(
 			indexColumnAliases = new String[indexSpan];
 			int i = 0;
 			boolean hasFormula = false;
-			for ( Selectable selectable: indexedCollection.getIndex().getSelectables() ) {
+			for ( Selectable selectable: index.getSelectables() ) {
 				indexColumnAliases[i] = selectable.getAlias( dialect );
 				if ( selectable.isFormula() ) {
 					final Formula indexForm = (Formula) selectable;
@@ -428,19 +428,16 @@ public AbstractCollectionPersister(
 					indexFormulas[i] = indexForm.getFormula();
 					hasFormula = true;
 				}
-				// Treat a mapped-by index like a formula to avoid trying to set it in insert/update
-				// Previously this was a sub-query formula, but was changed to represent the proper mapping
-				// which enables optimizations for queries. The old insert/update code wasn't adapted yet though.
-				// For now, this is good enough, because the formula is never used anymore,
-				// since all read paths go through the new code that can properly handle this case
-				else if ( indexedCollection instanceof org.hibernate.mapping.Map map
-						&& map.getMapKeyPropertyName() != null ) {
-					final Column indexCol = (Column) selectable;
-					indexFormulaTemplates[i] = Template.TEMPLATE + indexCol.getQuotedName( dialect );
-					indexFormulas[i] = indexCol.getQuotedName( dialect );
-					hasFormula = true;
-				}
 				else {
+					if ( indexedCollection.hasMapKeyProperty() ) {
+						// If the Map key is set via @MapKey, it should not be written
+						// since it is a reference to a field of the associated entity.
+						// (Note that the analogous situation for Lists never arises
+						// because @OrderBy is treated as defining a sorted bag.)
+						indexColumnInsertability[i] = false;
+						indexColumnUpdatability[i] = false;
+						hasFormula = true; // this is incorrect, but needed for some reason
+					}
 					final Column indexCol = (Column) selectable;
 					indexColumnNames[i] = indexCol.getQuotedName( dialect );
 					indexColumnIsGettable[i] = true;

hibernate-core/src/test/java/org/hibernate/orm/test/collection/mapkey/Book.java

@@ -0,0 +1,53 @@
+/*
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ * Copyright Red Hat Inc. and Hibernate Authors
+ */
+package org.hibernate.orm.test.collection.mapkey;
+
+import jakarta.persistence.Column;
+import jakarta.persistence.Entity;
+import jakarta.persistence.Id;
+import jakarta.persistence.MapKey;
+import jakarta.persistence.OneToMany;
+import jakarta.validation.constraints.Size;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static jakarta.persistence.CascadeType.PERSIST;
+
+@Entity
+class Book {
+	@Id
+	@Size(min=10, max = 13)
+	String isbn;
+
+	@OneToMany(cascade = PERSIST,
+			mappedBy = "isbn")
+	@MapKey(name = "name")
+	Map<String,Chapter> chapters;
+
+	Book(String isbn) {
+		this.isbn = isbn;
+		chapters = new HashMap<>();
+	}
+
+	Book() {
+	}
+}
+
+@Entity
+class Chapter {
+	@Id String isbn;
+	@Id @Column(name = "chapter_name") String name;
+	String text;
+
+	Chapter(String isbn, String name, String text) {
+		this.isbn = isbn;
+		this.name = name;
+		this.text = text;
+	}
+
+	Chapter() {
+	}
+}