-
Notifications
You must be signed in to change notification settings - Fork 379
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,10 +16,12 @@ | |
| package org.springframework.data.relational.core.mapping; | ||
|
|
||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| import org.springframework.beans.BeansException; | ||
| import org.springframework.context.ApplicationContext; | ||
| import org.springframework.core.env.Environment; | ||
|
|
@@ -45,6 +47,8 @@ | |
| public class RelationalMappingContext | ||
| extends AbstractMappingContext<RelationalPersistentEntity<?>, RelationalPersistentProperty> { | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(RelationalMappingContext.class); | ||
|
|
||
| private final NamingStrategy namingStrategy; | ||
| private final Map<AggregatePathCacheKey, AggregatePath> aggregatePathCache = new ConcurrentHashMap<>(); | ||
|
|
||
|
|
@@ -142,6 +146,9 @@ protected <T> RelationalPersistentEntity<T> createPersistentEntity(TypeInformati | |
| this.namingStrategy, this.sqlIdentifierExpressionEvaluator); | ||
| entity.setForceQuote(isForceQuote()); | ||
|
|
||
| // Validate Set<T> properties in @MappedCollection context | ||
| validateSetMappedCollectionProperties(entity); | ||
|
|
||
| return entity; | ||
| } | ||
|
|
||
|
|
@@ -219,6 +226,78 @@ public AggregatePath getAggregatePath(RelationalPersistentEntity<?> type) { | |
| return aggregatePath; | ||
| } | ||
|
|
||
| /** | ||
| * Validates Set<T> properties in nested @MappedCollection scenarios. | ||
| * | ||
| * @param entity the entity to validate | ||
| */ | ||
| private <T> void validateSetMappedCollectionProperties(RelationalPersistentEntity<T> entity) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name is misleading. The validation should not depend on a |
||
| for (RelationalPersistentProperty property : entity) { | ||
| if (isSetMappedCollection(property)) { | ||
| validateSetMappedCollectionProperty(property); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a property is a Set with @MappedCollection annotation. | ||
| */ | ||
| private boolean isSetMappedCollection(RelationalPersistentProperty property) { | ||
| return property.isCollectionLike() | ||
| && Set.class.isAssignableFrom(property.getType()) | ||
| && property.isAnnotationPresent(MappedCollection.class); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, the presence of the annotation is irrelevant. |
||
| } | ||
|
|
||
| /** | ||
| * Validates a Set<T> property in @MappedCollection context. | ||
| * | ||
| * @param property the Set property to validate | ||
| */ | ||
| private void validateSetMappedCollectionProperty(RelationalPersistentProperty property) { | ||
| Class<?> elementType = property.getComponentType(); | ||
| if (elementType == null) { | ||
| return; | ||
| } | ||
|
|
||
| RelationalPersistentEntity<?> elementEntity = getPersistentEntity(elementType); | ||
| if (elementEntity == null) { | ||
| return; | ||
| } | ||
|
|
||
| boolean hasId = elementEntity.hasIdProperty(); | ||
| boolean hasEntityOrCollectionReferences = hasEntityOrCollectionReferences(elementEntity); | ||
|
|
||
| if (!hasId && hasEntityOrCollectionReferences) { | ||
| String message = String.format( | ||
| "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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the full class name, please. |
||
| property.getOwner().getType().getSimpleName(), | ||
| property.getName(), | ||
| elementType.getSimpleName() | ||
| ); | ||
|
|
||
| logger.warn(message); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should really throw an exception here. This kind of mapping is just plain invalid and we should fail early. |
||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Checks if an entity has any properties that are entities or collections. | ||
| */ | ||
| private boolean hasEntityOrCollectionReferences(RelationalPersistentEntity<?> entity) { | ||
| for (RelationalPersistentProperty prop : entity) { | ||
| if (prop.isIdProperty() || prop.isVersionProperty()) { | ||
| continue; | ||
| } | ||
|
|
||
| if (prop.isEntity() || prop.isCollectionLike()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need the We probably need to check against embedded entities though, since those should not cause a validation failure. |
||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private record AggregatePathCacheKey(RelationalPersistentEntity<?> root, | ||
| @Nullable PersistentPropertyPath<? extends RelationalPersistentProperty> path) { | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
|
|
||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
| import java.util.UUID; | ||
|
|
||
| import org.junit.jupiter.api.BeforeEach; | ||
|
|
@@ -152,4 +153,74 @@ static class Base { | |
| static class Inherit1 extends Base {} | ||
|
|
||
| static class Inherit2 extends Base {} | ||
|
|
||
| // GH-2061 - Tests for Set<T> validation in @MappedCollection context | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is redundant. |
||
|
|
||
| @Test // GH-2061 | ||
| void doesNotThrowExceptionForInvalidSetUsage() { | ||
| context = new RelationalMappingContext(); | ||
| context.setSimpleTypeHolder(holder); | ||
|
|
||
| // Should not throw exception, just log warning | ||
| assertThatCode(() -> context.getPersistentEntity(AggregateWithInvalidSet.class)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests, test almost nothing. A proper test should actually test the log message. |
||
| .doesNotThrowAnyException(); | ||
| } | ||
|
|
||
| @Test // GH-2061 | ||
| void doesNotThrowExceptionWhenSetElementHasId() { | ||
| context = new RelationalMappingContext(); | ||
| context.setSimpleTypeHolder(holder); | ||
|
|
||
| assertThatCode(() -> context.getPersistentEntity(AggregateWithValidSetHavingId.class)) | ||
| .doesNotThrowAnyException(); | ||
| } | ||
|
|
||
| @Test // GH-2061 | ||
| void doesNotThrowExceptionWhenSetElementWithoutIdHasNoReferences() { | ||
| context = new RelationalMappingContext(); | ||
| context.setSimpleTypeHolder(holder); | ||
|
|
||
| assertThatCode(() -> context.getPersistentEntity(AggregateWithValidSetWithoutReferences.class)) | ||
| .doesNotThrowAnyException(); | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should have additional tests for:
Both should not cause a validation exception. |
||
| // Test entities for GH-2061 | ||
| static class AggregateWithInvalidSet { | ||
| @Id Long id; | ||
| @MappedCollection(idColumn = "aggregate_id", keyColumn = "idx") | ||
| Set<InvalidElement> elements; | ||
| } | ||
|
|
||
| static class InvalidElement { | ||
| String name; | ||
| OtherEntity reference; | ||
| } | ||
|
|
||
| static class OtherEntity { | ||
| @Id Long id; | ||
| String value; | ||
| } | ||
|
|
||
| static class AggregateWithValidSetHavingId { | ||
| @Id Long id; | ||
| @MappedCollection(idColumn = "aggregate_id") | ||
| Set<ElementWithId> elements; | ||
| } | ||
|
|
||
| static class ElementWithId { | ||
| @Id Long id; | ||
| String name; | ||
| OtherEntity reference; | ||
| } | ||
|
|
||
| static class AggregateWithValidSetWithoutReferences { | ||
| @Id Long id; | ||
| @MappedCollection(idColumn = "aggregate_id", keyColumn = "idx") | ||
| Set<SimpleElement> elements; | ||
| } | ||
|
|
||
| static class SimpleElement { | ||
| String name; | ||
| int value; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not repeat in in-line comments, what is obvious in the code.