Legacy query shim edge cases (for 4.0.3)

Sources/PerfectCMongo/shim.h

@@ -1,4 +1,5 @@
 #include <mongoc/mongoc.h>
+#include <stdlib.h>
 #include <string.h>
 
 static bool _mongoc_cursor_next(mongoc_cursor_t *cursor, const void **bson)
@@ -16,40 +17,51 @@ static bool _mongoc_cursor_next(mongoc_cursor_t *cursor, const void **bson)
 // Split a legacy query document into a filter and find options.
 // { $query: {...}, $orderby: {...}, $hint: ... } becomes filter {...} and opts { sort: {...}, hint: ... }.
 // A query without $query is copied into filter unchanged.
-static void _perfect_split_legacy_query(const bson_t *query, bson_t *filter, bson_t *opts)
+// Like libmongoc 1.x, a $query that isn't a document, or a non-$ field next to $query, is an error.
+static bool _perfect_split_legacy_query(const bson_t *query, bson_t *filter, bson_t *opts, bson_error_t *error)
 {
 	bson_iter_t iter;
 	if (!query || !bson_iter_init_find(&iter, query, "$query")) {
 		if (query) {
 			bson_concat(filter, query);
 		}
-		return;
+		return true;
 	}
 	bson_iter_init(&iter, query);
 	while (bson_iter_next(&iter)) {
 		const char *key = bson_iter_key(&iter);
+		if (key[0] != '$') {
+			bson_set_error(error, MONGOC_ERROR_CURSOR, MONGOC_ERROR_CURSOR_INVALID_CURSOR,
+						   "Cannot mix $query with non-dollar field '%s'", key);
+			return false;
+		}
 		if (strcmp(key, "$query") == 0) {
-			if (BSON_ITER_HOLDS_DOCUMENT(&iter)) {
-				uint32_t len = 0;
-				const uint8_t *data = NULL;
-				bson_t sub;
-				bson_iter_document(&iter, &len, &data);
-				if (bson_init_static(&sub, data, len)) {
-					bson_concat(filter, &sub);
-				}
+			uint32_t len = 0;
+			const uint8_t *data = NULL;
+			bson_t sub;
+			if (!BSON_ITER_HOLDS_DOCUMENT(&iter)) {
+				bson_set_error(error, MONGOC_ERROR_BSON, MONGOC_ERROR_BSON_INVALID, "$query must be a document");
+				return false;
+			}
+			bson_iter_document(&iter, &len, &data);
+			if (!bson_init_static(&sub, data, len)) {
+				bson_set_error(error, MONGOC_ERROR_BSON, MONGOC_ERROR_BSON_INVALID, "Invalid BSON in $query subdocument");
+				return false;
 			}
+			// As in libmongoc 1.x, the last $query wins.
+			bson_reinit(filter);
+			bson_concat(filter, &sub);
 		} else if (strcmp(key, "$orderby") == 0) {
 			bson_append_iter(opts, "sort", -1, &iter);
 		} else if (strcmp(key, "$showDiskLoc") == 0) {
 			bson_append_iter(opts, "showRecordId", -1, &iter);
 		} else if (strcmp(key, "$explain") == 0 || strcmp(key, "$snapshot") == 0) {
 			// no longer supported by the server
-		} else if (key[0] == '$') {
-			bson_append_iter(opts, key + 1, -1, &iter);
 		} else {
-			bson_append_iter(filter, key, -1, &iter);
+			bson_append_iter(opts, key + 1, -1, &iter);
 		}
 	}
+	return true;
 }
 
 // Convert legacy query flags to find options. Returns true when MONGOC_QUERY_SECONDARY_OK is set.
@@ -88,8 +100,15 @@ static mongoc_cursor_t *_perfect_collection_find(mongoc_collection_t *collection
 	bson_t opts = BSON_INITIALIZER;
 	mongoc_read_prefs_t *prefs = NULL;
 	mongoc_cursor_t *cursor;
+	bson_error_t error;
 
-	_perfect_split_legacy_query(query, &filter, &opts);
+	// libmongoc 2.x can't hand back a cursor that carries this error, so an invalid legacy query
+	// returns NULL (find() returns nil).
+	if (!_perfect_split_legacy_query(query, &filter, &opts, &error)) {
+		bson_destroy(&filter);
+		bson_destroy(&opts);
+		return NULL;
+	}
 	if (_perfect_flags_to_opts(flags, &opts)) {
 		prefs = mongoc_read_prefs_new(MONGOC_READ_SECONDARY_PREFERRED);
 	}
@@ -114,30 +133,51 @@ static mongoc_cursor_t *_perfect_collection_find(mongoc_collection_t *collection
 	return cursor;
 }
 
+// Legacy count on top of count_documents, which runs { $match: filter } in an aggregate.
+// From the legacy query only hint, maxTimeMS, comment and collation carry over; $orderby, $max,
+// $min, $returnKey, $showDiskLoc and the other find-only options are ignored. Because the filter is a $match, it can't use
+// $where, $near or $nearSphere (the server rejects them); use $expr, $geoWithin with $center,
+// or $geoWithin with $centerSphere instead.
 static int64_t _perfect_collection_count(mongoc_collection_t *collection,
 										 mongoc_query_flags_t flags,
 										 const bson_t *query,
 										 int64_t skip,
 										 int64_t limit,
 										 bson_error_t *error)
 {
+	static const char *const passed_opts[] = { "hint", "maxTimeMS", "comment", "collation" };
 	bson_t filter = BSON_INITIALIZER;
 	bson_t find_opts = BSON_INITIALIZER;
 	bson_t opts = BSON_INITIALIZER;
 	mongoc_read_prefs_t *prefs = NULL;
-	int64_t count;
+	bson_iter_t iter;
+	size_t i;
+	int64_t count = -1;
 
-	_perfect_split_legacy_query(query, &filter, &find_opts);
+	if (!_perfect_split_legacy_query(query, &filter, &find_opts, error)) {
+		goto done;
+	}
 	if (_perfect_flags_to_opts(flags, &find_opts)) {
 		prefs = mongoc_read_prefs_new(MONGOC_READ_SECONDARY_PREFERRED);
 	}
+	for (i = 0; i < sizeof(passed_opts) / sizeof(passed_opts[0]); i++) {
+		if (bson_iter_init_find(&iter, &find_opts, passed_opts[i])) {
+			bson_append_iter(&opts, passed_opts[i], -1, &iter);
+		}
+	}
 	if (skip) {
 		BSON_APPEND_INT64(&opts, "skip", skip);
 	}
+	// The legacy count command treated a negative limit like a positive one.
+	if (limit == INT64_MIN) {
+		bson_set_error(error, MONGOC_ERROR_COMMAND, MONGOC_ERROR_COMMAND_INVALID_ARG, "limit cannot be INT64_MIN");
+		goto done;
+	}
 	if (limit) {
-		BSON_APPEND_INT64(&opts, "limit", limit);
+		BSON_APPEND_INT64(&opts, "limit", llabs(limit));
 	}
 	count = mongoc_collection_count_documents(collection, &filter, &opts, prefs, NULL, error);
+done:
 	if (prefs) {
 		mongoc_read_prefs_destroy(prefs);
 	}
@@ -151,10 +191,12 @@ static mongoc_gridfs_file_list_t *_perfect_gridfs_find(mongoc_gridfs_t *gridfs,
 {
 	bson_t filter = BSON_INITIALIZER;
 	bson_t opts = BSON_INITIALIZER;
-	mongoc_gridfs_file_list_t *list;
+	mongoc_gridfs_file_list_t *list = NULL;
+	bson_error_t error;
 
-	_perfect_split_legacy_query(query, &filter, &opts);
-	list = mongoc_gridfs_find_with_opts(gridfs, &filter, &opts);
+	if (_perfect_split_legacy_query(query, &filter, &opts, &error)) {
+		list = mongoc_gridfs_find_with_opts(gridfs, &filter, &opts);
+	}
 	bson_destroy(&filter);
 	bson_destroy(&opts);
 	return list;
@@ -185,22 +227,30 @@ static bool _perfect_collection_insert(mongoc_collection_t *collection,
 }
 
 // Legacy update accepted either an update-operator document or a replacement document.
+// A replacement document can only replace one document, so MULTI_UPDATE with one is an error,
+// as the server reported it for the legacy update.
 static bool _perfect_collection_update(mongoc_collection_t *collection,
 									   mongoc_update_flags_t flags,
 									   const bson_t *selector,
 									   const bson_t *update,
 									   bson_error_t *error)
 {
 	bson_t opts = BSON_INITIALIZER;
+	bool replacement = !_perfect_is_update_document(update);
 	bool ret;
 
+	if (replacement && (flags & MONGOC_UPDATE_MULTI_UPDATE)) {
+		bson_set_error(error, MONGOC_ERROR_COMMAND, MONGOC_ERROR_COMMAND_INVALID_ARG,
+					   "multi update is not supported for replacement-style update");
+		return false;
+	}
 	if (flags & MONGOC_UPDATE_UPSERT) {
 		BSON_APPEND_BOOL(&opts, "upsert", true);
 	}
 	if (flags & MONGOC_UPDATE_NO_VALIDATE) {
 		BSON_APPEND_BOOL(&opts, "validate", false);
 	}
-	if (!_perfect_is_update_document(update)) {
+	if (replacement) {
 		ret = mongoc_collection_replace_one(collection, selector, update, &opts, NULL, error);
 	} else if (flags & MONGOC_UPDATE_MULTI_UPDATE) {
 		ret = mongoc_collection_update_many(collection, selector, update, &opts, NULL, error);

Sources/PerfectMongoDB/MongoCollection.swift

@@ -322,7 +322,7 @@ public class MongoCollection {
 	*
 	*  - parameter selector: BSON document with selection criteria
 	*  - parameter update: BSON document to be used to update
-	*  - parameter flag: Optional MongoUpdateFlag defaults to .None
+	*  - parameter flag: Optional MongoUpdateFlag defaults to .None. `.multiUpdate` needs an update-operator document such as `{ $set: ... }`; with a replacement document it returns an error.
 	*
 	*  - returns: Result object with status of update
 	*/
@@ -555,7 +555,7 @@ public class MongoCollection {
      *  - parameter limit:    Optional. return no more than the supplied number of records.
      *  - parameter batchSize:    Optional. Change number of automatically iterated documents.
      *
-     *  - returns:	A cursor to the documents that match the query criteria. When the find() method “returns documents,” the method is actually returning a cursor to the documents.
+     *  - returns:	A cursor to the documents that match the query criteria. When the find() method “returns documents,” the method is actually returning a cursor to the documents. nil when a legacy `$query` query is invalid: `$query` isn't a document, or it's mixed with fields that don't start with `$`.
     */
     public func find(query: BSON = BSON(), fields: BSON? = nil, flags: MongoQueryFlag = MongoQueryFlag.none, skip: Int = 0, limit: Int = 0, batchSize: Int = 0) -> MongoCursor? {
 		//	@available(*, deprecated, message: "Use find(filter: BSON, options: BSON?)")
@@ -672,6 +672,8 @@ public class MongoCollection {
      *  - parameter batchSize:    Optional. Change number of automatically iterated documents.
      *
      *  - returns: the count of documents that would match a find() query. The count() method does not perform the find() operation but instead counts and returns the number of results that match a query.
+     *
+     *  The count runs as an aggregate `$match`, so the query can't use `$where`, `$near` or `$nearSphere`; use `$expr`, or `$geoWithin` with `$center` / `$centerSphere`. In a legacy `$query` query, `$hint`, `$maxTimeMS`, `$comment` and `$collation` apply to the count; other modifiers such as `$orderby`, `$max` and `$min` are ignored. A negative `limit` counts like a positive one.
      */
     public func count(query: BSON, flags: MongoQueryFlag = MongoQueryFlag.none, skip: Int = 0, limit: Int = 0, batchSize: Int = 0) -> Result {
         guard let ptr = self.ptr else {

Tests/PerfectMongoDBTests/PerfectMongoDBTests.swift

@@ -988,6 +988,112 @@ class PerfectMongoDBTests: XCTestCase {
 		XCTAssertEqual(count(collection), 0)
 	}
 
+	private func assertCountFails(_ collection: MongoCollection, _ json: String, containing text: String? = nil, file: StaticString = #filePath, line: UInt = #line) {
+		switch collection.count(query: try! BSON(json: json)) {
+		case .error(_, _, let message):
+			if let text {
+				XCTAssert(message.contains(text), message, file: file, line: line)
+			}
+		case let other:
+			XCTFail("count(\(json)) should fail, got \(other)", file: file, line: line)
+		}
+	}
+
+	func testInvalidLegacyQueriesAreRejected() {
+		let (client, collection) = freshCollection("testinvalidlegacy")
+		defer { withExtendedLifetime(client) {} }
+		defer { _ = collection.drop() }
+
+		// A $query that isn't a document used to match every document.
+		let notDocument = "{\"$query\": 5}"
+		XCTAssertNil(collection.find(query: try! BSON(json: notDocument)))
+		assertCountFails(collection, notDocument, containing: "$query must be a document")
+
+		// A non-$ field next to $query used to be merged into the filter.
+		let mixed = "{\"$query\": {\"a\": 1}, \"b\": \"value 1\"}"
+		XCTAssertNil(collection.find(query: try! BSON(json: mixed)))
+		assertCountFails(collection, mixed, containing: "Cannot mix $query with non-dollar field 'b'")
+
+		let gridfs = try! client.gridFS(database: "test")
+		defer { gridfs.close() }
+		XCTAssertThrowsError(try gridfs.list(filter: try! BSON(json: notDocument)))
+
+		// Well-formed legacy queries still work.
+		let valid = "{\"$query\": {\"a\": {\"$gte\": 2}}, \"$orderby\": {\"a\": 1}}"
+		XCTAssertEqual(collection.find(query: try! BSON(json: valid))?.map { $0 }.count, 2)
+		XCTAssertEqual(count(collection, valid), 2)
+	}
+
+	func testLegacyCountOptions() {
+		let (client, collection) = freshCollection("testlegacycount")
+		defer { withExtendedLifetime(client) {} }
+		defer { _ = collection.drop() }
+
+		// A negative legacy limit counts like a positive one.
+		if case .replyInt(let limited) = collection.count(query: BSON(), limit: -2) {
+			XCTAssertEqual(limited, 2)
+		} else {
+			XCTFail("count with a negative limit failed")
+		}
+
+		// hint reaches the server: an unknown index is an error.
+		assertCountFails(collection, "{\"$query\": {}, \"$hint\": \"no_such_index\"}")
+		XCTAssertEqual(count(collection, "{\"$query\": {}, \"$hint\": {\"_id\": 1}}"), 3)
+
+		// maxTimeMS reaches the server: a negative value is rejected.
+		assertCountFails(collection, "{\"$query\": {}, \"$maxTimeMS\": -1}")
+
+		// collation reaches the server: case-insensitive match.
+		XCTAssertEqual(count(collection, "{\"$query\": {\"b\": \"VALUE 1\"}}"), 0)
+		XCTAssertEqual(count(collection, "{\"$query\": {\"b\": \"VALUE 1\"}, \"$collation\": {\"locale\": \"en\", \"strength\": 2}}"), 1)
+
+		// Duplicate $query: the last one wins, as in libmongoc 1.x.
+		XCTAssertEqual(count(collection, "{\"$query\": {\"a\": 1}, \"$query\": {\"a\": 2}}"), 1)
+		XCTAssertEqual(count(collection, "{\"$query\": {\"a\": 1}, \"$query\": {\"b\": \"value 2\"}}"), 1)
+
+		// Int.min has no absolute value; it's rejected before llabs.
+		if case .error(_, _, let message) = collection.count(query: BSON(), limit: Int.min) {
+			XCTAssert(message.contains("INT64_MIN"), message)
+		} else {
+			XCTFail("count with limit Int.min should fail")
+		}
+	}
+
+	func testLegacyCountComment() throws {
+		// Checked through the profiler, in a database of its own so the test's profiling
+		// level and profile entries don't touch the shared test database.
+		let client = try MongoClient(uri: testURI)
+		let db = client.getDatabase(name: "perfect_test_count_comment")
+		defer { _ = db.drop() }
+		guard case .replyCollection(let collection) = db.createCollection(name: "c", options: nil) else {
+			return XCTFail("could not create collection")
+		}
+		guard case .success = collection.insert(document: try BSON(json: "{\"a\": 1}")) else {
+			return XCTFail("insert failed")
+		}
+		guard case .replyDoc = collection.runCommand(try BSON(json: "{\"profile\": 2}")) else {
+			throw XCTSkip("profiling isn't available on this server")
+		}
+		defer { _ = collection.runCommand(try! BSON(json: "{\"profile\": 0}")) }
+		let comment = "perfect-count-\(UUID().uuidString)"
+		XCTAssertEqual(count(collection, "{\"$query\": {}, \"$comment\": \"\(comment)\"}"), 1)
+		let profile = client.getCollection(databaseName: "perfect_test_count_comment", collectionName: "system.profile")
+		XCTAssertEqual(profile.find(query: try BSON(json: "{\"command.comment\": \"\(comment)\"}"))?.map { $0 }.count, 1)
+	}
+
+	func testMultiUpdateWithReplacementIsRejected() {
+		let (client, collection) = freshCollection("testmultireplace")
+		defer { withExtendedLifetime(client) {} }
+		defer { _ = collection.drop() }
+
+		guard case .error(_, _, let message) = collection.update(selector: try! BSON(json: "{}"), update: try! BSON(json: "{\"replaced\": true}"), flag: .multiUpdate) else {
+			return XCTFail("multi update with a replacement document succeeded")
+		}
+		XCTAssert(message.contains("replacement"), message)
+		XCTAssertEqual(count(collection, "{\"replaced\": true}"), 0)
+		XCTAssertEqual(count(collection, "{\"a\": {\"$exists\": true}}"), 3)
+	}
+
 	func testBulkWrites() {
 		let (client, collection) = freshCollection("testbulkwrites")
 		defer { withExtendedLifetime(client) {} }