From 034164794cd19d589c5aebc0b291633f283941cd Mon Sep 17 00:00:00 2001 From: Jon Brisbin Date: Fri, 8 Apr 2011 15:59:46 -0500 Subject: [PATCH 1/2] Fix to get rid of problem using Arrays.asList(Object[]). Now creates new LinkedList to preserve order of original array. --- .../convert/MappingMongoConverter.java | 69 +++++++++++-------- .../document/mongodb/index/CompoundIndex.java | 14 ++-- .../data/document/mongodb/index/Indexed.java | 2 +- .../mapping/CustomCollectionWithIndex.java | 44 ++++++++++++ .../mapping/DetectedCollectionWithIndex.java | 45 ++++++++++++ .../mongodb/mapping/MappingTests.java | 39 +++++++++++ 6 files changed, 175 insertions(+), 38 deletions(-) create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/CustomCollectionWithIndex.java create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/DetectedCollectionWithIndex.java 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 a7d2903f6..cb114a4ed 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 @@ -27,8 +27,10 @@ import java.util.Collection; import java.util.Date; import java.util.HashMap; import java.util.LinkedHashMap; +import java.util.LinkedList; import java.util.List; import java.util.Map; + import com.mongodb.BasicDBList; import com.mongodb.BasicDBObject; import com.mongodb.DB; @@ -89,28 +91,28 @@ public class MappingMongoConverter implements MongoConverter, ApplicationContext /** * Creates a new {@link MappingMongoConverter} with the given {@link MappingContext}. - * + * * @param mappingContext */ public MappingMongoConverter(MappingContext mappingContext) { this.mappingContext = mappingContext; this.conversionService.removeConvertible(Object.class, String.class); } - - /** - * Add custom {@link Converter} or {@link ConverterFactory} instances to be used that will take presidence over - * metadata driven conversion between of objects to/from DBObject - * - * @param converters - */ - public void setConverters(List> converters) { - if (null != converters) { - for (Converter c : converters) { - registerConverter(c); - conversionService.addConverter(c); - } - } - } + + /** + * Add custom {@link Converter} or {@link ConverterFactory} instances to be used that will take presidence over + * metadata driven conversion between of objects to/from DBObject + * + * @param converters + */ + public void setConverters(List> converters) { + if (null != converters) { + for (Converter c : converters) { + registerConverter(c); + conversionService.addConverter(c); + } + } + } /** * Inspects the given {@link Converter} for the types it can convert and registers the pair for custom type conversion @@ -379,17 +381,17 @@ public class MappingMongoConverter implements MongoConverter, ApplicationContext private void initializeConverters() { if (!conversionService.canConvert(ObjectId.class, String.class)) { - conversionService.addConverter(ObjectIdToStringConverter.INSTANCE); - } - if (!conversionService.canConvert(String.class, ObjectId.class)) { - conversionService.addConverter(StringToObjectIdConverter.INSTANCE); - } - if (!conversionService.canConvert(ObjectId.class, BigInteger.class)) { - conversionService.addConverter(ObjectIdToBigIntegerConverter.INSTANCE); - } - if (!conversionService.canConvert(BigInteger.class, ObjectId.class)) { - conversionService.addConverter(BigIntegerToObjectIdConverter.INSTANCE); - } + conversionService.addConverter(ObjectIdToStringConverter.INSTANCE); + } + if (!conversionService.canConvert(String.class, ObjectId.class)) { + conversionService.addConverter(StringToObjectIdConverter.INSTANCE); + } + if (!conversionService.canConvert(ObjectId.class, BigInteger.class)) { + conversionService.addConverter(ObjectIdToBigIntegerConverter.INSTANCE); + } + if (!conversionService.canConvert(BigInteger.class, ObjectId.class)) { + conversionService.addConverter(BigIntegerToObjectIdConverter.INSTANCE); + } MappingBeanHelper.setConversionService(conversionService); } @@ -405,7 +407,10 @@ public class MappingMongoConverter implements MongoConverter, ApplicationContext BasicDBList dbList = new BasicDBList(); Collection coll; if (type.isArray()) { - coll = Arrays.asList((Object[]) obj); + coll = new ArrayList(); + for (Object o : (Object[]) obj) { + ((List) coll).add(o); + } } else { coll = (Collection) obj; } @@ -562,7 +567,11 @@ public class MappingMongoConverter implements MongoConverter, ApplicationContext items[i] = dbObjItem; } } - return Arrays.asList(items); + List itemsToReturn = new LinkedList(); + for (Object obj : items) { + itemsToReturn.add(obj); + } + return itemsToReturn; } Class toType = findTypeToBeUsed((DBObject) dbObj); @@ -602,7 +611,7 @@ public class MappingMongoConverter implements MongoConverter, ApplicationContext } public void afterPropertiesSet() { - initializeConverters(); + initializeConverters(); } protected class PersistentPropertyWrapper { diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/index/CompoundIndex.java b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/index/CompoundIndex.java index de5b01061..54b7d3622 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/index/CompoundIndex.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/index/CompoundIndex.java @@ -28,18 +28,18 @@ import java.lang.annotation.Target; @Retention(RetentionPolicy.RUNTIME) public @interface CompoundIndex { - String def(); + String def(); - IndexDirection direction() default IndexDirection.ASCENDING; + IndexDirection direction() default IndexDirection.ASCENDING; - boolean unique() default false; + boolean unique() default false; - boolean sparse() default false; + boolean sparse() default false; - boolean dropDups() default true; + boolean dropDups() default false; - String name() default ""; + String name() default ""; - String collection() default ""; + String collection() default ""; } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/index/Indexed.java b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/index/Indexed.java index ec1f19411..ba7f45b2e 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/index/Indexed.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/index/Indexed.java @@ -34,7 +34,7 @@ public @interface Indexed { boolean sparse() default false; - boolean dropDups() default true; + boolean dropDups() default false; String name() default ""; diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/CustomCollectionWithIndex.java b/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/CustomCollectionWithIndex.java new file mode 100644 index 000000000..b84a3c277 --- /dev/null +++ b/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/CustomCollectionWithIndex.java @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2011 by the original author(s). + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.data.document.mongodb.mapping; + +import org.springframework.data.annotation.Id; +import org.springframework.data.document.mongodb.index.Indexed; + +/** + * @author Jon Brisbin + */ +@Document(collection = "foobar") +public class CustomCollectionWithIndex { + + @Id + private String id; + @Indexed + private String name; + + public CustomCollectionWithIndex(String name) { + this.name = name; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } +} diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/DetectedCollectionWithIndex.java b/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/DetectedCollectionWithIndex.java new file mode 100644 index 000000000..9adb9ec49 --- /dev/null +++ b/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/DetectedCollectionWithIndex.java @@ -0,0 +1,45 @@ +/* + * Copyright (c) 2011 by the original author(s). + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.data.document.mongodb.mapping; + +import org.springframework.data.annotation.Id; +import org.springframework.data.document.mongodb.index.Indexed; + +/** + * @author Jon Brisbin + */ +@Document +public class DetectedCollectionWithIndex { + + @Id + private String id; + @Indexed + private String name; + + public DetectedCollectionWithIndex(String name) { + this.name = name; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + +} diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/MappingTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/MappingTests.java index a2e1572fa..4cbe0e56a 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/MappingTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/MappingTests.java @@ -25,13 +25,18 @@ import java.util.List; import java.util.Map; import com.mongodb.DB; +import com.mongodb.DBCollection; +import com.mongodb.DBObject; import com.mongodb.Mongo; +import com.mongodb.MongoException; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.junit.Before; import org.junit.Test; import org.springframework.context.ApplicationContext; import org.springframework.context.support.ClassPathXmlApplicationContext; +import org.springframework.dao.DataAccessException; +import org.springframework.data.document.mongodb.CollectionCallback; import org.springframework.data.document.mongodb.MongoDbUtils; import org.springframework.data.document.mongodb.MongoTemplate; import org.springframework.data.document.mongodb.query.Criteria; @@ -44,6 +49,7 @@ public class MappingTests { private static final Log LOGGER = LogFactory.getLog(MongoDbUtils.class); private final String[] collectionsToDrop = new String[]{ + "foobar", "person", "personmapproperty", "personpojo", @@ -223,4 +229,37 @@ public class MappingTests { assertThat(result.size(), is(1)); } + @Test + public void testIndexesCreatedInRightCollection() { + CustomCollectionWithIndex ccwi = new CustomCollectionWithIndex("test"); + template.insert(ccwi); + + assertTrue(template.execute("foobar", new CollectionCallback() { + public Boolean doInCollection(DBCollection collection) throws MongoException, DataAccessException { + List indexes = collection.getIndexInfo(); + for (DBObject dbo : indexes) { + if ("name_1".equals(dbo.get("name"))) { + return true; + } + } + return false; + } + })); + + DetectedCollectionWithIndex dcwi = new DetectedCollectionWithIndex("test"); + template.insert(dcwi); + + assertTrue(template.execute(DetectedCollectionWithIndex.class.getSimpleName().toLowerCase(), new CollectionCallback() { + public Boolean doInCollection(DBCollection collection) throws MongoException, DataAccessException { + List indexes = collection.getIndexInfo(); + for (DBObject dbo : indexes) { + if ("name_1".equals(dbo.get("name"))) { + return true; + } + } + return false; + } + })); + } + } From e9a4975cc7f23b4d60a81550ccda867adfcf9d03 Mon Sep 17 00:00:00 2001 From: Thomas Risberg Date: Fri, 8 Apr 2011 17:40:31 -0400 Subject: [PATCH 2/2] DATADOC-85 DATADOC-84 fixed some issues with id conversion for queries and save/inserts --- .../data/document/mongodb/MongoTemplate.java | 105 +++++++++++++++--- .../repository/SimpleMongoRepository.java | 2 +- 2 files changed, 91 insertions(+), 16 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/MongoTemplate.java b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/MongoTemplate.java index 4be12a5e6..0f45c8ada 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/MongoTemplate.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/MongoTemplate.java @@ -23,6 +23,7 @@ import java.beans.IntrospectionException; import java.beans.PropertyDescriptor; import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -704,6 +705,18 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica return null; } + //TODO: Need to move this to more central place + if (dbDoc.containsField("_id")) { + if (dbDoc.get("_id") instanceof String) { + ObjectId oid = convertIdValue(this.mongoConverter, dbDoc.get("_id")); + if (oid != null) { + dbDoc.put("_id", oid); + } + } + } + if (LOGGER.isDebugEnabled()) { + LOGGER.debug("insert DBObject: " + dbDoc); + } return execute(collectionName, new CollectionCallback() { public Object doInCollection(DBCollection collection) throws MongoException, DataAccessException { if (writeConcern == null) { @@ -722,6 +735,17 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica return Collections.emptyList(); } + //TODO: Need to move this to more central place + for (DBObject dbDoc : dbDocList) { + if (dbDoc.containsField("_id")) { + if (dbDoc.get("_id") instanceof String) { + ObjectId oid = convertIdValue(this.mongoConverter, dbDoc.get("_id")); + if (oid != null) { + dbDoc.put("_id", oid); + } + } + } + } execute(collectionName, new CollectionCallback() { public Void doInCollection(DBCollection collection) throws MongoException, DataAccessException { if (writeConcern == null) { @@ -752,6 +776,18 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica return null; } + //TODO: Need to move this to more central place + if (dbDoc.containsField("_id")) { + if (dbDoc.get("_id") instanceof String) { + ObjectId oid = convertIdValue(this.mongoConverter, dbDoc.get("_id")); + if (oid != null) { + dbDoc.put("_id", oid); + } + } + } + if (LOGGER.isDebugEnabled()) { + LOGGER.debug("save DBObject: " + dbDoc); + } return execute(collectionName, new CollectionCallback() { public Object doInCollection(DBCollection collection) throws MongoException, DataAccessException { if (writeConcern == null) { @@ -1134,32 +1170,71 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica // no ids in this query return; } - final MongoPropertyDescriptor descriptor; + MongoPropertyDescriptor descriptor; try { - descriptor = new MongoPropertyDescriptor(new PropertyDescriptor(idKey, targetClass), targetClass); + MongoPropertyDescriptor mpd = new MongoPropertyDescriptor(new PropertyDescriptor(idKey, targetClass), targetClass); + descriptor = mpd; } catch (IntrospectionException e) { - // no property descriptor for this key - return; + // no property descriptor for this key - try the other + try { + String theOtherIdKey = "id".equals(idKey) ? "_id" : "id"; + MongoPropertyDescriptor mpd2 = new MongoPropertyDescriptor(new PropertyDescriptor(theOtherIdKey, targetClass), targetClass); + descriptor = mpd2; + } catch (IntrospectionException e2) { + // no property descriptor for this key either - bail + return; + } } if (descriptor.isIdProperty() && descriptor.isOfIdType()) { Object value = query.get(idKey); - ObjectId newValue = null; - try { - if (value instanceof String && ObjectId.isValid((String) value)) { - newValue = converter.convertObjectId(value); + if (value instanceof DBObject) { + DBObject dbo = (DBObject) value; + if (dbo.containsField("$in")) { + List ids = new ArrayList(); + int count = 0; + for (Object o : (Object[])dbo.get("$in")) { + count++; + ObjectId newValue = convertIdValue(converter, o); + if (newValue != null) { + ids.add(newValue); + } + } + if (ids.size() > 0 && ids.size() != count) { + throw new InvalidDataAccessApiUsageException("Inconsistent set of id values provided " + + Arrays.asList((Object[])dbo.get("$in"))); + } + if (ids.size() > 0) { + dbo.removeField("$in"); + dbo.put("$in", ids.toArray()); + } } - } catch (ConversionFailedException iae) { - LOGGER.warn("Unable to convert the String " + value + " to an ObjectId"); - } - query.removeField(idKey); - if (newValue != null) { - query.put(MongoPropertyDescriptor.ID_KEY, newValue); - } else { + query.removeField(idKey); query.put(MongoPropertyDescriptor.ID_KEY, value); } + else { + ObjectId newValue = convertIdValue(converter, value); + query.removeField(idKey); + if (newValue != null) { + query.put(MongoPropertyDescriptor.ID_KEY, newValue); + } else { + query.put(MongoPropertyDescriptor.ID_KEY, value); + } + } } } + private ObjectId convertIdValue(MongoConverter converter, Object value) { + ObjectId newValue = null; + try { + if (value instanceof String && ObjectId.isValid((String) value)) { + newValue = converter.convertObjectId(value); + } + } catch (ConversionFailedException iae) { + LOGGER.warn("Unable to convert the String " + value + " to an ObjectId"); + } + return newValue; + } + /** * Substitutes the id key if it is found in he query. Any 'id' keys will be replaced with '_id'. No conversion * of the value to an ObjectId is possible since we don't have access to a targetClass or a converter. This diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/repository/SimpleMongoRepository.java b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/repository/SimpleMongoRepository.java index d25727379..bfc249e6e 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/repository/SimpleMongoRepository.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/repository/SimpleMongoRepository.java @@ -137,7 +137,7 @@ public class SimpleMongoRepository implements Paging */ public void delete(T entity) { - template.remove(entityInformation.getCollectionName(), getIdQuery(entityInformation.getId(entity))); + template.remove(entityInformation.getCollectionName(), getIdQuery(entityInformation.getId(entity)), entity.getClass()); } /*