mongo updateimpl: check update, not field for updates

[benjones]

I'm not sure about this fix since I'm not at all a mongo expert, but this does seem in line with the documentation for what update commands should look like. Figured this was a good starting point.

[benjones]

Related to this: https://forum.dlang.org/post/abszouhaybgstqdbatyl@forum.dlang.org

@WebFreak001 seems like you probably know this part of the code better than anyone else

[WebFreak001]

ah thank you! Actually it looks like the entire check before that is useless then, since it should warn the developer if they do something wrong. And if it only runs on the query and not on the replacement, there is no point to it. It should also use the ubson instead of qbson.

Can you maybe also add a MongoDB test for this? (there is a test folder that runs tests with multiple different MongoDB versions)

[benjones]

To be clear the "mustBeDocument" check is useless? I honestly know very little about the mongo stuff :)

Yeah, I can add a test. I think this means that there's no tests for the mongo session store, since it uses a very similar updateOne call to my issue

[benjones]

I added a test, but either I didn't fix the issue, or I wrote the test wrong... any ideas?

[WebFreak001]

you forgot to pass the options to the updateOne command, so it never upserts!

[benjones]

ugh, thank you. Tests not quite ready, but that should get me unstuck.

[benjones]

OK, now it looks like the collections.d line 386 is expecting the wrong format for upsert responses. It looks like it expects an array of IDs, and it's actually getting an array of objects with an ID field... I'm assuming that's also untested/out of date and I should fix it?

[benjones]

OK, I think this is good to go. it worked for my project that had previously been using update

[WebFreak001]

ok looks good, can you further apply this diff to your modifications? There was the same issue in the code that the changed code was based on, which is resolved with this. Furthermore tests that the asserts are actually triggered in the right place.

diff --git a/mongodb/vibe/db/mongo/collection.d b/mongodb/vibe/db/mongo/collection.d
index 190027dd..1fb5c42f 100644
--- a/mongodb/vibe/db/mongo/collection.d
+++ b/mongodb/vibe/db/mongo/collection.d
@@ -326,23 +326,23 @@ struct MongoCollection {
 			auto updateBson = Bson.emptyObject;
 			auto qbson = serializeToBson(q);
 			updateBson["q"] = qbson;
+			auto ubson = serializeToBson(documents[i]);
 			if (mustBeDocument)
 			{
-				if (qbson.type != Bson.Type.object)
+				if (ubson.type != Bson.Type.object)
 					assert(false, "Passed in non-document into a place where only replacements are expected. "
 						~ "Maybe you want to call updateOne or updateMany instead?");
 
-				foreach (string k, v; qbson.byKeyValue)
+				foreach (string k, v; ubson.byKeyValue)
 				{
 					if (k.startsWith("$"))
 						assert(false, "Passed in atomic modifiers (" ~ k
 							~ ") into a place where only replacements are expected. "
 							~ "Maybe you want to call updateOne or updateMany instead?");
-					debug break; // server checks that the rest is consistent (only $ or only non-$ allowed)
-					// however in debug mode we check the full document, as we can give better error messages to the dev
+					debug {} // server checks that the rest is consistent (only $ or only non-$ allowed)
+					else break; // however in debug mode we check the full document, as we can give better error messages to the dev
 				}
 			}
-			auto ubson = serializeToBson(documents[i]);
 			if (mustBeModification)
 			{
 				if (ubson.type == Bson.Type.object)
@@ -352,8 +352,8 @@ struct MongoCollection {
 					{
 						if (k.startsWith("$"))
 							anyDollar = true;
-						debug break; // server checks that the rest is consistent (only $ or only non-$ allowed)
-						// however in debug mode we check the full document, as we can give better error messages to the dev
+						debug {} // server checks that the rest is consistent (only $ or only non-$ allowed)
+						else break; // however in debug mode we check the full document, as we can give better error messages to the dev
 						// also nice side effect: if this is an empty document, this also matches the assert(false) branch.
 					}
 
diff --git a/tests/mongodb/_connection/source/app.d b/tests/mongodb/_connection/source/app.d
index fa79bb82..d8c8577c 100644
--- a/tests/mongodb/_connection/source/app.d
+++ b/tests/mongodb/_connection/source/app.d
@@ -5,6 +5,7 @@ import vibe.core.log;
 import core.time;
 import std.algorithm;
 import std.conv;
+import core.exception : AssertError;
 import std.exception;
 
 int main(string[] args)
@@ -157,20 +158,24 @@ int main(string[] args)
 		.map!(c => c["name"].get!string)
 		.equal(["collection"]));
 
-    auto options = UpdateOptions();
-    options.upsert = true;
+	auto options = UpdateOptions();
+	options.upsert = true;
 
-    //test updateOne
-    auto newID = BsonObjectID.generate;
-    foreach(const str; ["a", "b", "c"])
-    {
-        coll.updateOne(["_id": newID], ["$push" : ["array" : str ]], options);
-    }
+	//test updateOne
+	auto newID = BsonObjectID.generate;
+	foreach(const str; ["a", "b", "c"])
+	{
+		coll.updateOne(["_id": newID], ["$push" : ["array" : str ]], options);
+	}
+
+	auto arrResult = coll.findOne(["_id" : newID])["array"];
+	assert(arrResult[0].get!string =="a");
+	assert(arrResult[1].get!string =="b");
+	assert(arrResult[2].get!string =="c");
 
-    auto arrResult = coll.findOne(["_id" : newID])["array"];
-    assert(arrResult[0].get!string =="a");
-    assert(arrResult[1].get!string =="b");
-    assert(arrResult[2].get!string =="c");
+	assertThrown!AssertError(coll.updateOne(["_id": newID], ["this is a replacement": "which is not allowed here"]));
+	assertThrown!AssertError(coll.replaceOne(["_id": newID], ["$updateOp": "is not allowed here"]));
+	assertThrown!AssertError(coll.replaceOne(["_id": newID], null));
 
 	coll.drop();
 

[WebFreak001]

LGTM, @s-ludwig can you merge?

[benjones]

@s-ludwig ping