jdaugherty commented on code in PR #16028:
URL: https://github.com/apache/grails-core/pull/16028#discussion_r3652288249
##########
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:
This returns the table name verbatim, and the two call sites then treat it
differently: `joinTableColumName` passes it through `BackticksRemover`, while
`resolveJoinTableForeignKeyColumnName` concatenates `_id` onto it directly.
`TableForManyCalculator.calculateTableForMany` also strips backticks from
`getTableName(...)`, because backtick-quoting a reserved word in `table` is
supported and used (e.g. `grails/gorm/tests/multitenancy/User` maps `table
'`user`'`).
With a quoted table on the far side of a unidirectional `hasMany`, the FK
column name is now malformed:
```groovy
@Entity class ProbeQuoted { String label
static mapping = { table '`user`' } }
@Entity class ProbeShelf { String label
static hasMany = [quoted: ProbeQuoted] }
```
```
Error executing DDL "create table probe_shelf_user (`user`_id bigint,
probe_shelf_quoted_id bigint, unique (probe_shelf_quoted_id, `user`_id))"
via JDBC [Unknown data type: "_ID"]
```
On 8.0.x the same mapping produces `probe_shelf_user(probe_quoted_id,
probe_shelf_quoted_id)`. Without `hibernate.hbm2ddl.halt_on_error` the
statement fails silently and the join table is simply missing from the
generated schema, which makes it an easy one to ship unnoticed.
Stripping backticks here (or at the `resolveJoinTableForeignKeyColumnName`
call site, matching `joinTableColumName`) fixes it. A test with a
backtick-quoted `table` mapping would be worth adding alongside the two new
cases.
##########
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:
As noted in the comment on
`HibernateToManyProperty#resolveJoinTableForeignKeyColumnName`, a bidirectional
many-to-many join table is not affected by this change: I checked the exact
mapping used in this section's example (`Book` with `table 'catalog_book'`,
`Author`/`Book` many-to-many) and both join columns are still derived from the
class names on this branch, identical to 8.0.x.
As written, this section asks many-to-many users to migrate or pin a schema
that isn't changing — which is worse than no note at all, since the suggested
`joinTable` mapping would itself be the schema change. It needs to be re-scoped
to what actually changes (the element FK column of a unidirectional `hasMany`
join table), or the code change extended to cover many-to-many.
One more change worth listing here if it stays in: switching the property
prefix from `resolveTableName` to `resolveColumnName` alters the element column
of basic and enum collections under any strategy whose table and column rules
differ.
##########
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:
Same issue as the upgrade note: this states that the default `hasMany`
join-table foreign keys are derived from the physical table names, but that
only holds for a unidirectional `hasMany`. Add `belongsTo` (or a many-to-many)
and both columns still come from the class names, so a reader with the far more
common bidirectional mapping will not see `book_id`.
Since the paragraph doesn't say which association shape it applies to, I'd
make that explicit, and add a cross-reference to the corresponding
upgrade-guide section so the schema-migration advice is 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:
This one asserts the interactions with a mocked naming strategy rather than
an outcome, which is how it passes for a branch that binding never takes (see
the comment on `joinTableColumName`), and it also pins
`resolveColumnName`/`resolveTableName` call counts that are implementation
detail rather than behavior.
For a regression guard on #15736, could we add at least one test that
completes binding and asserts the resulting join-table columns — e.g. boot a
`HibernateDatastore` over an `Author`/`Book` pair with explicit `table`
mappings and assert the collection table's column names? A test at that level
is what would have surfaced the two behavioral gaps noted in the other comments.
##########
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:
Both callers of `joinTableColumName` take a `HibernateBasicProperty`
(`BasicCollectionElementBinder#bind` and
`EnumTypeBinder#bindEnumTypeForColumn`), and `HibernateBasicProperty extends
BasicWithMapping` which extends `Basic` — so `isBasic()` is always `true` here
and the association branch never executes during binding. The only thing
reaching it is the mocked naming strategy in the new spec.
If it is intended as future-proofing, I'd rather drop the ternary (or move
`joinTableColumName` onto the basic-collection interface, where its two callers
already are) so the code doesn't suggest an association path that doesn't
exist. If there is a mapping that does reach it, a test that goes through the
binder rather than a mock would make that clear.
##########
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:
`resolveJoinTableForeignKeyColumnName` only runs for **unidirectional**
`hasMany` join tables, so a bidirectional many-to-many is not affected by this
change.
`CollectionSecondPassBinder` sends a bidirectional many-to-many element to
`ManyToOneElementBinder` → `ManyToOneBinder` → `SimpleValueBinder` →
`DefaultColumnNameFetcher`, which for a `HibernateManyToManyProperty` returns
`resolveForeignKeyForPropertyDomainClass(...)` — still the decapitalized class
simple name run through `resolveColumnName`. The only production call site of
the method changed here is `CollectionWithJoinTableBinder`, reached from
`UnidirectionalOneToManyBinder`.
I compared the generated H2 schema on this branch against the merge base
with these domain classes:
```groovy
@Entity class ProbeAuthor { String name
static hasMany = [books: ProbeBook]
static mapping = { table 'writer' } }
@Entity class ProbeBook { String title
static belongsTo = ProbeAuthor
static hasMany = [authors: ProbeAuthor]
static mapping = { table 'catalog_book' } }
@Entity class ProbeShelf { String label
static hasMany = [shelved: ProbeBook] } // unidirectional
```
| join table | 8.0.x | this branch |
|---|---|---|
| `writer_books` (bidirectional many-to-many) | `probe_author_id`,
`probe_book_id` | unchanged |
| `probe_shelf_catalog_book` (unidirectional) | `probe_book_id`,
`probe_shelf_shelved_id` | `catalog_book_id`, `probe_shelf_shelved_id` |
Two consequences worth deciding on explicitly:
- the many-to-many shape described in both doc changes (and in #15736's
example app, if its associations are bidirectional) gets no new behavior;
- even in the path that did change, the owner-side key column
(`probe_shelf_shelved_id`) is still derived from the owner *class* name via
`DefaultColumnNameFetcher`, so one column of the join table now follows the
resolved table name and the other does not.
Could we either extend the resolution to
`DefaultColumnNameFetcher#getDefaultColumnName` /
`resolveForeignKeyForPropertyDomainClass` so both sides of a join table agree,
or keep the code change as-is and narrow the documentation to the
unidirectional case it actually covers?
##########
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:
Every other domain class in this spec is `HTMP`-prefixed to keep the file's
entities namespaced, and there are already several unrelated `Book` domain
classes elsewhere in this test source set. `HTMPMappedTableBook` with, say,
`table 'catalog_book'` would keep the convention and still demonstrate that the
explicit `table` mapping wins over the class name.
--
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]
