borinquenkid commented on code in PR #16539: URL: https://github.com/apache/grails-core/pull/16539#discussion_r4226098699
########## grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/CompositeForeignKeyColumnTypesSpec.groovy: ########## @@ -0,0 +1,393 @@ +/* + * 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.binder + +import java.sql.Connection + +import grails.gorm.annotation.Entity +import grails.gorm.hibernate.mapping.MappingBuilder +import org.grails.orm.hibernate.HibernateDatastore +import org.hibernate.dialect.H2Dialect +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.lang.Specification + +/** + * A foreign key to an entity whose composite identifier has parts of different types must name + * and type each column after the referenced primary key column it points at. + */ +class CompositeForeignKeyColumnTypesSpec extends Specification { + + @Shared + @AutoCleanup + HibernateDatastore datastore = new HibernateDatastore( + [ + 'dataSource.url' : 'jdbc:h2:mem:compositeFkTypesDB;LOCK_TIMEOUT=10000', + 'dataSource.dbCreate' : 'create-drop', + 'dataSource.dialect' : H2Dialect.name, + 'hibernate.hbm2ddl.auto': 'create', + ], + CfkParent, CfkChild, CfkGrandParent, CfkMiddle, CfkLeaf, + CfkOrdParent, CfkOrdChild, CfkOrdGrand, CfkOrdMiddle, CfkOrdLeaf, + PrbGrand, PrbMiddle, PrbLeaf, PrbSpanLeaf) + + private List<List> columns(String table) { + List<List> rows = [] + datastore.sessionFactory.openSession().withCloseable { session -> + session.doWork { Connection c -> + c.createStatement().withCloseable { st -> + st.executeQuery( + "select column_name, data_type from information_schema.columns where table_name = '${table}' order by ordinal_position".toString()) + .withCloseable { rs -> + while (rs.next()) { + rows << [rs.getString(1).toLowerCase(), rs.getString(2).toUpperCase()] + } + } + } + } + } + rows + } + + + /** + * The foreign keys of a table, each as its column pairs [foreign key column, referenced column] in + * {@code KEY_SEQ} order, which is the order the database matches the columns of a key by. + */ + private Map<String, List<List<String>>> foreignKeyPairs(String table) { + Map<String, List<List<String>>> keys = [:] + datastore.sessionFactory.openSession().withCloseable { session -> + session.doWork { Connection c -> + c.metaData.getImportedKeys(null, null, table).withCloseable { rs -> + List<List> rows = [] + while (rs.next()) { + rows << [rs.getString('FK_NAME'), rs.getInt('KEY_SEQ'), + rs.getString('PKTABLE_NAME').toLowerCase(), + rs.getString('FKCOLUMN_NAME').toLowerCase(), rs.getString('PKCOLUMN_NAME').toLowerCase()] + } + rows.sort { it[1] }.each { List row -> + keys.get(row[0] + ' -> ' + row[2], []) << [row[3], row[4]] + } + } + } + } + keys.collectEntries { String name, List<List<String>> pairs -> [(name.substring(name.indexOf(' -> ') + 4)): pairs] } + } + + void "a foreign key to a composite parent pairs its columns with the referenced key columns in key order"() { + expect: + foreignKeyPairs('CFK_CHILD') == [ + cfk_parent: [['cfk_parent_lucky_number', 'lucky_number'], ['cfk_parent_name', 'name']] + ] + } + + void "a foreign key to a composite parent whose parts are declared out of name order pairs the same-typed parts in key order"() { + expect: 'the parts are declared as zeta, alpha, and the primary key and the foreign key both follow the name order' + foreignKeyPairs('CFK_ORD_CHILD') == [ + cfk_ord_parent: [['cfk_ord_parent_alpha', 'alpha'], ['cfk_ord_parent_zeta', 'zeta']] + ] + } + + void "the foreign keys of a three level composite chain pair their columns with the referenced key columns in key order"() { + expect: + foreignKeyPairs('CFK_MIDDLE') == [ + cfk_grand_parent: [['cfk_grand_parent_lucky_number', 'lucky_number'], ['cfk_grand_parent_name', 'name']] + ] + foreignKeyPairs('CFK_LEAF') == [ + cfk_middle: [ + ['cfk_middle_grand_parent_lucky_number', 'cfk_grand_parent_lucky_number'], + ['cfk_middle_grand_parent_name', 'cfk_grand_parent_name'], + ['cfk_middle_name', 'name']] + ] + } + + void "the foreign keys of a three level chain with out of order, same-typed parts pair them in key order"() { + expect: 'the nested composite is declared as zeta, alpha, so only its order tells the columns apart' + foreignKeyPairs('CFK_ORD_MIDDLE') == [ + cfk_ord_grand: [['cfk_ord_grand_alpha', 'alpha'], ['cfk_ord_grand_zeta', 'zeta']] + ] + foreignKeyPairs('CFK_ORD_LEAF') == [ + cfk_ord_middle: [ + ['cfk_ord_middle_grand_parent_alpha', 'cfk_ord_grand_alpha'], + ['cfk_ord_middle_grand_parent_zeta', 'cfk_ord_grand_zeta'], + ['cfk_ord_middle_name', 'name']] + ] + } + + void "foreign key columns carry the type of the primary key column they are named after"() { + when: + Map<String, String> parent = columns('CFK_PARENT').collectEntries { [(it[0]): it[1]] } + Map<String, String> child = columns('CFK_CHILD').collectEntries { [(it[0]): it[1]] } + + then: + parent.name == 'CHARACTER VARYING' + parent.lucky_number == 'INTEGER' + child.cfk_parent_name == parent.name + child.cfk_parent_lucky_number == parent.lucky_number + } + + void "foreign key columns of a three level composite chain are named and typed after the referenced key"() { + when: + Map<String, String> grand = columns('CFK_GRAND_PARENT').collectEntries { [(it[0]): it[1]] } + Map<String, String> middle = columns('CFK_MIDDLE').collectEntries { [(it[0]): it[1]] } + + Map<String, String> leaf = columns('CFK_LEAF').collectEntries { [(it[0]): it[1]] } + + then: + grand.name + middle.cfk_grand_parent_name == grand.name + middle.cfk_grand_parent_lucky_number == grand.lucky_number + leaf.cfk_middle_grand_parent_name == grand.name + leaf.cfk_middle_grand_parent_lucky_number == grand.lucky_number + } + + void "a leaf of a three level composite chain is saved, reloaded and found through the association"() { + when: + CfkGrandParent.withNewTransaction { + CfkGrandParent grand = new CfkGrandParent(name: 'Fred', luckyNumber: 7).save(failOnError: true) + CfkMiddle middle = new CfkMiddle(name: 'Bob', grandParent: grand).save(failOnError: true) + new CfkLeaf(name: 'Chuck', middle: middle).save(failOnError: true, flush: true) + } + + then: + CfkLeaf.withNewSession { + CfkLeaf leaf = CfkLeaf.findByName('Chuck') + leaf.middle.name == 'Bob' && leaf.middle.grandParent.luckyNumber == 7 + } + } + + void "a child referencing a composite parent is saved, reloaded and found through the association"() { + when: + CfkParent.withNewTransaction { + CfkParent parent = new CfkParent(name: 'Fred', luckyNumber: 7).save(failOnError: true) + new CfkChild(label: 'kid', parent: parent).save(failOnError: true, flush: true) + } + + then: + CfkChild.withNewSession { + CfkChild child = CfkChild.findByLabel('kid') + child.parent.name == 'Fred' && child.parent.luckyNumber == 7 + } + CfkChild.withNewSession { + CfkChild.where { parent.name == 'Fred' && parent.luckyNumber == 7 }.count() == 1 + } + } + + void "a nested composite part that sorts after a plain part moves with all of its columns"() { + expect: 'the middle key is declared as name, grandParent, so grandParent spans the columns that sort before and after name' + foreignKeyPairs('PRB_MIDDLE') == [ + prb_grand: [['prb_grand_alpha', 'alpha'], ['prb_grand_zeta', 'zeta']] + ] + foreignKeyPairs('PRB_LEAF') == [ + prb_middle: [ + ['prb_middle_grand_parent_alpha', 'prb_grand_alpha'], + ['prb_middle_grand_parent_zeta', 'prb_grand_zeta'], + ['prb_middle_name', 'name']] + ] + } + + void "a leaf whose composite key nests a part declared after a plain part is saved and reloaded"() { + when: + PrbGrand.withNewTransaction { + PrbGrand grand = new PrbGrand(zeta: 'z', alpha: 'a').save(failOnError: true) + PrbMiddle middle = new PrbMiddle(name: 'm', grandParent: grand).save(failOnError: true) + new PrbLeaf(name: 'l', middle: middle).save(failOnError: true, flush: true) + } + + then: + PrbLeaf.withNewSession { + PrbLeaf leaf = PrbLeaf.findByName('l') + leaf.middle.name == 'm' && leaf.middle.grandParent.alpha == 'a' && leaf.middle.grandParent.zeta == 'z' + } + } + + void "a multi-column nested part between two plain parts keeps its columns together in key order"() { Review Comment: You're right: `PrbSpanLeaf`'s own key is laid out by Hibernate and nothing references it, so the permutation never runs there. I'll switch both features to your `PrbHub`/`PrbHubTag`/`PrbHubRef` layout (key pairs on `PRB_HUB_REF` and on the join table, plus a save and reload) and update the Tests section of the description. ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/GrailsHibernatePersistentEntity.java: ########## @@ -384,9 +385,10 @@ default boolean isLazy(HibernatePersistentProperty property) { default void sortOrIndexForeignKeyColumns(SimpleValue value) { PersistentClass pc = getPersistentClass(); KeyValue identifier = pc != null ? pc.getIdentifier() : null; - int[] originalOrder = identifier instanceof Component c ? c.sortProperties() : null; + Component component = identifier instanceof Component c ? c : null; + int[] originalOrder = component != null ? component.sortProperties() : null; if (originalOrder != null) { - value.sortColumns(originalOrder); + value.sortColumns(toColumnPermutation(originalOrder, partColumnSpans(component, originalOrder))); } else { Review Comment: Agreed, that is a real bug and it belongs with this fix. I'll handle it in this PR: defer the sort until the referenced identifier is bound (not marking the columns sorted while the referenced entity is unbound), with a spec that registers the classes in alphabetical order (`PrbGrand, PrbLeaf, PrbMiddle`) and asserts the key pairs plus a save and reload, and the same for a middle registered before its grand parent. -- 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]
