Hibernate 7: bind each collection of an embedded entity with its own second pass

[borinquenkid]

Problem

An entity that is also embedded in another can declare a hasMany. With

@Entity class EmbeddedHolder { EmbeddedWithMany embed; static embedded = ['embed'] }
@Entity class EmbeddedWithMany { static hasMany = [members: EmbeddedMember] }
@Entity class EmbeddedMember { String name }

the session factory fails to build with Collection key not bound (an NPE on Collection.getElement()).

Cause

Such an entity is bound twice, once as a component of its holder and once as an entity, and its hasMany property is bound with a different Hibernate collection each time. The property only remembers the last collection. The second passes bound property.getCollection(), so the collection of the first binding never got its element and key.

Fix

ListSecondPass, MapSecondPass and SetSecondPass now keep the collection they were created for and restore it on the property before binding. CollectionBinder passes it in. The existing two-argument constructors are kept.

Tests

EmbeddedEntityCollectionKindsSpec saves and reads back the set, list and map of an embedded entity. With the production files reverted all three cases fail with NullPointerException: Cannot invoke "org.hibernate.mapping.Value.getType()" because the return value of "org.hibernate.mapping.Collection.getElement()" is null. With the fix all three pass.

Known limitation

An embedded entity that is also an @Entity is always bound standalone as well, and both bindings share one join table, so the standalone collection's key ends up with the holder's foreign key. This is out of scope here and tracked in #16634.

Why this is separate

It was found while moving the graphql examples to Hibernate 7 and was sitting inside the Hibernate 5 removal branch (#16350). A removal should only delete, so the fix lands here on 8.0.x and reaches 9.0.x through the merge-up.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[borinquenkid]

Full :grails-data-hibernate7-core:test on this branch: 421 suites, 3723 tests, 0 failures, 0 errors, 22 skipped. codenarcMain and checkstyleMain pass.

🤖 Generated with Claude Code

[borinquenkid]

Added tests for the other two collection kinds and for CollectionBinderSpec:

• CollectionBinderSpec: bindCollection registers the second pass that matches the collection (set, list, map), and binding one property twice registers a pass and a collection for each binding.
• EmbeddedEntityCollectionKindsSpec (integration, HibernateGormDatastoreSpec): an embedded entity whose hasMany is a set, a list and a map is saved and read back through a real session. With the four production files reverted all three fail with the same Collection.getElement() NPE; with the fix they pass.

Full :grails-data-hibernate7-core:test on the branch: 422 suites, 3730 tests, 0 failures, 0 errors, 22 skipped. codenarcTest and checkstyleTest pass.

Not covered here: an entity that is both embedded and used on its own. Its standalone hasMany join table has a foreign key to the holder's table, because the two bindings share one collection table. That limitation exists independently of this fix, since the session factory did not build at all before it.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.5715%. Comparing base (c424313) to head (fcee01d).

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #16617        +/-   ##
==================================================
- Coverage     59.5761%   59.5715%   -0.0046%     
+ Complexity      25033      25030         -3     
==================================================
  Files            2175       2175                
  Lines          108651     108651                
  Branches        19821      19821                
==================================================
- Hits            64730      64725         -5     
- Misses          34975      34984         +9     
+ Partials         8946       8942         -4     

see 2 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]

The fix is right: CollectionBinder.bindCollection creates a fresh collection per binding but the property only holds the last one, so a second pass that reads property.getCollection() binds the wrong collection. Carrying the collection in the second pass and restoring it before binding is also how the Hibernate 5 binder worked (GrailsCollectionSecondPass/ListSecondPass/MapSecondPass all took the collection), so this restores parity rather than inventing a new model. The keys still come from collection.getOwner(), so each binding gets the right owner even though setPersistentClass on the shared entity is overwritten by whichever binding ran last. Merges cleanly into 8.1.x and 9.0.x; Checkstyle, CodeNarc and the repository-conventions check are clean on the module.

Two non-blocking notes inline.

[jdaugherty]

On the limitation in the description: GormMappingConfigurationStrategy.getOrCreateEmbeddedEntity returns the same PersistentEntity when the embedded type is itself an @Entity, so an embedded entity with a hasMany is always also bound standalone. Both bindings then resolve the same join table name (TableForManyCalculator names it from the property's owner) and mappings.addTable hands back the same Table, which is why the standalone collection's key ends up with the holder's foreign key. Agreed that is out of scope here, but could you open an issue for it and link it from the PR description so it is tracked now that the session factory builds?

[jdaugherty]

This covers the same set case as EmbeddedEntityCollectionKindsSpec, on a different base class. Consider folding it into that spec so the embedded hasMany scenario lives in one place.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: c2bc931
▶️ Tests: 84038 executed
⚪️ Checks: 89/89 completed

---

Learn more about TestLens at testlens.app/docs.

[jdaugherty]

Both follow-ups from the last round are in. The standalone-binding limitation is tracked in #16634 and linked from the description, and EmbeddedEntityWithHasManySpec is folded into EmbeddedEntityCollectionKindsSpec, which already asserted the same set case alongside the list and map cases, so no coverage was lost. The production change is unchanged since fcee01d. EmbeddedEntityCollectionKindsSpec and CollectionBinderSpec pass locally at c2bc931 on top of the current 8.0.x, and the branch still merges cleanly.