Inconsistent @MappedCollection behaviour for Set vs. List

[MoellJ]

Issue

When nesting multiple @MappedCollection of Lists any only specifying the @Id for the root, saving and loading work fine. An entity on a lower level is identified using the id of the root and multiple keyColumns of the @MappedCollections in between.

I'm unsure if this is indented behavior or if every intermediary entity should have an @Id. I didn't even question if this was intended until I figured out that it doesn't work for Sets.

When using @MappedCollection for a Set nested within another @MappedCollection (e.g. LList), the Set isn't stored correctly. The keyColumn of the encapsulating List isn't populated in the resulting table entry.

Example project

To see an example of this, checkout https://github.com/MoellJ/SpringJDBCMappedCollectionBug (and it's tests)

Details on example

RestaurantQueue
├── Person (via @MappedCollection(idColumn = "queue_id", keyColumn = "position_in_queue") in RestaurantQueue)
│ └── Skill (via @MappedCollection(idColumn = "queue_id") in Person

For a nested @MappedCollection of type List the behaviour is that the keyColumns of all parent collections are taken over. However in the provided example using Set, skill.position_in_queue isn't populated in the database.

The aforementioned behaviour doesn't change by adding keyColumn to the @MappedCollection of the Set
As is expected from documentation and Javadoc. See:

spring-data-relational/spring-data-relational/src/main/java/org/springframework/data/relational/core/mapping/MappedCollection.java

Line 49 in d411be4

* The column name for key columns of {@link List} or {@link Map} collections in the corresponding relationship table.

Intended behaviour

The behaviour shouldn't be inconsistent between Lists and Sets (or at least be well documented).
First I thought that this behaviour should also apply to Sets. This would work for a Set nested in another collection, but not for a Set nested within a Set (as far as I understand there would be no way to differentiate between the inner Sets)

It seems to me as if the intention was that every entity with a @MappedCollection must specify an @Id column and the idColumn attribute should refer to this. (Just a guess)
This is not currently the case case (´idColumn` might refer to the id of the grand-parent if parent doesn't have an id).

I'm unsure if this issue should only result in improved documentation

• or changing behaviour of @MappedCollections Sets
• or restricting the possibly unintended behaviour of using a grand-parent id for List (and Map?). This change sounds like it could be breaking

I hope this issue isn't to verbose and but comprehensible. I'm happy to clarify if needed

[schauder]

We should add documentation and add a check that results in an error if there are nested Sets without @Id

[young0264]

@schauder

I agree with the idea of enforcing @Id on elements of nested Set typed @MappedCollections.

Since Set does not have any ordering, the lack of an identifier makes it impossible to map the collection to its parent correctly — especially when nested. This frequently results in null foreign keys or orphaned records.

To help users catch this early, would it make sense to add a validation step in RelationalPersistentEntitySchemaVerifier (or related metadata initialization) that throws an exception if:

• A @MappedCollection-annotated property has type Set<T>
• And T lacks an @Id-annotated field

This would make the current undocumented behavior more predictable and guide users toward safer modeling.

I’d be happy to explore a PR in this direction if helpful.

[schauder]

I like the basic idea of adding a validation step.

• We should avoid the term Schema here since this is unrelated to schema validation.
• This is not about presence of @MappedCollection just having a Set<T> is sufficient.
• T not having an id is ok, as long as T doesn't own any further references, i.e. it must not have any entity or collection of entities as members.

This should be tied to the construction of RelationalPersistentEntity implementations.

[young0264]

Hi @schauder
Thanks again for the guidance.

I plan to implement a validation step during the construction of RelationalPersistentEntity in RelationalPersistentEntity. The validation will:

1. Identify properties of type Set -> using Set.class.isAssignableFrom(property.getType())
2. Check if T lacks an @Id field.
3. Check if T contains any reference to other entities or collections.
4. If both conditions are met, log a warning or throw an exception, since such usage is likely to result in an incomplete or broken mapping.

I plan to place this check inside RelationalMappingContext.createPersistentEntity(...) after instantiating BasicRelationalPersistentEntity, so we can inspect the type's properties right after the entity is created.

Would this direction align with what you had in mind?

[schauder]

Sounds good.

[young0264]

Great, I'll go ahead and work on it.
Could you assign this issue to me??