Fix join-table foreign keys to use resolved entity table names (#15736)

[gsartori]

Description

Fixes #15736.

GORM previously generated default hasMany join-table foreign-key columns from the associated domain class name. This produced inconsistent names when the entity’s effective table name differed because of a custom Hibernate PhysicalNamingStrategy or an explicit table mapping.

Join-table foreign-key prefixes are now derived from GrailsHibernatePersistentEntity#getTableName(...), which resolves explicit mappings first and otherwise applies the configured naming strategy.

Examples covered by regression tests:

• HTMPBook transformed by a naming strategy to table book produces book_id.
• Book explicitly mapped to table htmp_book produces htmp_book_id.

The same table-name resolution is used by association join-table naming while preserving the existing behavior for basic and enum collections. Explicit join-table column mappings continue to take precedence.

The custom naming strategy documentation now explains its effect on join-table foreign-key columns and warns that existing applications may require a schema migration or explicit column configuration.

Contributor Checklist

Issue and Scope

• This PR is linked to an existing issue that has been acknowledged or approved by the project team. The PR fixes #15736, but its approval status has not been independently verified.
• This PR addresses the complete scope of the linked issue.
• This PR contains a single, focused change.
• This PR targets the 8.0.x major release branch.

Code Quality

• I have added regression tests covering both custom physical naming and explicit entity table mappings.

• I have not run the complete ./gradlew build --rerun-tasks.

• I have not run the complete ./gradlew codeStyle.

• The targeted test was run successfully:

  ./gradlew :grails-data-hibernate7-core:test \
    --tests "org.grails.orm.hibernate.cfg.domainbinding.hibernate.HibernateToManyPropertySpec" \
    --rerun-tasks
  

  Result: 47 tests passed, 0 failures, 0 skipped.

• This PR does not contain mass reformatting, style-only changes, or large-scale refactoring.

• Generative AI tooling was used with a quality model, and the resulting changes were reviewed and tested.

Licensing and Attribution

• All changes are provided under the Apache License 2.0. No new source files were introduced; modified files retain their existing Apache license headers.
• I have the necessary rights to submit this contribution and confirm it is my own original work.
• I have followed the Apache Software Foundation’s policy on generative tooling and properly attributed its use.

Documentation

• The custom naming strategy documentation explains how resolved table names affect join-table foreign-key columns.
• This is a bug fix rather than a new feature, so no What’s New update is required.
• The documentation contains a schema-migration warning, but the corresponding Upgrade Notes have not been updated.
• This description explains what changed and why.

[gsartori]

@borinquenkid this code has been made with the support of AI, please consider it as a starting point to address the issue

[borinquenkid]

Thanks for tracking this down, @gsartori — the root cause you identified is correct: deriving the join-table FK/column prefix from the raw simple class name instead of the entity's actually-resolved table name (mapping override or PhysicalNamingStrategy) was the bug behind #15736, and routing it through GrailsHibernatePersistentEntity#getTableName(...) is the right fix.

While reviewing it, I noticed the same getHibernateAssociatedEntity().getHibernateRootEntity().getTableName(namingStrategy) expression was duplicated between resolveJoinTableForeignKeyColumnName() and joinTableColumName(), and that duplication was masking an adjacent latent bug: joinTableColumName()'s property-name prefix was resolved via resolveTableName(getName()) even though the result is used as a column, not a table. That's invisible under Grails' default snake-case strategy (table and column resolution happen to produce identical output there), but it silently produces the wrong prefix under a strategy that treats table and column naming differently — the same class of bug as #15736, just in the sibling method.

I pushed a follow-up commit (aa02d74) directly onto this branch that:

• extracts the duplicated table-name lookup into HibernateAssociation#resolveAssociatedEntityTableName(namingStrategy), shared by both HibernateToOneProperty and HibernateToManyProperty — HibernateAssociation is the right home since it's only ever implemented by real association properties, never embeddables (which don't have a table of their own)
• fixes joinTableColumName()'s property prefix to use resolveColumnName(getName())
• corrects the one existing test that was asserting the buggy interaction (joinTableColumName applies table naming to an associated entity)

Ran the full grails-data-hibernate7-core suite and codeStyle locally against this — all green.

One thing needs to happen before this can merge, though: this PR also bundles in a removal of grails.controllers.upload.* multipart config in favor of spring.servlet.multipart.*, including a hard IllegalStateException at startup for any app still using the old keys. That's a real, breaking behavioral change — it doesn't carry the weight of a PR titled and scoped around join-table FK naming, and it isn't covered by #15736 or by anything in this PR's description. It needs its own issue, its own PR, and its own upgrade-notes entry so it gets reviewed and communicated on its own merits, not folded into this one. Please pull the following out:

• THREAT_MODEL.md
• grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/ControllersAutoConfiguration.java
• grails-controllers/src/test/groovy/org/grails/plugins/web/controllers/ControllersAutoConfigurationSpec.groovy
• grails-core/src/main/groovy/grails/config/Settings.groovy
• grails-doc/src/en/guide/theWebLayer/controllers/uploadingFiles.adoc
• grails-web-core/src/main/resources/META-INF/spring-configuration-metadata.json
• threat-model.yaml

Once those are out, this is in good shape to merge on the join-table naming fix alone.

[borinquenkid]

Do the changes as described

[gsartori]

@borinquenkid I've messed up two different branches when updating the codebase to Grails 8 without noticing. I've reverted the unwanted commits, it should be clean now

[bito-code-review]

The change from resolveTableName to resolveColumnName in HibernateToManyProperty.java is intended to ensure that property names are processed using the naming strategy's column naming rules rather than table naming rules. In the context of the modified code, getName() refers to a property name, which should be mapped to a column name in the database schema. Using resolveTableName for a property name was likely incorrect because property-to-column mapping logic can differ from entity-to-table mapping logic, especially when custom naming strategies are involved.

grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateToManyProperty.java

var clazz = isBasic() ?
                    namingStrategy.resolveColumnName(referencedType.getName()) :
                    resolveAssociatedEntityTableName(namingStrategy);
            var prop = namingStrategy.resolveColumnName(getName());

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.4910%. Comparing base (fa30af0) to head (0ca2559).
⚠️ Report is 25 commits behind head on 8.0.x.

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #16028        +/-   ##
==================================================
+ Coverage     51.4351%   51.4910%   +0.0559%     
- Complexity      17723      17762        +39     
==================================================
  Files            2039       2039                
  Lines           95497      95537        +40     
  Branches        16564      16571         +7     
==================================================
+ Hits            49119      49193        +74     
+ Misses          39078      39040        -38     
- Partials         7300       7304         +4     

see 33 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[matrei]

This change warrants an entry in the upgrade guide as it could be breaking for existing applications.

[gsartori]

@materi I've added a paragraph to the upgrade guide, feel free to change it if you feel it's not to the point

[jdaugherty]

Prior to reviewing this, I asked AI to review this PR and have included the comments here. Please take a look and then I'll take another pass myself:

Thanks for digging into this — the direction is right, and resolving the prefix through GrailsHibernatePersistentEntity#getTableName(...) is consistent with what TableForManyCalculator already does for the join-table name.

Two things I think need to be resolved before this goes in:

1. The change only reaches unidirectional hasMany join tables. A bidirectional many-to-many still names both join columns from the class names, so the upgrade note and the naming-strategy doc currently describe a change that does not happen for the shape most readers will have.
2. Appending _id to the raw table name regresses backtick-quoted table mappings (table 'user') into invalid DDL. TableForManyCalculator strips backticks for exactly this reason.

Repro details and before/after schemas are in the line comments.

The red functional-test job looks unrelated: it failed pulling images from registry-1.docker.io and connecting to develocity.apache.org, so a rerun should clear it.

[jdaugherty]

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:

@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 Grails 8 GORM ignores NamingStrategy for hasMany join table column names (prefixed with entity class name) #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?

[borinquenkid]

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 basic/enum collections.

[jdaugherty]

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:

@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.

[borinquenkid]

Fixed in 546fea4 (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).

[jdaugherty]

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.

[borinquenkid]

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.

[jdaugherty]

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.

[borinquenkid]

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.

[jdaugherty]

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.

[borinquenkid]

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.

[jdaugherty]

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.

[borinquenkid]

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.

[jdaugherty]

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.

[borinquenkid]

Renamed to HTMPMappedTableBook in 4da31cc to keep the file's HTMP-prefix convention.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: 4da31cc
▶️ Tests: 57603 executed
⚪️ Checks: 60/60 completed

---

Learn more about TestLens at testlens.app.