From 22ab2007da1419b9855954935e339135c305c76d Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Sun, 22 May 2011 14:54:08 +0200 Subject: [PATCH] DATADOC-145 - Fixed mapping collections with abstract component types. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let recursive mapping calls of collection elements use the value type instead of the collections component type. Refactored MappingMongoConverter to make collection handling more maintainable. Added code to always add custom type information if the actual value being stored differs from the declared one. Moved some of the DBRef discovering code into implementations of MongoPersistentProperty. Renamed MongoPersistentProperty.getKey() to ….getFieldName(). --- .../convert/MappingMongoConverter.java | 180 +++++++++++------- .../mapping/BasicMongoPersistentProperty.java | 16 +- .../document/mongodb/mapping/FieldName.java | 2 + .../mapping/MongoPersistentProperty.java | 24 ++- .../mapping/SimpleMongoMappingContext.java | 16 +- .../ConvertingParameterAccessor.java | 34 +++- ...BasicMongoPersistentPropertyUnitTests.java | 6 +- .../MappingMongoConverterUnitTests.java | 52 +++++ 8 files changed, 257 insertions(+), 73 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/convert/MappingMongoConverter.java b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/convert/MappingMongoConverter.java index d13477095..0bfef869a 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/convert/MappingMongoConverter.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/convert/MappingMongoConverter.java @@ -68,6 +68,7 @@ import org.springframework.data.util.TypeInformation; import org.springframework.expression.Expression; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.expression.spel.support.StandardEvaluationContext; +import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; /** @@ -118,6 +119,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App * * @param converters */ + @Override public void setCustomConverters(Set converters) { if (null != converters) { for (Object c : converters) { @@ -261,7 +263,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App String name = parameter.getName(); TypeInformation type = parameter.getType(); Class rawType = parameter.getRawType(); - String key = idProperty == null ? name : idProperty.getName().equals(name) ? idProperty.getKey() : name; + String key = idProperty == null ? name : idProperty.getName().equals(name) ? idProperty.getFieldName() : name; Object obj = dbo.get(key); ctorParamNames.add(name); @@ -288,7 +290,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App public void doWithPersistentProperty(MongoPersistentProperty prop) { boolean isConstructorProperty = ctorParamNames.contains(prop.getName()); - boolean hasValueForProperty = dbo.containsField(prop.getKey()); + boolean hasValueForProperty = dbo.containsField(prop.getFieldName()); if (!hasValueForProperty || isConstructorProperty) { return; @@ -366,7 +368,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App } if (Map.class.isAssignableFrom(obj.getClass())) { - writeMapInternal((Map) obj, dbo); + writeMapInternal((Map) obj, dbo, null); return; } @@ -421,7 +423,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App if (!isSimpleType(propertyObj.getClass())) { writePropertyInternal(prop, propertyObj, dbo); } else { - writeSimpleInternal(prop.getKey(), propertyObj, dbo); + writeSimpleInternal(prop.getFieldName(), propertyObj, dbo); } } } @@ -474,66 +476,28 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App @SuppressWarnings({"unchecked"}) protected void writePropertyInternal(MongoPersistentProperty prop, Object obj, DBObject dbo) { - org.springframework.data.document.mongodb.mapping.DBRef dbref = prop.getField().getAnnotation( - org.springframework.data.document.mongodb.mapping.DBRef.class); - - String name = prop.getKey(); - Class type = prop.getType(); - if (prop.isCollection()) { - BasicDBList dbList = new BasicDBList(); - Collection coll; - if (type.isArray()) { - coll = new ArrayList(); - for (Object o : (Object[]) obj) { - ((List) coll).add(o); - } - } else { - coll = (Collection) obj; - } - for (Object propObjItem : coll) { - if (null != dbref) { - DBRef dbRef = createDBRef(propObjItem, dbref); - dbList.add(dbRef); - } else if (type.isArray() && isSimpleType(prop.getComponentType())) { - dbList.add(propObjItem); - } else if (propObjItem instanceof List) { - List propObjColl = (List) propObjItem; - TypeInformation typeInfo = ClassTypeInformation.from(propObjItem.getClass()); - while (typeInfo.isCollectionLike()) { - typeInfo = typeInfo.getComponentType(); - } - if (isSimpleType(typeInfo.getType())) { - dbList.add(propObjColl); - } else { - BasicDBList propNestedDbList = new BasicDBList(); - for (Object propNestedObjItem : propObjColl) { - BasicDBObject propDbObj = new BasicDBObject(); - writeInternal(propNestedObjItem, propDbObj); - propNestedDbList.add(propDbObj); - } - dbList.add(propNestedDbList); - } - } else if (isSimpleType(propObjItem.getClass())) { - dbList.add(propObjItem); - } else { - BasicDBObject propDbObj = new BasicDBObject(); - writeInternal(propObjItem, propDbObj, mappingContext.getPersistentEntity(prop.getComponentType())); - dbList.add(propDbObj); - } - } - dbo.put(name, dbList); + + if (obj == null) { return; } - if (null != obj && obj instanceof Map) { + String name = prop.getFieldName(); + + if (prop.isCollection()) { + DBObject collectionInternal = writeCollectionInternal(prop, obj); + dbo.put(name, collectionInternal); + return; + } + + if (prop.isMap()) { BasicDBObject mapDbObj = new BasicDBObject(); - writeMapInternal((Map) obj, mapDbObj); + writeMapInternal((Map) obj, mapDbObj, prop.getTypeInformation()); dbo.put(name, mapDbObj); return; } - if (null != dbref) { - DBRef dbRefObj = createDBRef(obj, dbref); + if (prop.isDbReference()) { + DBRef dbRefObj = createDBRef(obj, prop.getDBRef()); if (null != dbRefObj) { dbo.put(name, dbRefObj); return; @@ -549,11 +513,62 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App } BasicDBObject propDbObj = new BasicDBObject(); + addCustomTypeKeyIfNecessary(prop.getTypeInformation(), obj, propDbObj); writeInternal(obj, propDbObj, mappingContext.getPersistentEntity(prop.getTypeInformation())); dbo.put(name, propDbObj); } + + @SuppressWarnings("unchecked") + protected DBObject writeCollectionInternal(MongoPersistentProperty property, Object obj) { + + BasicDBList dbList = new BasicDBList(); + Class type = property.getType(); + Collection coll = type.isArray() ? CollectionUtils.arrayToList(obj) : (Collection) obj; + TypeInformation componentType = property.getTypeInformation().getComponentType(); + + for (Object element : coll) { + + if (element == null) { + continue; + } + + TypeInformation valueType = ClassTypeInformation.from(element.getClass()); + + if (property.isDbReference()) { + DBRef dbRef = createDBRef(element, property.getDBRef()); + dbList.add(dbRef); + } else if (type.isArray() && isSimpleType(property.getComponentType())) { + dbList.add(element); + } else if (element instanceof List) { + List propObjColl = (List) element; + while (valueType.isCollectionLike()) { + valueType = valueType.getComponentType(); + } + if (isSimpleType(valueType.getType())) { + dbList.add(propObjColl); + } else { + BasicDBList propNestedDbList = new BasicDBList(); + for (Object propNestedObjItem : propObjColl) { + BasicDBObject propDbObj = new BasicDBObject(); + writeInternal(propNestedObjItem, propDbObj); + propNestedDbList.add(propDbObj); + } + dbList.add(propNestedDbList); + } + } else if (isSimpleType(element.getClass())) { + dbList.add(element); + } else { + BasicDBObject propDbObj = new BasicDBObject(); + writeInternal(element, propDbObj, mappingContext.getPersistentEntity(valueType)); + addCustomTypeKeyIfNecessary(componentType, element, propDbObj); + dbList.add(propDbObj); + } + } + + return dbList; + } - protected void writeMapInternal(Map obj, DBObject dbo) { + protected void writeMapInternal(Map obj, DBObject dbo, TypeInformation propertyType) { for (Map.Entry entry : obj.entrySet()) { Object key = entry.getKey(); Object val = entry.getValue(); @@ -565,15 +580,8 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App writeSimpleInternal(simpleKey, val, dbo); } else { DBObject newDbo = new BasicDBObject(); - Class componentType = val.getClass(); - if (componentType.isArray() || componentType.isAssignableFrom(Collection.class) - || componentType.isAssignableFrom(List.class)) { - Class ctype = val.getClass().getComponentType(); - dbo.put("_class", (null != ctype ? ctype.getName() : componentType.getName())); - } else { - dbo.put("_class", componentType.getName()); - } writeInternal(val, newDbo); + addCustomTypeKeyIfNecessary(propertyType, val, newDbo); dbo.put(simpleKey, newDbo); } } else { @@ -582,6 +590,46 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App } } + /** + * Adds custom type information to the given {@link DBObject} if necessary. That is if the value is not the same as + * the one given. This is usually the case if you store a subtype of the actual declared type of the property. + * + * @param type + * @param value + * @param dbObject + */ + public void addCustomTypeKeyIfNecessary(TypeInformation type, Object value, DBObject dbObject) { + + if (type == null) { + return; + } + + Class reference = getValueType(type).getType(); + + boolean notTheSameClass = !value.getClass().equals(reference); + if (notTheSameClass) { + dbObject.put(CUSTOM_TYPE_KEY, value.getClass().getName()); + } + } + + /** + * Returns the type type information of the actual value to be stored. That is, for maps it will return the map value + * type, for collections it will return the component type as well as the given type if it is a non-collection or + * non-map one. + * + * @param type + * @return + */ + public TypeInformation getValueType(TypeInformation type) { + if (type.isMap()) { + return type.getMapValueType(); + } else if (type.isCollectionLike()) { + return type.getComponentType(); + } else { + return type; + } + } + /** * Writes the given simple value to the given {@link DBObject}. Will store enum names for enum values. * @@ -634,7 +682,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App o = x.getValue(ctx); } else { - Object dbObj = dbo.get(prop.getKey()); + Object dbObj = dbo.get(prop.getFieldName()); if (dbObj == null) { return null; diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/BasicMongoPersistentProperty.java b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/BasicMongoPersistentProperty.java index c42e66bf7..cba86b164 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/BasicMongoPersistentProperty.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/BasicMongoPersistentProperty.java @@ -94,7 +94,7 @@ public class BasicMongoPersistentProperty extends AnnotationBasedPersistentPrope * * @return */ - public String getKey() { + public String getFieldName() { if (isIdProperty()) { return "_id"; @@ -111,4 +111,18 @@ public class BasicMongoPersistentProperty extends AnnotationBasedPersistentPrope protected Association createAssociation() { return new Association(this, null); } + + /* (non-Javadoc) + * @see org.springframework.data.document.mongodb.mapping.MongoPersistentProperty#isDbReference() + */ + public boolean isDbReference() { + return getField().isAnnotationPresent(DBRef.class); + } + + /* (non-Javadoc) + * @see org.springframework.data.document.mongodb.mapping.MongoPersistentProperty#getDBRef() + */ + public DBRef getDBRef() { + return getField().getAnnotation(DBRef.class); + } } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/FieldName.java b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/FieldName.java index fac41437f..cbcec956a 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/FieldName.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/FieldName.java @@ -15,6 +15,7 @@ */ package org.springframework.data.document.mongodb.mapping; +import java.lang.annotation.Documented; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -28,6 +29,7 @@ import java.lang.annotation.Target; */ @Retention(RetentionPolicy.RUNTIME) @Target({ ElementType.FIELD }) +@Documented public @interface FieldName { String value(); diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/MongoPersistentProperty.java b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/MongoPersistentProperty.java index 3e610efb6..97b92ae8c 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/MongoPersistentProperty.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/MongoPersistentProperty.java @@ -23,5 +23,27 @@ import org.springframework.data.mapping.model.PersistentProperty; * @author Oliver Gierke */ public interface MongoPersistentProperty extends PersistentProperty { - String getKey(); + + /** + * Returns the name of the field a property is persisted to. + * + * @return + */ + String getFieldName(); + + /** + * Returns whether the propert is a {@link com.mongodb.DBRef}. If this returns {@literal true} you can expect + * {@link #getDBRef()} to return an non-{@literal null} value. + * + * @return + */ + boolean isDbReference(); + + /** + * Returns the {@link DBRef} if the property is a reference. + * + * @see #isDbReference() + * @return + */ + DBRef getDBRef(); } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/SimpleMongoMappingContext.java b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/SimpleMongoMappingContext.java index 0e16f74ec..474cb6509 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/SimpleMongoMappingContext.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/mapping/SimpleMongoMappingContext.java @@ -77,7 +77,7 @@ public class SimpleMongoMappingContext extends /* (non-Javadoc) * @see org.springframework.data.document.mongodb.mapping.MongoPersistentProperty#getKey() */ - public String getKey() { + public String getFieldName() { return isIdProperty() ? "_id" : getName(); } @@ -88,6 +88,20 @@ public class SimpleMongoMappingContext extends protected Association createAssociation() { return new Association(this, null); } + + /* (non-Javadoc) + * @see org.springframework.data.document.mongodb.mapping.MongoPersistentProperty#isDbReference() + */ + public boolean isDbReference() { + return false; + } + + /* (non-Javadoc) + * @see org.springframework.data.document.mongodb.mapping.MongoPersistentProperty#getDBRef() + */ + public DBRef getDBRef() { + return null; + } } static class SimpleMongoPersistentEntity extends BasicPersistentEntity implements diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/repository/ConvertingParameterAccessor.java b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/repository/ConvertingParameterAccessor.java index 801f75c2e..cd615c5a6 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/repository/ConvertingParameterAccessor.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/repository/ConvertingParameterAccessor.java @@ -19,9 +19,11 @@ import java.util.HashMap; import java.util.Iterator; import java.util.Map; +import com.mongodb.BasicDBList; import com.mongodb.BasicDBObject; import com.mongodb.DBObject; import org.springframework.data.document.mongodb.MongoWriter; +import org.springframework.data.document.mongodb.convert.MappingMongoConverter; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Sort; import org.springframework.data.repository.query.ParameterAccessor; @@ -91,7 +93,37 @@ public class ConvertingParameterAccessor implements ParameterAccessor { DBObject result = new BasicDBObject(); writer.write(new ValueHolder(value), result); - return ((DBObject) result.get("value")).get("value"); + Object resultValue = ((DBObject) result.get("value")).get("value"); + return removeTypeInfoRecursively(resultValue); + } + + /** + * Removes the type information from the conversion result. + * + * @param object + * @return + */ + private Object removeTypeInfoRecursively(Object object) { + + if (!(object instanceof DBObject)) { + return object; + } + + DBObject dbObject = (DBObject) object; + + dbObject.removeField(MappingMongoConverter.CUSTOM_TYPE_KEY); + for (String key : dbObject.keySet()) { + Object value = dbObject.get(key); + if (value instanceof BasicDBList) { + for (Object element : (BasicDBList) value) { + removeTypeInfoRecursively(element); + } + } else { + removeTypeInfoRecursively(value); + } + } + + return dbObject; } /** diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/BasicMongoPersistentPropertyUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/BasicMongoPersistentPropertyUnitTests.java index 6a2bbc060..be1e0d0a5 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/BasicMongoPersistentPropertyUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/BasicMongoPersistentPropertyUnitTests.java @@ -43,7 +43,7 @@ public class BasicMongoPersistentPropertyUnitTests { public void usesAnnotatedFieldName() { Field field = ReflectionUtils.findField(Person.class, "firstname"); - assertThat(getPropertyFor(field).getKey(), is("foo")); + assertThat(getPropertyFor(field).getFieldName(), is("foo")); } @Test @@ -51,14 +51,14 @@ public class BasicMongoPersistentPropertyUnitTests { Field field = ReflectionUtils.findField(Person.class, "id"); MongoPersistentProperty property = getPropertyFor(field); assertThat(property.isIdProperty(), is(true)); - assertThat(property.getKey(), is("_id")); + assertThat(property.getFieldName(), is("_id")); } @Test public void returnsPropertyNameForUnannotatedProperties() { Field field = ReflectionUtils.findField(Person.class, "lastname"); - assertThat(getPropertyFor(field).getKey(), is("lastname")); + assertThat(getPropertyFor(field).getFieldName(), is("lastname")); } private MongoPersistentProperty getPropertyFor(Field field) { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/MappingMongoConverterUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/MappingMongoConverterUnitTests.java index d9137f0a8..fae37313c 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/MappingMongoConverterUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/MappingMongoConverterUnitTests.java @@ -20,6 +20,7 @@ import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Date; import java.util.HashSet; @@ -35,6 +36,8 @@ import org.junit.runner.RunWith; import org.mockito.runners.MockitoJUnitRunner; import org.springframework.core.convert.converter.Converter; import org.springframework.data.document.mongodb.convert.MappingMongoConverter; + +import com.mongodb.BasicDBList; import com.mongodb.BasicDBObject; import com.mongodb.DBObject; @@ -218,6 +221,51 @@ public class MappingMongoConverterUnitTests { assertThat(result.firstname, is("Oliver")); } + /** + * @see DATADOC-145 + */ + @Test + public void writesCollectionWithInterfaceCorrectly() { + Person person = new Person(); + person.birthDate = new LocalDate(); + person.firstname = "Oliver"; + + CollectionWrapper wrapper = new CollectionWrapper(); + wrapper.contacts = Arrays.asList((Contact) person); + + BasicDBObject dbObject = new BasicDBObject(); + converter.write(wrapper, dbObject); + + Object result = dbObject.get("contacts"); + assertThat(result, is(BasicDBList.class)); + BasicDBList contacts = (BasicDBList) result; + DBObject personDbObject = (DBObject) contacts.get(0); + assertThat(personDbObject.get("foo").toString(), is("Oliver")); + assertThat((String) personDbObject.get(MappingMongoConverter.CUSTOM_TYPE_KEY), is(Person.class.getName())); + } + + /** + * @see DATADOC-145 + */ + @Test + public void readsCollectionWithInterfaceCorrectly() { + + BasicDBObject person = new BasicDBObject(MappingMongoConverter.CUSTOM_TYPE_KEY, Person.class.getName()); + person.put("foo", "Oliver"); + + BasicDBList contacts = new BasicDBList(); + contacts.add(person); + + CollectionWrapper result = converter.read(CollectionWrapper.class, new BasicDBObject("contacts", contacts)); + assertThat(result.contacts, is(notNullValue())); + assertThat(result.contacts.size(), is(1)); + Contact contact = result.contacts.get(0); + assertThat(contact, is(Person.class)); + assertThat(((Person) contact).firstname, is("Oliver")); + + } + + class ClassWithEnumProperty { SampleEnum sampleEnum; @@ -250,6 +298,10 @@ public class MappingMongoConverterUnitTests { public static class BirthDateContainer { LocalDate birthDate; } + + class CollectionWrapper { + List contacts; + } private class LocalDateToDateConverter implements Converter {