Add validation warning for invalid Set<T> in @MappedCollection (GH-2061) #2204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

young0264 wants to merge 1 commit into spring-projects:main from young0264:feat/kevin/validate-set-mapped-collection

young0264 commented Dec 28, 2025 •

edited

Loading

Add validation during RelationalMappingContext.createPersistentEntity construction
Add unit tests covering valid and invalid Set-mapped collection scenarios
issue link

You have read the Spring Data contribution guidelines.
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.
You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

spring-projects-issues added the status: waiting-for-triage label

young0264 force-pushed the feat/kevin/validate-set-mapped-collection branch from 6465f79 to 8cfcd67 Compare

December 28, 2025 05:54


          Add validation warning for invalid Set<T> in @MappedCollection (sprin…

1dbac6c

…g-projectsGH-2061)

- Add validation in RelationalMappingContext.createPersistentEntity
- Add unit tests for validation scenarios

Signed-off-by: euiyoung <euiyoung1403@gmail.com>

young0264 force-pushed the feat/kevin/validate-set-mapped-collection branch from 8cfcd67 to 1dbac6c Compare

December 28, 2025 05:57

young0264 mentioned this pull request

Inconsistent @MappedCollection behaviour for Set vs. List #2061

Open

schauder requested changes

View reviewed changes

Contributor

schauder left a comment

Thanks for working on this.

I left various comments with requests for changes.

...src/main/java/org/springframework/data/relational/core/mapping/RelationalMappingContext.java

    
              	 * 

              	 * @param entity the entity to validate

              	 */

              	private <T> void validateSetMappedCollectionProperties(RelationalPersistentEntity<T> entity) {

Contributor

schauder Jan 2, 2026

The name is misleading. The validation should not depend on a MappedCollection annotation being present or not.

...src/main/java/org/springframework/data/relational/core/mapping/RelationalMappingContext.java

    
              				this.namingStrategy, this.sqlIdentifierExpressionEvaluator);

              		entity.setForceQuote(isForceQuote());

              		// Validate Set<T> properties in @MappedCollection context

Contributor

schauder Jan 2, 2026

Do not repeat in in-line comments, what is obvious in the code.

...src/main/java/org/springframework/data/relational/core/mapping/RelationalMappingContext.java

    
              	private boolean isSetMappedCollection(RelationalPersistentProperty property) {

              		return property.isCollectionLike() 

              			&& Set.class.isAssignableFrom(property.getType())

              			&& property.isAnnotationPresent(MappedCollection.class);

Contributor

schauder Jan 2, 2026

Again, the presence of the annotation is irrelevant.

...src/main/java/org/springframework/data/relational/core/mapping/RelationalMappingContext.java

    
              				elementType.getSimpleName()

              			);

              			logger.warn(message);

Contributor

schauder Jan 2, 2026

We should really throw an exception here. This kind of mapping is just plain invalid and we should fail early.

...src/main/java/org/springframework/data/relational/core/mapping/RelationalMappingContext.java

    
              				"Invalid @MappedCollection usage: Set<%s> in %s.%s. " +

              				"Set elements without @Id must not contain entity or collection references. " +

              				"Consider using List instead or add @Id to %s.",

              				elementType.getSimpleName(),

Contributor

schauder Jan 2, 2026

Use the full class name, please.

...src/main/java/org/springframework/data/relational/core/mapping/RelationalMappingContext.java

    
              				continue;

              			}

              			if (prop.isEntity() || prop.isCollectionLike()) {

Contributor

schauder Jan 2, 2026

I don't think we need the isCollectionLike condition, since a collection of entities should return isEntity true.

We probably need to check against embedded entities though, since those should not cause a validation failure.

...java/org/springframework/data/relational/core/mapping/RelationalMappingContextUnitTests.java

    
              	static class Inherit2 extends Base {}

              	// GH-2061 - Tests for Set<T> validation in @MappedCollection context

Contributor

schauder Jan 2, 2026

This comment is redundant.

...java/org/springframework/data/relational/core/mapping/RelationalMappingContextUnitTests.java

    
              		context.setSimpleTypeHolder(holder);

              		// Should not throw exception, just log warning

              		assertThatCode(() -> context.getPersistentEntity(AggregateWithInvalidSet.class))

Contributor

schauder Jan 2, 2026

These tests, test almost nothing. A proper test should actually test the log message.
Fortunately, testing becomes much easier, when we actually throw exceptions.

...java/org/springframework/data/relational/core/mapping/RelationalMappingContextUnitTests.java

    
              		assertThatCode(() -> context.getPersistentEntity(AggregateWithValidSetWithoutReferences.class))

              			.doesNotThrowAnyException();

              	}

Contributor

schauder Jan 2, 2026

We should have additional tests for:

element contains a collection that maps to a database array.
element contains an embedded entity.

Both should not cause a validation exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: waiting-for-triage