From 5e5459b383ff5c2d6d554ed9d0f083d6c6cb24e7 Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Mon, 10 Nov 2025 10:14:08 +0100 Subject: [PATCH] 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 --- .../mongodb/core/convert/QueryMapper.java | 38 ++++++---- .../MappingMongoConverterUnitTests.java | 21 +++++- .../convert/NullReplacingValueConverter.java | 44 ++++++++++++ .../core/convert/QueryMapperUnitTests.java | 70 +++++++++++++++---- .../core/convert/ReversingValueConverter.java | 9 +-- 5 files changed, 145 insertions(+), 37 deletions(-) create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/NullReplacingValueConverter.java diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java index 92fd30b40..0cde0cc75 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java +++ b/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; -import java.util.AbstractMap; -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.*; import java.util.Map.Entry; -import java.util.Optional; -import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -38,7 +29,7 @@ import org.bson.Document; import org.bson.conversions.Bson; import org.bson.types.ObjectId; import org.jspecify.annotations.Nullable; - +import org.springframework.core.GenericTypeResolver; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.converter.Converter; import org.springframework.data.annotation.Reference; @@ -713,7 +704,7 @@ public class QueryMapper { List converted = new ArrayList<>(collection.size()); for (Object o : collection) { - converted.add(valueConverter.write(o, conversionContext)); + converted.add(typeSafeSingleWriteConversion(valueConverter, conversionContext, o)); } 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> 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 diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/MappingMongoConverterUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/MappingMongoConverterUnitTests.java index 002876e8c..ed4bf669f 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/MappingMongoConverterUnitTests.java +++ b/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"); } + @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 void beanConverter() { @@ -3357,12 +3372,12 @@ class MappingMongoConverterUnitTests { new PropertyValueConverter() { @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"); } @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); } }); @@ -4615,6 +4630,8 @@ class MappingMongoConverterUnitTests { @ValueConverter(Converter2.class) String converterEnum; + @ValueConverter(NullReplacingValueConverter.class) String nullConverter; + String viaRegisteredConverter; } diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/NullReplacingValueConverter.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/NullReplacingValueConverter.java new file mode 100644 index 000000000..3081d3bab --- /dev/null +++ b/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 { + + @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"; + } +} diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java index e95003608..fe1062fc9 100755 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java +++ b/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.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; - import org.springframework.core.convert.converter.Converter; import org.springframework.data.annotation.Id; 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.geo.GeoJsonPoint; import org.springframework.data.mongodb.core.geo.GeoJsonPolygon; -import org.springframework.data.mongodb.core.mapping.DBRef; -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.*; 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.Criteria; import org.springframework.data.mongodb.core.query.Query; @@ -98,7 +88,8 @@ public class QueryMapperUnitTests { @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.setSimpleTypeHolder(conversions.getSimpleTypeHolder()); @@ -133,7 +124,8 @@ public class QueryMapperUnitTests { void handlesBigIntegerIdsCorrectly/*in legacy string format*/() { 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(); QueryMapper mapper = new QueryMapper(converter); @@ -1663,7 +1655,7 @@ public class QueryMapperUnitTests { Query query = query(expr(Expr.valueOf(ComparisonOperators.valueOf("field").greaterThan("budget")))); org.bson.Document mappedObject = mapper.getMappedObject(query.getQueryObject(), - context.getPersistentEntity(Object.class)); + context.getPersistentEntity(Object.class)); 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()))); 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)); } + @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 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 { Map simpleMap; } @@ -2021,6 +2051,16 @@ public class QueryMapperUnitTests { @ValueConverter(ReversingValueConverter.class) String text; } + static class WithNullReplacingPropertyValueConverter { + + @ValueConverter(NullReplacingValueConverter.class) String text; + } + + static class WithNullReplacingPropertyValueConverterOnList { + + @ValueConverter(NullReplacingValueConverter.class) List text; + } + @WritingConverter public static class MyAddressToDocumentConverter implements Converter { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/ReversingValueConverter.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/ReversingValueConverter.java index 0fe791784..1812df766 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/ReversingValueConverter.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/ReversingValueConverter.java @@ -24,22 +24,17 @@ class ReversingValueConverter implements MongoValueConverter { @Nullable @Override - public String read(@Nullable String value, MongoConversionContext context) { + public String read(String value, MongoConversionContext context) { return reverse(value); } @Nullable @Override - public String write(@Nullable String value, MongoConversionContext context) { + public String write(String value, MongoConversionContext context) { return reverse(value); } private String reverse(String source) { - - if (source == null) { - return null; - } - return new StringBuilder(source).reverse().toString(); } }