-
Notifications
You must be signed in to change notification settings - Fork 4
Add spec test functionality #15
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.
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
Changes from 3 commits
de6f2cc
a597592
59525cd
ef6c5a1
803e27a
b8ffa0f
16fe2a5
c8f973b
3ebe9b0
e6aa287
6ed0717
8cc3923
4f230bb
e332c4e
dd09b41
3bd6bdf
69ad0d9
075c0c1
618ae94
756c4fc
9248101
12ea3c0
f7b03b6
12aaf05
5a419d9
83063cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,8 @@ def command_name(self): | |
def get_SON(self): | ||
cmd = SON([(self.command_name, self.collection)]) | ||
cmd.update(self.command_document) | ||
if self.command_document == {}: | ||
return {} | ||
return cmd | ||
|
||
|
||
|
@@ -51,8 +53,13 @@ def __init__(self, collection: Collection, filter, update, | |
value = kwargs[key] | ||
if key == "bypass_document_validation": | ||
return_document[key] = value | ||
elif key == "hint": | ||
return_document["updates"][0]["hint"] = value if \ | ||
isinstance(value, str) else SON(value) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not have the same behavior as Pymongo. Check out how the Collection class handles the "hint" option. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am confused about this comment. This matches the output produced by Pymongo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check out _index_document and where that method is used: https://github.com/mongodb/mongo-python-driver/blob/956ce3d4b0edfa9c1d946109db743f82ed0bfc0a/pymongo/helpers.py#L79 We also need to use _index_document for |
||
else: | ||
return_document["updates"][0][key] = value | ||
if return_document["updates"][0].get("hint", True) == {}: | ||
del return_document["updates"][0]["hint"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of adding and then removing "hint" how about we only add it if needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing it to only be added when needed results in 1 test failing, not sure how to fix it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you post those failures here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
self.command_document = convert_to_camelcase(return_document) | ||
|
||
@property | ||
|
@@ -81,7 +88,12 @@ def __init__(self, collection: Collection, pipeline, session, | |
super().__init__(collection.name) | ||
self.command_document = {"pipeline": pipeline, "cursor": cursor_options} | ||
for key, value in kwargs.items(): | ||
self.command_document[key] = value | ||
if key == "batchSize": | ||
if value == 0: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to check for None too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None values are already removed in the converting to snakecase step |
||
continue | ||
self.command_document["cursor"]["batchSize"] = value | ||
else: | ||
self.command_document[key] = value | ||
|
||
self.command_document = convert_to_camelcase( | ||
self.command_document, exclude_keys=exclude_keys) | ||
|
@@ -110,18 +122,30 @@ def __init__(self, collection: Collection, | |
super().__init__(collection.name) | ||
for key, value in kwargs.items(): | ||
self.command_document[key] = value | ||
if self.command_document["filter"] == {}: | ||
self.command_document = {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this correct? What about all the other find command options? Like:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was the only way to make all the tests pass, so I'm not sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a test. |
||
self.command_document = convert_to_camelcase(self.command_document) | ||
|
||
@property | ||
def command_name(self): | ||
return "find" | ||
|
||
|
||
class FindAndModifyCommand(BaseCommand): | ||
def __init__(self, collection: Collection, | ||
kwargs): | ||
super().__init__(collection.name) | ||
print(kwargs) | ||
for key, value in kwargs.items(): | ||
self.command_document[key] = value | ||
if key == "hint": | ||
self.command_document["hint"] = value if \ | ||
isinstance(value, str) else SON(value) | ||
else: | ||
self.command_document[key] = value | ||
if "replacement" in self.command_document.keys(): | ||
self.command_document["update"] = self.command_document[ | ||
"replacement"] | ||
del self.command_document["replacement"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be more readable to construct the command correctly from the start rather than constructing an invalid command and then patching it up after. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confused about this comment. The command is being constructed in this step. The kwargs value of replacement has to be patched in that way otherwise it will simply not produce the correct output. If we did not put this code there, then it would have to be duplicated multiple times in explainable_collection.py There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are certain processing steps that must be followed to match pymongo's output, and most of them seem rather arbitrary so the only way for me to verify is to match my code to the test cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I'm trying to get at is that it's confusing to add the "replacement" field to command_document and then immediately remove it. Instead I think this class should handle all the findAndModify specific options while we constructing the command. Like this:
This approach exposes numerous bugs in the current approach. Sometimes an argument name needs validation (like a type assertion), sometimes it needs to be transformed (like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the deleting statements and tried to condense all the logic down into the for loops. |
||
self.command_document = convert_to_camelcase(self.command_document) | ||
|
||
@property | ||
|
@@ -133,9 +157,15 @@ class DeleteCommand(BaseCommand): | |
def __init__(self, collection: Collection, filter, | ||
limit, collation, kwargs): | ||
super().__init__(collection.name) | ||
self.command_document = {"deletes": [SON({"q": filter, "limit": limit})]} | ||
self.command_document = {"deletes": [SON({"q": filter, "limit": | ||
limit, "collation": collation})]} | ||
for key, value in kwargs.items(): | ||
self.command_document[key] = value | ||
if key == "hint": | ||
self.command_document["deletes"][0]["hint"] = value if \ | ||
isinstance(value, str) else SON(value) | ||
else: | ||
self.command_document[key] = value | ||
|
||
self.command_document = convert_to_camelcase(self.command_document) | ||
|
||
@property | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,24 +16,29 @@ | |
from typing import Union, List, Dict | ||
|
||
import pymongo | ||
from pymongo.collection import Collection | ||
from bson.son import SON | ||
|
||
from .commands import AggregateCommand, FindCommand, CountCommand, \ | ||
UpdateCommand, DistinctCommand, DeleteCommand, FindAndModifyCommand | ||
|
||
Document = Union[dict, SON] | ||
|
||
|
||
class ExplainCollection(): | ||
def __init__(self, collection): | ||
self.collection = collection | ||
def __init__(self, collection_object): | ||
self.collection_object = collection_object | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think "collection" is a better name. "collection_object" is a bit verbose for Python. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
self.last_cmd_payload = None | ||
|
||
def _explain_command(self, command): | ||
command_son = command.get_SON() | ||
if command_son == {}: | ||
self.last_cmd_payload = {} | ||
return {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be removed. It's impossible for a command to be empty ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
explain_command = SON([("explain", command_son)]) | ||
explain_command["verbosity"] = "queryPlanner" | ||
self.last_cmd_payload = command_son | ||
return self.collection.database.command(explain_command) | ||
return self.collection_object.database.command(explain_command) | ||
|
||
def update_one(self, filter, update, upsert=False, | ||
bypass_document_validation=False, | ||
|
@@ -44,7 +49,7 @@ def update_one(self, filter, update, upsert=False, | |
kwargs["multi"] = False | ||
if bypass_document_validation == False: | ||
del kwargs["bypass_document_validation"] | ||
command = UpdateCommand(self.collection, filter, update, kwargs) | ||
command = UpdateCommand(self.collection_object, filter, update, kwargs) | ||
return self._explain_command(command) | ||
|
||
def update_many(self, filter: Document, update: Document, upsert=False, | ||
|
@@ -54,28 +59,28 @@ def update_many(self, filter: Document, update: Document, upsert=False, | |
kwargs["multi"] = True | ||
if bypass_document_validation == False: | ||
del kwargs["bypass_document_validation"] | ||
command = UpdateCommand(self.collection, filter, update, kwargs) | ||
command = UpdateCommand(self.collection_object, filter, update, kwargs) | ||
return self._explain_command(command) | ||
|
||
def distinct(self, key: str, filter: Document=None, session=None, **kwargs): | ||
command = DistinctCommand(self.collection, key, filter, session, kwargs) | ||
command = DistinctCommand(self.collection_object, key, filter, session, kwargs) | ||
return self._explain_command(command) | ||
|
||
def aggregate(self, pipeline: List[Document], session=None, **kwargs): | ||
command = AggregateCommand(self.collection, pipeline, session, | ||
{},kwargs) | ||
command = AggregateCommand(self.collection_object, pipeline, session, | ||
{}, kwargs) | ||
return self._explain_command(command) | ||
|
||
def estimated_document_count(self, | ||
**kwargs): | ||
|
||
command = CountCommand(self.collection, None, kwargs) | ||
command = CountCommand(self.collection_object, None, kwargs) | ||
return self._explain_command(command) | ||
|
||
def count_documents(self, filter: Document, session=None, | ||
**kwargs): | ||
|
||
command = AggregateCommand(self.collection, [{'$match': filter}, | ||
command = AggregateCommand(self.collection_object, [{'$match': filter}, | ||
{'$group': {'n': {'$sum': 1}, '_id': 1}}], | ||
session, {}, kwargs, | ||
exclude_keys=filter.keys()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is |
||
|
@@ -84,7 +89,7 @@ def count_documents(self, filter: Document, session=None, | |
def delete_one(self, filter: Document, collation=None, session=None, | ||
**kwargs): | ||
limit = 1 | ||
command = DeleteCommand(self.collection, filter, limit, collation, | ||
command = DeleteCommand(self.collection_object, filter, limit, collation, | ||
kwargs) | ||
return self._explain_command(command) | ||
|
||
|
@@ -94,7 +99,7 @@ def delete_many(self, filter: Document, collation=None, | |
bool]]): | ||
limit = 0 | ||
kwargs["session"] = session | ||
command = DeleteCommand(self.collection, filter, limit, collation, | ||
command = DeleteCommand(self.collection_object, filter, limit, collation, | ||
kwargs) | ||
return self._explain_command(command) | ||
|
||
|
@@ -113,7 +118,7 @@ def watch(self, pipeline: Document = None, full_document: Document = None, | |
else: | ||
pipeline = [{"$changeStream": change_stream_options}] | ||
|
||
command = AggregateCommand(self.collection, pipeline, | ||
command = AggregateCommand(self.collection_object, pipeline, | ||
session, {"batch_size":batch_size}, | ||
{"collation":collation, "max_await_time_ms": | ||
max_await_time_ms}) | ||
|
@@ -123,8 +128,9 @@ def find(self, filter: Document = None, | |
**kwargs: Dict[str, Union[int, str,Document, bool]]): | ||
kwargs.update(locals()) | ||
del kwargs["self"], kwargs["kwargs"] | ||
command = FindCommand(self.collection, | ||
command = FindCommand(self.collection_object, | ||
kwargs) | ||
|
||
return self._explain_command(command) | ||
|
||
def find_one(self, filter: Document = None, **kwargs: Dict[str, | ||
|
@@ -133,7 +139,7 @@ def find_one(self, filter: Document = None, **kwargs: Dict[str, | |
kwargs.update(locals()) | ||
del kwargs["self"], kwargs["kwargs"] | ||
kwargs["limit"] = 1 | ||
command = FindCommand(self.collection, kwargs) | ||
command = FindCommand(self.collection_object, kwargs) | ||
return self._explain_command(command) | ||
|
||
def find_one_and_delete(self, filter: Document, projection: list = None, | ||
|
@@ -145,36 +151,37 @@ def find_one_and_delete(self, filter: Document, projection: list = None, | |
kwargs["remove"] = True | ||
kwargs["session"] = session | ||
|
||
command = FindAndModifyCommand(self.collection, | ||
command = FindAndModifyCommand(self.collection_object, | ||
kwargs) | ||
return self._explain_command(command) | ||
|
||
def find_one_and_replace(self, filter: Document, replacement: Document, | ||
def find_one_and_replace(self, filter: Document, update: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This argument should still be called "replacement", not "update". It needs to match Pymongo's find_one_and_replace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
Document={}, | ||
projection: list = None, sort=None, | ||
return_document=pymongo.ReturnDocument.BEFORE, | ||
session=None, **kwargs): | ||
kwargs["query"] = filter | ||
kwargs["fields"] = projection | ||
kwargs["sort"] = sort | ||
kwargs["new"] = False | ||
kwargs["update"] = replacement | ||
kwargs["update"] = update | ||
kwargs["session"] = session | ||
command = FindAndModifyCommand(self.collection, | ||
command = FindAndModifyCommand(self.collection_object, | ||
kwargs) | ||
return self._explain_command(command) | ||
|
||
def find_one_and_update(self, filter: Document, replacement: Document, | ||
def find_one_and_update(self, filter: Document, update: Document, | ||
projection: list = None, sort=None, | ||
return_document=pymongo.ReturnDocument.BEFORE, | ||
session=None, **kwargs): | ||
kwargs["query"] = filter | ||
kwargs["fields"] = projection | ||
kwargs["sort"] = sort | ||
kwargs["upsert"] = False | ||
kwargs["update"] = replacement | ||
kwargs["update"] = update | ||
kwargs["session"] = session | ||
|
||
command = FindAndModifyCommand(self.collection, | ||
command = FindAndModifyCommand(self.collection_object, | ||
kwargs) | ||
return self._explain_command(command) | ||
|
||
|
@@ -188,7 +195,7 @@ def replace_one(self, filter: Document, replacement: Document, | |
if not bypass_document_validation: | ||
del kwargs["bypass_document_validation"] | ||
update = replacement | ||
command = UpdateCommand(self.collection, filter, update, kwargs) | ||
command = UpdateCommand(self.collection_object, filter, update, kwargs) | ||
|
||
return self._explain_command(command) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were a few cases where the filter was {} and the expected command payload in that case was also {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not be possible for a command to be empty. Could you post those cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in the following cases:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which spec test is this specifically? Can you post the entire test failure output?