Skip to content

Commit 37149f8

Browse files
sakthivel-aximsakthivel-axim
committed
Resolves FoundationDB#156: The query parsing and validation part moved to the updateDocument constructor
1 parent 34cc0b0 commit 37149f8

File tree

5 files changed

+136
-68
lines changed

5 files changed

+136
-68
lines changed

‎src/ExtCmd.actor.cpp

+4-6
Original file line numberDiff line numberDiff line change
@@ -568,24 +568,22 @@ ACTOR static Future<Reference<ExtMsgReply>> doFindAndModify(Reference<ExtConnect
568568
if ((isremove && isupdate) || !(isremove || isupdate))
569569
throw bad_find_and_modify();
570570

571-
if (isoperatorUpdate) {
572-
staticValidateUpdateObject(updateDoc, false, isupsert);
573-
}
574-
575571
state Reference<DocTransaction> tr = ec->getOperationTransaction();
576572
state Reference<UnboundCollectionContext> ucx = wait(ec->mm->getUnboundCollectionContext(tr, query->ns));
573+
state Reference<UnboundCollectionContext> cx =
574+
Reference<UnboundCollectionContext>(new UnboundCollectionContext(*ucx));
577575

578576
state Reference<IUpdateOp> updater;
579577
state Reference<IInsertOp> upserter;
580578
if (isremove)
581579
updater = ref(new DeleteDocument());
582580
else if (isoperatorUpdate)
583-
updater = operatorUpdate(updateDoc);
581+
updater = operatorUpdate(cx, updateDoc, false, isupsert);
584582
else
585583
updater = replaceUpdate(updateDoc);
586584
if (isupsert) {
587585
if (isoperatorUpdate)
588-
upserter = operatorUpsert(selector, updateDoc);
586+
upserter = operatorUpsert(cx, selector, updateDoc);
589587
else
590588
upserter = simpleUpsert(selector, updateDoc);
591589
}

‎src/ExtMsg.actor.cpp

+57-59
Original file line numberDiff line numberDiff line change
@@ -962,74 +962,56 @@ void staticValidateModifiedFields(std::string fieldName,
962962
}
963963
}
964964

965-
std::vector<std::string> staticValidateUpdateObject(bson::BSONObj update, bool multi, bool upsert) {
966-
std::set<std::string> affectedFields;
967-
std::set<std::string> prefixesOfAffectedFields;
968-
for (auto i = update.begin(); i.more();) {
969-
970-
auto el = i.next();
971-
std::string operatorName = el.fieldName();
972-
if (!el.isABSONObj() || el.Obj().nFields() == 0) {
973-
throw update_operator_empty_parameter();
974-
}
975-
976-
if (upsert && operatorName == DocLayerConstants::SET_ON_INSERT)
977-
operatorName = DocLayerConstants::SET;
978-
979-
for (auto j = el.Obj().begin(); j.more();) {
980-
bson::BSONElement subel = j.next();
981-
auto fn = std::string(subel.fieldName());
982-
if (fn == DocLayerConstants::ID_FIELD) {
983-
if (operatorName != DocLayerConstants::SET && operatorName != DocLayerConstants::SET_ON_INSERT) {
984-
throw cant_modify_id();
985-
}
986-
}
987-
staticValidateModifiedFields(fn, &affectedFields, &prefixesOfAffectedFields);
988-
if (operatorName == DocLayerConstants::RENAME) {
989-
if (!subel.isString())
990-
throw bad_rename_target();
991-
staticValidateModifiedFields(subel.String(), &affectedFields, &prefixesOfAffectedFields);
992-
}
993-
}
994-
}
995-
996-
std::vector<std::string> bannedIndexFields;
997-
bannedIndexFields.insert(std::end(bannedIndexFields), std::begin(affectedFields), std::end(affectedFields));
998-
bannedIndexFields.insert(std::end(bannedIndexFields), std::begin(prefixesOfAffectedFields),
999-
std::end(prefixesOfAffectedFields));
1000-
1001-
return bannedIndexFields;
1002-
}
1003-
1004965
bool shouldCreateRoot(std::string operatorName) {
1005966
return operatorName == DocLayerConstants::SET || operatorName == DocLayerConstants::INC ||
1006967
operatorName == DocLayerConstants::MUL || operatorName == DocLayerConstants::CURRENT_DATE ||
1007968
operatorName == DocLayerConstants::MAX || operatorName == DocLayerConstants::MIN ||
1008969
operatorName == DocLayerConstants::PUSH || operatorName == DocLayerConstants::ADD_TO_SET;
1009970
}
1010971

972+
// #156: staticValidateUpdateObject function is moved to updateDocument constructor
973+
974+
/* staticValidateUpdateObject function part has more similar and related functionalities
975+
* present in updateDocument constructor. Instead of using StaticValidateUpdate function
976+
* part, that part is moved to updateDocument constructor. So there is no need to call staticValidateUpdate
977+
* function separately, if operatorUpdate is enabled it will execute updateDocument which now has
978+
* staticValidateUpdate functionalities also.
979+
*/
980+
1011981
ACTOR Future<Void> updateDocument(Reference<IReadWriteContext> cx,
982+
Reference<UnboundCollectionContext> ucx,
1012983
bson::BSONObj update,
1013984
bool upsert,
1014-
Future<Standalone<StringRef>> fEncodedId) {
985+
bool multi = false) {
986+
987+
state std::set<std::string> affectedFields;
988+
state std::set<std::string> prefixesOfAffectedFields;
1015989
state std::vector<Future<Void>> futures;
1016-
state Standalone<StringRef> encodedId = wait(fEncodedId);
990+
state Standalone<StringRef> encodedId = wait(cx->getKeyEncodedId());
1017991
for (auto i = update.begin(); i.more();) {
1018992
auto el = i.next();
1019993

1020994
std::string operatorName = el.fieldName();
995+
if (!el.isABSONObj() || el.Obj().nFields() == 0) {
996+
throw update_operator_empty_parameter();
997+
}
998+
1021999
if (upsert && operatorName == DocLayerConstants::SET_ON_INSERT)
10221000
operatorName = DocLayerConstants::SET;
10231001
for (auto j = el.Obj().begin(); j.more();) {
10241002
bson::BSONElement subel = j.next();
10251003
auto fn = std::string(subel.fieldName());
10261004
if (fn == DocLayerConstants::ID_FIELD) {
1027-
if (operatorName == DocLayerConstants::SET || operatorName == DocLayerConstants::SET_ON_INSERT) {
1005+
if (operatorName != DocLayerConstants::SET && operatorName != DocLayerConstants::SET_ON_INSERT) {
1006+
throw cant_modify_id();
1007+
} else if (operatorName == DocLayerConstants::SET || operatorName == DocLayerConstants::SET_ON_INSERT) {
10281008
if (extractEncodedIds(subel.wrap()).get().keyEncoded != encodedId) {
10291009
throw cant_modify_id();
10301010
}
10311011
}
10321012
}
1013+
1014+
staticValidateModifiedFields(fn, &affectedFields, &prefixesOfAffectedFields);
10331015
if (shouldCreateRoot(operatorName)) {
10341016
auto upOneFn = upOneLevel(fn);
10351017
if (!upOneFn.empty())
@@ -1042,7 +1024,10 @@ ACTOR Future<Void> updateDocument(Reference<IReadWriteContext> cx,
10421024
futures.push_back(ensureValidObject(cx, upOneFn, getLastPart(fn), false));
10431025
}
10441026
if (operatorName == DocLayerConstants::RENAME) {
1027+
if (!subel.isString())
1028+
throw bad_rename_target();
10451029
auto renameTarget = subel.String();
1030+
staticValidateModifiedFields(renameTarget, &affectedFields, &prefixesOfAffectedFields);
10461031
auto upOneRenameTarget = upOneLevel(renameTarget);
10471032
if (!upOneRenameTarget.empty())
10481033
futures.push_back(ensureValidObject(cx, renameTarget, upOneRenameTarget, false));
@@ -1054,6 +1039,11 @@ ACTOR Future<Void> updateDocument(Reference<IReadWriteContext> cx,
10541039
futures.push_back(ExtUpdateOperator::execute(operatorName, cx, encodeMaybeDotted(fn), subel));
10551040
}
10561041
}
1042+
std::vector<std::string> bannedIndexFields;
1043+
bannedIndexFields.insert(std::end(bannedIndexFields), std::begin(affectedFields), std::end(affectedFields));
1044+
bannedIndexFields.insert(std::end(bannedIndexFields), std::begin(prefixesOfAffectedFields),
1045+
std::end(prefixesOfAffectedFields));
1046+
ucx->setBannedFieldNames(bannedIndexFields);
10571047
wait(waitForAll(futures));
10581048
return Void();
10591049
}
@@ -1068,32 +1058,27 @@ ACTOR Future<WriteCmdResult> doUpdateCmd(Namespace ns,
10681058
try {
10691059
state ExtUpdateCmd* cmd = &((*cmds)[idx]);
10701060
state int isoperatorUpdate = hasOperatorFieldnames(cmd->update, 0);
1071-
state std::vector<std::string> bannedIndexFields;
10721061

10731062
if (!isoperatorUpdate && cmd->multi) {
10741063
throw literal_multi_update();
10751064
}
1076-
1077-
if (isoperatorUpdate) {
1078-
bannedIndexFields = staticValidateUpdateObject(cmd->update, cmd->multi, cmd->upsert);
1079-
}
1065+
// #156: staticValidateUpdateObject function is moved to updateDocument constructor
10801066

10811067
state Optional<bson::BSONObj> upserted = Optional<bson::BSONObj>();
10821068
state Reference<DocTransaction> dtr = ec->getOperationTransaction();
10831069
Reference<UnboundCollectionContext> ocx = wait(ec->mm->getUnboundCollectionContext(dtr, ns));
10841070
state Reference<UnboundCollectionContext> cx =
10851071
Reference<UnboundCollectionContext>(new UnboundCollectionContext(*ocx));
1086-
cx->setBannedFieldNames(bannedIndexFields);
10871072

10881073
Reference<IUpdateOp> updater;
10891074
Reference<IInsertOp> upserter;
10901075
if (isoperatorUpdate)
1091-
updater = operatorUpdate(cmd->update);
1076+
updater = operatorUpdate(cx, cmd->update, cmd->multi, cmd->upsert);
10921077
else
10931078
updater = replaceUpdate(cmd->update);
10941079
if (cmd->upsert) {
10951080
if (isoperatorUpdate)
1096-
upserter = operatorUpsert(cmd->selector, cmd->update);
1081+
upserter = operatorUpsert(cx, cmd->selector, cmd->update);
10971082
else
10981083
upserter = simpleUpsert(cmd->selector, cmd->update);
10991084
}
@@ -1341,12 +1326,18 @@ Future<Void> ExtMsgKillCursors::run(Reference<ExtConnection> ec) {
13411326
/* FIXME: These don't really belong here*/
13421327

13431328
struct ExtOperatorUpdate : ConcreteUpdateOp<ExtOperatorUpdate> {
1329+
Reference<UnboundCollectionContext> cx;
13441330
bson::BSONObj msgUpdate;
1345-
1346-
explicit ExtOperatorUpdate(bson::BSONObj const& msgUpdate) : msgUpdate(msgUpdate) {}
1331+
bool upsert;
1332+
bool multi;
1333+
explicit ExtOperatorUpdate(Reference<UnboundCollectionContext> cx,
1334+
bson::BSONObj const& msgUpdate,
1335+
bool multi,
1336+
bool upsert)
1337+
: msgUpdate(msgUpdate), multi(multi), upsert(upsert), cx(cx) {}
13471338

13481339
Future<Void> update(Reference<IReadWriteContext> document) override {
1349-
return updateDocument(document, msgUpdate, false, document->getKeyEncodedId());
1340+
return updateDocument(document, cx, msgUpdate, upsert, multi);
13501341
}
13511342

13521343
std::string describe() override { return "OperatorUpdate(" + msgUpdate.toString() + ")"; }
@@ -1393,10 +1384,12 @@ struct ExtReplaceUpdate : ConcreteUpdateOp<ExtReplaceUpdate> {
13931384
};
13941385

13951386
struct ExtOperatorUpsert : ConcreteInsertOp<ExtOperatorUpsert> {
1387+
Reference<UnboundCollectionContext> ucx;
13961388
bson::BSONObj selector;
13971389
bson::BSONObj update;
13981390

1399-
ExtOperatorUpsert(bson::BSONObj selector, bson::BSONObj update) : selector(selector), update(update) {}
1391+
ExtOperatorUpsert(Reference<UnboundCollectionContext> ucx, bson::BSONObj selector, bson::BSONObj update)
1392+
: selector(selector), update(update), ucx(ucx) {}
14001393

14011394
ACTOR static Future<Reference<IReadWriteContext>> upsertActor(ExtOperatorUpsert* self,
14021395
Reference<CollectionContext> cx) {
@@ -1411,7 +1404,7 @@ struct ExtOperatorUpsert : ConcreteInsertOp<ExtOperatorUpsert> {
14111404
thingToInsert = transformOperatorQueryToUpdatableDocument(self->selector);
14121405
}
14131406
state Reference<IReadWriteContext> dcx = wait(insertDocument(cx, thingToInsert, encodedIds));
1414-
wait(updateDocument(dcx, self->update, true, dcx->getKeyEncodedId()));
1407+
wait(updateDocument(dcx, self->ucx, self->update, true));
14151408
return dcx;
14161409
}
14171410

@@ -1442,15 +1435,20 @@ struct ExtSimpleUpsert : ConcreteInsertOp<ExtSimpleUpsert> {
14421435
}
14431436
};
14441437

1445-
Reference<IUpdateOp> operatorUpdate(bson::BSONObj const& msgUpdate) {
1446-
return ref(new ExtOperatorUpdate(msgUpdate));
1438+
Reference<IUpdateOp> operatorUpdate(Reference<UnboundCollectionContext> cx,
1439+
bson::BSONObj const& msgUpdate,
1440+
bool multi,
1441+
bool upsert) {
1442+
return ref(new ExtOperatorUpdate(cx, msgUpdate, multi, upsert));
14471443
}
14481444
Reference<IUpdateOp> replaceUpdate(bson::BSONObj const& replaceWith) {
14491445
return ref(new ExtReplaceUpdate(replaceWith));
14501446
}
14511447
Reference<IInsertOp> simpleUpsert(bson::BSONObj const& selector, bson::BSONObj const& update) {
14521448
return ref(new ExtSimpleUpsert(selector, update));
14531449
}
1454-
Reference<IInsertOp> operatorUpsert(bson::BSONObj const& selector, bson::BSONObj const& update) {
1455-
return ref(new ExtOperatorUpsert(selector, update));
1450+
Reference<IInsertOp> operatorUpsert(Reference<UnboundCollectionContext> cx,
1451+
bson::BSONObj const& selector,
1452+
bson::BSONObj const& update) {
1453+
return ref(new ExtOperatorUpsert(cx, selector, update));
14561454
}

‎src/ExtMsg.actor.h

+7-3
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,6 @@ struct ExtMsgKillCursors : ExtMsg, FastAllocated<ExtMsgKillCursors> {
238238
};
239239

240240
Reference<Plan> planQuery(Reference<UnboundCollectionContext> cx, const bson::BSONObj& query);
241-
std::vector<std::string> staticValidateUpdateObject(bson::BSONObj update, bool multi, bool upsert);
242241
ACTOR Future<WriteCmdResult> attemptIndexInsertion(bson::BSONObj firstDoc,
243242
Reference<ExtConnection> ec,
244243
Reference<DocTransaction> tr,
@@ -256,9 +255,14 @@ ACTOR Future<WriteCmdResult> doUpdateCmd(Namespace ns,
256255
Reference<ExtConnection> ec);
257256

258257
// FIXME: these don't really belong here either
259-
Reference<IUpdateOp> operatorUpdate(bson::BSONObj const& msgUpdate);
258+
Reference<IUpdateOp> operatorUpdate(Reference<UnboundCollectionContext> cx,
259+
bson::BSONObj const& msgUpdate,
260+
bool multi,
261+
bool upsert);
260262
Reference<IUpdateOp> replaceUpdate(bson::BSONObj const& replaceWith);
261263
Reference<IInsertOp> simpleUpsert(bson::BSONObj const& selector, bson::BSONObj const& update);
262-
Reference<IInsertOp> operatorUpsert(bson::BSONObj const& selector, bson::BSONObj const& update);
264+
Reference<IInsertOp> operatorUpsert(Reference<UnboundCollectionContext> ucx,
265+
bson::BSONObj const& selector,
266+
bson::BSONObj const& update);
263267

264268
#endif /* _EXT_MSG_ACTOR_H_ */

‎test/correctness/smoke/test_update.py

+34
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#
2323

2424
from collections import OrderedDict
25+
from pymongo.errors import OperationFailure
2526
import pytest
2627

2728

@@ -49,4 +50,37 @@ def test_addToSet_with_none_value(fixture_collection):
4950
updatedDoc = [i for i in collection.find( {'_id':1})][0]
5051
assert updatedDoc['B'] == [{'a':1}, None], "Expect field 'B' contains the None but got {}".format(updatedDoc)
5152

53+
#156: staticValidateUpdateObject function is moved to updateDocument constructor
5254

55+
def test_update_exception_error_1(fixture_collection):
56+
collection = fixture_collection
57+
58+
collection.delete_many({})
59+
60+
collection.insert_one({'_id':1, "B":[{'a':1}]})
61+
#'_id' cannot be modified
62+
#In case try to update '_id' it should throw an error
63+
try:
64+
collection.update_one({'_id':1}, {'$set': {'_id': 2}})
65+
except OperationFailure as e:
66+
serverErrObj = e.details
67+
assert serverErrObj['code'] != None
68+
# 20010 : You may not modify '_id' in an update
69+
assert serverErrObj['code'] == 20010, "Expected:20010, Found: {}".format(serverErrObj)
70+
71+
72+
def test_update_exception_error_2(fixture_collection):
73+
collection = fixture_collection
74+
75+
collection.delete_many({})
76+
77+
collection.insert_one({'_id':1, "B":"qty"})
78+
#'$rename' operator should not be empty
79+
#In case '$rename' operator is empty it should throw an error
80+
try:
81+
collection.update_one({'_id':1}, {'$rename': {}})
82+
except OperationFailure as e:
83+
serverErrObj = e.details
84+
assert serverErrObj['code'] != None
85+
# 26840 : Update operator has empty object for parameter. You must specify a field.
86+
assert serverErrObj['code'] == 26840, "Expected:26840, Found: {}".format(serverErrObj)

‎test/correctness/smoke/test_upsert.py

+34
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,40 @@ def create_operator_permutation_tests(oplist, depth, repeat, update):
401401
},
402402
)
403403

404+
#156: staticValidateUpdateObject function is moved to updateDocument constructor
405+
406+
def test_upsert_exception_error_1(fixture_collection):
407+
collection = fixture_collection
408+
409+
collection.delete_many({})
410+
411+
collection.insert_one({'_id':1, "B":[{'a':1}]})
412+
#'_id' cannot be modified
413+
#In case try to update '_id' it should throw an error
414+
try:
415+
collection.update_one({'_id':1}, {'$set': {'_id': 2}}, upsert=True)
416+
except OperationFailure as e:
417+
serverErrObj = e.details
418+
assert serverErrObj['code'] != None
419+
# 20010 : You may not modify '_id' in an update
420+
assert serverErrObj['code'] == 20010, "Expected:20010, Found: {}".format(serverErrObj)
421+
422+
423+
def test_upsert_exception_error_2(fixture_collection):
424+
collection = fixture_collection
425+
426+
collection.delete_many({})
427+
428+
collection.insert_one({'_id':1, "B":"qty"})
429+
#'$rename' operator should not be empty
430+
#In case '$rename' operator is empty it should throw an error
431+
try:
432+
collection.update_one({'_id':1}, {'$rename': {}}, upsert=True)
433+
except OperationFailure as e:
434+
serverErrObj = e.details
435+
assert serverErrObj['code'] != None
436+
# 26840 : Update operator has empty object for parameter. You must specify a field.
437+
assert serverErrObj['code'] == 26840, "Expected:26840, Found: {}".format(serverErrObj)
404438

405439
def operators_test_with_depth(dl_collection, depth):
406440
for update in updates:

0 commit comments

Comments
 (0)