jdaugherty commented on code in PR #16701: URL: https://github.com/apache/grails-core/pull/16701#discussion_r4361167375
########## grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/SharedJoinTableHibernate7Spec.groovy: ########## @@ -0,0 +1,142 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package grails.gorm.tests + +import java.time.Duration + +import grails.gorm.annotation.Entity +import org.testcontainers.mariadb.MariaDBContainer +import org.testcontainers.mysql.MySQLContainer +import org.testcontainers.oracle.OracleContainer +import org.testcontainers.postgresql.PostgreSQLContainer +import org.springframework.dao.DataIntegrityViolationException +import org.testcontainers.spock.Testcontainers +import spock.lang.Requires +import spock.lang.Shared + +/** + * A join table two unidirectional hasMany associations share is unique per association on every database: the + * same member twice in one association of one owner is rejected, while the same member in the other association, + * in the same association of another owner, and denied by two owners is allowed. The last case is the one a unique + * key per association over a nullable key column breaks on Oracle, and the first the one a single key over every key + * column breaks where nulls are distinct. + */ +@Testcontainers +@Requires({ isDockerAvailable() }) Review Comment: The class-level `@Requires` skips the H2 row together with the container rows, so without Docker the rejection of a repeated member in the same association runs nowhere: `MultipleUnidirectionalHasManySpec` writes to the shared table but never asserts that a duplicate is refused, and the binder spec checks the metadata, not the database. An H2-only feature for that case in `MultipleUnidirectionalHasManySpec` would keep the regression test in the plain module run. Non-blocking. ########## grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/SharedJoinTableUniqueKeyBinderSpec.groovy: ########## @@ -0,0 +1,263 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.orm.hibernate.cfg.domainbinding.secondpass + +import grails.gorm.annotation.Entity +import grails.gorm.tests.HibernateGormDatastoreSpec +import org.hibernate.boot.model.relational.internal.SqlStringGenerationContextImpl +import org.hibernate.boot.spi.MetadataBuildingContext +import org.hibernate.dialect.DB2Dialect +import org.hibernate.dialect.Dialect +import org.hibernate.dialect.H2Dialect +import org.hibernate.dialect.MariaDBDialect +import org.hibernate.dialect.MySQLDialect +import org.hibernate.dialect.OracleDialect +import org.hibernate.dialect.PostgreSQLDialect +import org.hibernate.dialect.SQLServerDialect +import org.hibernate.engine.jdbc.env.internal.JdbcEnvironmentImpl +import org.hibernate.mapping.Collection +import org.hibernate.mapping.Column +import org.hibernate.mapping.Table +import org.hibernate.mapping.UniqueKey +import spock.lang.Unroll + +/** + * The join table two unidirectional hasMany associations to the same class share gets one implicit unique key from + * Hibernate, over the first association's key column and the element column, and nothing for the second. The binder + * gives every association a constraint of its own over its key column and the element, in the shape the database + * needs: a unique key where nulls are distinct or the dialect renders a filtered unique index, and on Oracle, which + * compares the non-null columns of a row, one unique key over every association's key column and the element. An association with a join table of its own, and one whose key the other side + * owns, are left as Hibernate binds them. + */ +class SharedJoinTableUniqueKeyBinderSpec extends HibernateGormDatastoreSpec { + + void setupSpec() { + manager.registerDomainClasses(SjtOwner, SjtSeparateOwner, SjtParent, SjtChild, SjtRole) + } + + private Collection collectionOf(Class owner, String property) { + datastore.metadata.getCollectionBinding("${owner.name}.${property}".toString()) + } + + private static List<List<String>> uniqueKeyColumns(Table table) { + table.uniqueKeys.values().collect { UniqueKey key -> key.columns*.name.sort() }.sort() + } + + void "the shared join table has a unique key per association, over its own key column and the element"() { + given: + Collection roles = collectionOf(SjtOwner, 'roles') + Collection denyRoles = collectionOf(SjtOwner, 'denyRoles') + + expect: 'both associations share the table' + roles.collectionTable.is(denyRoles.collectionTable) + roles.collectionTable.columns*.name.sort() == ['sjt_owner_deny_roles_id', 'sjt_owner_roles_id', 'sjt_role_id'] + + and: 'each association is unique by its own key column and the element; H2 treats nulls as distinct' + uniqueKeyColumns(roles.collectionTable) == [ + ['sjt_owner_deny_roles_id', 'sjt_role_id'], + ['sjt_owner_roles_id', 'sjt_role_id'] + ] + roles.collectionTable.indexes.isEmpty() + + and: 'both keys are updatable, so rows of both associations are written' + roles.key.updateable + denyRoles.key.updateable + } + + void "an association with a join table of its own keeps the unique key Hibernate creates for it"() { Review Comment: When the guard is narrowed, please cover the three collection kinds it excludes here, from the metadata as this feature does: a `List`, a `Map` and a `Collection` (bag) hasMany whose tables gain no unique key. And in `MultipleUnidirectionalHasManySpec`, or a spec of its own, a save and read-back of a list with a repeated element, a map with one value under two keys, and a bag with a repeated element. The module suite was green at this commit, so nothing in it saves a repeated element into a list, map or bag today. ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/SharedJoinTableUniqueKeyBinder.java: ########## @@ -0,0 +1,170 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.orm.hibernate.cfg.domainbinding.secondpass; + +import java.util.ArrayList; +import java.util.List; + +import org.hibernate.boot.model.naming.Identifier; +import org.hibernate.boot.model.naming.ImplicitIndexNameSource; +import org.hibernate.boot.model.naming.ImplicitNamingStrategy; +import org.hibernate.boot.model.naming.ImplicitUniqueKeyNameSource; +import org.hibernate.boot.spi.MetadataBuildingContext; +import org.hibernate.dialect.Dialect; +import org.hibernate.mapping.Collection; +import org.hibernate.mapping.Column; +import org.hibernate.mapping.Selectable; +import org.hibernate.mapping.Table; + +/** + * Gives every collection with a join table a uniqueness constraint of its own over its key column and the element, + * so that a join table several collections of one owner share stays unique per collection. + * + * <p>Two unidirectional {@code hasMany} to the same class without a {@code joinTable} mapping share one join table, + * with a key column for each of them and one element column. Hibernate creates the table's implicit unique key when + * it creates the keys of the first collection, over that collection's key column and the element column, and creates + * none for the others, as the table has a unique key already ({@code Set.createPrimaryKey}). This binder runs after + * the keys of each collection are created and completes what is missing, in the shape the database needs + * ({@link SharedJoinTableUniqueness}). A collection whose table is its own already has its constraint and is left as + * Hibernate bound it, and so is a collection without a join table or whose key the other side owns.</p> + * + * @since 8.0.1 + */ +public class SharedJoinTableUniqueKeyBinder { + + private final MetadataBuildingContext metadataBuildingContext; + private final SharedJoinTableUniqueness uniqueness; + + /** Resolves the uniqueness for the dialect of the metadata being built. */ + public SharedJoinTableUniqueKeyBinder(MetadataBuildingContext metadataBuildingContext) { + this(metadataBuildingContext, metadataBuildingContext.getMetadataCollector().getDatabase().getDialect()); + } + + SharedJoinTableUniqueKeyBinder(MetadataBuildingContext metadataBuildingContext, Dialect dialect) { + this.metadataBuildingContext = metadataBuildingContext; + this.uniqueness = SharedJoinTableUniqueness.forDialect(dialect); + } + + /** The uniqueness this binder applies. */ + public SharedJoinTableUniqueness getUniqueness() { + return uniqueness; + } + + /** Gives the collection a uniqueness constraint over its key columns and its element on its join table. */ + public void bind(Collection collection) { + Table table = collection.getCollectionTable(); + if (table == null || collection.isInverse() || collection.isOneToMany() || collection.getKey() == null) { Review Comment: This guard lets every collection with a join table through, and `createCollectionKeys` is now called from the list and map binders as well as from `bindSetSecondPass`, which the bag of a `Collection`-typed property also goes through (`CollectionBinder.registerSecondPass`, the `else` branch). On the `DEFAULT` path the table of each of those gets a unique key over (key, element) that Hibernate never made: their primary key is (key, index), or absent for a bag, so `coveredByPrimaryKey` and `present` are both false and `SharedJoinTableUniqueness` line 99 adds one. Reproduced at 9066f901f1 against its base 8b7515a7c0 with one spec on H2 (`:grails-data-hibernate7-core:test`): six properties of two owners, each saved with a repeated element and read back after a session clear. | property | base | this PR | unique index the PR adds (H2 `information_schema.indexes`) | |---|---|---|---| | `List<String> nicknames = ['a', 'a']` | passes | `DuplicateKeyException` | `(p16701_owner_id, nicknames_java_lang_string)` | | `List<Role> roles = [role, role]` | passes | `DuplicateKeyException` | `(p16701_owner_roles_id, p16701_role_id)` | | `Map<String, String> attrs = [k1: 'v', k2: 'v']` | passes | `DuplicateKeyException` | `(p16701_owner_id, attrs_elt)` | | `Map<String, Role> roleMap = [a: role, b: role]` | passes | `DuplicateKeyException` | `(p16701_owner_role_map_id, p16701_role_id)` | | `Collection<String> labels = ['x', 'x']` | passes | `DuplicateKeyException` | `(p16701_bag_owner_id, labels_java_lang_string)` | | `Collection<Role> bagRoles = [role, role]` | passes | `DuplicateKeyException` | `(p16701_bag_owner_bag_roles_id, p16701_role_id)` | The base schema has no unique index on any of the six tables. Oracle is not affected, since its path only widens a unique key that already exists, so this is every other database. A list, a map and a bag may hold an element twice, so the binder has to skip them: return here unless `collection.isSet()` (a sorted set is a `Set` too). With that in place the existing `coveredByPrimaryKey` and `present` checks already leave a set on a table of its own alone, so the "shared" part needs no sibling lookup. ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/GrailsSecondPass.java: ########## @@ -1,28 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.grails.orm.hibernate.cfg.domainbinding.secondpass; - -import org.hibernate.mapping.Collection; - -public interface GrailsSecondPass { Review Comment: `GrailsSecondPass` is a public interface that shipped in v8.0.0 (added in 9c8ba25a0a), and this removes it in the 8.0.1 patch. Nothing in the repository references it any more, and a mixin with one default method is unlikely to have implementors outside, but a patch release deleting a public type is not something a user can see coming. Deprecating it for one release, or at least a line in the PR body saying it is gone, would be the safer shape. Non-blocking. ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/SharedJoinTableUniqueness.java: ########## @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.orm.hibernate.cfg.domainbinding.secondpass; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +import org.hibernate.dialect.Dialect; +import org.hibernate.dialect.OracleDialect; +import org.hibernate.mapping.Column; +import org.hibernate.mapping.Table; +import org.hibernate.mapping.UniqueKey; + +/** + * How a join table that several collections of one owner share keeps the rows of each collection unique, by + * database. + * + * <p>Such a table has a key column per collection and one element column, and every row leaves the key columns of + * the other collections null. A row is unique by the key column of its own collection and the element, so each + * collection needs a constraint of its own over those two; one constraint over all key columns would hold a null in + * every row and reject nothing on a database that treats nulls as distinct. What that per-collection constraint has + * to look like depends on how the database compares a null in a unique key:</p> + * <ul> + * <li>{@link #DEFAULT}: a unique key over the key column and the element. H2, PostgreSQL, MySQL and MariaDB treat + * nulls as distinct, so the rows of the other collections, null in this key column, never collide. SQL Server and + * DB2 compare nulls as equal, but Hibernate's unique delegate for them renders a unique key over nullable columns as + * a filtered unique index that leaves null rows out, which comes to the same.</li> + * <li>{@link #ORACLE}: Oracle compares the non-null columns of a partially null row, so a unique key per + * collection would reject the rows of the other collections, null in that key column and equal in the element. + * One unique key over the key columns of every collection and the element is right there: a row collides only + * with a row that is non-null in the same key column, that is a row of its own collection, for the same element. + * Hibernate renders that key as a plain constraint on Oracle, where it is what is wanted; on a database that treats + * nulls as distinct the same key would hold a null in every row and reject nothing, which is why it is Oracle's + * alone.</li> + * </ul> + * + * @since 8.0.1 + */ +public enum SharedJoinTableUniqueness { + + /** Oracle: one unique key over the key columns of every collection sharing the table and the element. */ + ORACLE(OracleDialect.class) { + @Override + void apply(Table table, List<Column> keyColumns, List<Column> elementColumns, Naming naming) { + List<Column> own = concat(keyColumns, elementColumns); + if (coveredByPrimaryKey(table, own)) { + return; + } + // the unique key Hibernate created for the first collection sharing this table takes the key columns of + // every later one: Oracle compares the non-null columns of a row, so a row collides only with a row of + // its own collection for the same element, and a plain key per collection would reject the rows of the + // other collections, null in that key column and equal in the element + for (UniqueKey uniqueKey : table.getUniqueKeys().values()) { + if (uniqueKey.isExplicit() || !elementColumns.stream().allMatch(uniqueKey::containsColumn)) { + continue; + } + for (Column column : keyColumns) { + if (!uniqueKey.containsColumn(column)) { + uniqueKey.addColumn(column); + } + } + } + } + }, + + /** Every other database: a unique key per collection over its key column and the element. */ + DEFAULT(Dialect.class) { + @Override + void apply(Table table, List<Column> keyColumns, List<Column> elementColumns, Naming naming) { Review Comment: Reproduced on H2 at 9066f901f1 against the base for lists and maps of basic values and of entities, and also for a `Collection`-typed hasMany: the bag goes through `SetSecondPass` and `bindSetSecondPass`, so it gets the same unique key over (key, element), having no primary key at all. All six save a repeated element on the base and fail with `DuplicateKeyException` here. The table of results and the guard suggestion are on `SharedJoinTableUniqueKeyBinder.bind`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
