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

Fix the bug where dropIndex command is not replying all necessary fie… #175

Merged
merged 1 commit into from
Apr 24, 2019
Merged

Fix the bug where dropIndex command is not replying all necessary fie… #175

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
6 changes: 5 additions & 1 deletion src/ExtCmd.actor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,11 @@ ACTOR static Future<Reference<ExtMsgReply>> doDropIndexesActor(Reference<ExtConn
reply->addDocument(BSON("nIndexesWas" << dropped + 1 << "ok" << 1.0));
return reply;
} else {
reply->addDocument(BSON("ok" << 0.0));
// clang-format off
reply->addDocument(BSON("ok" << 0.0 <<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't change this. Can you make sure if ok value needs to be float or int?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both are fine. I believe in most main stream languages, user can compare double, float, int and long with each other without having to explicitly cast the types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And our code base does have mixed usage of double and int, and sometime long. We'd better clean that up too someday.

Copy link
Contributor

@apkar apkar Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please raise an issue for that? Also, OP_REPLY response flag need to be set apparently for $err to work. We do set for OP_GETMORE but nowhere else. You might want to mention that in the issue.

https://docs.mongodb.com/v3.0/reference/mongodb-wire-protocol/#wire-op-reply

"$err" << "'index' must be a string or an object" <<
"errmsg" << "'index' must be a string or an object"));
// clang-format on
return reply;
}
} else {
Expand Down
11 changes: 11 additions & 0 deletions test/correctness/smoke/test_errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#

from pymongo.errors import OperationFailure
from bson.son import SON

def test_error_reply(fixture_db):
try:
Expand All @@ -30,3 +31,13 @@ def test_error_reply(fixture_db):
serverErrObj = e.details
assert serverErrObj['$err'] != None
assert "no such cmd: sometotallyrandomcmd" in serverErrObj['$err']

def test_drop_index_cmd_error_reply(fixture_db):
try:
fixture_db.command(SON([("dropIndexes", "testcoll"), ("index", 1)]))
except Exception as e:
assert isinstance(e, OperationFailure)
serverErrObj = e.details
assert serverErrObj['$err'] != None
assert serverErrObj['errmsg'] != None