HHH-18830 fallout from fix

remove ugly workaround for @MapKey/@MapKeyColumn, and add tests

It looks like @beikov had already run into this bug in the context of map
keys, and worked around it, not realizing that the problem also affected
list indexes.

Signed-off-by: Gavin King <[email protected]>

Author: gavinking

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

@@ -428,18 +428,6 @@ 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 {
 					final Column indexCol = (Column) selectable;
 					indexColumnNames[i] = indexCol.getQuotedName( dialect );

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() {
+	}
+}

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

@@ -0,0 +1,35 @@
+/*
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ * Copyright Red Hat Inc. and Hibernate Authors
+ */
+package org.hibernate.orm.test.collection.mapkey;
+
+import org.hibernate.testing.jdbc.SQLStatementInspector;
+import org.hibernate.testing.orm.junit.EntityManagerFactoryScope;
+import org.hibernate.testing.orm.junit.Jpa;
+import org.junit.jupiter.api.Test;
+
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+@Jpa(annotatedClasses = {Chapter.class, Book.class},
+		useCollectingStatementInspector = true)
+public class MapKeyMappedByTest {
+	@Test void test(EntityManagerFactoryScope scope) {
+		scope.inTransaction( em -> {
+			Book book = new Book( "XXX" );
+			em.persist( book );
+			book.chapters.put( "one", new Chapter("XXX", "one", "Lorem ipsum") );
+		} );
+		scope.inTransaction( em -> {
+			Book book = em.find( Book.class, "XXX" );
+			book.chapters.put( "two", new Chapter("XXX", "two", "Lorem ipsum") );
+		} );
+		List<String> queries = ((SQLStatementInspector) scope.getStatementInspector()).getSqlQueries();
+		assertEquals( 5, queries.size() ); // we can't put in a Map without initializing it
+		scope.inTransaction( em -> {
+			assertEquals(  em.find( Book.class, "XXX" ).chapters.size(), 2 );
+		} );
+	}
+}

hibernate-core/src/test/java/org/hibernate/orm/test/collection/mapkeycolumn/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.mapkeycolumn;
+
+import jakarta.persistence.Column;
+import jakarta.persistence.Entity;
+import jakarta.persistence.Id;
+import jakarta.persistence.MapKeyColumn;
+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")
+	@MapKeyColumn(name = "chapter_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() {
+	}
+}

hibernate-core/src/test/java/org/hibernate/orm/test/collection/mapkeycolumn/MapKeyColumnMappedByTest.java

@@ -0,0 +1,35 @@
+/*
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ * Copyright Red Hat Inc. and Hibernate Authors
+ */
+package org.hibernate.orm.test.collection.mapkeycolumn;
+
+import org.hibernate.testing.jdbc.SQLStatementInspector;
+import org.hibernate.testing.orm.junit.EntityManagerFactoryScope;
+import org.hibernate.testing.orm.junit.Jpa;
+import org.junit.jupiter.api.Test;
+
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+@Jpa(annotatedClasses = {Chapter.class, Book.class},
+		useCollectingStatementInspector = true)
+public class MapKeyColumnMappedByTest {
+	@Test void test(EntityManagerFactoryScope scope) {
+		scope.inTransaction( em -> {
+			Book book = new Book( "XXX" );
+			em.persist( book );
+			book.chapters.put( "one", new Chapter("XXX", "one", "Lorem ipsum") );
+		} );
+		scope.inTransaction( em -> {
+			Book book = em.find( Book.class, "XXX" );
+			book.chapters.put( "two", new Chapter("XXX", "two", "Lorem ipsum") );
+		} );
+		List<String> queries = ((SQLStatementInspector) scope.getStatementInspector()).getSqlQueries();
+		assertEquals( 5, queries.size() ); // we can't put in a Map without initializing it
+		scope.inTransaction( em -> {
+			assertEquals(  em.find( Book.class, "XXX" ).chapters.size(), 2 );
+		} );
+	}
+}