From 99bb92db4edeaaa6f8530df74a591acd72609b12 Mon Sep 17 00:00:00 2001
From: Oliver Gierke <ogierke@pivotal.io>
Date: Sat, 4 Jul 2015 14:12:35 +0200
Subject: [PATCH] DATAMONGO-1250 - Fixed accidental duplicate invocation of
 value conversion in UpdateMapper.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

UpdateMapper.getMappedObjectForField(…) invokes the very same method of the super class but handed in an already mapped value so that value conversion was invoked twice.

This was especially problematic in cases a dedicated converter had been registered for an object that is already a Mongo-storable one (e.g. an enum-to-string converter and back) without indicating which of the tow converter is the reading or the writing one. This basically caused the source value converted back and forth during the update mapping creating the impression the value wasn't converted at all.

This is now fixed by removing the superfluous mapping.
---
 .../mongodb/core/convert/UpdateMapper.java    |  2 +-
 .../core/convert/UpdateMapperUnitTests.java   | 77 +++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/UpdateMapper.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/UpdateMapper.java
index b4cb5dd8f..83b0aa5f5 100644
--- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/UpdateMapper.java
+++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/UpdateMapper.java
@@ -90,7 +90,7 @@ public class UpdateMapper extends QueryMapper {
 			return getMappedUpdateModifier(field, rawValue);
 		}
 
-		return super.getMappedObjectForField(field, getMappedValue(field, rawValue));
+		return super.getMappedObjectForField(field, rawValue);
 	}
 
 	private Entry<String, Object> getMappedUpdateModifier(Field field, Object rawValue) {
diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/UpdateMapperUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/UpdateMapperUnitTests.java
index c9e0c852d..7b4670d70 100644
--- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/UpdateMapperUnitTests.java
+++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/UpdateMapperUnitTests.java
@@ -42,6 +42,9 @@ import org.springframework.data.convert.WritingConverter;
 import org.springframework.data.mapping.model.MappingException;
 import org.springframework.data.mongodb.MongoDbFactory;
 import org.springframework.data.mongodb.core.DBObjectTestUtils;
+import org.springframework.data.mongodb.core.convert.UpdateMapperUnitTests.ClassWithEnum.Allocation;
+import org.springframework.data.mongodb.core.convert.UpdateMapperUnitTests.ClassWithEnum.AllocationToStringConverter;
+import org.springframework.data.mongodb.core.convert.UpdateMapperUnitTests.ClassWithEnum.StringToAllocationConverter;
 import org.springframework.data.mongodb.core.mapping.Field;
 import org.springframework.data.mongodb.core.mapping.MongoMappingContext;
 import org.springframework.data.mongodb.core.query.Criteria;
@@ -692,6 +695,33 @@ public class UpdateMapperUnitTests {
 		assertThat(mappedUpdate, isBsonObject().notContaining("$set.concreteMap.jasnah._class"));
 	}
 
+	/**
+	 * @see DATAMONGO-1250
+	 */
+	@Test
+	@SuppressWarnings("unchecked")
+	public void mapsUpdateWithBothReadingAndWritingConverterRegistered() {
+
+		CustomConversions conversions = new CustomConversions(
+				Arrays.asList(AllocationToStringConverter.INSTANCE, StringToAllocationConverter.INSTANCE));
+
+		MongoMappingContext mappingContext = new MongoMappingContext();
+		mappingContext.setSimpleTypeHolder(conversions.getSimpleTypeHolder());
+		mappingContext.afterPropertiesSet();
+
+		MappingMongoConverter converter = new MappingMongoConverter(mock(DbRefResolver.class), mappingContext);
+		converter.setCustomConversions(conversions);
+		converter.afterPropertiesSet();
+
+		UpdateMapper mapper = new UpdateMapper(converter);
+
+		Update update = new Update().set("allocation", Allocation.AVAILABLE);
+		DBObject result = mapper.getMappedObject(update.getUpdateObject(),
+				mappingContext.getPersistentEntity(ClassWithEnum.class));
+
+		assertThat(result, isBsonObject().containing("$set.allocation", Allocation.AVAILABLE.code));
+	}
+
 	static class DomainTypeWrappingConcreteyTypeHavingListOfInterfaceTypeAttributes {
 		ListModelWrapper concreteTypeWithListAttributeOfInterfaceType;
 	}
@@ -914,4 +944,51 @@ public class UpdateMapperUnitTests {
 		Map<Object, Object> map;
 		Map<Object, NestedDocument> concreteMap;
 	}
+
+	static class ClassWithEnum {
+
+		Allocation allocation;
+
+		static enum Allocation {
+
+			AVAILABLE("V"), ALLOCATED("A");
+
+			String code;
+
+			private Allocation(String code) {
+				this.code = code;
+			}
+
+			public static Allocation of(String code) {
+
+				for (Allocation value : values()) {
+					if (value.code.equals(code)) {
+						return value;
+					}
+				}
+
+				throw new IllegalArgumentException();
+			}
+		}
+
+		static enum AllocationToStringConverter implements Converter<Allocation, String> {
+
+			INSTANCE;
+
+			@Override
+			public String convert(Allocation source) {
+				return source.code;
+			}
+		}
+
+		static enum StringToAllocationConverter implements Converter<String, Allocation> {
+
+			INSTANCE;
+
+			@Override
+			public Allocation convert(String source) {
+				return Allocation.of(source);
+			}
+		}
+	}
 }