jdaugherty commented on code in PR #16574:
URL: https://github.com/apache/grails-core/pull/16574#discussion_r4289862618
##########
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/CollectionKeyBinderSpec.groovy:
##########
@@ -18,6 +18,7 @@
*/
package org.grails.orm.hibernate.cfg.domainbinding.secondpass
+import
org.grails.orm.hibernate.cfg.domainbinding.binder.CompositeIdentifierToManyToOneBinder
Review Comment:
Duplicate import: the same class is imported again on line 24. CodeNarc's
`DuplicateImport` rule is enabled but the style tasks skip test sources, so
this is a cleanup rather than a gate failure.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/BidirectionalOneToManyLinker.java:
##########
@@ -67,6 +81,20 @@ public void link(
key.addColumn(mappingColumn);
key.getTable().addColumn(mappingColumn);
}
- key.sortProperties();
+ key.setSorted(true);
Review Comment:
Non-blocking. A sentence in the class javadoc that this flag is required,
not only a way to skip the sort, would protect it from a later simplification:
`MappingModelCreationHelper.getPropertyOrder` asserts the collection key is
sorted and, when it is not, maps the key's columns through the identifier
component's original property order. With this line removed,
`InverseHasManyCompositeIdentifierOrderSpec` fails in `initializationError`
with that `AssertionError` while the session factory is built.
##########
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/BidirectionalOneToManyLinkerSpec.groovy:
##########
@@ -80,4 +87,72 @@ class BidirectionalOneToManyLinkerSpec extends
HibernateGormDatastoreSpec {
!key.isForeignKeyEnabled()
!key.isConstrained()
}
+
+ void "the copied key is marked sorted so Hibernate does not apply the
identifier permutation to it again"() {
+ given:
+ def metadataContext =
getGrailsDomainBinder().getMetadataBuildingContext()
+ RootClass rootClass = new RootClass(metadataContext)
+ rootClass.setEntityName("TestEntity")
+ Table table = new Table("test_table")
+ rootClass.setTable(table)
+ Property otherSideProperty = new Property()
+ otherSideProperty.setName("owner")
+ BasicValue value = new BasicValue(metadataContext, table)
+ value.addColumn(new Column("owner_id"))
+ otherSideProperty.setValue(value)
+ rootClass.addProperty(otherSideProperty)
+ Collection collection = new Bag(metadataContext, rootClass)
+ DependantValue key = new DependantValue(metadataContext, new
Table("collection_table"), null)
+ HibernateToManyProperty otherSide = Mock(HibernateToManyProperty)
+ otherSide.getName() >> "owner"
+
+ when:
+ linker.link(collection, rootClass, key, otherSide)
+
+ then:
+ key.isSorted()
+ 0 * binder._
+ }
+
+ void "the to-one side is aligned with the identifier of the owner before
its columns are copied"() {
+ given:
+ def metadataContext =
getGrailsDomainBinder().getMetadataBuildingContext()
+ RootClass rootClass = new RootClass(metadataContext)
+ rootClass.setEntityName("TestEntity")
+ Table table = new Table("test_table")
+ rootClass.setTable(table)
+ ManyToOne toOneValue = new ManyToOne(metadataContext, table)
+ toOneValue.addColumn(new Column("owner_alpha"))
+ toOneValue.addColumn(new Column("owner_zeta"))
+ Property otherSideProperty = new Property()
+ otherSideProperty.setName("owner")
+ otherSideProperty.setValue(toOneValue)
+ rootClass.addProperty(otherSideProperty)
+ Collection collection = new Bag(metadataContext, rootClass)
+ DependantValue key = new DependantValue(metadataContext, new
Table("collection_table"), null)
+ HibernateCompositeIdentity compositeId =
Mock(HibernateCompositeIdentity)
+ GrailsHibernatePersistentEntity ownerEntity =
Mock(GrailsHibernatePersistentEntity) {
+ getHibernateCompositeIdentity() >> Optional.of(compositeId)
+ }
+ HibernateManyToOneProperty otherSide =
Mock(HibernateManyToOneProperty) {
+ getName() >> "owner"
+ getHibernateAssociatedEntity() >> ownerEntity
+ }
+ List<String> columnsWhenAligned = null
+
+ when:
+ linker.link(collection, rootClass, key, otherSide)
+
+ then: 'the to-one side is aligned once, before the key exists'
+ 1 * binder.alignWithReferencedIdentifier(otherSide, toOneValue,
compositeId, ownerEntity) >> {
+ columnsWhenAligned = key.columns*.name
+ }
+ columnsWhenAligned == []
+
+ and: 'the key copies the aligned columns and Hibernate leaves their
order alone'
+ key.columns*.name == ['owner_alpha', 'owner_zeta']
+ key.isSorted()
+ key.sortProperties() == null
Review Comment:
Non-blocking. For a key that wraps `null`, this assertion and the column
check after it hold with or without `setSorted(true)`:
`DependantValue.sortProperties()` only permutes columns when the wrapped value
is a `SortableValue`, so here it returns null and leaves the columns alone
either way. To make the `and:` label true, give the key an embedded `Component`
prototype whose properties are declared out of name order (say `zeta`, `alpha`,
with `setEmbedded(true)`): `sortProperties()` then returns a real permutation,
and the assertion fails on a key the linker did not mark sorted and passes on
one it did. The integration specs cover the behavior end to end, so this is
only about what the unit test proves.
##########
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/CollectionSecondPassBinderSpec.groovy:
##########
@@ -20,6 +20,7 @@
package org.grails.orm.hibernate.cfg.domainbinding.secondpass
+import
org.grails.orm.hibernate.cfg.domainbinding.binder.CompositeIdentifierToManyToOneBinder
Review Comment:
Same here: duplicate of the import on line 39.
##########
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/BidirectionalOneToManyLinkerSpec.groovy:
##########
@@ -80,4 +87,72 @@ class BidirectionalOneToManyLinkerSpec extends
HibernateGormDatastoreSpec {
!key.isForeignKeyEnabled()
!key.isConstrained()
}
+
+ void "the copied key is marked sorted so Hibernate does not apply the
identifier permutation to it again"() {
+ given:
+ def metadataContext =
getGrailsDomainBinder().getMetadataBuildingContext()
+ RootClass rootClass = new RootClass(metadataContext)
+ rootClass.setEntityName("TestEntity")
+ Table table = new Table("test_table")
+ rootClass.setTable(table)
+ Property otherSideProperty = new Property()
+ otherSideProperty.setName("owner")
+ BasicValue value = new BasicValue(metadataContext, table)
+ value.addColumn(new Column("owner_id"))
+ otherSideProperty.setValue(value)
+ rootClass.addProperty(otherSideProperty)
+ Collection collection = new Bag(metadataContext, rootClass)
+ DependantValue key = new DependantValue(metadataContext, new
Table("collection_table"), null)
+ HibernateToManyProperty otherSide = Mock(HibernateToManyProperty)
+ otherSide.getName() >> "owner"
+
+ when:
+ linker.link(collection, rootClass, key, otherSide)
+
+ then:
+ key.isSorted()
Review Comment:
Non-blocking. `key.isSorted()` was true under the old `sortProperties()`
call too, since that call sets the flag, so this feature passes on 8.0.x
unchanged. The note on the `sortProperties() == null` assertion below has a
shape that tells the two apart.
--
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]
