From f8aa915df4df5ae3b5060966889213a6bf6e9364 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 28 Aug 2024 08:56:50 +0200 Subject: [PATCH] Refactor DocumentAccessor, consider readNull/writeNull for null values. Original pull request: #4728 See #4710 --- .../core/convert/DocumentAccessor.java | 20 +---- .../core/convert/MappingMongoConverter.java | 73 +++++++++++-------- .../MappingMongoConverterUnitTests.java | 72 +++++++++++++++--- 3 files changed, 109 insertions(+), 56 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DocumentAccessor.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DocumentAccessor.java index 0595afb2ce..0bfdfee010 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DocumentAccessor.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DocumentAccessor.java @@ -92,6 +92,10 @@ public void put(MongoPersistentProperty prop, @Nullable Object value) { Assert.notNull(prop, "MongoPersistentProperty must not be null"); + if (value == null && !prop.writeNullValues()) { + return; + } + Iterator parts = Arrays.asList(prop.getMongoField().getName().parts()).iterator(); Bson document = this.document; @@ -173,20 +177,4 @@ private static Document getOrCreateNestedDocument(String key, Bson source) { return nested; } - DocumentAccessor withCheckFieldMapping(boolean checkFieldMapping) { - - if(!checkFieldMapping) { - return this; - } - - return new DocumentAccessor(this.document) { - @Override - public void put(MongoPersistentProperty prop, @Nullable Object value) { - if(value != null || prop.writeNullValues()) { - super.put(prop, value); - } - } - }; - - } } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java index 25b3c6a059..c6a9ae5c73 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java @@ -49,7 +49,9 @@ import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.data.annotation.Reference; import org.springframework.data.convert.CustomConversions; +import org.springframework.data.convert.PropertyValueConverter; import org.springframework.data.convert.TypeMapper; +import org.springframework.data.convert.ValueConversionContext; import org.springframework.data.mapping.Association; import org.springframework.data.mapping.InstanceCreatorMetadata; import org.springframework.data.mapping.MappingException; @@ -164,12 +166,11 @@ public MappingMongoConverter(DbRefResolver dbRefResolver, this.idMapper = new QueryMapper(this); this.spELContext = new SpELContext(DocumentPropertyAccessor.INSTANCE); - this.dbRefProxyHandler = new DefaultDbRefProxyHandler(spELContext, mappingContext, - (prop, bson, evaluator, path) -> { + this.dbRefProxyHandler = new DefaultDbRefProxyHandler(spELContext, mappingContext, (prop, bson, evaluator, path) -> { - ConversionContext context = getConversionContext(path); - return MappingMongoConverter.this.getValueInternal(context, prop, bson, evaluator); - }); + ConversionContext context = getConversionContext(path); + return MappingMongoConverter.this.getValueInternal(context, prop, bson, evaluator); + }); this.referenceLookupDelegate = new ReferenceLookupDelegate(mappingContext, spELContext); this.documentPointerFactory = new DocumentPointerFactory(conversionService, mappingContext); @@ -880,7 +881,10 @@ private void writeProperties(Bson bson, MongoPersistentEntity entity, Persist Object value = accessor.getProperty(prop); if (value == null) { - if (prop.writeNullValues()) { + + if (conversions.hasValueConverter(prop)) { + dbObjectAccessor.put(prop, applyPropertyConversion(null, prop, accessor)); + } else { dbObjectAccessor.put(prop, null); } } else if (!conversions.isSimpleType(value.getClass())) { @@ -918,14 +922,7 @@ protected void writePropertyInternal(@Nullable Object obj, DocumentAccessor acce TypeInformation type = prop.getTypeInformation(); if (conversions.hasValueConverter(prop)) { - accessor.put(prop, conversions.getPropertyValueConversions().getValueConverter(prop).write(obj, - new MongoConversionContext(new PropertyValueProvider<>() { - @Nullable - @Override - public T getPropertyValue(MongoPersistentProperty property) { - return (T) persistentPropertyAccessor.getProperty(property); - } - }, prop, this, spELContext))); + accessor.put(prop, applyPropertyConversion(obj, prop, persistentPropertyAccessor)); return; } @@ -964,8 +961,8 @@ public T getPropertyValue(MongoPersistentProperty property) { dbRefObj = proxy.toDBRef(); } - if(obj !=null && conversions.hasCustomWriteTarget(obj.getClass())) { - accessor.withCheckFieldMapping(true).put(prop, doConvert(obj, conversions.getCustomWriteTarget(obj.getClass()).get())); + if (obj != null && conversions.hasCustomWriteTarget(obj.getClass())) { + accessor.put(prop, doConvert(obj, conversions.getCustomWriteTarget(obj.getClass()).get())); return; } @@ -1267,17 +1264,10 @@ private void writeSimpleInternal(@Nullable Object value, Bson bson, String key) private void writeSimpleInternal(@Nullable Object value, Bson bson, MongoPersistentProperty property, PersistentPropertyAccessor persistentPropertyAccessor) { - DocumentAccessor accessor = new DocumentAccessor(bson).withCheckFieldMapping(true); + DocumentAccessor accessor = new DocumentAccessor(bson); if (conversions.hasValueConverter(property)) { - accessor.put(property, conversions.getPropertyValueConversions().getValueConverter(property).write(value, - new MongoConversionContext(new PropertyValueProvider<>() { - @Nullable - @Override - public T getPropertyValue(MongoPersistentProperty property) { - return (T) persistentPropertyAccessor.getProperty(property); - } - }, property, this, spELContext))); + accessor.put(property, applyPropertyConversion(value, property, persistentPropertyAccessor)); return; } @@ -1285,6 +1275,23 @@ public T getPropertyValue(MongoPersistentProperty property) { property.hasExplicitWriteTarget() ? property.getFieldType() : Object.class)); } + @Nullable + @SuppressWarnings("unchecked") + private Object applyPropertyConversion(@Nullable Object value, MongoPersistentProperty property, + PersistentPropertyAccessor persistentPropertyAccessor) { + MongoConversionContext context = new MongoConversionContext(new PropertyValueProvider<>() { + + @Nullable + @Override + public T getPropertyValue(MongoPersistentProperty property) { + return (T) persistentPropertyAccessor.getProperty(property); + } + }, property, this, spELContext); + PropertyValueConverter> valueConverter = conversions + .getPropertyValueConversions().getValueConverter(property); + return value != null ? valueConverter.write(value, context) : valueConverter.writeNull(context); + } + /** * Checks whether we have a custom conversion registered for the given value into an arbitrary simple Mongo type. * Returns the converted value if so. If not, we perform special enum handling or simply return the value as is. @@ -1925,14 +1932,18 @@ public T getPropertyValue(MongoPersistentProperty property) { String expression = property.getSpelExpression(); Object value = expression != null ? evaluator.evaluate(expression) : accessor.get(property); - if (value == null) { - return null; - } - CustomConversions conversions = context.getCustomConversions(); if (conversions.hasValueConverter(property)) { - return (T) conversions.getPropertyValueConversions().getValueConverter(property).read(value, - new MongoConversionContext(this, property, context.getSourceConverter(), spELContext)); + MongoConversionContext conversionContext = new MongoConversionContext(this, property, + context.getSourceConverter(), spELContext); + PropertyValueConverter> valueConverter = conversions + .getPropertyValueConversions().getValueConverter(property); + return (T) (value != null ? valueConverter.read(value, conversionContext) + : valueConverter.readNull(conversionContext)); + } + + if (value == null) { + return null; } ConversionContext contextToUse = context.forProperty(property); 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 3ccdb54a12..f74241eabe 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 @@ -2700,7 +2700,8 @@ void shouldWriteNullPropertyCorrectly() { @Test // GH-4710 void shouldWriteSimplePropertyCorrectlyAfterConversionReturnsNull() { - MongoCustomConversions conversions = new MongoCustomConversions(ConverterBuilder.writing(Integer.class, String.class, it -> null).andReading(it -> null).getConverters().stream().toList()); + MongoCustomConversions conversions = new MongoCustomConversions(ConverterBuilder + .writing(Integer.class, String.class, it -> null).andReading(it -> null).getConverters().stream().toList()); converter = new MappingMongoConverter(resolver, mappingContext); converter.setCustomConversions(conversions); @@ -2719,7 +2720,8 @@ void shouldWriteSimplePropertyCorrectlyAfterConversionReturnsNull() { @Test // GH-4710 void shouldWriteComplexPropertyCorrectlyAfterConversionReturnsNull() { - MongoCustomConversions conversions = new MongoCustomConversions(ConverterBuilder.writing(Person.class, String.class, it -> null).andReading(it -> null).getConverters().stream().toList()); + MongoCustomConversions conversions = new MongoCustomConversions(ConverterBuilder + .writing(Person.class, String.class, it -> null).andReading(it -> null).getConverters().stream().toList()); converter = new MappingMongoConverter(resolver, mappingContext); converter.setCustomConversions(conversions); @@ -2738,7 +2740,9 @@ void shouldWriteComplexPropertyCorrectlyAfterConversionReturnsNull() { @Test // GH-4710 void shouldDelegateWriteOfDBRefToCustomConversionIfConfigured() { - MongoCustomConversions conversions = new MongoCustomConversions(ConverterBuilder.writing(Person.class, DBRef.class, it -> new DBRef("persons", "n/a")).andReading(it -> null).getConverters().stream().toList()); + MongoCustomConversions conversions = new MongoCustomConversions( + ConverterBuilder.writing(Person.class, DBRef.class, it -> new DBRef("persons", "n/a")).andReading(it -> null) + .getConverters().stream().toList()); converter = new MappingMongoConverter(resolver, mappingContext); converter.setCustomConversions(conversions); @@ -2751,13 +2755,14 @@ void shouldDelegateWriteOfDBRefToCustomConversionIfConfigured() { org.bson.Document document = new org.bson.Document(); converter.write(fieldWrite, document); - assertThat(document).containsEntry("writeAlwaysPersonDBRef", new DBRef("persons", "n/a"));//.doesNotContainKey("writeNonNullPersonDBRef"); + assertThat(document).containsEntry("writeAlwaysPersonDBRef", new DBRef("persons", "n/a"));// .doesNotContainKey("writeNonNullPersonDBRef"); } @Test // GH-4710 void shouldDelegateWriteOfDBRefToCustomConversionIfConfiguredAndCheckNulls() { - MongoCustomConversions conversions = new MongoCustomConversions(ConverterBuilder.writing(Person.class, DBRef.class, it -> null).andReading(it -> null).getConverters().stream().toList()); + MongoCustomConversions conversions = new MongoCustomConversions(ConverterBuilder + .writing(Person.class, DBRef.class, it -> null).andReading(it -> null).getConverters().stream().toList()); converter = new MappingMongoConverter(resolver, mappingContext); converter.setCustomConversions(conversions); @@ -2773,6 +2778,50 @@ void shouldDelegateWriteOfDBRefToCustomConversionIfConfiguredAndCheckNulls() { assertThat(document).containsEntry("writeAlwaysPersonDBRef", null).doesNotContainKey("writeNonNullPersonDBRef"); } + @Test // GH-4710 + void shouldApplyNullConversionToPropertyValueConverters() { + + MongoCustomConversions conversions = new MongoCustomConversions( + MongoCustomConversions.MongoConverterConfigurationAdapter.from(Collections.emptyList()) + .configurePropertyConversions(registrar -> { + registrar.registerConverter(Person.class, "firstname", new MongoValueConverter() { + @Override + public String readNull(MongoConversionContext context) { + return "NULL"; + } + + @Override + public String writeNull(MongoConversionContext context) { + return "NULL"; + } + + @Override + public String read(String value, MongoConversionContext context) { + return ""; + } + + @Override + public String write(String value, MongoConversionContext context) { + return ""; + } + }); + })); + + converter = new MappingMongoConverter(resolver, mappingContext); + converter.setCustomConversions(conversions); + converter.afterPropertiesSet(); + + org.bson.Document document = new org.bson.Document(); + converter.write(new Person(), document); + + assertThat(document).containsEntry("foo", "NULL"); + + document = new org.bson.Document("foo", null); + Person result = converter.read(Person.class, document); + + assertThat(result.firstname).isEqualTo("NULL"); + } + @Test // GH-3686 void readsCollectionContainingNullValue() { @@ -3086,7 +3135,7 @@ void beanConverter() { })); converter.afterPropertiesSet(); - WithValueConverters wvc = new WithValueConverters(); + WithContextValueConverters wvc = new WithContextValueConverters(); wvc.converterBean = "spring"; org.bson.Document target = new org.bson.Document(); @@ -3097,7 +3146,7 @@ void beanConverter() { assertThat((String) it.get("ooo")).startsWith("spring - "); }); - WithValueConverters read = converter.read(WithValueConverters.class, target); + WithContextValueConverters read = converter.read(WithContextValueConverters.class, target); assertThat(read.converterBean).startsWith("spring -"); } @@ -4180,10 +4229,10 @@ static class WithFieldWrite { write = org.springframework.data.mongodb.core.mapping.Field.Write.ALWAYS) Integer writeAlways; @org.springframework.data.mongodb.core.mapping.Field( - write = org.springframework.data.mongodb.core.mapping.Field.Write.NON_NULL) Person writeNonNullPerson; + write = org.springframework.data.mongodb.core.mapping.Field.Write.NON_NULL) Person writeNonNullPerson; @org.springframework.data.mongodb.core.mapping.Field( - write = org.springframework.data.mongodb.core.mapping.Field.Write.ALWAYS) Person writeAlwaysPerson; + write = org.springframework.data.mongodb.core.mapping.Field.Write.ALWAYS) Person writeAlwaysPerson; @org.springframework.data.mongodb.core.mapping.DBRef @org.springframework.data.mongodb.core.mapping.Field( @@ -4201,6 +4250,11 @@ static class WithValueConverters { @ValueConverter(Converter2.class) String converterEnum; + String viaRegisteredConverter; + } + + static class WithContextValueConverters { + @ValueConverter(Converter3.class) String converterBean; String viaRegisteredConverter;