Browse Source

Invoke PropertyValueConverter only for matching types.

When the type does not match we simply use the unconverted value.
This happens when comparing properties to regexes.
Applying the conversion to the String value of the regex doesn't makes sense since we are not dealing with complete values.

Users may still provide converters taking Objects and mangle Regex patterns as they desire.

Also fixed the null handling of PropertyValueConverters.

Removed a few superfluous `@Nullable` annotations where they became obvious

Closes #4346
issue/4346-property-value-convert-cce
Jens Schauder 1 month ago
parent
commit
5e5459b383
No known key found for this signature in database
GPG Key ID: 2BE5D185CD2A1CE6
  1. 38
      spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java
  2. 21
      spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/MappingMongoConverterUnitTests.java
  3. 44
      spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/NullReplacingValueConverter.java
  4. 70
      spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java
  5. 9
      spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/ReversingValueConverter.java

38
spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java

@ -15,17 +15,8 @@
*/ */
package org.springframework.data.mongodb.core.convert; package org.springframework.data.mongodb.core.convert;
import java.util.AbstractMap; import java.util.*;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry; import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Matcher; import java.util.regex.Matcher;
import java.util.regex.Pattern; import java.util.regex.Pattern;
import java.util.stream.Collectors; import java.util.stream.Collectors;
@ -38,7 +29,7 @@ import org.bson.Document;
import org.bson.conversions.Bson; import org.bson.conversions.Bson;
import org.bson.types.ObjectId; import org.bson.types.ObjectId;
import org.jspecify.annotations.Nullable; import org.jspecify.annotations.Nullable;
import org.springframework.core.GenericTypeResolver;
import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.ConversionService;
import org.springframework.core.convert.converter.Converter; import org.springframework.core.convert.converter.Converter;
import org.springframework.data.annotation.Reference; import org.springframework.data.annotation.Reference;
@ -713,7 +704,7 @@ public class QueryMapper {
List<Object> converted = new ArrayList<>(collection.size()); List<Object> converted = new ArrayList<>(collection.size());
for (Object o : collection) { for (Object o : collection) {
converted.add(valueConverter.write(o, conversionContext)); converted.add(typeSafeSingleWriteConversion(valueConverter, conversionContext, o));
} }
return converted; return converted;
@ -730,7 +721,28 @@ public class QueryMapper {
}); });
} }
return value != null ? valueConverter.write(value, conversionContext) : value; return typeSafeSingleWriteConversion(valueConverter, conversionContext, value);
}
private static @Nullable Object typeSafeSingleWriteConversion(PropertyValueConverter<Object, Object, ValueConversionContext<MongoPersistentProperty>> valueConverter, MongoConversionContext conversionContext, @Nullable Object o) {
Class<?>[] typeArguments = GenericTypeResolver.resolveTypeArguments(valueConverter.getClass(), PropertyValueConverter.class);
if (o == null) {
return valueConverter.writeNull(conversionContext);
}
// don't apply the converter if we can determine, that the types don't match.
// this happens for regex comparisons.
if (typeArguments != null && typeArguments.length > 0) {
Class<?> converterWriteArgumentType = typeArguments[0];
if (!converterWriteArgumentType.isAssignableFrom(o.getClass())) {
return o;
}
}
return valueConverter.write(o, conversionContext);
} }
@Nullable @Nullable

21
spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/MappingMongoConverterUnitTests.java

@ -3315,6 +3315,21 @@ class MappingMongoConverterUnitTests {
assertThat(read.converterEnum).isEqualTo("spring"); assertThat(read.converterEnum).isEqualTo("spring");
} }
@Test // GH-4346
void nullConverter() {
WithValueConverters wvc = new WithValueConverters();
wvc.nullConverter = null;
org.bson.Document target = new org.bson.Document();
converter.write(wvc, target);
assertThat(target).containsEntry("nullConverter", "W");
WithValueConverters read = converter.read(WithValueConverters.class, org.bson.Document.parse("{ nullConverter : null}"));
assertThat(read.nullConverter).isEqualTo("R");
}
@Test // GH-3596 @Test // GH-3596
void beanConverter() { void beanConverter() {
@ -3357,12 +3372,12 @@ class MappingMongoConverterUnitTests {
new PropertyValueConverter<String, org.bson.Document, MongoConversionContext>() { new PropertyValueConverter<String, org.bson.Document, MongoConversionContext>() {
@Override @Override
public @Nullable String read(org.bson.@Nullable Document nativeValue, MongoConversionContext context) { public @Nullable String read(org.bson.Document nativeValue, MongoConversionContext context) {
return nativeValue.getString("bar"); return nativeValue.getString("bar");
} }
@Override @Override
public org.bson.@Nullable Document write(@Nullable String domainValue, MongoConversionContext context) { public org.bson.Document write(String domainValue, MongoConversionContext context) {
return new org.bson.Document("bar", domainValue); return new org.bson.Document("bar", domainValue);
} }
}); });
@ -4615,6 +4630,8 @@ class MappingMongoConverterUnitTests {
@ValueConverter(Converter2.class) String converterEnum; @ValueConverter(Converter2.class) String converterEnum;
@ValueConverter(NullReplacingValueConverter.class) String nullConverter;
String viaRegisteredConverter; String viaRegisteredConverter;
} }

44
spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/NullReplacingValueConverter.java

@ -0,0 +1,44 @@
/*
* Copyright 2022-2025 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.springframework.data.mongodb.core.convert;
import org.jspecify.annotations.Nullable;
/**
* @author Jens Schauder
*/
class NullReplacingValueConverter implements MongoValueConverter<String, String> {
@Override
public @Nullable String read(String value, MongoConversionContext context) {
return value;
}
@Override
public @Nullable String readNull(MongoConversionContext context) {
return "R";
}
@Override
public @Nullable String write(String value, MongoConversionContext context) {
return value;
}
@Override
public @Nullable String writeNull(MongoConversionContext context) {
return "W";
}
}

70
spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java

@ -38,7 +38,6 @@ import org.bson.types.Decimal128;
import org.bson.types.ObjectId; import org.bson.types.ObjectId;
import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Test;
import org.springframework.core.convert.converter.Converter; import org.springframework.core.convert.converter.Converter;
import org.springframework.data.annotation.Id; import org.springframework.data.annotation.Id;
import org.springframework.data.annotation.Transient; import org.springframework.data.annotation.Transient;
@ -58,17 +57,8 @@ import org.springframework.data.mongodb.core.convert.MongoCustomConversions.BigD
import org.springframework.data.mongodb.core.convert.MongoCustomConversions.MongoConverterConfigurationAdapter; import org.springframework.data.mongodb.core.convert.MongoCustomConversions.MongoConverterConfigurationAdapter;
import org.springframework.data.mongodb.core.geo.GeoJsonPoint; import org.springframework.data.mongodb.core.geo.GeoJsonPoint;
import org.springframework.data.mongodb.core.geo.GeoJsonPolygon; import org.springframework.data.mongodb.core.geo.GeoJsonPolygon;
import org.springframework.data.mongodb.core.mapping.DBRef; import org.springframework.data.mongodb.core.mapping.*;
import org.springframework.data.mongodb.core.mapping.Document;
import org.springframework.data.mongodb.core.mapping.DocumentReference;
import org.springframework.data.mongodb.core.mapping.Field;
import org.springframework.data.mongodb.core.mapping.FieldName.Type; import org.springframework.data.mongodb.core.mapping.FieldName.Type;
import org.springframework.data.mongodb.core.mapping.FieldType;
import org.springframework.data.mongodb.core.mapping.MongoId;
import org.springframework.data.mongodb.core.mapping.MongoMappingContext;
import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity;
import org.springframework.data.mongodb.core.mapping.TextScore;
import org.springframework.data.mongodb.core.mapping.Unwrapped;
import org.springframework.data.mongodb.core.query.BasicQuery; import org.springframework.data.mongodb.core.query.BasicQuery;
import org.springframework.data.mongodb.core.query.Criteria; import org.springframework.data.mongodb.core.query.Criteria;
import org.springframework.data.mongodb.core.query.Query; import org.springframework.data.mongodb.core.query.Query;
@ -98,7 +88,8 @@ public class QueryMapperUnitTests {
@BeforeEach @BeforeEach
void beforeEach() { void beforeEach() {
MongoCustomConversions conversions = new MongoCustomConversions(new MongoConverterConfigurationAdapter().bigDecimal(BigDecimalRepresentation.DECIMAL128)); MongoCustomConversions conversions = new MongoCustomConversions(
new MongoConverterConfigurationAdapter().bigDecimal(BigDecimalRepresentation.DECIMAL128));
this.context = new MongoMappingContext(); this.context = new MongoMappingContext();
this.context.setSimpleTypeHolder(conversions.getSimpleTypeHolder()); this.context.setSimpleTypeHolder(conversions.getSimpleTypeHolder());
@ -133,7 +124,8 @@ public class QueryMapperUnitTests {
void handlesBigIntegerIdsCorrectly/*in legacy string format*/() { void handlesBigIntegerIdsCorrectly/*in legacy string format*/() {
MappingMongoConverter converter = new MappingMongoConverter(NoOpDbRefResolver.INSTANCE, context); MappingMongoConverter converter = new MappingMongoConverter(NoOpDbRefResolver.INSTANCE, context);
converter.setCustomConversions(MongoCustomConversions.create(adapter -> adapter.bigDecimal(BigDecimalRepresentation.STRING))); converter.setCustomConversions(
MongoCustomConversions.create(adapter -> adapter.bigDecimal(BigDecimalRepresentation.STRING)));
converter.afterPropertiesSet(); converter.afterPropertiesSet();
QueryMapper mapper = new QueryMapper(converter); QueryMapper mapper = new QueryMapper(converter);
@ -1663,7 +1655,7 @@ public class QueryMapperUnitTests {
Query query = query(expr(Expr.valueOf(ComparisonOperators.valueOf("field").greaterThan("budget")))); Query query = query(expr(Expr.valueOf(ComparisonOperators.valueOf("field").greaterThan("budget"))));
org.bson.Document mappedObject = mapper.getMappedObject(query.getQueryObject(), org.bson.Document mappedObject = mapper.getMappedObject(query.getQueryObject(),
context.getPersistentEntity(Object.class)); context.getPersistentEntity(Object.class));
assertThat(mappedObject).isEqualTo("{ $expr : { $expr : { $gt : [ '$field', '$budget'] } } }"); assertThat(mappedObject).isEqualTo("{ $expr : { $expr : { $gt : [ '$field', '$budget'] } } }");
} }
@ -1730,11 +1722,49 @@ public class QueryMapperUnitTests {
Criteria criteria = new Criteria().andOperator(where("name").isNull().and("id").all(List.of(oid.toString()))); Criteria criteria = new Criteria().andOperator(where("name").isNull().and("id").all(List.of(oid.toString())));
org.bson.Document mappedObject = mapper.getMappedObject(criteria.getCriteriaObject(), org.bson.Document mappedObject = mapper.getMappedObject(criteria.getCriteriaObject(),
context.getPersistentEntity(Customer.class)); context.getPersistentEntity(Customer.class));
assertThat(mappedObject).containsEntry("$and.[0]._id.$all", List.of(oid)); assertThat(mappedObject).containsEntry("$and.[0]._id.$all", List.of(oid));
} }
@Test // GH-4346
void propertyValueConverterOnlyGetsInvokedOnMatchingType() {
Criteria criteria = new Criteria().andOperator(where("text").regex("abc"));
org.bson.Document mappedObject = mapper.getMappedObject(criteria.getCriteriaObject(),
context.getPersistentEntity(WithPropertyValueConverter.class));
org.bson.Document parsedExpected = org.bson.Document.parse("{ '$and' : [{ 'text' : /abc/ }] }");
// We are comparing BsonDocument instances instead of Document instances, because Document.parse applies a Codec,
// which the ObjectMapper doesn't, yielding slightly different results.
// converting both to BsonDocuments applies the Codec to both.
assertThat(mappedObject.toBsonDocument()).isEqualTo(parsedExpected.toBsonDocument());
}
@Test // GH-4346
void nullConversionIsApplied(){
org.bson.Document mappedObject = mapper.getMappedObject(new org.bson.Document("text", null),
context.getPersistentEntity(WithNullReplacingPropertyValueConverter.class));
assertThat(mappedObject).isEqualTo(new org.bson.Document("text", "W"));
}
@Test // GH-4346
void nullConversionIsAppliedToLists(){
List<String> listOfStrings = new ArrayList<>();
listOfStrings.add("alpha");
listOfStrings.add(null);
listOfStrings.add("beta");
org.bson.Document mappedObject = mapper.getMappedObject(new org.bson.Document("text", listOfStrings),
context.getPersistentEntity(WithNullReplacingPropertyValueConverter.class));
assertThat(mappedObject).isEqualTo(new org.bson.Document("text", List.of("alpha", "W", "beta")));
}
class WithSimpleMap { class WithSimpleMap {
Map<String, String> simpleMap; Map<String, String> simpleMap;
} }
@ -2021,6 +2051,16 @@ public class QueryMapperUnitTests {
@ValueConverter(ReversingValueConverter.class) String text; @ValueConverter(ReversingValueConverter.class) String text;
} }
static class WithNullReplacingPropertyValueConverter {
@ValueConverter(NullReplacingValueConverter.class) String text;
}
static class WithNullReplacingPropertyValueConverterOnList {
@ValueConverter(NullReplacingValueConverter.class) List<String> text;
}
@WritingConverter @WritingConverter
public static class MyAddressToDocumentConverter implements Converter<MyAddress, org.bson.Document> { public static class MyAddressToDocumentConverter implements Converter<MyAddress, org.bson.Document> {

9
spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/ReversingValueConverter.java

@ -24,22 +24,17 @@ class ReversingValueConverter implements MongoValueConverter<String, String> {
@Nullable @Nullable
@Override @Override
public String read(@Nullable String value, MongoConversionContext context) { public String read(String value, MongoConversionContext context) {
return reverse(value); return reverse(value);
} }
@Nullable @Nullable
@Override @Override
public String write(@Nullable String value, MongoConversionContext context) { public String write(String value, MongoConversionContext context) {
return reverse(value); return reverse(value);
} }
private String reverse(String source) { private String reverse(String source) {
if (source == null) {
return null;
}
return new StringBuilder(source).reverse().toString(); return new StringBuilder(source).reverse().toString();
} }
} }

Loading…
Cancel
Save