borinquenkid commented on code in PR #16701: URL: https://github.com/apache/grails-core/pull/16701#discussion_r4362625537
########## 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: Fixed in f1bdd129a0: `bind` returns unless `collection.isSet()`. I reproduced your six properties first: with the guard removed, a list, a map and a bag with a repeated element fail to save with a duplicate key on H2, PostgreSQL, MySQL and MariaDB. A bag went through the set second pass and got the key as well, as you found. With the guard, the set on a table of its own is still left alone by the primary key and `present` checks, as you said. ########## 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: Added in f1bdd129a0. `SharedJoinTableUniqueKeyBinderSpec` binds, from the metadata, a `List<String>`, a `List<Role>`, a `Map<String, String>`, a `Map<String, Role>`, a `Collection<String>` and a `Collection<Role>` hasMany, on the H2 and the Oracle dialect, and asserts none gains a unique key or an index (the lists and maps have their primary key, the bags none); it fails for all twelve rows without the guard. `MultipleUnidirectionalHasManySpec` saves and reads back a repeated element in each of the six on plain H2, and `SharedJoinTableHibernate7Spec` does the same on H2, PostgreSQL, MySQL, MariaDB and Oracle. Each collection that points at the same class maps its own join table in those entities, because two associations to one class with a list among them share a table, which is a separate matter. ########## 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: Kept in f1bdd129a0 as a deprecated type, with its original default method and a javadoc that says the second passes no longer implement it and what creates the keys now. Nothing in the repository references it. ########## 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: Added in f1bdd129a0, in `MultipleUnidirectionalHasManySpec` so it runs without Docker: the same role allowed twice by one account of a shared table is refused with a `DataIntegrityViolationException`, while denying the same role by the same account is accepted, because it is the other association. The container spec keeps the rejection across the other databases. ########## 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: Fixed in f1bdd129a0, for lists, maps and bags: the binder acts only on a set. Your map with one value under two keys fails to save without the guard on H2, PostgreSQL, MySQL and MariaDB, and saves with it on all five databases (`SharedJoinTableHibernate7Spec`). -- 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]
