borinquenkid commented on code in PR #16028:
URL: https://github.com/apache/grails-core/pull/16028#discussion_r3632280523
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateToManyProperty.java:
##########
@@ -227,8 +224,10 @@ default String
joinTableColumName(PersistentEntityNamingStrategy namingStrategy)
if (present) {
columnName = joinColumnMappingOptional.get().getName();
} else {
- var clazz =
namingStrategy.resolveColumnName(referencedType.getName());
- var prop = namingStrategy.resolveTableName(getName());
+ var clazz = isBasic() ?
+ namingStrategy.resolveColumnName(referencedType.getName())
:
+ resolveAssociatedEntityTableName(namingStrategy);
+ var prop = namingStrategy.resolveColumnName(getName());
Review Comment:
Good catch to double check — yes, this was a real bug, not just a stylistic
change.
While working the #15736 fix, the AI assistant flagged that
`joinTableColumName()` had the same class of problem as the original issue: it
was resolving `getName()` (a **property** name) through `resolveTableName()`,
when the result is used as a column prefix, not a table. I confirmed that was
correct and worked out where the fix needed to land — extracting the shared
table-name lookup into
`HibernateAssociation#resolveAssociatedEntityTableName(namingStrategy)` so both
`HibernateToOneProperty` and `HibernateToManyProperty` use one path, and
switching the property-prefix call to `resolveColumnName()`.
To spell out why it's a genuine bug and not just cleanup:
`PersistentEntityNamingStrategy` declares `resolveColumnName(String)` and
`resolveTableName(String)` as distinct methods. `NamingStrategyWrapper` shows
they delegate to different Hibernate hooks — `resolveColumnName` →
`PhysicalNamingStrategy#toPhysicalColumnName`, `resolveTableName` →
`PhysicalNamingStrategy#toPhysicalTableName`. Those are independently
overridable in Hibernate's contract; a custom strategy can legitimately
pluralize table names, or case them differently, without touching column naming.
It stayed invisible because Grails' default
(`PhysicalNamingStrategySnakeCaseImpl`) applies identical logic to both, so
`resolveTableName("books") == resolveColumnName("books")` there. The old code
happened to produce the right answer for the wrong reason.
The new test locks this in with a `Mock(PersistentEntityNamingStrategy)`:
```groovy
1 * namingStrategy.resolveColumnName("books") >> "books"
0 * namingStrategy.resolveTableName("books")
```
That assertion fails against the pre-fix code, which did call
`resolveTableName("books")` — so it's a real regression test, not just a
refactor for clarity.
--
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]
