diff options
19 files changed, 180 insertions, 105 deletions
diff --git a/jstests/core/apply_ops1.js b/jstests/core/apply_ops1.js index 9ecaef4c763..86754d7b9ba 100644 --- a/jstests/core/apply_ops1.js +++ b/jstests/core/apply_ops1.js @@ -457,7 +457,7 @@ res = assert.commandFailed(db.adminCommand({ } ] })); -assert.eq(res.code, 4772604); +assert.eq(res.code, 4772600); // When we explicitly specify {$v: 1}, we should get 'UpdateNode' update semantics, and $set // operations get performed in lexicographic order. @@ -503,7 +503,7 @@ res = assert.commandFailed(db.adminCommand({ } ] })); -assert.eq(res.code, 4772604); +assert.eq(res.code, 4772600); var insert_op1 = {_id: 13, x: 'inserted apply ops1'}; var insert_op2 = {_id: 14, x: 'inserted apply ops2'}; diff --git a/jstests/replsets/update_with_dollar_fields.js b/jstests/replsets/update_with_dollar_fields.js new file mode 100644 index 00000000000..9ecb8a4399c --- /dev/null +++ b/jstests/replsets/update_with_dollar_fields.js @@ -0,0 +1,50 @@ +/** + * Tests that replacement style update with $v field in the document is correctly applied. + * @tags: [ + * requires_fcv_60, + * ] + */ + +(function() { +"use strict"; + +const rst = new ReplSetTest({nodes: 2}); +rst.startSet(); +rst.initiate(); + +const dbName = 'testDb'; +const collName = 'testColl'; +const primary = rst.getPrimary(); +const coll = primary.getDB(dbName).getCollection(collName); +const oplog = primary.getDB('local').getCollection('oplog.rs'); + +function assertLastUpdateOplogEntryIsReplacement() { + const lastUpdate = oplog.find({op: 'u'}).sort({$natural: -1}).limit(1).next(); + assert(lastUpdate.o._id); +} + +[true].forEach($v => { + const _id = assert.commandWorked(coll.insertOne({$v})).insertedId; + assert.commandWorked(coll.update({_id}, [{$set: {p: 1, q: 1}}])); + assertLastUpdateOplogEntryIsReplacement(); +}); + +[true, "hello", 0, 1, 2, 3].forEach($v => { + const _id = assert.commandWorked(coll.insertOne({})).insertedId; + assert.commandWorked(coll.update( + {_id}, + [{$replaceWith: {"$setField": {field: {$literal: "$v"}, input: "$$ROOT", value: $v}}}])); + assertLastUpdateOplogEntryIsReplacement(); +}); + +(function() { +const _id = assert.commandWorked(coll.insertOne({})).insertedId; +assert.commandWorked(coll.update( + {_id}, + [{$replaceWith: {"$setField": {field: {$literal: "$set"}, input: "$$ROOT", value: {a: 1}}}}])); +assertLastUpdateOplogEntryIsReplacement(); +})(); + +rst.awaitReplication(); +rst.stopSet(); +}()); diff --git a/jstests/sharding/update_sharded.js b/jstests/sharding/update_sharded.js index ee9ec595bb6..73da8316f4e 100644 --- a/jstests/sharding/update_sharded.js +++ b/jstests/sharding/update_sharded.js @@ -40,6 +40,14 @@ for (let i = 0; i < 2; i++) { assert.commandWorked(coll.update({_id: 2, key: 2}, {key: 2, foo: 'bar'}, {upsert: true})); assert.commandWorked(coll.update({_id: 3, key: 3}, {$set: {foo: 'bar'}}, {upsert: true})); + // Mixing operator & non-operator fields in updates is not allowed. + assert.commandFailedWithCode( + coll.update({_id: 4, key: 4}, {key: 4, $baz: {foo: 'bar'}}, {upsert: true}), + ErrorCodes.UnsupportedFormat); + assert.commandFailedWithCode( + coll.update({_id: 5, key: 5}, {$baz: {foo: 'bar'}, key: 5}, {upsert: true}), + ErrorCodes.UnsupportedFormat); + assert.eq(coll.count(), 3, "count A"); assert.eq(coll.findOne({_id: 3}).key, 3, "findOne 3 key A"); assert.eq(coll.findOne({_id: 3}).foo, 'bar', "findOne 3 foo A"); diff --git a/src/mongo/db/ops/write_ops.cpp b/src/mongo/db/ops/write_ops.cpp index b2b0758b284..c4b6d5bf54a 100644 --- a/src/mongo/db/ops/write_ops.cpp +++ b/src/mongo/db/ops/write_ops.cpp @@ -134,6 +134,11 @@ int32_t getStmtIdForWriteAt(const WriteCommandRequestBase& writeCommandBase, siz return kFirstStmtId + writePos; } +bool isClassicalUpdateReplacement(const BSONObj& update) { + // An empty update object will be treated as replacement as firstElementFieldName() returns "". + return update.firstElementFieldName()[0] != '$'; +} + } // namespace write_ops write_ops::InsertCommandRequest InsertOp::parse(const OpMsgRequest& request) { @@ -223,15 +228,20 @@ void DeleteOp::validate(const DeleteCommandRequest& deleteOp) { write_ops::UpdateModification write_ops::UpdateModification::parseFromOplogEntry( const BSONObj& oField, const DiffOptions& options) { BSONElement vField = oField[kUpdateOplogEntryVersionFieldName]; + BSONElement idField = oField["_id"]; - // If this field appears it should be an integer. + // If _id field is present, we're getting a replacement style update in which $v can be a user + // field. Otherwise, $v field has to be either missing or be one of the version flag $v:1 / + // $v:2. uassert(4772600, - str::stream() << "Expected $v field to be missing or an integer, but got type: " - << vField.type(), - !vField.ok() || - (vField.type() == BSONType::NumberInt || vField.type() == BSONType::NumberLong)); - - if (vField.ok() && vField.numberInt() == static_cast<int>(UpdateOplogEntryVersion::kDeltaV2)) { + str::stream() << "Expected _id field or $v field missing or $v:1/$v:2, but got: " + << vField, + idField.ok() || !vField.ok() || + vField.numberInt() == static_cast<int>(UpdateOplogEntryVersion::kUpdateNodeV1) || + vField.numberInt() == static_cast<int>(UpdateOplogEntryVersion::kDeltaV2)); + + if (!idField.ok() && vField.ok() && + vField.numberInt() == static_cast<int>(UpdateOplogEntryVersion::kDeltaV2)) { // Make sure there's a diff field. BSONElement diff = oField[update_oplog_entry::kDiffObjectFieldName]; uassert(4772601, @@ -240,18 +250,11 @@ write_ops::UpdateModification write_ops::UpdateModification::parseFromOplogEntry diff.type() == BSONType::Object); return UpdateModification(doc_diff::Diff{diff.embeddedObject()}, options); - } else if (!vField.ok() || - vField.numberInt() == static_cast<int>(UpdateOplogEntryVersion::kUpdateNodeV1)) { + } else { // Treat it as a "classic" update which can either be a full replacement or a - // modifier-style update. Which variant it is will be determined when the update driver is - // constructed. - return UpdateModification(oField, ClassicTag{}); + // modifier-style update. Use "_id" field to determine whether which style it is. + return UpdateModification(oField, ClassicTag{}, idField.ok()); } - - // The $v field must be present, but have some unsupported value. - uasserted(4772604, - str::stream() << "Unrecognized value for '$v' (Version) field: " - << vField.numberInt()); } write_ops::UpdateModification::UpdateModification(doc_diff::Diff diff, DiffOptions options) @@ -263,7 +266,7 @@ write_ops::UpdateModification::UpdateModification(TransformFunc transform) write_ops::UpdateModification::UpdateModification(BSONElement update) { const auto type = update.type(); if (type == BSONType::Object) { - _update = ClassicUpdate{update.Obj()}; + _update = UpdateModification(update.Obj(), ClassicTag{})._update; return; } @@ -274,19 +277,24 @@ write_ops::UpdateModification::UpdateModification(BSONElement update) { _update = PipelineUpdate{parsePipelineFromBSON(update)}; } -write_ops::UpdateModification::UpdateModification(const BSONObj& update, ClassicTag) { - // Do a sanity check that the $v field is either not provided or has value of 1. - const auto versionElem = update["$v"]; - uassert(4772602, - str::stream() << "Expected classic update either contain no '$v' field, or " - << "'$v' field with value 1, but found: " << versionElem, - !versionElem.ok() || - versionElem.numberInt() == - static_cast<int>(UpdateOplogEntryVersion::kUpdateNodeV1)); - - _update = ClassicUpdate{update}; +// If we know whether the update is a replacement, use that value. For example, when we're parsing +// the oplog entry, we know if the update is a replacement by checking whether there's an _id field. +write_ops::UpdateModification::UpdateModification(const BSONObj& update, + ClassicTag, + bool isReplacement) { + if (isReplacement) { + _update = ReplacementUpdate{update}; + } else { + _update = ModifierUpdate{update}; + } } +// If we don't know whether the update is a replacement, for example while we are parsing a user +// request, we infer this by checking whether the first element is a $-field to distinguish modifier +// style updates. +write_ops::UpdateModification::UpdateModification(const BSONObj& update, ClassicTag) + : UpdateModification(update, ClassicTag{}, isClassicalUpdateReplacement(update)) {} + write_ops::UpdateModification::UpdateModification(std::vector<BSONObj> pipeline) : _update{PipelineUpdate{std::move(pipeline)}} {} @@ -301,7 +309,8 @@ write_ops::UpdateModification write_ops::UpdateModification::parseFromBSON(BSONE int write_ops::UpdateModification::objsize() const { return stdx::visit( visit_helper::Overloaded{ - [](const ClassicUpdate& classic) -> int { return classic.bson.objsize(); }, + [](const ReplacementUpdate& replacement) -> int { return replacement.bson.objsize(); }, + [](const ModifierUpdate& modifier) -> int { return modifier.bson.objsize(); }, [](const PipelineUpdate& pipeline) -> int { int size = 0; std::for_each(pipeline.begin(), pipeline.end(), [&size](const BSONObj& obj) { @@ -319,7 +328,8 @@ int write_ops::UpdateModification::objsize() const { write_ops::UpdateModification::Type write_ops::UpdateModification::type() const { return stdx::visit( visit_helper::Overloaded{ - [](const ClassicUpdate& classic) { return Type::kClassic; }, + [](const ReplacementUpdate& replacement) { return Type::kReplacement; }, + [](const ModifierUpdate& modifier) { return Type::kModifier; }, [](const PipelineUpdate& pipelineUpdate) { return Type::kPipeline; }, [](const DeltaUpdate& delta) { return Type::kDelta; }, [](const TransformUpdate& transform) { return Type::kTransform; }}, @@ -335,7 +345,12 @@ void write_ops::UpdateModification::serializeToBSON(StringData fieldName, stdx::visit( visit_helper::Overloaded{ - [fieldName, bob](const ClassicUpdate& classic) { *bob << fieldName << classic.bson; }, + [fieldName, bob](const ReplacementUpdate& replacement) { + *bob << fieldName << replacement.bson; + }, + [fieldName, bob](const ModifierUpdate& modifier) { + *bob << fieldName << modifier.bson; + }, [fieldName, bob](const PipelineUpdate& pipeline) { BSONArrayBuilder arrayBuilder(bob->subarrayStart(fieldName)); for (auto&& stage : pipeline) { diff --git a/src/mongo/db/ops/write_ops_parsers.h b/src/mongo/db/ops/write_ops_parsers.h index 4a70bfdc0da..797af279df7 100644 --- a/src/mongo/db/ops/write_ops_parsers.h +++ b/src/mongo/db/ops/write_ops_parsers.h @@ -79,7 +79,7 @@ repl::OpTime opTimeParser(BSONElement elem); class UpdateModification { public: - enum class Type { kClassic, kPipeline, kDelta, kTransform }; + enum class Type { kReplacement, kModifier, kPipeline, kDelta, kTransform }; using TransformFunc = std::function<boost::optional<BSONObj>(const BSONObj&)>; /** @@ -109,6 +109,7 @@ public: // Creates an transform-style update. The transform function MUST preserve the _id element. UpdateModification(TransformFunc transform); // This constructor exists only to provide a fast-path for constructing classic-style updates. + UpdateModification(const BSONObj& update, ClassicTag, bool isReplacement); UpdateModification(const BSONObj& update, ClassicTag); /** @@ -129,9 +130,14 @@ public: Type type() const; - BSONObj getUpdateClassic() const { - invariant(type() == Type::kClassic); - return stdx::get<ClassicUpdate>(_update).bson; + BSONObj getUpdateReplacement() const { + invariant(type() == Type::kReplacement); + return stdx::get<ReplacementUpdate>(_update).bson; + } + + BSONObj getUpdateModifier() const { + invariant(type() == Type::kModifier); + return stdx::get<ModifierUpdate>(_update).bson; } const std::vector<BSONObj>& getUpdatePipeline() const { @@ -159,8 +165,11 @@ public: stdx::visit( visit_helper::Overloaded{ - [&sb](const ClassicUpdate& classic) { - sb << "{type: Classic, update: " << classic.bson << "}"; + [&sb](const ReplacementUpdate& replacement) { + sb << "{type: Replacement, update: " << replacement.bson << "}"; + }, + [&sb](const ModifierUpdate& modifier) { + sb << "{type: Modifier, update: " << modifier.bson << "}"; }, [&sb](const PipelineUpdate& pipeline) { sb << "{type: Pipeline, update: " << Value(pipeline).toString() << "}"; @@ -176,7 +185,10 @@ public: private: // Wrapper class used to avoid having a variant where multiple alternatives have the same type. - struct ClassicUpdate { + struct ReplacementUpdate { + BSONObj bson; + }; + struct ModifierUpdate { BSONObj bson; }; using PipelineUpdate = std::vector<BSONObj>; @@ -187,7 +199,8 @@ private: struct TransformUpdate { TransformFunc transform; }; - stdx::variant<ClassicUpdate, PipelineUpdate, DeltaUpdate, TransformUpdate> _update; + stdx::variant<ReplacementUpdate, ModifierUpdate, PipelineUpdate, DeltaUpdate, TransformUpdate> + _update; }; } // namespace write_ops diff --git a/src/mongo/db/ops/write_ops_parsers_test.cpp b/src/mongo/db/ops/write_ops_parsers_test.cpp index 7fd55f1ef32..dff6ab2cb0b 100644 --- a/src/mongo/db/ops/write_ops_parsers_test.cpp +++ b/src/mongo/db/ops/write_ops_parsers_test.cpp @@ -311,8 +311,8 @@ TEST(CommandWriteOpsParsers, UpdateCommandRequest) { ASSERT_BSONOBJ_EQ(op.getUpdates()[0].getQ(), query); const auto& updateMod = op.getUpdates()[0].getU(); - ASSERT(updateMod.type() == write_ops::UpdateModification::Type::kClassic); - ASSERT_BSONOBJ_EQ(updateMod.getUpdateClassic(), update); + ASSERT(updateMod.type() == write_ops::UpdateModification::Type::kModifier); + ASSERT_BSONOBJ_EQ(updateMod.getUpdateModifier(), update); ASSERT_BSONOBJ_EQ(write_ops::collationOf(op.getUpdates()[0]), collation); ASSERT_EQ(write_ops::arrayFiltersOf(op.getUpdates()[0]).size(), 1u); diff --git a/src/mongo/db/pipeline/document_source_merge.cpp b/src/mongo/db/pipeline/document_source_merge.cpp index 0715ac8e65a..80daf31c772 100644 --- a/src/mongo/db/pipeline/document_source_merge.cpp +++ b/src/mongo/db/pipeline/document_source_merge.cpp @@ -136,7 +136,7 @@ MergeStrategy makeInsertStrategy() { // The batch stores replacement style updates, but for this "insert" style of $merge we'd // like to just insert the new document without attempting any sort of replacement. std::transform(batch.begin(), batch.end(), objectsToInsert.begin(), [](const auto& obj) { - return std::get<UpdateModification>(obj).getUpdateClassic(); + return std::get<UpdateModification>(obj).getUpdateReplacement(); }); uassertStatusOK(expCtx->mongoProcessInterface->insert( expCtx, ns, std::move(objectsToInsert), wc, epoch)); @@ -151,7 +151,7 @@ BatchTransform makeUpdateTransform(const std::string& updateOp) { return [updateOp](auto& batch) { for (auto&& obj : batch) { std::get<UpdateModification>(obj) = UpdateModification::parseFromClassicUpdate( - BSON(updateOp << std::get<UpdateModification>(obj).getUpdateClassic())); + BSON(updateOp << std::get<UpdateModification>(obj).getUpdateReplacement())); } }; } diff --git a/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp b/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp index 0d1f3189c66..cf5dbc04b55 100644 --- a/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp +++ b/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp @@ -226,8 +226,8 @@ protected: ASSERT_EQ(itExpected->getUpsert(), itActual->getUpsert()); ASSERT_EQ(itExpected->getMulti(), itActual->getMulti()); ASSERT_BSONOBJ_EQ(itExpected->getQ(), itActual->getQ()); - ASSERT_BSONOBJ_EQ(itExpected->getU().getUpdateClassic(), - itActual->getU().getUpdateClassic()); + ASSERT_BSONOBJ_EQ(itExpected->getU().getUpdateReplacement(), + itActual->getU().getUpdateReplacement()); } BatchedCommandResponse response; @@ -268,8 +268,8 @@ protected: ASSERT_EQ(itExpected->getUpsert(), itActual->getUpsert()); ASSERT_EQ(itExpected->getMulti(), itActual->getMulti()); ASSERT_BSONOBJ_EQ(itExpected->getQ(), itActual->getQ()); - ASSERT_BSONOBJ_EQ(itExpected->getU().getUpdateClassic(), - itActual->getU().getUpdateClassic()); + ASSERT_BSONOBJ_EQ(itExpected->getU().getUpdateReplacement(), + itActual->getU().getUpdateReplacement()); } return statusToReturn; diff --git a/src/mongo/db/s/dist_lock_catalog_replset_test.cpp b/src/mongo/db/s/dist_lock_catalog_replset_test.cpp index 780e82f6d88..aedba30df96 100644 --- a/src/mongo/db/s/dist_lock_catalog_replset_test.cpp +++ b/src/mongo/db/s/dist_lock_catalog_replset_test.cpp @@ -1246,7 +1246,7 @@ TEST_F(DistLockCatalogReplSetTest, BasicUnlockAll) { ASSERT(update.getMulti()); ASSERT_BSONOBJ_EQ(BSON(LocksType::process("processID")), update.getQ()); ASSERT_BSONOBJ_EQ(BSON("$set" << BSON(LocksType::state(LocksType::UNLOCKED))), - update.getU().getUpdateClassic()); + update.getU().getUpdateModifier()); return BSON("ok" << 1); }); diff --git a/src/mongo/db/timeseries/timeseries_update_delete_util.cpp b/src/mongo/db/timeseries/timeseries_update_delete_util.cpp index 91bf4fccc4b..5ed466dfe01 100644 --- a/src/mongo/db/timeseries/timeseries_update_delete_util.cpp +++ b/src/mongo/db/timeseries/timeseries_update_delete_util.cpp @@ -34,12 +34,6 @@ namespace mongo::timeseries { namespace { -/** - * Returns whether the given document is a replacement document. - */ -bool isDocReplacement(const BSONObj& doc) { - return doc.isEmpty() || (doc.firstElementFieldNameStringData().find("$") == std::string::npos); -} /** * Returns whether the given metaField is the first element of the dotted path in the given @@ -157,10 +151,11 @@ write_ops::UpdateModification translateUpdate(const write_ops::UpdateModificatio "Cannot perform an update on a time-series collection using a pipeline update", updateMod.type() != write_ops::UpdateModification::Type::kPipeline); - const auto& document = updateMod.getUpdateClassic(); uassert(ErrorCodes::InvalidOptions, "Cannot perform an update on a time-series collection using a replacement document", - !isDocReplacement(document)); + updateMod.type() != write_ops::UpdateModification::Type::kReplacement); + + const auto& document = updateMod.getUpdateModifier(); // Make a mutable copy of the update document in order to replace all occurrences of the // metaField with "meta". diff --git a/src/mongo/db/timeseries/timeseries_update_delete_util_test.cpp b/src/mongo/db/timeseries/timeseries_update_delete_util_test.cpp index b13e9bc2c02..397e6cb7c41 100644 --- a/src/mongo/db/timeseries/timeseries_update_delete_util_test.cpp +++ b/src/mongo/db/timeseries/timeseries_update_delete_util_test.cpp @@ -60,7 +60,7 @@ protected: BSONObj _translateUpdate(const BSONObj& update) const { return timeseries::translateUpdate( write_ops::UpdateModification::parseFromClassicUpdate(update), _metaField) - .getUpdateClassic(); + .getUpdateModifier(); } BSONObj _translateUpdate(const char* update) const { diff --git a/src/mongo/db/transaction_participant.cpp b/src/mongo/db/transaction_participant.cpp index 6d1e69e92a4..46ff893a3bb 100644 --- a/src/mongo/db/transaction_participant.cpp +++ b/src/mongo/db/transaction_participant.cpp @@ -266,7 +266,9 @@ void updateSessionEntry(OperationContext* opCtx, const LogicalSessionId& sessionId, TxnNumber txnNum) { // Current code only supports replacement update. - dassert(UpdateDriver::isDocReplacement(updateRequest.getUpdateModification())); + dassert(updateRequest.getUpdateModification().type() == + write_ops::UpdateModification::Type::kReplacement); + const auto updateMod = updateRequest.getUpdateModification().getUpdateReplacement(); // TODO SERVER-58243: evaluate whether this is safe or whether acquiring the lock can block. AllowLockAcquisitionOnTimestampedUnitOfWork allowLockAcquisition(opCtx->lockState()); @@ -297,7 +299,6 @@ void updateSessionEntry(OperationContext* opCtx, dassert(idToFetch.fieldNameStringData() == "_id"_sd); auto recordId = indexAccess->findSingle(opCtx, *collection, toUpdateIdDoc); auto startingSnapshotId = opCtx->recoveryUnit()->getSnapshotId(); - const auto updateMod = updateRequest.getUpdateModification().getUpdateClassic(); if (recordId.isNull()) { // Upsert case. diff --git a/src/mongo/db/update/update_driver.cpp b/src/mongo/db/update/update_driver.cpp index 5b4809342be..d76c787be22 100644 --- a/src/mongo/db/update/update_driver.cpp +++ b/src/mongo/db/update/update_driver.cpp @@ -154,12 +154,12 @@ void UpdateDriver::parse( uassert(51198, "Constant values may only be specified for pipeline updates", !constants); // Check if the update expression is a full object replacement. - if (isDocReplacement(updateMod)) { + if (updateMod.type() == write_ops::UpdateModification::Type::kReplacement) { uassert(ErrorCodes::FailedToParse, "multi update is not supported for replacement-style update", !multi); - _updateExecutor = std::make_unique<ObjectReplaceExecutor>(updateMod.getUpdateClassic()); + _updateExecutor = std::make_unique<ObjectReplaceExecutor>(updateMod.getUpdateReplacement()); // Register the fact that this driver will only do full object replacements. _updateType = UpdateType::kReplacement; @@ -185,7 +185,7 @@ void UpdateDriver::parse( // checked whether this is a delta update so we check that the $v field isn't present, or has a // value of 1. - auto updateExpr = updateMod.getUpdateClassic(); + auto updateExpr = updateMod.getUpdateModifier(); BSONElement versionElement = updateExpr[kUpdateOplogEntryVersionFieldName]; if (versionElement) { uassert(ErrorCodes::FailedToParse, @@ -316,10 +316,4 @@ void UpdateDriver::setCollator(const CollatorInterface* collator) { } } -bool UpdateDriver::isDocReplacement(const write_ops::UpdateModification& updateMod) { - return (updateMod.type() == write_ops::UpdateModification::Type::kClassic && - *updateMod.getUpdateClassic().firstElementFieldName() != '$') || - updateMod.type() == write_ops::UpdateModification::Type::kPipeline; -} - } // namespace mongo diff --git a/src/mongo/db/update/update_driver.h b/src/mongo/db/update/update_driver.h index d512d89b1cf..94d1f73c8d4 100644 --- a/src/mongo/db/update/update_driver.h +++ b/src/mongo/db/update/update_driver.h @@ -150,8 +150,6 @@ public: return _updateType; } - static bool isDocReplacement(const write_ops::UpdateModification& updateMod); - bool modsAffectIndices() const { return _affectIndices; } diff --git a/src/mongo/s/catalog/sharding_catalog_client_test.cpp b/src/mongo/s/catalog/sharding_catalog_client_test.cpp index f2b831e24db..63dd880b63f 100644 --- a/src/mongo/s/catalog/sharding_catalog_client_test.cpp +++ b/src/mongo/s/catalog/sharding_catalog_client_test.cpp @@ -1084,7 +1084,7 @@ TEST_F(ShardingCatalogClientTest, UpdateDatabase) { ASSERT(update.getUpsert()); ASSERT(!update.getMulti()); ASSERT_BSONOBJ_EQ(update.getQ(), BSON(DatabaseType::name(dbt.getName()))); - ASSERT_BSONOBJ_EQ(update.getU().getUpdateClassic(), dbt.toBSON()); + ASSERT_BSONOBJ_EQ(update.getU().getUpdateReplacement(), dbt.toBSON()); BatchedCommandResponse response; response.setStatus(Status::OK()); diff --git a/src/mongo/s/chunk_manager_targeter.cpp b/src/mongo/s/chunk_manager_targeter.cpp index f87abb745a9..bdb386420c6 100644 --- a/src/mongo/s/chunk_manager_targeter.cpp +++ b/src/mongo/s/chunk_manager_targeter.cpp @@ -69,7 +69,7 @@ constexpr auto kIdFieldName = "_id"_sd; const ShardKeyPattern kVirtualIdShardKey(BSON(kIdFieldName << 1)); -using UpdateType = ChunkManagerTargeter::UpdateType; +using UpdateType = write_ops::UpdateModification::Type; // Tracks the number of {multi:false} updates with an exact match on _id that are broadcasted to // multiple shards. @@ -85,37 +85,33 @@ ServerStatusMetricField<Counter64> updateOneOpStyleBroadcastWithExactIDStats( * or * coll.update({x: 1}, [{$addFields: {y: 2}}]) */ -UpdateType getUpdateExprType(const write_ops::UpdateOpEntry& updateDoc) { +void validateUpdateDoc(const write_ops::UpdateOpEntry& updateDoc) { const auto& updateMod = updateDoc.getU(); if (updateMod.type() == write_ops::UpdateModification::Type::kPipeline) { - return UpdateType::kOpStyle; + return; } - const auto& updateExpr = updateMod.getUpdateClassic(); - - // Empty update is replacement-style by default. - auto updateType = (updateExpr.isEmpty() ? UpdateType::kReplacement : UpdateType::kUnknown); + const auto updateType = updateMod.type(); + invariant(updateType == UpdateType::kReplacement || updateType == UpdateType::kModifier); + const auto& updateExpr = updateType == UpdateType::kReplacement + ? updateMod.getUpdateReplacement() + : updateMod.getUpdateModifier(); // Make sure that the update expression does not mix $op and non-$op fields. for (const auto& curField : updateExpr) { - const auto curFieldType = - (curField.fieldNameStringData()[0] == '$' ? UpdateType::kOpStyle - : UpdateType::kReplacement); - - // If the current field's type does not match the existing updateType, abort. - if (updateType == UpdateType::kUnknown) - updateType = curFieldType; + const auto updateTypeFromField = curField.fieldNameStringData()[0] != '$' + ? UpdateType::kReplacement + : UpdateType::kModifier; uassert(ErrorCodes::UnsupportedFormat, str::stream() << "update document " << updateExpr << " has mixed $operator and non-$operator style fields", - updateType == curFieldType); + updateType == updateTypeFromField); } uassert(ErrorCodes::InvalidOptions, "Replacement-style updates cannot be {multi:true}", - updateType == UpdateType::kOpStyle || !updateDoc.getMulti()); - return updateType; + updateType == UpdateType::kModifier || !updateDoc.getMulti()); } /** @@ -128,25 +124,21 @@ UpdateType getUpdateExprType(const write_ops::UpdateOpEntry& updateDoc) { */ BSONObj getUpdateExprForTargeting(const boost::intrusive_ptr<ExpressionContext> expCtx, const ShardKeyPattern& shardKeyPattern, - UpdateType updateType, const BSONObj& updateQuery, const write_ops::UpdateModification& updateMod) { - // We should never see an invalid update type here. - invariant(updateType != UpdateType::kUnknown); - // If this is not a replacement update, then the update expression remains unchanged. - if (updateType != UpdateType::kReplacement) { + if (updateMod.type() != UpdateType::kReplacement) { BSONObjBuilder objBuilder; updateMod.serializeToBSON("u", &objBuilder); return objBuilder.obj(); } // Extract the raw update expression from the request. - invariant(updateMod.type() == write_ops::UpdateModification::Type::kClassic); + invariant(updateMod.type() == UpdateType::kReplacement); // Replace any non-existent shard key values with a null value. auto updateExpr = - shardKeyPattern.emplaceMissingShardKeyValuesForDocument(updateMod.getUpdateClassic()); + shardKeyPattern.emplaceMissingShardKeyValuesForDocument(updateMod.getUpdateReplacement()); // If we aren't missing _id, return the update expression as-is. if (updateExpr.hasField(kIdFieldName)) { @@ -443,9 +435,9 @@ std::vector<ShardEndpoint> ChunkManagerTargeter::targetUpdate(OperationContext* } } - const auto updateType = getUpdateExprType(updateOp); + validateUpdateDoc(updateOp); const auto updateExpr = - getUpdateExprForTargeting(expCtx, shardKeyPattern, updateType, query, updateOp.getU()); + getUpdateExprForTargeting(expCtx, shardKeyPattern, query, updateOp.getU()); // Utility function to target an update by shard key, and to handle any potential error results. auto targetByShardKey = [this, &collation](StatusWith<BSONObj> swShardKey, std::string msg) { @@ -473,7 +465,7 @@ std::vector<ShardEndpoint> ChunkManagerTargeter::targetUpdate(OperationContext* // Replacement-style updates must always target a single shard. If we were unable to do so using // the query, we attempt to extract the shard key from the replacement and target based on it. - if (updateType == UpdateType::kReplacement) { + if (updateOp.getU().type() == write_ops::UpdateModification::Type::kReplacement) { return targetByShardKey(shardKeyPattern.extractShardKeyFromDoc(updateExpr), "Failed to target update by replacement document"); } diff --git a/src/mongo/s/chunk_manager_targeter.h b/src/mongo/s/chunk_manager_targeter.h index 5bee40f821f..5dfb08dc878 100644 --- a/src/mongo/s/chunk_manager_targeter.h +++ b/src/mongo/s/chunk_manager_targeter.h @@ -58,7 +58,6 @@ using StaleShardVersionMap = std::map<ShardId, ChunkVersion>; */ class ChunkManagerTargeter : public NSTargeter { public: - enum class UpdateType { kReplacement, kOpStyle, kUnknown }; enum class LastErrorType { kCouldNotTarget, kStaleShardVersion, kStaleDbVersion }; /** * Initializes the targeter with the latest routing information for the namespace, which means diff --git a/src/mongo/s/sharding_router_test_fixture.cpp b/src/mongo/s/sharding_router_test_fixture.cpp index 1597ca75e8e..f3348801a4e 100644 --- a/src/mongo/s/sharding_router_test_fixture.cpp +++ b/src/mongo/s/sharding_router_test_fixture.cpp @@ -322,7 +322,11 @@ void ShardingTestFixture::expectUpdateCollection(const HostAndPort& expectedHost ASSERT(!update.getMulti()); ASSERT_BSONOBJ_EQ(BSON(CollectionType::kNssFieldName << coll.getNss().toString()), update.getQ()); - ASSERT_BSONOBJ_EQ(coll.toBSON(), update.getU().getUpdateClassic()); + const auto& updateBSON = + update.getU().type() == write_ops::UpdateModification::Type::kReplacement + ? update.getU().getUpdateReplacement() + : update.getU().getUpdateModifier(); + ASSERT_BSONOBJ_EQ(coll.toBSON(), updateBSON); BatchedCommandResponse response; response.setStatus(Status::OK()); diff --git a/src/mongo/shell/bench.cpp b/src/mongo/shell/bench.cpp index c2614960c11..3624bbb69bb 100644 --- a/src/mongo/shell/bench.cpp +++ b/src/mongo/shell/bench.cpp @@ -1173,9 +1173,15 @@ void BenchRunOp::executeOnce(DBClientBase* conn, BSONObjBuilder singleUpdate; singleUpdate.append("q", query); switch (this->update.type()) { - case write_ops::UpdateModification::Type::kClassic: { + case write_ops::UpdateModification::Type::kReplacement: { singleUpdate.append("u", - fixQuery(this->update.getUpdateClassic(), + fixQuery(this->update.getUpdateReplacement(), + *state->bsonTemplateEvaluator)); + break; + } + case write_ops::UpdateModification::Type::kModifier: { + singleUpdate.append("u", + fixQuery(this->update.getUpdateModifier(), *state->bsonTemplateEvaluator)); break; } |