borinquenkid commented on PR #16028: URL: https://github.com/apache/grails-core/pull/16028#issuecomment-5029678721
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. -- 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]
