Fix DDL failure for hasMany collections of enums

[borinquenkid]

Summary

• HibernateBasicProperty.getTable() always resolved to the owning entity's table instead of the collection's join table, so a hasMany-of-enum property's element column was bound to the wrong table: it appeared (wrongly) on the owner and was missing from the join table's own CREATE TABLE, breaking schema generation.
• HibernateToManyProperty.joinTableColumName() derived the enum element column name from the enum's fully-qualified class name instead of its simple name.
• Introduces HibernateBasicEnumProperty so hasMany-of-enum elements are recognized as HibernateEnumProperty like their singular counterparts, collapsing EnumTypeBinder down to a single bindEnumType() entry point and letting each HibernateEnumProperty implementation supply its own table, column name, nullability, and enum-storage configuration instead of the binder branching on property shape.

Fixes #16051

Test plan

• Added EnumHasManyDdlSpec reproducing the issue: join table gets its element column, owner table gets no spurious column, and a hasMany-of-enum can be saved and reloaded end-to-end.
• Updated EnumTypeBinderSpec and BasicCollectionElementBinderSpec for the collapsed EnumTypeBinder API.
• Full :grails-data-hibernate7-core:test suite green (3009 tests, 0 failures).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR fixes Hibernate schema-generation (DDL) failures for hasMany collections of enums in grails-data-hibernate7 by ensuring enum collection elements bind their column to the join table (not the owning entity table) and by stabilizing the derived element column name (simple enum name vs fully-qualified name). It also refactors enum binding so both singular-enum properties and enum-collection elements flow through a single EnumTypeBinder.bindEnumType(...) path, with each HibernateEnumProperty implementation providing its enum/table/column/nullability details.

Changes:

• Fix basic collection element table resolution so enum element columns are added to the collection join table rather than the owning entity table.
• Correct join-table element column naming for enum collections to use the enum simple name instead of the fully-qualified name.
• Consolidate enum binding into a single binder entry point and add/adjust tests to cover the DDL and binding behavior.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateBasicProperty.java	Overrides getTable() to use the collection join table when binding basic collection elements.
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateBasicEnumProperty.java	New enum-collection-element property type implementing HibernateEnumProperty for correct enum binding metadata.
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateEnumProperty.java	Expands the marker interface to supply enum type, column name, and nullability for a unified binder path.
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateToManyProperty.java	Fixes enum join-table element column naming to use the enum simple name.
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateMappingFactory.groovy	Creates HibernateBasicEnumProperty for enum basic collections so they participate in enum binding correctly.
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/GrailsPropertyBinder.java	Prevents enum-collection properties from bypassing collection binding (ensures join table creation still happens).
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/EnumTypeBinder.java	Collapses enum binding to bindEnumType(HibernateEnumProperty, path) and delegates configuration to the property + GrailsEnumType.
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/util/GrailsEnumType.java	Centralizes enum BasicValue configuration (STRING/ORDINAL/IDENTITY) behind a configure(...) method.
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/BasicCollectionElementBinder.java	Routes enum collection elements through the unified enum binder API.
grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/EnumHasManyDdlSpec.groovy	New regression test reproducing #16051: join table contains the enum element column; owner table does not; save/reload works.
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/EnumTypeBinderSpec.groovy	Updates enum binder tests for the unified bindEnumType(...) API and enum-collection property shape.
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/BasicCollectionElementBinderSpec.groovy	Updates collection element binder tests to expect delegation via bindEnumType(...).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.0000%. Comparing base (743730d) to head (70f8751).
⚠️ Report is 328 commits behind head on 8.0.x.

Additional details and impacted files

@@                Coverage Diff                @@
##                8.0.x   #16052         +/-   ##
=================================================
- Coverage     51.4879%        0   -51.4879%     
=================================================
  Files            2039        0       -2039     
  Lines           95537        0      -95537     
  Branches        16571        0      -16571     
=================================================
- Hits            49190        0      -49190     
+ Misses          39042        0      -39042     
+ Partials         7305        0       -7305     

see 2039 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.

[jdaugherty]

I had AI review this and think it's comments are relevant. Take a look and then I'll take another pass.

[jdaugherty]

HibernateToManyPropertySpec is the direct unit test for this method and it was not touched. Its enum case currently asserts nothing useful:

void "joinTableColumName returns derived column name for enum collection"() {
    given:
    def property = createTestHibernateToManyProperty(HTMPEntityWithEnum, "statuses")
    def namingStrategy = getGrailsDomainBinder().namingStrategy

    expect:
    property.joinTableColumName(namingStrategy) != null
}

That passed before this change and passes after it, so the behaviour you are fixing here has no unit-level guard. Since the sibling case two features down ("joinTableColumName uses explicit join table column name when present") already asserts an exact string, please pin this one the same way — == "htmp_status" or whatever the strategy resolves for that enum's simple name. That also documents the Grails 7 parity this restores.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: 70f8751
▶️ Tests: 55689 executed
⚪️ Checks: 60/60 completed

---

Learn more about TestLens at testlens.app.

[jdaugherty]

@borinquenkid have you finished working through the feedback on this PR? Can I take another look yet?

[borinquenkid]

@borinquenkid have you finished working through the feedback on this PR? Can I take another look yet?

@jdaugherty yes

[jdaugherty]

Approving — the fix is correct and now properly guarded. Reverting joinTableColumName to getName() fails three EnumHasManyDdlSpec features, so the regression is genuinely pinned. The round-one items all landed and I've resolved those threads: EnumHasManyDdlSpec asserts exact column names and reads inside doReturningWork with try-with-resources, the joinTable column: and nullable: false cases are there, the GrailsPropertyBinder guard moved onto the interface as isCollectionElement(), and the HibernateEnumProperty javadoc is accurate. The extra HibernateCustomProperty coverage in GrailsPropertyBinderSpec is a good addition too.

Everything below is non-blocking follow-up — none of it is a correctness problem and none of it should hold up the fix. Findings are verified by mutation: I broke the production code and checked whether the suite noticed, rather than inferring from reading.

Three tests that don't assert what they're named for

1. EnumHasManyDdlSpec, the ordinal feature. Substituting enumType: 'string' for 'ordinal' leaves it green. Column names are identical under both styles, and the round trip reads through the mapping it wrote.
2. HibernateMappingFactorySpec. Deleting the HibernateBasicEnumProperty branch outright leaves it 29/29 green — all three createBasicCollection features assert the supertype.
3. HibernateToManyPropertySpec (carried over from round one, still unchanged — I've left that thread open). Its joinTableColumName feature asserts != null and passes against the bug it is named for.

None of these are uncovered behaviour — the suite catches all three regressions elsewhere, which is why they stayed green. They're tests that wouldn't fail if the thing they're named for broke, and each is a small fix. Happy for them to land as a follow-up rather than here.

One design question

HibernateBasicProperty.getTable() is overridden on the shared base class, but only the enum path reads it, and it changes what TableForManyCalculator.getJoinTableSchema() sees. That still yields the right schema, but only because CollectionType.create() seeds collectionTable to the owner's table before getJoinTableSchema() runs. Details inline — worth deciding deliberately rather than inheriting, but not a blocker.

[jdaugherty]

This feature does not test what its name claims. I ran it against the PR with the only change being enumType: 'ordinal' -> enumType: 'string' on OrdinalSurveyResponse, and it still passes:

EnumHasManyDdlSpec > a hasMany of enum with enumType ordinal stores the ordinal, not the name PASSED
Results: SUCCESS (6 tests, 6 successes, 0 failures, 0 skipped)

That is expected from the two assertions:

• columnNamesFor(...) compares column names, which are identical for STRING and ORDINAL. The expect: label even says "the element column is a numeric ordinal column, not a string one", but nothing in the expression looks at the type.
• The round trip reads through the same mapping it wrote through, so it succeeds under either style.

This is the same shape as the contains('answer') issue from the last round — the feature would go green against a build where GrailsEnumType.ORDINAL.configure() was never reached, which is exactly the branch this PR rewrote.

IdentityEnumTypeSpec in this same package is the established pattern for pinning storage representation — it selects the raw column and asserts the stored value:

ResultSet resultSet = ds.getConnection().prepareStatement('select status from enum_entity_domain').executeQuery()

Something equivalent here would actually discriminate, e.g. asserting the raw survey_answer value is 0 for FOR_SURE, or reading data_type from information_schema.columns and asserting it is numeric rather than character. As written the ORDINAL and IDENTITY arms of the new GrailsEnumType.configure() have no coverage on the collection path at all.

Minor, same block: expect: followed by when:/then: puts two unrelated assertions in one feature. Splitting the DDL check from the round trip would let each fail with its own message.

[jdaugherty]

Repeating this from the last round because it is unchanged: HibernateToManyPropertySpec is the direct unit test for this method and it still asserts nothing about the value.

void "joinTableColumName returns derived column name for enum collection"() {
    ...
    expect:
    property.joinTableColumName(namingStrategy) != null
}

I confirmed the gap is real. Reverting just this line to referencedType.getName() and running both specs:

EnumHasManyDdlSpec > join table for a hasMany of enum is created with the element column FAILED
EnumHasManyDdlSpec > a hasMany of enum with enumType ordinal ... FAILED
EnumHasManyDdlSpec > the hasMany enum element column stays nullable ... FAILED
HibernateToManyPropertySpec > joinTableColumName returns derived column name for enum collection PASSED

So the integration spec now guards the fix (good — that is an improvement over last round), but the unit spec for the changed method still passes against the bug. The sibling feature two down, "joinTableColumName uses explicit join table column name when present", already asserts == "tag_val"; please pin this one the same way.

[jdaugherty]

HibernateMappingFactorySpec has three features covering exactly this method, and none of them can tell whether this branch is present. Reducing it back to new HibernateBasicProperty(ghpEntity, context, property) leaves the spec fully green:

Results: SUCCESS (29 tests, 29 successes, 0 failures, 0 skipped)

The three features are:

• "createBasicCollection produces HibernateBasicProperty for a basic element collection"
• "createBasicCollection sets custom marshaller for enum hasMany"
• "createBasicCollection uses Enum base marshaller when no specific marshaller for enum collection type"

The latter two build entities whose collections are enums, and all three assert instanceof HibernateBasicProperty — the supertype, which stays true either way.

The latter two should assert HibernateBasicEnumProperty; the first should assert the negative (!(sectionsProp instanceof HibernateBasicEnumProperty)) so the split is pinned from both sides. EnumHasManyDdlSpec does catch the regression at boot, so this is a weak-assertion problem rather than uncovered behaviour — but it is a one-word fix in each case.

[jdaugherty]

Two things about placing this override on HibernateBasicProperty rather than on HibernateBasicEnumProperty.

Only the enum path needs it. BasicCollectionElementBinder's non-enum branch reads collection.getCollectionTable() directly and never calls property.getTable(), so scalar basic collections gain nothing here and only inherit the risk. Scoping it to the enum subclass would match where resolveEnumColumnName and isEnumColumnNullable were placed.

It changes what TableForManyCalculator.getJoinTableSchema() reads.

String owningTableSchema = property.getTable().getSchema();

For a basic collection that expression no longer means what the variable is named. It still returns the right value, but only because of an ordering coincidence in CollectionBinder.bindCollection():

1. collectionHolder.create(property) -> CollectionType.create() does coll.setCollectionTable(owner.getTable())
2. property.setCollection(collection, path) -> the field here becomes non-null, so getTable() starts returning the collection table
3. bindCollectionTable() calls getJoinTableSchema(), which now reads the collection table — still the owner's table from step 1
4. collection.setCollectionTable(<real join table>)

The correct schema survives only because of the seed in step 1. The javadoc says the fallback covers "before the collection table has been assigned", but by the time getJoinTableSchema() runs the collection table is assigned — to the owner's table. Anything that reorders steps 1 and 3 silently changes the schema of every basic join table.

Either scope the override to the enum subclass, or have getJoinTableSchema() ask the owner directly (property.getPersistentClass().getTable().getSchema()) so it stops depending on this ordering.

EnumHasManyDdlSpec covers the override end-to-end, so there is no coverage hole — but a feature in HibernateBasicPropertySpec pinning both arms of the ternary would be worth adding alongside whichever fix you pick.

[jdaugherty]

The new branch in GrailsPropertyBinder — && !hibernateEnumProperty.isCollectionElement() — has no test in this spec. The only thing currently standing behind it is EnumHasManyDdlSpec booting successfully, which will not tell you which side of the condition broke.

Two features in the same style as the ones already here would pin it at the level it was written:

• a scalar enum property yields the BasicValue from enumTypeBinder
• a hasMany-of-enum property falls through to collectionBinder and yields a Collection

The second is the one that regresses silently if isCollectionElement() ever returns the wrong thing.

[jdaugherty]

The debug log that used to accompany the table-per-hierarchy case in EnumTypeBinder is gone:

LOG.debug("[GrailsDomainBinder] Sub class property [{}] for column name [{}] forced to nullable", ...)

That message is the only signal a user gets that their nullable: false on a table-per-hierarchy subclass was deliberately overridden. isEnumColumnNullable() returns a boolean so it has nowhere natural to log; keeping the message at the EnumTypeBinder call site would preserve it.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (1)

grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/EnumTypeBinder.java:99

• EnumTypeBinder.bindEnumType() only applies ColumnConfig from pc.getColumns(), which is correct for scalar enum properties but skips the joinTable column config for hasMany-of-enum collection elements (where the relevant config is PropertyConfig.joinTable.column). This drops index/DDL settings that BasicCollectionElementBinder previously applied via getColumnConfigOptional(). Consider selecting the ColumnConfig based on property.isCollectionElement() and (for collections) reading it from HibernateToManyProperty.getColumnConfigOptional().

        if (!pc.getColumns().isEmpty()) {
            ColumnConfig columnConfig = pc.getColumns().get(0);
            indexBinder.bindIndex(columnName, column, columnConfig, t);
            columnConfigToColumnBinder.bindColumnConfigToColumn(column, columnConfig, pc);
        }