jdaugherty commented on code in PR #16539:
URL: https://github.com/apache/grails-core/pull/16539#discussion_r4225917979
##########
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:
This branch runs when the referenced entity hasn't been bound yet.
`getPersistentClass()` is still null, so the columns keep their declared order
and only get sequential type indexes. `markValueSorted` then sets `sorted`, so
Hibernate's `ToOne.sortProperties()` skips them later too. Whether a key gets
sorted depends on whether the entity it references was registered first.
The spec always registers the referenced entity first. Here are the same
classes in alphabetical order:
```groovy
new HibernateDatastore(config, PrbGrand, PrbLeaf, PrbMiddle)
```
The leaf's key columns stay `prb_middle_name, prb_middle_grand_parent_alpha,
prb_middle_grand_parent_zeta` while the values are bound in key order, and
saving a leaf fails:
```
Referential integrity constraint violation: "FKDEN36EQGOPJY1BD3T41V9BTYS:
PUBLIC.PRB_LEAF FOREIGN KEY(PRB_MIDDLE_NAME, PRB_MIDDLE_GRAND_PARENT_ALPHA,
PRB_MIDDLE_GRAND_PARENT_ZETA) REFERENCES PUBLIC.PRB_MIDDLE(NAME,
PRB_GRAND_ALPHA, PRB_GRAND_ZETA) ('a', 'z', 'm')"
```
The middle's own key has the same problem when the middle is registered
before its grand parent. Applications don't choose this order; their domain
classes come from the class-path scan. Leaving `sorted` unset would not be
enough on its own, because Hibernate's sort applies the property permutation
and runs into the column-span problem this commit fixes.
This predates the PR: 8.0.x fails the same way, with the columns crossed
differently. Are you willing to handle it here, for example by deferring the
sort until the referenced identifier is bound? Or would you rather we open a
follow-up issue for it?
##########
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:
`PrbSpanLeaf`'s foreign key points at `PrbMiddle`, so these two features
check the same key as the `PrbLeaf` ones. `composite('zed', 'middle', 'ace')`
only lays out `PrbSpanLeaf`'s own primary key. Hibernate orders that key
itself, and nothing references it, so the permutation for a multi-column part
between two plain parts never runs here.
An entity that references a key with that layout does exercise it:
```groovy
@Entity
class PrbHub implements Serializable {
String zed
String ace
PrbGrand grand
static hasMany = [tags: PrbHubTag]
static mapping = MappingBuilder.define {
composite('zed', 'grand', 'ace')
}
}
@Entity
class PrbHubTag {
String label
}
@Entity
class PrbHubRef {
String name
PrbHub hub
}
```
With this commit, the `PRB_HUB_REF` key pairs are `[prb_hub_ace, ace],
[prb_hub_grand_alpha, prb_grand_alpha], [prb_hub_grand_zeta, prb_grand_zeta],
[prb_hub_zed, zed]`. The `PRB_HUB_PRB_HUB_TAG` join table gets the same pairs,
and a hub with a tag and a ref saves and reloads. On 8.0.x the pairs come out
as `[prb_hub_grand_alpha, prb_grand_zeta], [prb_hub_grand_zeta,
prb_grand_alpha], [prb_hub_zed, zed], [prb_hub_ace, ace]`, and the save fails
with a referential integrity violation.
Could these two features use this layout instead? The join table also covers
the `CollectionWithJoinTableBinder` path. The Tests section of the description
would need to change to match.
--
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]
