Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Resolves #156: The query parsing and validation part moved to the updateDocument constructor #215

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Resolves #156: The query parsing and validation part moved to the updateDocument constructor #215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions src/ExtCmd.actor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,24 +568,22 @@ ACTOR static Future<Reference<ExtMsgReply>> doFindAndModify(Reference<ExtConnect
if ((isremove && isupdate) || !(isremove || isupdate))
throw bad_find_and_modify();

if (isoperatorUpdate) {
staticValidateUpdateObject(updateDoc, false, isupsert);
}

state Reference<DocTransaction> tr = ec->getOperationTransaction();
state Reference<UnboundCollectionContext> ucx = wait(ec->mm->getUnboundCollectionContext(tr, query->ns));
state Reference<UnboundCollectionContext> cx =
Reference<UnboundCollectionContext>(new UnboundCollectionContext(*ucx));

state Reference<IUpdateOp> updater;
state Reference<IInsertOp> 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);
}
Expand Down
116 changes: 57 additions & 59 deletions src/ExtMsg.actor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,74 +962,56 @@ void staticValidateModifiedFields(std::string fieldName,
}
}

std::vector<std::string> staticValidateUpdateObject(bson::BSONObj update, bool multi, bool upsert) {
std::set<std::string> affectedFields;
std::set<std::string> 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<std::string> 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 ||
operatorName == DocLayerConstants::MAX || operatorName == DocLayerConstants::MIN ||
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<Void> updateDocument(Reference<IReadWriteContext> cx,
Reference<UnboundCollectionContext> ucx,
bson::BSONObj update,
bool upsert,
Future<Standalone<StringRef>> fEncodedId) {
bool multi = false) {

state std::set<std::string> affectedFields;
state std::set<std::string> prefixesOfAffectedFields;
state std::vector<Future<Void>> futures;
state Standalone<StringRef> encodedId = wait(fEncodedId);
state Standalone<StringRef> 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())
Expand All @@ -1042,7 +1024,10 @@ ACTOR Future<Void> updateDocument(Reference<IReadWriteContext> 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));
Expand All @@ -1054,6 +1039,11 @@ ACTOR Future<Void> updateDocument(Reference<IReadWriteContext> cx,
futures.push_back(ExtUpdateOperator::execute(operatorName, cx, encodeMaybeDotted(fn), subel));
}
}
std::vector<std::string> 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();
}
Expand All @@ -1068,32 +1058,27 @@ ACTOR Future<WriteCmdResult> doUpdateCmd(Namespace ns,
try {
state ExtUpdateCmd* cmd = &((*cmds)[idx]);
state int isoperatorUpdate = hasOperatorFieldnames(cmd->update, 0);
state std::vector<std::string> 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<bson::BSONObj> upserted = Optional<bson::BSONObj>();
state Reference<DocTransaction> dtr = ec->getOperationTransaction();
Reference<UnboundCollectionContext> ocx = wait(ec->mm->getUnboundCollectionContext(dtr, ns));
state Reference<UnboundCollectionContext> cx =
Reference<UnboundCollectionContext>(new UnboundCollectionContext(*ocx));
cx->setBannedFieldNames(bannedIndexFields);

Reference<IUpdateOp> updater;
Reference<IInsertOp> 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);
}
Expand Down Expand Up @@ -1341,12 +1326,18 @@ Future<Void> ExtMsgKillCursors::run(Reference<ExtConnection> ec) {
/* FIXME: These don't really belong here*/

struct ExtOperatorUpdate : ConcreteUpdateOp<ExtOperatorUpdate> {
Reference<UnboundCollectionContext> cx;
bson::BSONObj msgUpdate;

explicit ExtOperatorUpdate(bson::BSONObj const& msgUpdate) : msgUpdate(msgUpdate) {}
bool upsert;
bool multi;
explicit ExtOperatorUpdate(Reference<UnboundCollectionContext> cx,
bson::BSONObj const& msgUpdate,
bool multi,
bool upsert)
: msgUpdate(msgUpdate), multi(multi), upsert(upsert), cx(cx) {}

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

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

struct ExtOperatorUpsert : ConcreteInsertOp<ExtOperatorUpsert> {
Reference<UnboundCollectionContext> ucx;
bson::BSONObj selector;
bson::BSONObj update;

ExtOperatorUpsert(bson::BSONObj selector, bson::BSONObj update) : selector(selector), update(update) {}
ExtOperatorUpsert(Reference<UnboundCollectionContext> ucx, bson::BSONObj selector, bson::BSONObj update)
: selector(selector), update(update), ucx(ucx) {}

ACTOR static Future<Reference<IReadWriteContext>> upsertActor(ExtOperatorUpsert* self,
Reference<CollectionContext> cx) {
Expand All @@ -1411,7 +1404,7 @@ struct ExtOperatorUpsert : ConcreteInsertOp<ExtOperatorUpsert> {
thingToInsert = transformOperatorQueryToUpdatableDocument(self->selector);
}
state Reference<IReadWriteContext> dcx = wait(insertDocument(cx, thingToInsert, encodedIds));
wait(updateDocument(dcx, self->update, true, dcx->getKeyEncodedId()));
wait(updateDocument(dcx, self->ucx, self->update, true));
return dcx;
}

Expand Down Expand Up @@ -1442,15 +1435,20 @@ struct ExtSimpleUpsert : ConcreteInsertOp<ExtSimpleUpsert> {
}
};

Reference<IUpdateOp> operatorUpdate(bson::BSONObj const& msgUpdate) {
return ref(new ExtOperatorUpdate(msgUpdate));
Reference<IUpdateOp> operatorUpdate(Reference<UnboundCollectionContext> cx,
bson::BSONObj const& msgUpdate,
bool multi,
bool upsert) {
return ref(new ExtOperatorUpdate(cx, msgUpdate, multi, upsert));
}
Reference<IUpdateOp> replaceUpdate(bson::BSONObj const& replaceWith) {
return ref(new ExtReplaceUpdate(replaceWith));
}
Reference<IInsertOp> simpleUpsert(bson::BSONObj const& selector, bson::BSONObj const& update) {
return ref(new ExtSimpleUpsert(selector, update));
}
Reference<IInsertOp> operatorUpsert(bson::BSONObj const& selector, bson::BSONObj const& update) {
return ref(new ExtOperatorUpsert(selector, update));
Reference<IInsertOp> operatorUpsert(Reference<UnboundCollectionContext> cx,
bson::BSONObj const& selector,
bson::BSONObj const& update) {
return ref(new ExtOperatorUpsert(cx, selector, update));
}
10 changes: 7 additions & 3 deletions src/ExtMsg.actor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,6 @@ struct ExtMsgKillCursors : ExtMsg, FastAllocated<ExtMsgKillCursors> {
};

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

// FIXME: these don't really belong here either
Reference<IUpdateOp> operatorUpdate(bson::BSONObj const& msgUpdate);
Reference<IUpdateOp> operatorUpdate(Reference<UnboundCollectionContext> cx,
bson::BSONObj const& msgUpdate,
bool multi,
bool upsert);
Reference<IUpdateOp> replaceUpdate(bson::BSONObj const& replaceWith);
Reference<IInsertOp> simpleUpsert(bson::BSONObj const& selector, bson::BSONObj const& update);
Reference<IInsertOp> operatorUpsert(bson::BSONObj const& selector, bson::BSONObj const& update);
Reference<IInsertOp> operatorUpsert(Reference<UnboundCollectionContext> ucx,
bson::BSONObj const& selector,
bson::BSONObj const& update);

#endif /* _EXT_MSG_ACTOR_H_ */
34 changes: 34 additions & 0 deletions test/correctness/smoke/test_update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#

from collections import OrderedDict
from pymongo.errors import OperationFailure
import pytest


Expand Down Expand Up @@ -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)
34 changes: 34 additions & 0 deletions test/correctness/smoke/test_upsert.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down