diff --git a/src/ExtCmd.actor.cpp b/src/ExtCmd.actor.cpp index ce00844..cc9cc68 100644 --- a/src/ExtCmd.actor.cpp +++ b/src/ExtCmd.actor.cpp @@ -568,24 +568,22 @@ ACTOR static Future> doFindAndModify(Reference tr = ec->getOperationTransaction(); state Reference ucx = wait(ec->mm->getUnboundCollectionContext(tr, query->ns)); + state Reference cx = + Reference(new UnboundCollectionContext(*ucx)); state Reference updater; state Reference upserter; if (isremove) updater = ref(new DeleteDocument()); else if (isoperatorUpdate) - updater = operatorUpdate(updateDoc); + updater = operatorUpdate(cx, updateDoc, false, isupsert); else updater = replaceUpdate(updateDoc); if (isupsert) { if (isoperatorUpdate) - upserter = operatorUpsert(selector, updateDoc); + upserter = operatorUpsert(cx, selector, updateDoc); else upserter = simpleUpsert(selector, updateDoc); } diff --git a/src/ExtMsg.actor.cpp b/src/ExtMsg.actor.cpp index 9f96d80..f276c4a 100644 --- a/src/ExtMsg.actor.cpp +++ b/src/ExtMsg.actor.cpp @@ -962,45 +962,6 @@ void staticValidateModifiedFields(std::string fieldName, } } -std::vector staticValidateUpdateObject(bson::BSONObj update, bool multi, bool upsert) { - std::set affectedFields; - std::set prefixesOfAffectedFields; - for (auto i = update.begin(); i.more();) { - - auto el = i.next(); - std::string operatorName = el.fieldName(); - if (!el.isABSONObj() || el.Obj().nFields() == 0) { - throw update_operator_empty_parameter(); - } - - if (upsert && operatorName == DocLayerConstants::SET_ON_INSERT) - operatorName = DocLayerConstants::SET; - - for (auto j = el.Obj().begin(); j.more();) { - bson::BSONElement subel = j.next(); - auto fn = std::string(subel.fieldName()); - if (fn == DocLayerConstants::ID_FIELD) { - if (operatorName != DocLayerConstants::SET && operatorName != DocLayerConstants::SET_ON_INSERT) { - throw cant_modify_id(); - } - } - staticValidateModifiedFields(fn, &affectedFields, &prefixesOfAffectedFields); - if (operatorName == DocLayerConstants::RENAME) { - if (!subel.isString()) - throw bad_rename_target(); - staticValidateModifiedFields(subel.String(), &affectedFields, &prefixesOfAffectedFields); - } - } - } - - std::vector bannedIndexFields; - bannedIndexFields.insert(std::end(bannedIndexFields), std::begin(affectedFields), std::end(affectedFields)); - bannedIndexFields.insert(std::end(bannedIndexFields), std::begin(prefixesOfAffectedFields), - std::end(prefixesOfAffectedFields)); - - return bannedIndexFields; -} - bool shouldCreateRoot(std::string operatorName) { return operatorName == DocLayerConstants::SET || operatorName == DocLayerConstants::INC || operatorName == DocLayerConstants::MUL || operatorName == DocLayerConstants::CURRENT_DATE || @@ -1008,28 +969,49 @@ bool shouldCreateRoot(std::string operatorName) { operatorName == DocLayerConstants::PUSH || operatorName == DocLayerConstants::ADD_TO_SET; } +// #156: staticValidateUpdateObject function is moved to updateDocument constructor + +/* staticValidateUpdateObject function part has more similar and related functionalities + * present in updateDocument constructor. Instead of using StaticValidateUpdate function + * part, that part is moved to updateDocument constructor. So there is no need to call staticValidateUpdate + * function separately, if operatorUpdate is enabled it will execute updateDocument which now has + * staticValidateUpdate functionalities also. + */ + ACTOR Future updateDocument(Reference cx, + Reference ucx, bson::BSONObj update, bool upsert, - Future> fEncodedId) { + bool multi = false) { + + state std::set affectedFields; + state std::set prefixesOfAffectedFields; state std::vector> futures; - state Standalone encodedId = wait(fEncodedId); + state Standalone encodedId = wait(cx->getKeyEncodedId()); for (auto i = update.begin(); i.more();) { auto el = i.next(); std::string operatorName = el.fieldName(); + if (!el.isABSONObj() || el.Obj().nFields() == 0) { + throw update_operator_empty_parameter(); + } + if (upsert && operatorName == DocLayerConstants::SET_ON_INSERT) operatorName = DocLayerConstants::SET; for (auto j = el.Obj().begin(); j.more();) { bson::BSONElement subel = j.next(); auto fn = std::string(subel.fieldName()); if (fn == DocLayerConstants::ID_FIELD) { - if (operatorName == DocLayerConstants::SET || operatorName == DocLayerConstants::SET_ON_INSERT) { + if (operatorName != DocLayerConstants::SET && operatorName != DocLayerConstants::SET_ON_INSERT) { + throw cant_modify_id(); + } else if (operatorName == DocLayerConstants::SET || operatorName == DocLayerConstants::SET_ON_INSERT) { if (extractEncodedIds(subel.wrap()).get().keyEncoded != encodedId) { throw cant_modify_id(); } } } + + staticValidateModifiedFields(fn, &affectedFields, &prefixesOfAffectedFields); if (shouldCreateRoot(operatorName)) { auto upOneFn = upOneLevel(fn); if (!upOneFn.empty()) @@ -1042,7 +1024,10 @@ ACTOR Future updateDocument(Reference cx, futures.push_back(ensureValidObject(cx, upOneFn, getLastPart(fn), false)); } if (operatorName == DocLayerConstants::RENAME) { + if (!subel.isString()) + throw bad_rename_target(); auto renameTarget = subel.String(); + staticValidateModifiedFields(renameTarget, &affectedFields, &prefixesOfAffectedFields); auto upOneRenameTarget = upOneLevel(renameTarget); if (!upOneRenameTarget.empty()) futures.push_back(ensureValidObject(cx, renameTarget, upOneRenameTarget, false)); @@ -1054,6 +1039,11 @@ ACTOR Future updateDocument(Reference cx, futures.push_back(ExtUpdateOperator::execute(operatorName, cx, encodeMaybeDotted(fn), subel)); } } + std::vector bannedIndexFields; + bannedIndexFields.insert(std::end(bannedIndexFields), std::begin(affectedFields), std::end(affectedFields)); + bannedIndexFields.insert(std::end(bannedIndexFields), std::begin(prefixesOfAffectedFields), + std::end(prefixesOfAffectedFields)); + ucx->setBannedFieldNames(bannedIndexFields); wait(waitForAll(futures)); return Void(); } @@ -1068,32 +1058,27 @@ ACTOR Future doUpdateCmd(Namespace ns, try { state ExtUpdateCmd* cmd = &((*cmds)[idx]); state int isoperatorUpdate = hasOperatorFieldnames(cmd->update, 0); - state std::vector bannedIndexFields; if (!isoperatorUpdate && cmd->multi) { throw literal_multi_update(); } - - if (isoperatorUpdate) { - bannedIndexFields = staticValidateUpdateObject(cmd->update, cmd->multi, cmd->upsert); - } + // #156: staticValidateUpdateObject function is moved to updateDocument constructor state Optional upserted = Optional(); state Reference dtr = ec->getOperationTransaction(); Reference ocx = wait(ec->mm->getUnboundCollectionContext(dtr, ns)); state Reference cx = Reference(new UnboundCollectionContext(*ocx)); - cx->setBannedFieldNames(bannedIndexFields); Reference updater; Reference upserter; if (isoperatorUpdate) - updater = operatorUpdate(cmd->update); + updater = operatorUpdate(cx, cmd->update, cmd->multi, cmd->upsert); else updater = replaceUpdate(cmd->update); if (cmd->upsert) { if (isoperatorUpdate) - upserter = operatorUpsert(cmd->selector, cmd->update); + upserter = operatorUpsert(cx, cmd->selector, cmd->update); else upserter = simpleUpsert(cmd->selector, cmd->update); } @@ -1341,12 +1326,18 @@ Future ExtMsgKillCursors::run(Reference ec) { /* FIXME: These don't really belong here*/ struct ExtOperatorUpdate : ConcreteUpdateOp { + Reference cx; bson::BSONObj msgUpdate; - - explicit ExtOperatorUpdate(bson::BSONObj const& msgUpdate) : msgUpdate(msgUpdate) {} + bool upsert; + bool multi; + explicit ExtOperatorUpdate(Reference cx, + bson::BSONObj const& msgUpdate, + bool multi, + bool upsert) + : msgUpdate(msgUpdate), multi(multi), upsert(upsert), cx(cx) {} Future update(Reference document) override { - return updateDocument(document, msgUpdate, false, document->getKeyEncodedId()); + return updateDocument(document, cx, msgUpdate, upsert, multi); } std::string describe() override { return "OperatorUpdate(" + msgUpdate.toString() + ")"; } @@ -1393,10 +1384,12 @@ struct ExtReplaceUpdate : ConcreteUpdateOp { }; struct ExtOperatorUpsert : ConcreteInsertOp { + Reference ucx; bson::BSONObj selector; bson::BSONObj update; - ExtOperatorUpsert(bson::BSONObj selector, bson::BSONObj update) : selector(selector), update(update) {} + ExtOperatorUpsert(Reference ucx, bson::BSONObj selector, bson::BSONObj update) + : selector(selector), update(update), ucx(ucx) {} ACTOR static Future> upsertActor(ExtOperatorUpsert* self, Reference cx) { @@ -1411,7 +1404,7 @@ struct ExtOperatorUpsert : ConcreteInsertOp { thingToInsert = transformOperatorQueryToUpdatableDocument(self->selector); } state Reference dcx = wait(insertDocument(cx, thingToInsert, encodedIds)); - wait(updateDocument(dcx, self->update, true, dcx->getKeyEncodedId())); + wait(updateDocument(dcx, self->ucx, self->update, true)); return dcx; } @@ -1442,8 +1435,11 @@ struct ExtSimpleUpsert : ConcreteInsertOp { } }; -Reference operatorUpdate(bson::BSONObj const& msgUpdate) { - return ref(new ExtOperatorUpdate(msgUpdate)); +Reference operatorUpdate(Reference cx, + bson::BSONObj const& msgUpdate, + bool multi, + bool upsert) { + return ref(new ExtOperatorUpdate(cx, msgUpdate, multi, upsert)); } Reference replaceUpdate(bson::BSONObj const& replaceWith) { return ref(new ExtReplaceUpdate(replaceWith)); @@ -1451,6 +1447,8 @@ Reference replaceUpdate(bson::BSONObj const& replaceWith) { Reference simpleUpsert(bson::BSONObj const& selector, bson::BSONObj const& update) { return ref(new ExtSimpleUpsert(selector, update)); } -Reference operatorUpsert(bson::BSONObj const& selector, bson::BSONObj const& update) { - return ref(new ExtOperatorUpsert(selector, update)); +Reference operatorUpsert(Reference cx, + bson::BSONObj const& selector, + bson::BSONObj const& update) { + return ref(new ExtOperatorUpsert(cx, selector, update)); } diff --git a/src/ExtMsg.actor.h b/src/ExtMsg.actor.h index 87dd6f9..1e2c3d1 100644 --- a/src/ExtMsg.actor.h +++ b/src/ExtMsg.actor.h @@ -238,7 +238,6 @@ struct ExtMsgKillCursors : ExtMsg, FastAllocated { }; Reference planQuery(Reference cx, const bson::BSONObj& query); -std::vector staticValidateUpdateObject(bson::BSONObj update, bool multi, bool upsert); ACTOR Future attemptIndexInsertion(bson::BSONObj firstDoc, Reference ec, Reference tr, @@ -256,9 +255,14 @@ ACTOR Future doUpdateCmd(Namespace ns, Reference ec); // FIXME: these don't really belong here either -Reference operatorUpdate(bson::BSONObj const& msgUpdate); +Reference operatorUpdate(Reference cx, + bson::BSONObj const& msgUpdate, + bool multi, + bool upsert); Reference replaceUpdate(bson::BSONObj const& replaceWith); Reference simpleUpsert(bson::BSONObj const& selector, bson::BSONObj const& update); -Reference operatorUpsert(bson::BSONObj const& selector, bson::BSONObj const& update); +Reference operatorUpsert(Reference ucx, + bson::BSONObj const& selector, + bson::BSONObj const& update); #endif /* _EXT_MSG_ACTOR_H_ */ diff --git a/test/correctness/smoke/test_update.py b/test/correctness/smoke/test_update.py index 53cc1bf..37cdab5 100644 --- a/test/correctness/smoke/test_update.py +++ b/test/correctness/smoke/test_update.py @@ -22,6 +22,7 @@ # from collections import OrderedDict +from pymongo.errors import OperationFailure import pytest @@ -49,4 +50,37 @@ def test_addToSet_with_none_value(fixture_collection): updatedDoc = [i for i in collection.find( {'_id':1})][0] assert updatedDoc['B'] == [{'a':1}, None], "Expect field 'B' contains the None but got {}".format(updatedDoc) +#156: staticValidateUpdateObject function is moved to updateDocument constructor +def test_update_exception_error_1(fixture_collection): + collection = fixture_collection + + collection.delete_many({}) + + collection.insert_one({'_id':1, "B":[{'a':1}]}) + #'_id' cannot be modified + #In case try to update '_id' it should throw an error + try: + collection.update_one({'_id':1}, {'$set': {'_id': 2}}) + except OperationFailure as e: + serverErrObj = e.details + assert serverErrObj['code'] != None + # 20010 : You may not modify '_id' in an update + assert serverErrObj['code'] == 20010, "Expected:20010, Found: {}".format(serverErrObj) + + +def test_update_exception_error_2(fixture_collection): + collection = fixture_collection + + collection.delete_many({}) + + collection.insert_one({'_id':1, "B":"qty"}) + #'$rename' operator should not be empty + #In case '$rename' operator is empty it should throw an error + try: + collection.update_one({'_id':1}, {'$rename': {}}) + except OperationFailure as e: + serverErrObj = e.details + assert serverErrObj['code'] != None + # 26840 : Update operator has empty object for parameter. You must specify a field. + assert serverErrObj['code'] == 26840, "Expected:26840, Found: {}".format(serverErrObj) diff --git a/test/correctness/smoke/test_upsert.py b/test/correctness/smoke/test_upsert.py index ba1399d..ffb4813 100644 --- a/test/correctness/smoke/test_upsert.py +++ b/test/correctness/smoke/test_upsert.py @@ -401,6 +401,40 @@ def create_operator_permutation_tests(oplist, depth, repeat, update): }, ) +#156: staticValidateUpdateObject function is moved to updateDocument constructor + +def test_upsert_exception_error_1(fixture_collection): + collection = fixture_collection + + collection.delete_many({}) + + collection.insert_one({'_id':1, "B":[{'a':1}]}) + #'_id' cannot be modified + #In case try to update '_id' it should throw an error + try: + collection.update_one({'_id':1}, {'$set': {'_id': 2}}, upsert=True) + except OperationFailure as e: + serverErrObj = e.details + assert serverErrObj['code'] != None + # 20010 : You may not modify '_id' in an update + assert serverErrObj['code'] == 20010, "Expected:20010, Found: {}".format(serverErrObj) + + +def test_upsert_exception_error_2(fixture_collection): + collection = fixture_collection + + collection.delete_many({}) + + collection.insert_one({'_id':1, "B":"qty"}) + #'$rename' operator should not be empty + #In case '$rename' operator is empty it should throw an error + try: + collection.update_one({'_id':1}, {'$rename': {}}, upsert=True) + except OperationFailure as e: + serverErrObj = e.details + assert serverErrObj['code'] != None + # 26840 : Update operator has empty object for parameter. You must specify a field. + assert serverErrObj['code'] == 26840, "Expected:26840, Found: {}".format(serverErrObj) def operators_test_with_depth(dl_collection, depth): for update in updates: