Skip to content

Commit f20f3cb

Browse files
bwaldvogelbwaldvogel
committed
Fix upsert inserts when $and is used with $eq #216 
1 parent 7ee8cff commit f20f3cb

File tree

7 files changed

+108
-23
lines changed

7 files changed

+108
-23
lines changed

‎core/src/main/java/de/bwaldvogel/mongo/backend/AbstractMongoCollection.java‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import de.bwaldvogel.mongo.bson.Document;
2828
import de.bwaldvogel.mongo.bson.ObjectId;
2929
import de.bwaldvogel.mongo.exception.ConflictingUpdateOperatorsException;
30+
import de.bwaldvogel.mongo.exception.ErrorCode;
3031
import de.bwaldvogel.mongo.exception.FailedToParseException;
3132
import de.bwaldvogel.mongo.exception.ImmutableFieldException;
3233
import de.bwaldvogel.mongo.exception.IndexBuildFailedException;
@@ -35,6 +36,7 @@
3536
import de.bwaldvogel.mongo.exception.InvalidIdFieldError;
3637
import de.bwaldvogel.mongo.exception.MongoServerError;
3738
import de.bwaldvogel.mongo.exception.MongoServerException;
39+
import de.bwaldvogel.mongo.exception.PathNotViableException;
3840
import de.bwaldvogel.mongo.oplog.Oplog;
3941

4042
public abstract class AbstractMongoCollection<P> implements MongoCollection<P> {
@@ -655,7 +657,6 @@ private P getSinglePosition(Document document) {
655657

656658
private Document handleUpsert(Document updateQuery, Document selector, ArrayFilters arrayFilters) {
657659
Document document = convertSelectorToDocument(selector);
658-
659660
Document newDocument = calculateUpdateDocument(document, updateQuery, arrayFilters, null, true);
660661
newDocument.computeIfAbsent(getIdField(), k -> deriveDocumentId(selector));
661662
addDocument(newDocument);
@@ -667,31 +668,56 @@ private Document handleUpsert(Document updateQuery, Document selector, ArrayFilt
667668
*/
668669
Document convertSelectorToDocument(Document selector) {
669670
Document document = new Document();
671+
convertSelectorToDocument(selector, document);
672+
return document;
673+
}
674+
675+
private void convertSelectorToDocument(Document selector, Document document) {
670676
for (Map.Entry<String, Object> entry : selector.entrySet()) {
671677
String key = entry.getKey();
672678
Object value = entry.getValue();
673679

674680
if (key.equals("$and")) {
675681
List<Document> andValues = (List<Document>) value;
676682
for (Document andValue : andValues) {
677-
document.putAll(convertSelectorToDocument(andValue));
683+
convertSelectorToDocument(andValue, document);
678684
}
679685
continue;
680686
} else if (key.equals("$or")) {
681687
List<Document> orValues = (List<Document>) value;
682688
if (orValues.size() == 1) {
683-
document.putAll(convertSelectorToDocument(orValues.get(0)));
689+
convertSelectorToDocument(orValues.get(0), document);
684690
}
685691
continue;
686692
} else if (key.startsWith("$")) {
687693
continue;
694+
} else if (value instanceof Document) {
695+
Document documentValue = (Document) value;
696+
if (documentValue.keySet().equals(Set.of("$eq"))) {
697+
changeSubdocumentValueOrThrow(document, key, documentValue.get("$eq"));
698+
}
688699
}
689700

690701
if (!Utils.containsQueryExpression(value)) {
691-
Utils.changeSubdocumentValue(document, key, value, (AtomicReference<Integer>) null);
702+
changeSubdocumentValueOrThrow(document, key, value);
692703
}
693704
}
694-
return document;
705+
}
706+
707+
private static void changeSubdocumentValueOrThrow(Document document, String key, Object value) {
708+
try {
709+
Object previousValue = Utils.changeSubdocumentValue(document, key, value, (AtomicReference<Integer>) null);
710+
if (previousValue != null) {
711+
throw new MongoServerError(ErrorCode.NotSingleValueField, "cannot infer query fields to set, path '" + key + "' is matched twice");
712+
}
713+
} catch (PathNotViableException e) {
714+
String violatingKey = e.getField();
715+
List<String> keyPathFragments = Utils.splitPath(key);
716+
int indexOfViolatingKey = keyPathFragments.indexOf(violatingKey);
717+
List<String> violatingKeyPathFragments = keyPathFragments.subList(0, indexOfViolatingKey);
718+
String violatingKeyPath = Utils.joinPath(violatingKeyPathFragments);
719+
throw new MongoServerError(ErrorCode.NotSingleValueField, "cannot infer query fields to set, both paths '" + key + "' and '" + violatingKeyPath + "' are matched");
720+
}
695721
}
696722

697723
@Override

‎core/src/main/java/de/bwaldvogel/mongo/backend/FieldUpdates.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
import de.bwaldvogel.mongo.exception.FailedToParseException;
2424
import de.bwaldvogel.mongo.exception.ImmutableFieldException;
2525
import de.bwaldvogel.mongo.exception.MongoServerError;
26-
import de.bwaldvogel.mongo.exception.PathNotViableException;
26+
import de.bwaldvogel.mongo.exception.CannotTraverseElementException;
2727
import de.bwaldvogel.mongo.exception.TypeMismatchException;
2828

2929
class FieldUpdates {
@@ -430,7 +430,7 @@ private void handleRename(String key, Object toField) {
430430
private void applyRenames() {
431431
for (Entry<String, String> entry : renames.entrySet()) {
432432
if (!Utils.canFullyTraverseSubkeyForRename(document, entry.getKey())) {
433-
throw new PathNotViableException("cannot traverse element");
433+
throw new CannotTraverseElementException();
434434
}
435435

436436
Object value = Utils.removeSubdocumentValue(document, entry.getKey(), matchPos);

‎core/src/main/java/de/bwaldvogel/mongo/backend/Utils.java‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -324,23 +324,22 @@ public static void markOkay(Document result) {
324324
result.put("ok", 1.0);
325325
}
326326

327-
private static void setListSafe(Object document, String key, String previousKey, Object obj) {
327+
private static Object setListSafe(Object document, String key, String previousKey, Object obj) {
328328
if (document instanceof List<?>) {
329329
@SuppressWarnings("unchecked")
330330
List<Object> list = ((List<Object>) document);
331331
if (!isNumeric(key)) {
332-
String element = new Document(previousKey, document).toString(true);
333-
throw new PathNotViableException("Cannot create field '" + key + "' in element " + element);
332+
throw new PathNotViableException(key, new Document(previousKey, document));
334333
}
335334
int pos = Integer.parseInt(key);
336335
while (list.size() <= pos) {
337336
list.add(null);
338337
}
339-
list.set(pos, obj);
338+
return list.set(pos, obj);
340339
} else {
341340
@SuppressWarnings("unchecked")
342341
Map<String, Object> documentAsMap = (Map<String, Object>) document;
343-
documentAsMap.put(key, obj);
342+
return documentAsMap.put(key, obj);
344343
}
345344
}
346345

@@ -399,29 +398,27 @@ public static void changeSubdocumentValue(Object document, String key, Object ne
399398
changeSubdocumentValue(document, key, newValue, new AtomicReference<>());
400399
}
401400

402-
static void changeSubdocumentValue(Object document, String key, Object newValue, AtomicReference<Integer> matchPos) {
403-
changeSubdocumentValue(document, key, newValue, null, matchPos);
401+
static Object changeSubdocumentValue(Object document, String key, Object newValue, AtomicReference<Integer> matchPos) {
402+
return changeSubdocumentValue(document, key, newValue, null, matchPos);
404403
}
405404

406-
private static void changeSubdocumentValue(Object document, String key, Object newValue, String previousKey, AtomicReference<Integer> matchPos) {
405+
private static Object changeSubdocumentValue(Object document, String key, Object newValue, String previousKey, AtomicReference<Integer> matchPos) {
407406
List<String> pathFragments = splitPath(key);
408407
String mainKey = pathFragments.get(0);
409408
if (pathFragments.size() == 1) {
410-
setListSafe(document, key, previousKey, newValue);
411-
return;
409+
return setListSafe(document, key, previousKey, newValue);
412410
}
413411
String subKey = getSubkey(pathFragments, matchPos);
414412
Object subObject = getFieldValueListSafe(document, mainKey);
415413
if (subObject instanceof Document || subObject instanceof List<?>) {
416-
changeSubdocumentValue(subObject, subKey, newValue, mainKey, matchPos);
414+
return changeSubdocumentValue(subObject, subKey, newValue, mainKey, matchPos);
417415
} else if (Missing.isNeitherNullNorMissing(subObject)) {
418-
String element = new Document(mainKey, subObject).toString(true);
419416
String subKeyFirst = splitPath(subKey).get(0);
420-
throw new PathNotViableException("Cannot create field '" + subKeyFirst + "' in element " + element);
417+
throw new PathNotViableException(subKeyFirst, new Document(mainKey, subObject));
421418
} else {
422419
Document obj = new Document();
423420
changeSubdocumentValue(obj, subKey, newValue, mainKey, matchPos);
424-
setListSafe(document, mainKey, previousKey, obj);
421+
return setListSafe(document, mainKey, previousKey, obj);
425422
}
426423
}
427424

‎core/src/main/java/de/bwaldvogel/mongo/exception/CannotTraverseElementException.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
package de.bwaldvogel.mongo.exception;
2+
3+
public class CannotTraverseElementException extends MongoServerError {
4+
5+
private static final long serialVersionUID = 1L;
6+
7+
public CannotTraverseElementException() {
8+
super(ErrorCode.PathNotViable, "cannot traverse element");
9+
}
10+
11+
}

‎core/src/main/java/de/bwaldvogel/mongo/exception/ErrorCode.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ public enum ErrorCode {
1212
NamespaceExists(48),
1313
DollarPrefixedFieldName(52),
1414
InvalidIdField(53),
15+
NotSingleValueField(54),
1516
CommandNotFound(59),
1617
ImmutableField(66),
1718
InvalidOptions(72),

‎core/src/main/java/de/bwaldvogel/mongo/exception/PathNotViableException.java‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,19 @@
11
package de.bwaldvogel.mongo.exception;
22

3+
import de.bwaldvogel.mongo.bson.Document;
4+
35
public class PathNotViableException extends MongoServerError {
46

57
private static final long serialVersionUID = 1L;
68

7-
public PathNotViableException(String message) {
8-
super(ErrorCode.PathNotViable, message);
9+
private final String field;
10+
11+
public PathNotViableException(String field, Document element) {
12+
super(ErrorCode.PathNotViable, "Cannot create field '" + field + "' in element " + element.toString(true));
13+
this.field = field;
914
}
1015

16+
public String getField() {
17+
return field;
18+
}
1119
}

‎test-common/src/main/java/de/bwaldvogel/mongo/backend/AbstractBackendTest.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6223,6 +6223,48 @@ void testUpsertWithPositionalAll() throws Exception {
62236223
assertThat(result).isEqualTo(json("_id: 1, a: [1, 1]"));
62246224
}
62256225

6226+
private static Stream<Arguments> upsertWithNonMatchingFilterTestData() {
6227+
return Stream.of(
6228+
Arguments.of("$and: [{key1: {$eq: 'value1'}}, {key2: {$eq: 'value2'}}]", "dummy: 'dummy', key1: 'value1', key2: 'value2'"),
6229+
Arguments.of("$and: [{key1: {$eq: {sub: 'value1'}}}, {key2: {$eq: 'value2'}}]", "dummy: 'dummy', key1: {sub: 'value1'}, key2: 'value2'"),
6230+
Arguments.of("$and: [{key1: {$gt: 1}}, {key2: 'value2'}]", "dummy: 'dummy', key2: 'value2'"),
6231+
Arguments.of("$and: [{'sub.key1': {$eq: 'value1'}}, {'sub.key2': {$eq: 'value2'}}]", "dummy: 'dummy', sub: {key1: 'value1', key2: 'value2'}"),
6232+
Arguments.of("$and: [{'sub.key1': 'value1'}, {'sub.key2': 'value2'}]", "dummy: 'dummy', sub: {key1: 'value1', key2: 'value2'}"),
6233+
Arguments.of("$or: [{'sub.key1': 'value1'}, {'sub.key1': 'value1b'}]", "dummy: 'dummy'"),
6234+
Arguments.of("$or: [{'sub.key1': 'value1'}]", "dummy: 'dummy', sub: {key1: 'value1'}"),
6235+
Arguments.of("$or: [{key1: 'value1'}, {key2: 'value2'}]", "dummy: 'dummy'")
6236+
);
6237+
}
6238+
6239+
// https://github.com/bwaldvogel/mongo-java-server/issues/216
6240+
@ParameterizedTest
6241+
@MethodSource("upsertWithNonMatchingFilterTestData")
6242+
void testUpsertAndFilterDoesNotMatch(String filter, String expectedDocument) throws Exception {
6243+
collection.updateOne(json(filter),
6244+
json("$set: {dummy: 'dummy'}"), new UpdateOptions().upsert(true));
6245+
6246+
assertThat(collection.find().projection(json("_id: 0")))
6247+
.containsExactly(json(expectedDocument));
6248+
}
6249+
6250+
private static Stream<Arguments> upsertWithIllegalFilterTestData() {
6251+
return Stream.of(
6252+
Arguments.of("$and: [{key1: 'value1'}, {key1: 'value1b'}, {key2: 'value2'}]", "cannot infer query fields to set, path 'key1' is matched twice"),
6253+
Arguments.of("$and: [{sub: 'value1'}, {'sub.key': 'conflict'}]", "cannot infer query fields to set, both paths 'sub.key' and 'sub' are matched"),
6254+
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"),
6255+
Arguments.of("$and: [{'sub': 'value1'}, {'sub.a.b': 'conflict'}]", "cannot infer query fields to set, both paths 'sub.a.b' and 'sub' are matched"),
6256+
Arguments.of("$and: [{'sub.key1': 'value1'}, {'sub.key1': 'value1b'}]", "cannot infer query fields to set, path 'sub.key1' is matched twice")
6257+
);
6258+
}
6259+
6260+
@ParameterizedTest
6261+
@MethodSource("upsertWithIllegalFilterTestData")
6262+
void testUpsertWithIllegalFilter(String filter, String expectedErrorMessage) throws Exception {
6263+
assertMongoWriteException(() -> {
6264+
collection.updateOne(json(filter), json("$set: {dummy: 'dummy'}"), new UpdateOptions().upsert(true));
6265+
}, 54, "NotSingleValueField", expectedErrorMessage);
6266+
}
6267+
62266268
// https://github.com/bwaldvogel/mongo-java-server/issues/164
62276269
@Test
62286270
void testFindOneAndUpdateWithReturnDocumentBeforeWhenDocumentDidNotExist() throws Exception {

0 commit comments

Comments
 (0)