Fix upsert inserts when $and is used with $eq #216

bwaldvogel · Jun 2, 2023 · f20f3cb · f20f3cb
1 parent 7ee8cff
commit f20f3cb
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 23 deletions.
diff --git a/core/src/main/java/de/bwaldvogel/mongo/backend/AbstractMongoCollection.java b/core/src/main/java/de/bwaldvogel/mongo/backend/AbstractMongoCollection.java
@@ -27,6 +27,7 @@
 import de.bwaldvogel.mongo.bson.Document;
 import de.bwaldvogel.mongo.bson.ObjectId;
 import de.bwaldvogel.mongo.exception.ConflictingUpdateOperatorsException;
+import de.bwaldvogel.mongo.exception.ErrorCode;
 import de.bwaldvogel.mongo.exception.FailedToParseException;
 import de.bwaldvogel.mongo.exception.ImmutableFieldException;
 import de.bwaldvogel.mongo.exception.IndexBuildFailedException;
@@ -35,6 +36,7 @@
 import de.bwaldvogel.mongo.exception.InvalidIdFieldError;
 import de.bwaldvogel.mongo.exception.MongoServerError;
 import de.bwaldvogel.mongo.exception.MongoServerException;
+import de.bwaldvogel.mongo.exception.PathNotViableException;
 import de.bwaldvogel.mongo.oplog.Oplog;
 
 public abstract class AbstractMongoCollection<P> implements MongoCollection<P> {
@@ -655,7 +657,6 @@ private P getSinglePosition(Document document) {
 
     private Document handleUpsert(Document updateQuery, Document selector, ArrayFilters arrayFilters) {
         Document document = convertSelectorToDocument(selector);
-
         Document newDocument = calculateUpdateDocument(document, updateQuery, arrayFilters, null, true);
         newDocument.computeIfAbsent(getIdField(), k -> deriveDocumentId(selector));
         addDocument(newDocument);
@@ -667,31 +668,56 @@ private Document handleUpsert(Document updateQuery, Document selector, ArrayFilt
      */
     Document convertSelectorToDocument(Document selector) {
         Document document = new Document();
+        convertSelectorToDocument(selector, document);
+        return document;
+    }
+
+    private void convertSelectorToDocument(Document selector, Document document) {
         for (Map.Entry<String, Object> entry : selector.entrySet()) {
             String key = entry.getKey();
             Object value = entry.getValue();
 
             if (key.equals("$and")) {
                 List<Document> andValues = (List<Document>) value;
                 for (Document andValue : andValues) {
-                    document.putAll(convertSelectorToDocument(andValue));
+                    convertSelectorToDocument(andValue, document);
                 }
                 continue;
             } else if (key.equals("$or")) {
                 List<Document> orValues = (List<Document>) value;
                 if (orValues.size() == 1) {
-                    document.putAll(convertSelectorToDocument(orValues.get(0)));
+                    convertSelectorToDocument(orValues.get(0), document);
                 }
                 continue;
             } else if (key.startsWith("$")) {
                 continue;
+            } else if (value instanceof Document) {
+                Document documentValue = (Document) value;
+                if (documentValue.keySet().equals(Set.of("$eq"))) {
+                    changeSubdocumentValueOrThrow(document, key, documentValue.get("$eq"));
+                }
             }
 
             if (!Utils.containsQueryExpression(value)) {
-                Utils.changeSubdocumentValue(document, key, value, (AtomicReference<Integer>) null);
+                changeSubdocumentValueOrThrow(document, key, value);
             }
         }
-        return document;
+    }
+
+    private static void changeSubdocumentValueOrThrow(Document document, String key, Object value) {
+        try {
+            Object previousValue = Utils.changeSubdocumentValue(document, key, value, (AtomicReference<Integer>) null);
+            if (previousValue != null) {
+                throw new MongoServerError(ErrorCode.NotSingleValueField, "cannot infer query fields to set, path '" + key + "' is matched twice");
+            }
+        } catch (PathNotViableException e) {
+            String violatingKey = e.getField();
+            List<String> keyPathFragments = Utils.splitPath(key);
+            int indexOfViolatingKey = keyPathFragments.indexOf(violatingKey);
+            List<String> violatingKeyPathFragments = keyPathFragments.subList(0, indexOfViolatingKey);
+            String violatingKeyPath = Utils.joinPath(violatingKeyPathFragments);
+            throw new MongoServerError(ErrorCode.NotSingleValueField, "cannot infer query fields to set, both paths '" + key + "' and '" + violatingKeyPath + "' are matched");
+        }
     }
 
     @Override

diff --git a/core/src/main/java/de/bwaldvogel/mongo/backend/FieldUpdates.java b/core/src/main/java/de/bwaldvogel/mongo/backend/FieldUpdates.java
@@ -23,7 +23,7 @@
 import de.bwaldvogel.mongo.exception.FailedToParseException;
 import de.bwaldvogel.mongo.exception.ImmutableFieldException;
 import de.bwaldvogel.mongo.exception.MongoServerError;
-import de.bwaldvogel.mongo.exception.PathNotViableException;
+import de.bwaldvogel.mongo.exception.CannotTraverseElementException;
 import de.bwaldvogel.mongo.exception.TypeMismatchException;
 
 class FieldUpdates {
@@ -430,7 +430,7 @@ private void handleRename(String key, Object toField) {
     private void applyRenames() {
         for (Entry<String, String> entry : renames.entrySet()) {
             if (!Utils.canFullyTraverseSubkeyForRename(document, entry.getKey())) {
-                throw new PathNotViableException("cannot traverse element");
+                throw new CannotTraverseElementException();
             }
 
             Object value = Utils.removeSubdocumentValue(document, entry.getKey(), matchPos);

diff --git a/core/src/main/java/de/bwaldvogel/mongo/backend/Utils.java b/core/src/main/java/de/bwaldvogel/mongo/backend/Utils.java
@@ -324,23 +324,22 @@ public static void markOkay(Document result) {
         result.put("ok", 1.0);
     }
 
-    private static void setListSafe(Object document, String key, String previousKey, Object obj) {
+    private static Object setListSafe(Object document, String key, String previousKey, Object obj) {
         if (document instanceof List<?>) {
             @SuppressWarnings("unchecked")
             List<Object> list = ((List<Object>) document);
             if (!isNumeric(key)) {
-                String element = new Document(previousKey, document).toString(true);
-                throw new PathNotViableException("Cannot create field '" + key + "' in element " + element);
+                throw new PathNotViableException(key, new Document(previousKey, document));
             }
             int pos = Integer.parseInt(key);
             while (list.size() <= pos) {
                 list.add(null);
             }
-            list.set(pos, obj);
+            return list.set(pos, obj);
         } else {
             @SuppressWarnings("unchecked")
             Map<String, Object> documentAsMap = (Map<String, Object>) document;
-            documentAsMap.put(key, obj);
+            return documentAsMap.put(key, obj);
         }
     }
 
@@ -399,29 +398,27 @@ public static void changeSubdocumentValue(Object document, String key, Object ne
         changeSubdocumentValue(document, key, newValue, new AtomicReference<>());
     }
 
-    static void changeSubdocumentValue(Object document, String key, Object newValue, AtomicReference<Integer> matchPos) {
-        changeSubdocumentValue(document, key, newValue, null, matchPos);
+    static Object changeSubdocumentValue(Object document, String key, Object newValue, AtomicReference<Integer> matchPos) {
+        return changeSubdocumentValue(document, key, newValue, null, matchPos);
     }
 
-    private static void changeSubdocumentValue(Object document, String key, Object newValue, String previousKey, AtomicReference<Integer> matchPos) {
+    private static Object changeSubdocumentValue(Object document, String key, Object newValue, String previousKey, AtomicReference<Integer> matchPos) {
         List<String> pathFragments = splitPath(key);
         String mainKey = pathFragments.get(0);
         if (pathFragments.size() == 1) {
-            setListSafe(document, key, previousKey, newValue);
-            return;
+            return setListSafe(document, key, previousKey, newValue);
         }
         String subKey = getSubkey(pathFragments, matchPos);
         Object subObject = getFieldValueListSafe(document, mainKey);
         if (subObject instanceof Document || subObject instanceof List<?>) {
-            changeSubdocumentValue(subObject, subKey, newValue, mainKey, matchPos);
+            return changeSubdocumentValue(subObject, subKey, newValue, mainKey, matchPos);
         } else if (Missing.isNeitherNullNorMissing(subObject)) {
-            String element = new Document(mainKey, subObject).toString(true);
             String subKeyFirst = splitPath(subKey).get(0);
-            throw new PathNotViableException("Cannot create field '" + subKeyFirst + "' in element " + element);
+            throw new PathNotViableException(subKeyFirst, new Document(mainKey, subObject));
         } else {
             Document obj = new Document();
             changeSubdocumentValue(obj, subKey, newValue, mainKey, matchPos);
-            setListSafe(document, mainKey, previousKey, obj);
+            return setListSafe(document, mainKey, previousKey, obj);
         }
     }
 

diff --git a/core/src/main/java/de/bwaldvogel/mongo/exception/CannotTraverseElementException.java b/core/src/main/java/de/bwaldvogel/mongo/exception/CannotTraverseElementException.java
@@ -0,0 +1,11 @@
+package de.bwaldvogel.mongo.exception;
+
+public class CannotTraverseElementException extends MongoServerError {
+
+    private static final long serialVersionUID = 1L;
+
+    public CannotTraverseElementException() {
+        super(ErrorCode.PathNotViable, "cannot traverse element");
+    }
+
+}
diff --git a/core/src/main/java/de/bwaldvogel/mongo/exception/ErrorCode.java b/core/src/main/java/de/bwaldvogel/mongo/exception/ErrorCode.java
@@ -12,6 +12,7 @@ public enum ErrorCode {
     NamespaceExists(48),
     DollarPrefixedFieldName(52),
     InvalidIdField(53),
+    NotSingleValueField(54),
     CommandNotFound(59),
     ImmutableField(66),
     InvalidOptions(72),

diff --git a/core/src/main/java/de/bwaldvogel/mongo/exception/PathNotViableException.java b/core/src/main/java/de/bwaldvogel/mongo/exception/PathNotViableException.java
@@ -1,11 +1,19 @@
 package de.bwaldvogel.mongo.exception;
 
+import de.bwaldvogel.mongo.bson.Document;
+
 public class PathNotViableException extends MongoServerError {
 
     private static final long serialVersionUID = 1L;
 
-    public PathNotViableException(String message) {
-        super(ErrorCode.PathNotViable, message);
+    private final String field;
+
+    public PathNotViableException(String field, Document element) {
+        super(ErrorCode.PathNotViable, "Cannot create field '" + field + "' in element " + element.toString(true));
+        this.field = field;
     }
 
+    public String getField() {
+        return field;
+    }
 }
diff --git a/test-common/src/main/java/de/bwaldvogel/mongo/backend/AbstractBackendTest.java b/test-common/src/main/java/de/bwaldvogel/mongo/backend/AbstractBackendTest.java
@@ -6223,6 +6223,48 @@ void testUpsertWithPositionalAll() throws Exception {
         assertThat(result).isEqualTo(json("_id: 1, a: [1, 1]"));
     }
 
+    private static Stream<Arguments> upsertWithNonMatchingFilterTestData() {
+        return Stream.of(
+            Arguments.of("$and: [{key1: {$eq: 'value1'}}, {key2: {$eq: 'value2'}}]", "dummy: 'dummy', key1: 'value1', key2: 'value2'"),
+            Arguments.of("$and: [{key1: {$eq: {sub: 'value1'}}}, {key2: {$eq: 'value2'}}]", "dummy: 'dummy', key1: {sub: 'value1'}, key2: 'value2'"),
+            Arguments.of("$and: [{key1: {$gt: 1}}, {key2: 'value2'}]", "dummy: 'dummy', key2: 'value2'"),
+            Arguments.of("$and: [{'sub.key1': {$eq: 'value1'}}, {'sub.key2': {$eq: 'value2'}}]", "dummy: 'dummy', sub: {key1: 'value1', key2: 'value2'}"),
+            Arguments.of("$and: [{'sub.key1': 'value1'}, {'sub.key2': 'value2'}]", "dummy: 'dummy', sub: {key1: 'value1', key2: 'value2'}"),
+            Arguments.of("$or: [{'sub.key1': 'value1'}, {'sub.key1': 'value1b'}]", "dummy: 'dummy'"),
+            Arguments.of("$or: [{'sub.key1': 'value1'}]", "dummy: 'dummy', sub: {key1: 'value1'}"),
+            Arguments.of("$or: [{key1: 'value1'}, {key2: 'value2'}]", "dummy: 'dummy'")
+        );
+    }
+
+    // https://github.com/bwaldvogel/mongo-java-server/issues/216
+    @ParameterizedTest
+    @MethodSource("upsertWithNonMatchingFilterTestData")
+    void testUpsertAndFilterDoesNotMatch(String filter, String expectedDocument) throws Exception {
+        collection.updateOne(json(filter),
+            json("$set: {dummy: 'dummy'}"), new UpdateOptions().upsert(true));
+
+        assertThat(collection.find().projection(json("_id: 0")))
+            .containsExactly(json(expectedDocument));
+    }
+
+    private static Stream<Arguments> upsertWithIllegalFilterTestData() {
+        return Stream.of(
+            Arguments.of("$and: [{key1: 'value1'}, {key1: 'value1b'}, {key2: 'value2'}]", "cannot infer query fields to set, path 'key1' is matched twice"),
+            Arguments.of("$and: [{sub: 'value1'}, {'sub.key': 'conflict'}]", "cannot infer query fields to set, both paths 'sub.key' and 'sub' are matched"),
+            Arguments.of("$and: [{'sub.a': 'value1'}, {'sub.a.b': 'conflict'}]", "cannot infer query fields to set, both paths 'sub.a.b' and 'sub.a' are matched"),
+            Arguments.of("$and: [{'sub': 'value1'}, {'sub.a.b': 'conflict'}]", "cannot infer query fields to set, both paths 'sub.a.b' and 'sub' are matched"),
+            Arguments.of("$and: [{'sub.key1': 'value1'}, {'sub.key1': 'value1b'}]", "cannot infer query fields to set, path 'sub.key1' is matched twice")
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("upsertWithIllegalFilterTestData")
+    void testUpsertWithIllegalFilter(String filter, String expectedErrorMessage) throws Exception {
+        assertMongoWriteException(() -> {
+            collection.updateOne(json(filter), json("$set: {dummy: 'dummy'}"), new UpdateOptions().upsert(true));
+        }, 54, "NotSingleValueField", expectedErrorMessage);
+    }
+
     // https://github.com/bwaldvogel/mongo-java-server/issues/164
     @Test
     void testFindOneAndUpdateWithReturnDocumentBeforeWhenDocumentDidNotExist() throws Exception {