jamesfredley commented on code in PR #16701: URL: https://github.com/apache/grails-core/pull/16701#discussion_r4338912955
########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/SharedJoinTableUniqueKeyBinder.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.mapping.Collection; +import org.hibernate.mapping.Column; +import org.hibernate.mapping.Selectable; +import org.hibernate.mapping.Table; +import org.hibernate.mapping.UniqueKey; + +/** + * Completes the implicit unique key of a join table that several collections of one owner share. + * + * <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 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}). A row of a later collection leaves the first key column + * null, so on a database that treats nulls as equal in a unique constraint two of its rows for the same element, from two + * owners, are duplicates. This binder adds the key columns of every collection to that unique key once the collection's + * keys are created, so a row is unique by the key column of its own collection and the element.</p> + * + * <p>A unique key the mapping declares explicitly is left alone, and so is a collection without a join table or whose + * key the other side owns.</p> + * + * @since 8.0.1 + */ +public class SharedJoinTableUniqueKeyBinder { + + /** Adds the key columns of the collection to the implicit unique key of its join table that covers its element. */ + public void bind(Collection collection) { + Table table = collection.getCollectionTable(); + if (table == null || collection.isInverse() || collection.isOneToMany() || collection.getKey() == null) { + return; + } + List<Column> keyColumns = collection.getKey().getColumns(); + List<Column> elementColumns = columnsOf(collection); + if (keyColumns.isEmpty() || elementColumns.isEmpty()) { + return; + } + 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); Review Comment: Adding the sibling key here weakens the constraint it is meant to extend. For `roles` and `denyRoles` sharing a join table, the implicit key becomes `UNIQUE(roles_id, deny_roles_id, role_id)`. A real `roles` row is `(roles_id=1, deny_roles_id=NULL, role_id=7)`. PostgreSQL, MySQL, and default H2 treat those NULLs as distinct, so that row can be inserted twice. The old `(roles_id, role_id)` constraint rejected the duplicate. Keep a per-association uniqueness constraint on `(own_collection_key, element)` for NULL-distinct dialects. Where the dialect treats NULLs as equal, use a null-safe or partial index instead of one widened constraint that is NULL for every normal row. Please add a regression that rejects duplicate membership of the same role in the same collection, and still allows that role in the other collection and in another owner. The current specs do not catch this. -- 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]
