Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gsartori wants to merge 10 commits into
base: 8.0.x
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bfdbbe8
PR for: https://github.com/apache/grails-core/issues/15644
gsartori Jul 20, 2026
d1ca11f
Merge fixes & cleanups
gsartori Jul 20, 2026
a77c8f2
Fix proposal for https://github.com/apache/grails-core/issues/15736
gsartori Jul 20, 2026
aa02d74
Consolidate join-table table-name resolution and fix property-prefix …
borinquenkid Jul 21, 2026
ba081ef
Revert "Merge fixes & cleanups"
gsartori Jul 22, 2026
3195e1d
Revert "PR for: https://github.com/apache/grails-core/issues/15644"
gsartori Jul 22, 2026
0ca2559
Upgrade guide
gsartori Jul 22, 2026
546fea4
Strip backticks from resolved associated-entity table names
borinquenkid Jul 27, 2026
4da31cc
Narrow join-table naming fix's scope in code and docs to unidirection…
borinquenkid Jul 30, 2026
d2655f0
Merge branch '8.0.x' into naming-strategy-table-name-mapping-fix
jamesfredley Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...ain/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateAssociation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.grails.datastore.mapping.model.PersistentEntity;
import org.grails.datastore.mapping.model.PersistentProperty;
import org.grails.orm.hibernate.cfg.Mapping;
import org.grails.orm.hibernate.cfg.PersistentEntityNamingStrategy;
import org.grails.orm.hibernate.cfg.PropertyConfig;

/**
Expand Down Expand Up @@ -80,6 +81,10 @@ default String getReferencedEntityName() {
return getHibernateAssociatedEntity().getName();
}

default String resolveAssociatedEntityTableName(PersistentEntityNamingStrategy namingStrategy) {

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 borinquenkid Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return getHibernateAssociatedEntity().getHibernateRootEntity().getTableName(namingStrategy);
}

@Override
default void validateAssociation() {
if (getUserType() != null) {
Expand Down
11 changes: 5 additions & 6 deletions .../groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateToManyProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) +

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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 borinquenkid Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

GrailsDomainBinder.FOREIGN_KEY_SUFFIX);
}

Expand All @@ -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() ?

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 borinquenkid Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

namingStrategy.resolveColumnName(referencedType.getName()) :
resolveAssociatedEntityTableName(namingStrategy);
var prop = namingStrategy.resolveColumnName(getName());
Comment thread
matrei marked this conversation as resolved.
columnName = referencedType.isEnum() ?
clazz :
new BackticksRemover().apply(prop) + UNDERSCORE + new BackticksRemover().apply(clazz);
Expand Down
69 changes: 69 additions & 0 deletions ...y/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateToManyPropertySpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,18 @@ package org.grails.orm.hibernate.cfg.domainbinding.hibernate
import grails.gorm.annotation.Entity
import grails.gorm.tests.HibernateGormDatastoreSpec
import org.hibernate.MappingException
import org.hibernate.boot.model.naming.Identifier
import org.hibernate.boot.model.naming.PhysicalNamingStrategyStandardImpl
import org.hibernate.engine.jdbc.env.spi.JdbcEnvironment
import org.grails.datastore.mapping.model.PersistentProperty
import org.grails.datastore.mapping.model.PersistentEntity
import org.grails.datastore.mapping.model.PropertyMapping
import org.grails.datastore.mapping.model.ClassMapping
import org.grails.datastore.mapping.reflect.EntityReflector
import org.grails.orm.hibernate.cfg.PropertyConfig
import org.grails.orm.hibernate.cfg.Mapping
import org.grails.orm.hibernate.cfg.PersistentEntityNamingStrategy
import org.grails.orm.hibernate.cfg.domainbinding.util.NamingStrategyWrapper

class HibernateToManyPropertySpec extends HibernateGormDatastoreSpec {

Expand Down Expand Up @@ -63,6 +68,27 @@ class HibernateToManyPropertySpec extends HibernateGormDatastoreSpec {
columnName == "custom_book_fk"
}

void "resolveJoinTableForeignKeyColumnName removes a domain prefix through a physical naming strategy"() {
given:
def property = createTestHibernateToManyProperty(HTMPAuthor, "books")
def namingStrategy = new NamingStrategyWrapper(
new HTMPPrefixRemovingPhysicalNamingStrategy(), getGrailsDomainBinder().jdbcEnvironment)
hibernateFirstPass()

expect:
property.resolveJoinTableForeignKeyColumnName(namingStrategy) == "book_id"
}

void "resolveJoinTableForeignKeyColumnName uses the associated entity explicit table mapping"() {
given:
def property = createTestHibernateToManyProperty(HTMPMappedTableAuthor, "books")
def namingStrategy = getGrailsDomainBinder().namingStrategy
hibernateFirstPass()

expect:
property.resolveJoinTableForeignKeyColumnName(namingStrategy) == "htmp_book_id"
}

void "isAssociationColumnNullable returns false for ManyToMany"() {
given: "Register only entities for this specific test"
createPersistentEntity(HTMPCourse) // Course is needed because Student refers to it
Expand Down Expand Up @@ -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"() {

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 borinquenkid Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

given:
def property = createTestHibernateToManyProperty(HTMPAuthor, "books")
def namingStrategy = Mock(PersistentEntityNamingStrategy)
hibernateFirstPass()

when:
String columnName = property.joinTableColumName(namingStrategy)

then:
1 * namingStrategy.resolveTableName(_ as GrailsHibernatePersistentEntity) >> "book"
1 * namingStrategy.resolveColumnName("books") >> "books"
0 * namingStrategy.resolveTableName("books")
columnName == "books_book"
}

void "joinTableColumName returns derived column name for enum collection"() {
given:
def property = createTestHibernateToManyProperty(HTMPEntityWithEnum, "statuses")
Expand Down Expand Up @@ -581,6 +623,33 @@ class HTMPBook {
String title
}

@Entity
class Book {

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 borinquenkid Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Long id
String title

static mapping = {
table 'htmp_book'
}
}

@Entity
class HTMPMappedTableAuthor {
Long id
String name
static hasMany = [books: Book]
}

class HTMPPrefixRemovingPhysicalNamingStrategy extends PhysicalNamingStrategyStandardImpl {

@Override
Identifier toPhysicalTableName(Identifier logicalName, JdbcEnvironment jdbcEnvironment) {
logicalName.text == HTMPBook.simpleName ?
Identifier.toIdentifier('book') :
super.toPhysicalTableName(logicalName, jdbcEnvironment)
}
}

@Entity
class HTMPAuthor {
Long id
Expand Down
2 changes: 2 additions & 0 deletions ...e7/docs/src/docs/asciidoc/advancedGORMFeatures/ormdsl/customNamingStrategy.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 borinquenkid Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading