From 02abfced9cafbf352edd818ba2e6b17d75d95f19 Mon Sep 17 00:00:00 2001 From: Thomas Darimont Date: Tue, 21 Jan 2014 09:42:44 +0100 Subject: [PATCH] DATAMONGO-686 - Fixed potential race-condition in QueryMapper. We now create a new a new DBObject in QueryMapper#getMappedValue() instead of replacing them in the original objects in order to prevent the original query object being manipulated. Added test case to verify that the original DBObject is not manipulated. Original pull request: #111. --- .../mongodb/core/convert/QueryMapper.java | 13 ++++++---- .../data/mongodb/core/MongoTemplateTests.java | 24 +++++++++++++++++++ .../core/convert/QueryMapperUnitTests.java | 19 ++++++++++++++- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java index 6f10a9a6f..3659174d1 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2013 the original author or authors. + * Copyright 2011-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -44,6 +44,7 @@ import com.mongodb.DBRef; * @author Jon Brisbin * @author Oliver Gierke * @author Patryk Wasik + * @author Thomas Darimont */ public class QueryMapper { @@ -176,20 +177,22 @@ public class QueryMapper { if (value instanceof DBObject) { DBObject valueDbo = (DBObject) value; + DBObject resultDbo = new BasicDBObject(valueDbo.toMap()); + if (valueDbo.containsField("$in") || valueDbo.containsField("$nin")) { String inKey = valueDbo.containsField("$in") ? "$in" : "$nin"; List ids = new ArrayList(); for (Object id : (Iterable) valueDbo.get(inKey)) { ids.add(convertId(id)); } - valueDbo.put(inKey, ids.toArray(new Object[ids.size()])); + resultDbo.put(inKey, ids.toArray(new Object[ids.size()])); } else if (valueDbo.containsField("$ne")) { - valueDbo.put("$ne", convertId(valueDbo.get("$ne"))); + resultDbo.put("$ne", convertId(valueDbo.get("$ne"))); } else { - return getMappedObject((DBObject) value, null); + return getMappedObject(resultDbo, null); } - return valueDbo; + return resultDbo; } else { return convertId(value); diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java index 725349292..75c24e5da 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java @@ -51,6 +51,7 @@ import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.data.annotation.Id; import org.springframework.data.annotation.PersistenceConstructor; import org.springframework.data.annotation.Version; +import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Sort; import org.springframework.data.domain.Sort.Direction; import org.springframework.data.mapping.model.MappingException; @@ -2158,6 +2159,29 @@ public class MongoTemplateTests { } } + /** + * @see DATAMONGO-686 + */ + @Test + public void itShouldBePossibleToReuseAnExistingQuery() { + + Sample sample = new Sample(); + sample.id = "42"; + sample.field = "A"; + + template.save(sample); + + Query query = new Query(); + query.addCriteria(where("_id").in("42", "43")); + + assertThat(template.count(query, Sample.class), is(1L)); + + query.with(new PageRequest(0, 10)); + query.with(new Sort("field")); + + assertThat(template.find(query, Sample.class), is(not(empty()))); + } + static interface Model { String value(); diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java index caa96a8bc..799278d5c 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2013 the original author or authors. + * Copyright 2011-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -484,6 +484,23 @@ public class QueryMapperUnitTests { assertThat(fieldsResult.get("reference"), is((Object) 0)); } + /** + * @see DATAMONGO-686 + */ + @Test + public void queryMapperShouldNotChangeStateInGivenQueryObjectWhenIdConstrainedByInList() { + + BasicMongoPersistentEntity persistentEntity = context.getPersistentEntity(Sample.class); + String idPropertyName = persistentEntity.getIdProperty().getName(); + DBObject queryObject = query(where(idPropertyName).in("42")).getQueryObject(); + + Object idValuesBefore = getAsDBObject(queryObject, idPropertyName).get("$in"); + mapper.getMappedObject(queryObject, persistentEntity); + Object idValuesAfter = getAsDBObject(queryObject, idPropertyName).get("$in"); + + assertThat(idValuesAfter, is(idValuesBefore)); + } + class IdWrapper { Object id; }