From aa06ea77de19e1b5fc0d88ea658aaff25af91dfe Mon Sep 17 00:00:00 2001 From: Tim Taplin Date: Sun, 4 Oct 2026 11:28:40 -0600 Subject: [PATCH] Reject invalid legacy queries; pass count options through; reject multi replace _perfect_split_legacy_query treated a $query that isn't a document as an empty filter, so find() and count() matched every document. It also merged non-$ fields next to $query into the filter. libmongoc 1.x's legacy find rejected both ("Cannot mix $query with non-dollar field", "Invalid BSON in $query subdocument"), and now the shim does too: count() returns the error, find() returns nil (libmongoc 2 can't hand back a cursor carrying the error, and MongoCursor exposes no error), and GridFS list(filter:) throws. A duplicate $query now means the last one wins, as in 1.x; before, the two were merged. (1.x's legacy count never unwrapped $query at all; current servers reject a raw $query in count.) _perfect_collection_count built the legacy find options and threw them away. hint, maxTimeMS, comment and collation now reach count_documents. $orderby, $max, $min and the other find-only modifiers are still ignored, and the doc comment says so. A negative legacy limit is passed as its absolute value, as the old count command treated it; before, the $limit stage rejected it. INT64_MIN, which has no absolute value, is an error. The doc comment also notes that count() can't use $where, $near or $nearSphere, since count_documents runs the query as a $match. update() with .multiUpdate and a replacement document replaced one document and reported success. It now returns an error without writing, with the server's own message for this case ("multi update is not supported for replacement-style update"; the server's code is 9, this is a client-side MONGOC_ERROR_COMMAND_INVALID_ARG). Tests: 4 new tests. 14 of their checks fail on main (the rest are baseline checks that pass either way). The comment check uses the profiler in a database of its own, which it drops, and skips when the server refuses profiling. All 40 tests pass against mongod 8.3.11 on macOS (debug and release) and on Linux (Scripts/test-linux.sh). Co-Authored-By: Claude Opus 5.5 --- Sources/PerfectCMongo/shim.h | 92 +++++++++++---- Sources/PerfectMongoDB/MongoCollection.swift | 6 +- .../PerfectMongoDBTests.swift | 106 ++++++++++++++++++ 3 files changed, 181 insertions(+), 23 deletions(-) diff --git a/Sources/PerfectCMongo/shim.h b/Sources/PerfectCMongo/shim.h index 0f9087d..13bfb2a 100644 --- a/Sources/PerfectCMongo/shim.h +++ b/Sources/PerfectCMongo/shim.h @@ -1,4 +1,5 @@ #include +#include #include 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,6 +133,11 @@ 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, @@ -121,23 +145,39 @@ static int64_t _perfect_collection_count(mongoc_collection_t *collection, 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,6 +227,8 @@ 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, @@ -192,15 +236,21 @@ static bool _perfect_collection_update(mongoc_collection_t *collection, 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); diff --git a/Sources/PerfectMongoDB/MongoCollection.swift b/Sources/PerfectMongoDB/MongoCollection.swift index 3869a19..9848e62 100644 --- a/Sources/PerfectMongoDB/MongoCollection.swift +++ b/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 { diff --git a/Tests/PerfectMongoDBTests/PerfectMongoDBTests.swift b/Tests/PerfectMongoDBTests/PerfectMongoDBTests.swift index 5d6ba43..563b3a5 100644 --- a/Tests/PerfectMongoDBTests/PerfectMongoDBTests.swift +++ b/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) {} }