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: https://github.com/spring-projects/spring-data-relational/blob/d411be41ac200a9f324621956b5724f2c88a5a74/spring-data-relational/src/main/java/org/springframework/data/relational/core/mapping/MappedCollection.java#L49

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<T> -> 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<T>, 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??

[schauder]

The issue came up again in https://stackoverflow.com/a/79828705/66686 There is a reproducer in https://github.com/schauder/stackoverflow/tree/main/jdbc/deep-aggregate/list-with-set-reproducer

After rereading this issue there seem to be two related but different issues being discussed:

a) Set within List doesn't work. b) Set within Set can't work.

The first is a bug. The later doesn't really make sense in the context of a relational database and issuing a warning would be a good idea.

[young0264]

I'm sorry for the delay. I'm currently transitioning to a new job while preparing for a demo at the same time. I'll do my best to make time and work on it within the end of the year.