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 4 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
6 changes: 3 additions & 3 deletions THREAT_MODEL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ The framework exposes a small number of configuration knobs whose value affects
| `grails.databinding.autoGrowCollectionLimit` | 256 *(documented: [`SimpleDataBinder.groovy`](./grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy))* | Caps automatic collection growth during data binding - hard limit on memory amplification from an attacker submitting deeply indexed parameters (`list[1000000]=x`). Raising removes the cap. | **§14 wave 2** - is the documented default the supported production posture, or is the operator expected to lower it? |
| `grails.databinding.dateFormats` / `dateParsingLenient` | RFC-3339 + locale defaults; lenient parsing on | Affects how strict date binding is. Loose parsing has historically been a source of validation-bypass findings in other frameworks. | **§14 wave 2** |
| `grails.views.default.codec` and codec defaults (`grails.views.gsp.codecs.expression`, `scriptlet`, `taglib`, `staticparts`) | `html` for expression / scriptlet contexts (XSS protection on by default) | Setting any of these to `none` **disables automatic output encoding** for that context - immediate `OUT-OF-MODEL: non-default-build` for XSS reports under non-default settings. *(documented: [xssPrevention.adoc](./grails-doc/src/en/guide/security/xssPrevention.adoc), [codecs.adoc](./grails-doc/src/en/guide/security/codecs.adoc))* | **§14 wave 1** - confirm the `html` default is the supported production posture. |
| `grails.controllers.upload.maxFileSize` / `maxRequestSize` | **128000 bytes (~125 KB) each**, set by `ControllersAutoConfiguration` (overrides Spring Boot's `MultipartProperties` defaults). *(documented: [`ControllersAutoConfiguration.java`](./grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/ControllersAutoConfiguration.java))* | Multipart upload size cap. Operators who raise these past their application's actual need expose themselves to DoS via large multipart bodies. | **§14 wave 2** |
| `spring.servlet.multipart.max-file-size` / `max-request-size` | **1 MB per file / 10 MB per request**, provided by Spring Boot's `MultipartProperties` defaults. *(documented: [uploadingFiles.adoc](./grails-doc/src/en/guide/theWebLayer/controllers/uploadingFiles.adoc))* | Multipart upload size cap. Operators who raise these past their application's actual need expose themselves to DoS via large multipart bodies. | **§14 wave 2** |
| `grails.allowedMethods` (per-controller) | None (developer opt-in) | Restricts HTTP methods accepted by each action. Absence is **not** a finding; the model treats per-action method gating as a developer responsibility. *(inferred)* | **§14 wave 1** |
| `grails.config.locations` (env var, system property, or config) | Empty | Adds external config file paths. **A non-empty value sourced from an untrusted location is a `BY-DESIGN: property-disclaimed` triage outcome** - see §9. | **§14 wave 1** - confirm this disposition. |
| `GRAILS_ENV` / `grails.env` | `development` from CLI, `production` for assembled bootJars | Selects the active environment block in `application.yml` / `application.groovy`. Operators who deploy with `GRAILS_ENV=development` inherit the looser dev defaults (e.g., stack traces in responses). | **§14 wave 1** - is deploying with `development` a `non-default-build` posture? |
Expand All @@ -212,7 +212,7 @@ The framework's public input boundary is the HTTP request. Per-parameter trust i
| `Controller.params` | All values | **Yes** - direct request parameter map | Type coercion correctness; never concatenate into HQL/SQL/JPQL/Groovy strings; never use as redirect target without an allow-list. *(documented: [securingAgainstAttacks.adoc](./grails-doc/src/en/guide/security/securingAgainstAttacks.adoc) "XSS", "HTML/URL injection")* |
| `Controller.request.headers` | All values | **Yes** - including `X-Forwarded-*`, `Host`, `User-Agent`, `Referer`, custom auth tokens | Treat presence as evidence of nothing; auth headers must be verified against the configured auth subsystem (Spring Security or equivalent). *(inferred)* |
| `Controller.request.cookies` | All values | **Yes** | Treat as attacker-supplied; if used for auth, integrity-protect via Spring Security or signed cookies. *(inferred)* |
| `Controller.request.JSON` / `XML` | Full body | **Yes** | Parser inputs are bounded by `maxRequestSize`; nested-depth limits are the parser's responsibility (Jackson, JAXP). *(inferred)* |
| `Controller.request.JSON` / `XML` | Full body | **Yes** | Configure server request-size limits appropriate for the deployment; nested-depth limits are the parser's responsibility (Jackson, JAXP). *(inferred)* |
| `bindData(target, source)` | `source` (any `Map` or request) | **Yes** for the source; **No** for the target type (developer-controlled) | Use `bindable`/`include`/`exclude` to whitelist fields. The framework will bind every settable property of `target` from matching keys in `source` unless told otherwise. *(documented: [GORM data binding guide](https://grails.apache.org/docs/latest/guide/single.html#dataBinding))* |
| Command-object binding (auto-bound controller action parameter) | Field values | **Yes** | Annotate command-object fields with `bindable=false` for fields that must not be set from the request. *(inferred)* |
| Domain-class binding (`new Book(params)`, `book.properties = params`) | Field values | **Yes** | **Mass-assignment risk.** Use command objects or explicit allow-lists rather than binding the request to a domain class. *(inferred)* |
Expand Down Expand Up @@ -295,7 +295,7 @@ Each property is stated with its conditions, the symptom of a violation, a sever
| P6 | **Data binding respects `bindable=false` and explicit `include`/`exclude` lists.** | [CWE-915](https://cwe.mitre.org/data/definitions/915.html) | Field is annotated or the binding call explicitly lists allowed/forbidden fields. | A field marked unbindable is set from request input. | **Security-critical (CVE-eligible)** | *(inferred)* |
| P7 | **Compile-time AST transforms (`@Resource`, `@Validateable`, etc.) only act on developer-authored source.** | [CWE-94](https://cwe.mitre.org/data/definitions/94.html) | Build runs on developer-controlled source. | A transform fires on or is influenced by attacker-supplied input. | **Correctness** (security-critical only if reachable from a non-build attacker) | *(inferred)* |
| P8 | **Configuration loading does not evaluate `application.groovy` from a path the framework itself chose at runtime - paths come from build-time classpath and operator-supplied environment/system properties.** | [CWE-94](https://cwe.mitre.org/data/definitions/94.html) | Operator has not pointed `grails.config.locations` at attacker-writable storage. | A user request causes evaluation of a Groovy file the operator did not authorize. | **Security-critical (CVE-eligible)** if violated. | *(inferred)* (§14 wave 1) |
| P9 | **`maxFileSize` / `maxRequestSize` / `autoGrowCollectionLimit` provide bounded data-binding memory.** | [CWE-770](https://cwe.mitre.org/data/definitions/770.html) | Operator does not raise the limits past application needs. | Memory growth proportional to attacker-controlled input regardless of limit. | **Resource bug** | *(inferred)* (§14 wave 2) |
| P9 | **Multipart upload limits and `autoGrowCollectionLimit` bound request-processing memory.** | [CWE-770](https://cwe.mitre.org/data/definitions/770.html) | Operator does not raise the limits past application needs. | Memory growth proportional to attacker-controlled input regardless of limit. | **Resource bug** | *(inferred)* (§14 wave 2) |

### Resource consumption line

Expand Down
40 changes: 20 additions & 20 deletions ...lers/src/main/groovy/org/grails/plugins/web/controllers/ControllersAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,23 @@
import jakarta.servlet.Filter;
import jakarta.servlet.MultipartConfigElement;

import org.springframework.beans.factory.ObjectProvider;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.boot.autoconfigure.AutoConfiguration;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication;
import org.springframework.boot.context.properties.bind.Bindable;
import org.springframework.boot.context.properties.bind.Binder;
import org.springframework.boot.servlet.autoconfigure.HttpEncodingAutoConfiguration;
import org.springframework.boot.servlet.filter.OrderedCharacterEncodingFilter;
import org.springframework.boot.web.servlet.FilterRegistrationBean;
import org.springframework.boot.webmvc.autoconfigure.DispatcherServletAutoConfiguration;
import org.springframework.boot.webmvc.autoconfigure.DispatcherServletRegistrationBean;
import org.springframework.boot.webmvc.autoconfigure.WebMvcAutoConfiguration;
import org.springframework.context.ApplicationContext;
import org.springframework.context.EnvironmentAware;
import org.springframework.context.annotation.Bean;
import org.springframework.core.env.Environment;
import org.springframework.util.ClassUtils;
import org.springframework.web.filter.CharacterEncodingFilter;
import org.springframework.web.servlet.DispatcherServlet;
Expand All @@ -58,7 +63,15 @@
after = {GrailsDomainClassAutoConfiguration.class}
)
@ConditionalOnWebApplication(type = ConditionalOnWebApplication.Type.SERVLET)
public class ControllersAutoConfiguration {
public class ControllersAutoConfiguration implements EnvironmentAware {

private static final String LEGACY_MULTIPART_CONFIGURATION = "grails.controllers.upload";

static final String LEGACY_MULTIPART_CONFIGURATION_ERROR =
"Configuration properties under 'grails.controllers.upload' are no longer supported. " +
"Use Spring Boot's 'spring.servlet.multipart' configuration instead. For example, set " +
"'spring.servlet.multipart.max-file-size=200MB' and " +
"'spring.servlet.multipart.max-request-size=200MB'.";

@Value("${" + Settings.FILTER_ENCODING + ":utf-8}")
private String filtersEncoding;
Expand All @@ -75,18 +88,6 @@ public class ControllersAutoConfiguration {
@Value("${" + Settings.RESOURCES_PATTERN + ":" + Settings.DEFAULT_RESOURCE_PATTERN + "}")
private String resourcesPattern;

@Value("${" + Settings.CONTROLLERS_UPLOAD_LOCATION + ":#{null}}")
private String uploadTmpDir;

@Value("${" + Settings.CONTROLLERS_UPLOAD_MAX_FILE_SIZE + ":128000}")
private long maxFileSize;

@Value("${" + Settings.CONTROLLERS_UPLOAD_MAX_REQUEST_SIZE + ":128000}")
private long maxRequestSize;

@Value("${" + Settings.CONTROLLERS_UPLOAD_FILE_SIZE_THRESHOLD + ":0}")
private int fileSizeThreshold;

@Value("${" + Settings.WEB_SERVLET_PATH + ":#{null}}")
String grailsServletPath;

Expand Down Expand Up @@ -150,12 +151,11 @@ public FilterRegistrationBean<GrailsWebRequestFilter> grailsWebRequestFilter(Gra
return registrationBean;
}

@Bean
public MultipartConfigElement multipartConfigElement() {
if (uploadTmpDir == null) {
uploadTmpDir = System.getProperty("java.io.tmpdir");
@Override
public void setEnvironment(Environment environment) {
if (Binder.get(environment).bind(LEGACY_MULTIPART_CONFIGURATION, Bindable.mapOf(String.class, Object.class)).isBound()) {
throw new IllegalStateException(LEGACY_MULTIPART_CONFIGURATION_ERROR);
}
return new MultipartConfigElement(uploadTmpDir, maxFileSize, maxRequestSize, fileSizeThreshold);
}

@Bean
Expand All @@ -164,15 +164,15 @@ public DispatcherServlet dispatcherServlet() {
}

@Bean
public DispatcherServletRegistrationBean dispatcherServletRegistration(GrailsApplication application, DispatcherServlet dispatcherServlet, MultipartConfigElement multipartConfigElement) {
public DispatcherServletRegistrationBean dispatcherServletRegistration(GrailsApplication application, DispatcherServlet dispatcherServlet, ObjectProvider<MultipartConfigElement> multipartConfigElement) {
if (grailsServletPath == null) {
boolean isTomcat = ClassUtils.isPresent("org.apache.catalina.startup.Tomcat", application.getClassLoader());
grailsServletPath = isTomcat ? Settings.DEFAULT_TOMCAT_SERVLET_PATH : Settings.DEFAULT_WEB_SERVLET_PATH;
}
DispatcherServletRegistrationBean dispatcherServletRegistration = new DispatcherServletRegistrationBean(dispatcherServlet, grailsServletPath);
dispatcherServletRegistration.setLoadOnStartup(2);
dispatcherServletRegistration.setAsyncSupported(true);
dispatcherServletRegistration.setMultipartConfig(multipartConfigElement);
multipartConfigElement.ifAvailable(dispatcherServletRegistration::setMultipartConfig);
return dispatcherServletRegistration;
}

Expand Down
24 changes: 24 additions & 0 deletions ...rc/test/groovy/org/grails/plugins/web/controllers/ControllersAutoConfigurationSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import java.util.function.Supplier
import grails.core.DefaultGrailsApplication
import grails.core.GrailsApplication

import org.springframework.beans.factory.BeanCreationException
import org.springframework.boot.autoconfigure.AutoConfigurations
import org.springframework.boot.test.context.runner.WebApplicationContextRunner
import org.springframework.boot.web.servlet.AbstractFilterRegistrationBean
Expand All @@ -32,10 +33,12 @@ import org.springframework.boot.web.servlet.ServletContextInitializerBeans
import org.springframework.boot.webmvc.autoconfigure.WebMvcAutoConfiguration
import org.springframework.context.ApplicationContext
import org.springframework.context.ConfigurableApplicationContext
import org.springframework.core.env.MapPropertySource
import org.springframework.mock.web.MockHttpServletRequest
import org.springframework.mock.web.MockHttpServletResponse
import org.springframework.mock.web.MockServletContext
import org.springframework.web.context.WebApplicationContext
import org.springframework.web.context.support.AnnotationConfigWebApplicationContext
import org.springframework.web.context.support.StaticWebApplicationContext
import org.springframework.web.filter.RequestContextFilter
import org.springframework.web.servlet.handler.SimpleMappingExceptionResolver
Expand All @@ -54,6 +57,27 @@ class ControllersAutoConfigurationSpec extends Specification {

def autoConfiguration = new ControllersAutoConfiguration()

def "legacy multipart configuration fails startup with migration instructions"() {
given:
def applicationContext = new AnnotationConfigWebApplicationContext()
applicationContext.servletContext = new MockServletContext()
applicationContext.environment.propertySources.addFirst(new MapPropertySource('test', [
'grails.controllers.upload.maxFileSize': 20000000,
]))
applicationContext.register(ControllersAutoConfiguration)

when:
applicationContext.refresh()

then:
BeanCreationException exception = thrown()
exception.rootCause instanceof IllegalStateException
exception.rootCause.message == ControllersAutoConfiguration.LEGACY_MULTIPART_CONFIGURATION_ERROR

cleanup:
applicationContext.close()
}

void 'grailsWebRequest filter is a RequestContextFilter so Boot WebMvcAutoConfiguration backs off its own RequestContextFilter'() {
when: 'the Grails request-binding filter bean is created'
GrailsWebRequestFilter filter = autoConfiguration.grailsWebRequest(applicationContext)
Expand Down
20 changes: 0 additions & 20 deletions grails-core/src/main/groovy/grails/config/Settings.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,26 +209,6 @@ interface Settings {
*/
String CONTROLLERS_DEFAULT_SCOPE = 'grails.controllers.defaultScope'

/**
* The upload directory for controllers, defaults to java.tmp.dir
*/
String CONTROLLERS_UPLOAD_LOCATION = 'grails.controllers.upload.location'

/**
* The maximum file size
*/
String CONTROLLERS_UPLOAD_MAX_FILE_SIZE = 'grails.controllers.upload.maxFileSize'

/**
* The maximum request size
*/
String CONTROLLERS_UPLOAD_MAX_REQUEST_SIZE = 'grails.controllers.upload.maxRequestSize'

/**
* The file size threshold
*/
String CONTROLLERS_UPLOAD_FILE_SIZE_THRESHOLD = 'grails.controllers.upload.fileSizeThreshold'

/**
* The encoding to use for filters, default to UTF-8
*/
Expand Down
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
Loading
Loading