From b1f1b8efaaa2cee52861e901ea84cc22ce1d77bd Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Thu, 1 Dec 2011 17:50:36 +0100 Subject: [PATCH] DATAMONGO-321 - Overhaul of id handling. Cleaned up the id handling on query mapping and mapping in general. We now only try to convert id values into an ObjectId and store it as is using potentially registered custom converters. Register BigInteger<->String converters by default now. --- .../data/mongodb/core/QueryMapper.java | 23 ++++-------- .../core/convert/AbstractMongoConverter.java | 8 ---- .../core/convert/CustomConversions.java | 4 ++ .../core/convert/MappingMongoConverter.java | 37 +++++-------------- .../MappingMongoConverterUnitTests.java | 18 +++++++++ .../core/query/QueryMapperUnitTests.java | 9 +++++ 6 files changed, 47 insertions(+), 52 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/QueryMapper.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/QueryMapper.java index dabf2b503..367e227c2 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/QueryMapper.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/QueryMapper.java @@ -16,7 +16,6 @@ package org.springframework.data.mongodb.core; import java.util.ArrayList; -import java.util.Arrays; import java.util.Iterator; import java.util.List; @@ -120,22 +119,14 @@ public class QueryMapper { * @param id * @return */ - @SuppressWarnings("unchecked") public Object convertId(Object id) { - - for (Class type : Arrays.asList(ObjectId.class, String.class)) { - - if (id.getClass().isAssignableFrom(type)) { - return id; - } - - try { - return conversionService.convert(id, type); - } catch (ConversionException e) { - // Ignore - } + + try { + return conversionService.convert(id, ObjectId.class); + } catch (ConversionException e) { + // Ignore } - - return id; + + return converter.convertToMongoType(id); } } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/AbstractMongoConverter.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/AbstractMongoConverter.java index 635dbaa56..eec07482b 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/AbstractMongoConverter.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/AbstractMongoConverter.java @@ -23,10 +23,8 @@ import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.support.ConversionServiceFactory; import org.springframework.core.convert.support.GenericConversionService; import org.springframework.data.mongodb.core.convert.MongoConverters.BigIntegerToObjectIdConverter; -import org.springframework.data.mongodb.core.convert.MongoConverters.BigIntegerToStringConverter; import org.springframework.data.mongodb.core.convert.MongoConverters.ObjectIdToBigIntegerConverter; import org.springframework.data.mongodb.core.convert.MongoConverters.ObjectIdToStringConverter; -import org.springframework.data.mongodb.core.convert.MongoConverters.StringToBigIntegerConverter; import org.springframework.data.mongodb.core.convert.MongoConverters.StringToObjectIdConverter; /** @@ -81,12 +79,6 @@ public abstract class AbstractMongoConverter implements MongoConverter, Initiali if (!conversionService.canConvert(BigInteger.class, ObjectId.class)) { conversionService.addConverter(BigIntegerToObjectIdConverter.INSTANCE); } - if (!conversionService.canConvert(BigInteger.class, String.class)) { - conversionService.addConverter(BigIntegerToStringConverter.INSTANCE); - } - if (!conversionService.canConvert(String.class, BigInteger.class)) { - conversionService.addConverter(StringToBigIntegerConverter.INSTANCE); - } conversions.registerConvertersIn(conversionService); } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/CustomConversions.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/CustomConversions.java index c19d18009..64338166e 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/CustomConversions.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/CustomConversions.java @@ -31,7 +31,9 @@ import org.springframework.core.convert.converter.GenericConverter.ConvertiblePa import org.springframework.core.convert.support.GenericConversionService; import org.springframework.data.mapping.model.SimpleTypeHolder; import org.springframework.data.mongodb.core.convert.MongoConverters.BigDecimalToStringConverter; +import org.springframework.data.mongodb.core.convert.MongoConverters.BigIntegerToStringConverter; import org.springframework.data.mongodb.core.convert.MongoConverters.StringToBigDecimalConverter; +import org.springframework.data.mongodb.core.convert.MongoConverters.StringToBigIntegerConverter; import org.springframework.data.mongodb.core.mapping.MongoSimpleTypes; import org.springframework.util.Assert; @@ -77,6 +79,8 @@ public class CustomConversions { this.converters.add(CustomToStringConverter.INSTANCE); this.converters.add(BigDecimalToStringConverter.INSTANCE); this.converters.add(StringToBigDecimalConverter.INSTANCE); + this.converters.add(BigIntegerToStringConverter.INSTANCE); + this.converters.add(StringToBigIntegerConverter.INSTANCE); this.converters.addAll(converters); for (Object c : this.converters) { 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 91fcc9889..7547cdc62 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 @@ -17,7 +17,6 @@ package org.springframework.data.mongodb.core.convert; import java.lang.reflect.Array; import java.lang.reflect.InvocationTargetException; -import java.math.BigInteger; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -28,7 +27,6 @@ import java.util.Map.Entry; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.bson.types.ObjectId; import org.springframework.beans.BeansException; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; @@ -80,9 +78,6 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App private static final TypeInformation COLLECTION_TYPE_INFORMATION = ClassTypeInformation .from(Collection.class); - private static final List> VALID_ID_TYPES = Arrays.asList(new Class[] { ObjectId.class, String.class, - BigInteger.class, byte[].class }); - protected static final Log log = LogFactory.getLog(MappingMongoConverter.class); protected final MappingContext, MongoPersistentProperty> mappingContext; @@ -362,29 +357,15 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App // Write the ID final MongoPersistentProperty idProperty = entity.getIdProperty(); if (!dbo.containsField("_id") && null != idProperty) { - Object idObj = null; - Class[] targetClasses = new Class[] { ObjectId.class, String.class, Object.class }; - for (Class targetClass : targetClasses) { - try { - idObj = wrapper.getProperty(idProperty, targetClass, useFieldAccessOnly); - if (null != idObj) { - break; - } - } catch (ConversionException ignored) { - } catch (IllegalAccessException e) { - throw new MappingException(e.getMessage(), e); - } catch (InvocationTargetException e) { - throw new MappingException(e.getMessage(), e); - } - } - - if (null != idObj) { - dbo.put("_id", idObj); - } else { - if (!VALID_ID_TYPES.contains(idProperty.getType())) { - throw new MappingException("Invalid data type " + idProperty.getType().getName() - + " for Id property. Should be one of " + VALID_ID_TYPES); - } + + try { + Object id = wrapper.getProperty(idProperty, Object.class, useFieldAccessOnly); + dbo.put("_id", idMapper.convertId(id)); + } catch (ConversionException ignored) { + } catch (IllegalAccessException e) { + throw new MappingException(e.getMessage(), e); + } catch (InvocationTargetException e) { + throw new MappingException(e.getMessage(), e); } } 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 8104b034c..af2e72251 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 @@ -844,6 +844,18 @@ public class MappingMongoConverterUnitTests { assertThat((String) inner.get("value"), is("testValue")); } + @Test + public void writesIntIdCorrectly() { + + ClassWithIntId value = new ClassWithIntId(); + value.id = 5; + + DBObject result = new BasicDBObject(); + converter.write(value, result); + + assertThat(result.get("_id"), is((Object) 5)); + } + class GenericType { T content; } @@ -947,6 +959,12 @@ public class MappingMongoConverterUnitTests { this.value = value; } } + + class ClassWithIntId { + + @Id + int id; + } private class LocalDateToDateConverter implements Converter { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/query/QueryMapperUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/query/QueryMapperUnitTests.java index f6884aed4..2a255a14e 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/query/QueryMapperUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/query/QueryMapperUnitTests.java @@ -91,6 +91,15 @@ public class QueryMapperUnitTests { assertThat(result.get("_id"), is((Object) "1")); } + @Test + public void handlesObjectIdCapableBigIntegerIdsCorrectly() { + + ObjectId id = new ObjectId(); + DBObject dbObject = new BasicDBObject("id", new BigInteger(id.toString(), 16)); + DBObject result = mapper.getMappedObject(dbObject, null); + assertThat(result.get("_id"), is((Object) id)); + } + /** * @see DATAMONGO-278 */