From e480ceb2b7a5af24b19d52569b703c278acb7718 Mon Sep 17 00:00:00 2001 From: David Julia Date: Sun, 27 Jun 2021 23:08:43 +1000 Subject: [PATCH] Fix Regression in generating queries with nested maps with numeric keys. While maps that have numeric keys work if there is only one map with an integer key, when there are multiple maps with numeric keys in a given query, it fails. Take the following example for a map called outer with numeric keys holding reference to another object with a map called inner with numeric keys: Updates that are meant to generate {"$set": {"outerMap.1234.inner.5678": "hello"}} are instead generating {"$set": {"outerMap.1234.inner.inner": "hello"}}, repeating the later map property name instead of using the integer key value. This commit adds unit tests both for the UpdateMapper and QueryMapper, which check multiple consecutive maps with numeric keys, and adds a fix in the KeyMapper. Because we cannot easily change the path parsing to somehow parse path parts corresponding to map keys differently, we address the issue in the KeyMapper. We keep track of the partial path corresponding to the current property and use it to skip adding the duplicated property name for the map to the query, and instead add the key. This is a bit redundant in that we now have both an iterator and an index-based way of accessing the path parts, but it gets the tests passing and fixes the issue without making a large change to the current approach. Fixes: #3688 Original Pull Request: #3689 --- .../mongodb/core/convert/QueryMapper.java | 22 ++++++++++++--- .../core/convert/QueryMapperUnitTests.java | 27 +++++++++++++++++++ .../core/convert/UpdateMapperUnitTests.java | 15 +++++++++++ 3 files changed, 61 insertions(+), 3 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 cf0a02bbf..b104404b7 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 @@ -66,6 +66,7 @@ import com.mongodb.DBRef; * @author Thomas Darimont * @author Christoph Strobl * @author Mark Paluch + * @author David Julia */ public class QueryMapper { @@ -1273,11 +1274,17 @@ public class QueryMapper { static class KeyMapper { private final Iterator iterator; + private int currentIndex; + private String currentPropertyRoot; + private final List pathParts; public KeyMapper(String key, MappingContext, MongoPersistentProperty> mappingContext) { - this.iterator = Arrays.asList(key.split("\\.")).iterator(); + this.pathParts = Arrays.asList(key.split("\\.")); + this.currentPropertyRoot = pathParts.get(0); + this.currentIndex = 0; + this.iterator = pathParts.iterator(); this.iterator.next(); } @@ -1295,16 +1302,25 @@ public class QueryMapper { while (inspect) { String partial = iterator.next(); + currentIndex++; - boolean isPositional = isPositionalParameter(partial) && property.isCollectionLike(); + boolean isPositional = isPositionalParameter(partial) && property.isCollectionLike() ; + if(property.isMap() && currentPropertyRoot.equals(partial) && iterator.hasNext()){ + partial = iterator.next(); + currentIndex++; + } - if (isPositional || property.isMap()) { + if (isPositional || property.isMap() && !currentPropertyRoot.equals(partial)) { mappedName.append(".").append(partial); } inspect = isPositional && iterator.hasNext(); } + if(currentIndex + 1 < pathParts.size()) { + currentIndex++; + currentPropertyRoot = pathParts.get(currentIndex); + } return mappedName.toString(); } 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 78405deb3..c6dccb829 100755 --- 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 @@ -75,6 +75,7 @@ import com.mongodb.client.model.Filters; * @author Thomas Darimont * @author Christoph Strobl * @author Mark Paluch + * @author David Julia */ @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) @@ -737,6 +738,28 @@ public class QueryMapperUnitTests { assertThat(document).containsKey("map.1.stringProperty"); } + @Test // GH-3688 + void mappingShouldRetainNestedNumericMapKeys() { + + Query query = query(where("outerMap.1.map.2.stringProperty").is("ba'alzamon")); + + org.bson.Document document = mapper.getMappedObject(query.getQueryObject(), + context.getPersistentEntity(EntityWithIntKeyedMapOfMap.class)); + + assertThat(document).containsKey("outerMap.1.map.2.stringProperty"); + } + + @Test // GH-3688 + void mappingShouldAllowSettingEntireNestedNumericKeyedMapValue() { + + Query query = query(where("outerMap.1.map").is(null)); //newEntityWithComplexValueTypeMap() + + org.bson.Document document = mapper.getMappedObject(query.getQueryObject(), + context.getPersistentEntity(EntityWithIntKeyedMapOfMap.class)); + + assertThat(document).containsKey("outerMap.1.map"); + } + @Test // DATAMONGO-1269 void mappingShouldRetainNumericPositionInList() { @@ -1278,6 +1301,10 @@ public class QueryMapperUnitTests { Map map; } + static class EntityWithIntKeyedMapOfMap{ + Map outerMap; + } + static class EntityWithComplexValueTypeList { List list; } 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 ec43ae33e..fa9ac1547 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 @@ -65,6 +65,7 @@ import com.mongodb.DBRef; * @author Thomas Darimont * @author Mark Paluch * @author Pavel Vodrazka + * @author David Julia */ @ExtendWith(MockitoExtension.class) class UpdateMapperUnitTests { @@ -1110,6 +1111,16 @@ class UpdateMapperUnitTests { .isEqualTo("{\"$set\": {\"map.601218778970110001827396.value\": \"testing\"}}"); } + @Test // GH-3688 + void multipleNumericKeysInNestedPath() { + + Update update = new Update().set("intKeyedMap.12345.map.0", "testing"); + Document mappedUpdate = mapper.getMappedObject(update.getUpdateObject(), + context.getPersistentEntity(EntityWithIntKeyedMap.class)); + + assertThat(mappedUpdate).isEqualTo("{\"$set\": {\"intKeyedMap.12345.map.0\": \"testing\"}}"); + } + @Test // GH-3566 void mapsObjectClassPropertyFieldInMapValueTypeAsKey() { @@ -1357,6 +1368,10 @@ class UpdateMapperUnitTests { Map concreteMap; } + static class EntityWithIntKeyedMap{ + Map intKeyedMap; + } + static class ClassWithEnum { Allocation allocation;