Refactor DocumentAccessor, consider readNull/writeNull for null values.

Original pull request: #4728
See #4710

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

@@ -92,6 +92,10 @@ class DocumentAccessor {
 
 		Assert.notNull(prop, "MongoPersistentProperty must not be null");
 
+		if (value == null && !prop.writeNullValues()) {
+			return;
+		}
+
 		Iterator<String> parts = Arrays.asList(prop.getMongoField().getName().parts()).iterator();
 		Bson document = this.document;
 
@@ -173,20 +177,4 @@ class DocumentAccessor {
 		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);
-					}
-				}
-			};
-
-	}
 }

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

@@ -49,7 +49,9 @@ import org.springframework.core.convert.TypeDescriptor;
 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 class MappingMongoConverter extends AbstractMongoConverter implements App
 		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 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 			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 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 		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> 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 class MappingMongoConverter extends AbstractMongoConverter implements App
 				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 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 	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> 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 class MappingMongoConverter extends AbstractMongoConverter implements App
 				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> T getPropertyValue(MongoPersistentProperty property) {
+				return (T) persistentPropertyAccessor.getProperty(property);
+			}
+		}, property, this, spELContext);
+		PropertyValueConverter<Object, Object, ValueConversionContext<MongoPersistentProperty>> 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 class MappingMongoConverter extends AbstractMongoConverter implements App
 			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<Object, Object, ValueConversionContext<MongoPersistentProperty>> 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);

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

@@ -2700,7 +2700,8 @@ class MappingMongoConverterUnitTests {
 	@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 @@ class MappingMongoConverterUnitTests {
 	@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 @@ class MappingMongoConverterUnitTests {
 	@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 @@ class MappingMongoConverterUnitTests {
 		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 @@ class MappingMongoConverterUnitTests {
 		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<String, String>() {
+								@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 @@ class MappingMongoConverterUnitTests {
 		}));
 		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 @@ class MappingMongoConverterUnitTests {
 			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 @@ class MappingMongoConverterUnitTests {
 				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 @@ class MappingMongoConverterUnitTests {
 
 		@ValueConverter(Converter2.class) String converterEnum;
 
+		String viaRegisteredConverter;
+	}
+
+	static class WithContextValueConverters {
+
 		@ValueConverter(Converter3.class) String converterBean;
 
 		String viaRegisteredConverter;