borinquenkid commented on code in PR #16028:
URL: https://github.com/apache/grails-core/pull/16028#discussion_r3687106494
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateToManyProperty.java:
##########
@@ -212,10 +212,7 @@ default String
resolveJoinTableForeignKeyColumnName(PersistentEntityNamingStrate
return ofNullable(getHibernateMappedForm())
.map(PropertyConfig::getJoinTableColumnConfig)
.map(ColumnConfig::getName)
- .orElseGet(() ->
namingStrategy.resolveColumnName(getHibernateAssociatedEntity()
- .getHibernateRootEntity()
- .getJavaClass()
- .getSimpleName()) +
+ .orElseGet(() ->
resolveAssociatedEntityTableName(namingStrategy) +
Review Comment:
Thanks for the detailed repro — went with your second option: kept the code
narrowly scoped to what it actually fixes (the associated-entity FK column of a
unidirectional `hasMany`, via `CollectionWithJoinTableBinder`) rather than
extending it into `DefaultColumnNameFetcher`/many-to-many, which would be a
materially larger change than #15736 asked for. Instead, re-scoped the docs to
describe exactly that in 4da31cc: upgrading80x.adoc §26.9 now leads with the
unidirectional-only scope, replaces the many-to-many example with a genuinely
unidirectional one (`Shelf hasMany books`, no `belongsTo`), and notes the
owner-side column (`shelf_id`) is unchanged. Also added a doc comment on
`resolveJoinTableForeignKeyColumnName` itself recording the scope and pointing
at `DefaultColumnNameFetcher#resolveForeignKeyForPropertyDomainClass` as the
unaffected many-to-many path, and a closing paragraph noting the
`resolveTableName`→`resolveColumnName` property-prefix change you flagged for
bas
ic/enum collections.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateAssociation.java:
##########
@@ -80,6 +81,10 @@ default String getReferencedEntityName() {
return getHibernateAssociatedEntity().getName();
}
+ default String
resolveAssociatedEntityTableName(PersistentEntityNamingStrategy namingStrategy)
{
Review Comment:
Fixed in 546fea44 (pushed before your review, sorry for the noise) —
stripped backticks once at the source in
`HibernateAssociation#resolveAssociatedEntityTableName`, which both
`joinTableColumName` and `resolveJoinTableForeignKeyColumnName` go through, so
it's no longer left to each caller. Added "resolveJoinTableForeignKeyColumnName
strips backticks from a backtick-quoted associated entity table name"
reproducing your `table '\`user\`'`-shaped repro
(`HTMPQuotedTableAuthor`/`HTMPQuotedTableBook` in the spec).
##########
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() ?
Review Comment:
Dropped the ternary in 4da31cc — confirmed both real callers
(`BasicCollectionElementBinder#bind`, `EnumTypeBinder#bindEnumTypeForColumn`)
type their parameter as `HibernateBasicProperty`, so `isBasic()` is always true
here and the association branch was dead. `joinTableColumName` now always
resolves `clazz` via `resolveColumnName(referencedType.getName())`, with a
comment explaining why `resolveAssociatedEntityTableName` doesn't apply on this
path. Went with removing it over relocating the method onto
`HibernateToManyCollectionProperty` to keep the diff small, since dropping the
branch already removes the misleading suggestion of an association path.
##########
grails-doc/src/en/guide/upgrading/upgrading80x.adoc:
##########
@@ -1215,6 +1215,64 @@ GORM's `createCriteria()` and `withCriteria()` DSL are
implemented on top of the
*`javax.persistence` → `jakarta.persistence`*: This migration was already
required for Grails 7; Grails 8 continues to require `jakarta.*`.
+===== 26.9 Many-to-Many Join-Table Column Names
Review Comment:
Re-scoped in 4da31cc — renamed the section, opened with an explicit callout
that only a unidirectional `hasMany` is affected and a bidirectional
many-to-many is not, and swapped the example for a genuinely unidirectional one
(`Shelf hasMany books`) instead of the `Author`/`Book` many-to-many that wasn't
actually changing. Also called out that the join table's other column
(owner-side) is unchanged, and added your point about the basic/enum collection
element-column prefix as a separate paragraph at the end.
##########
grails-data-hibernate7/docs/src/docs/asciidoc/advancedGORMFeatures/ormdsl/customNamingStrategy.adoc:
##########
@@ -68,3 +68,5 @@ class UpperCaseNamingStrategy implements
PhysicalNamingStrategy {
----
TIP: Individual column or table names set explicitly in the `mapping` block
always take precedence over what the naming strategy would produce.
+
+The default foreign-key column names in a `hasMany` join table are derived
from the physical table names of the associated domain classes. Consequently, a
custom strategy that changes a domain table name also changes the corresponding
join-table foreign-key column prefix. For example, if the strategy maps `TBook`
to the table `book`, the default foreign-key column is `book_id`, not
`tbook_id`. Applications upgrading from an earlier GORM version should account
for this schema change or configure the join-table columns explicitly in the
`mapping` block.
Review Comment:
Re-scoped this paragraph in 4da31cc to make the unidirectional-only
condition explicit up front, and added a cross-reference to the upgrade guide's
§26.9 (now titled "Join-Table Foreign-Key Column Names") so the migration
guidance lives in one place.
##########
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateToManyPropertySpec.groovy:
##########
@@ -351,6 +377,22 @@ class HibernateToManyPropertySpec extends
HibernateGormDatastoreSpec {
property.joinTableColumName(namingStrategy) != null
}
+ void "joinTableColumName applies table naming to the associated entity and
column naming to the property prefix"() {
Review Comment:
Replaced it in 4da31cc — the new test boots a real `PhysicalNamingStrategy`
(`HTMPColumnMarkingPhysicalNamingStrategy`) whose `toPhysicalColumnName`
diverges from its (default) `toPhysicalTableName` for the property name, then
asserts the resulting `joinTableColumName` prefix carries the column-naming
marker — i.e. an outcome that only holds if the property prefix actually goes
through `resolveColumnName`, not `resolveTableName`. No more mock
interaction/call-count assertions on the (now-removed) unreachable branch.
##########
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateToManyPropertySpec.groovy:
##########
@@ -581,6 +623,33 @@ class HTMPBook {
String title
}
+@Entity
+class Book {
Review Comment:
Renamed to `HTMPMappedTableBook` in 4da31cc to keep the file's `HTMP`-prefix
convention.
--
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]
