jdaugherty commented on code in PR #16686: URL: https://github.com/apache/grails-core/pull/16686#discussion_r4330750279
########## grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/MultipleUnidirectionalHasManySpec.groovy: ########## @@ -0,0 +1,88 @@ +/* + * 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 grails.gorm.annotation.Entity + +/** + * An owner with several unidirectional hasMany associations to different classes keeps a join table for each of + * them, and an element added to one of them is written to its own join table. + */ +class MultipleUnidirectionalHasManySpec extends HibernateGormDatastoreSpec { + + void setupSpec() { + manager.registerDomainClasses(MuhItem, MuhRole, MuhTag) + } + + void cleanup() { + MuhItem.withNewTransaction { + MuhItem.executeUpdate('delete from MuhItem', [:]) + MuhRole.executeUpdate('delete from MuhRole', [:]) + MuhTag.executeUpdate('delete from MuhTag', [:]) + } + } + + void "an element added to one of two unidirectional hasMany associations is written to its join table"() { + given: "a persisted item with two unidirectional hasMany associations to different classes" + def item = new MuhItem(name: 'Example item').save(flush: true) + + when: "a role is added to the managed item and the item is flushed" + item.addToRoles(new MuhRole(authority: 'ROLE_EXAMPLE')) + item.save(flush: true) + MuhItem.withSession { it.clear() } + + then: "the role is found through the reloaded item" + MuhItem.get(item.id).roles*.authority == ['ROLE_EXAMPLE'] + MuhItem.get(item.id).tags.isEmpty() + } + + void "elements added to both unidirectional hasMany associations are written to their own join tables"() { + given: + def item = new MuhItem(name: 'Example item').save(flush: true) + + when: + item.addToRoles(new MuhRole(authority: 'ROLE_A')) + item.addToTags(new MuhTag(name: 'tag-a')) + item.save(flush: true) + MuhItem.withSession { it.clear() } + + then: + MuhItem.get(item.id).roles*.authority == ['ROLE_A'] + MuhItem.get(item.id).tags*.name == ['tag-a'] + } +} + +@Entity +class MuhItem { + Long id + String name + static hasMany = [roles: MuhRole, tags: MuhTag] Review Comment: The tests cover two `hasMany` to different classes, but not the reporter's second shape from #16665: two `hasMany` to the same class, each with its own join table. ```groovy static hasMany = [roles: Role, denyRoles: Role] static mapping = { roles joinTable: 'item_allow_roles' denyRoles joinTable: 'item_deny_roles' } ``` This is the case where comparing collection tables gives a different answer from comparing associated classes, so it pins the rule this PR chose. As a local probe, adding a role to each and reading them back passes on this branch and fails on 8.0.x. Could you add it here? ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/CollectionKeyColumnUpdater.java: ########## @@ -39,19 +40,23 @@ public CollectionKeyColumnUpdater(CollectionKeyBinder collectionKeyBinder) { public void bind(HibernateToManyProperty property) { DependantValue key = collectionKeyBinder.bind(property); key.getColumns().stream().filter(Objects::nonNull).forEach(column -> column.setNullable(true)); + Table collectionTable = property.getCollection().getCollectionTable(); long unidirectionalCount = property.getHibernateOwner() .getPersistentPropertiesToBind() .stream() .filter(HibernateToManyProperty.class::isInstance) .map(HibernateToManyProperty.class::cast) .filter(p -> !p.isBidirectional()) + .filter(p -> p.getCollection() != null && + Objects.equals(p.getCollection().getCollectionTable(), collectionTable)) Review Comment: **This fails the session factory build when the owner also has a to-many that is not bound as a collection.** `getCollection()` never returns null, so the null check here never applies. It throws `MappingException` when the property has no Hibernate collection. That is the case for a to-many mapped with `type: 'serializable'` or with a user type, which `GrailsPropertyBinder` binds as a column because `HibernateToManyProperty.isBoundAsCollection` is false for them. The stream also runs before the `HibernateToManyCollectionProperty` check below, so a basic collection on such an owner fails too, although it worked before. ```groovy @Entity class Owner { Long id Set<String> names Set<String> tags static hasMany = [names: String, tags: String] static mapping = { tags type: 'serializable' } } ``` ``` org.hibernate.MappingException: Hibernate Collection has not been initialized for property [tags]. Call setCollection() first. ``` I ran three local probes against the base and against this branch: | Owner | 8.0.x (bd0c1394ce) | this PR | |---|---|---| | basic `names` next to a serializable `tags` | works | fails at startup | | entity `roles` next to a serializable `tags` | starts, `roles` rows not written | fails at startup | | entity `roles` next to a user-type `tags` (`UgTagSetType`) | starts, `roles` rows not written | fails at startup | Filtering with the existing predicate fixes all three. It also stops the column-bound sibling from counting, which is why `roles` was dropped on the base: ```suggestion .filter(HibernateToManyProperty::isBoundAsCollection) .map(HibernateToManyProperty.class::cast) .filter(p -> !p.isBidirectional()) .filter(p -> Objects.equals(p.getCollection().getCollectionTable(), collectionTable)) ``` With that change, the three probes pass, as do `MultipleUnidirectionalHasManySpec`, the `secondpass` and `hasmany` packages, `UniqueGroupEnumAndCollectionSpec` and `GrailsPropertyBinderSpec`. Could you add one of these shapes to `MultipleUnidirectionalHasManySpec`, so a column-bound sibling stays covered? The hand-ported version in #16572 needs the same change. -- 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]
