Skip to content

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

Merged
merged 26 commits into from
Jul 29, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
38 changes: 34 additions & 4 deletions pymongoexplain/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Copy link
Collaborator

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?

Copy link
Contributor Author

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 {}

Copy link
Collaborator

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?

Copy link
Contributor Author

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:

SON([('find', 'test_find_allowdiskuse'), ('filter', {})])
{}

Copy link
Collaborator

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?

return cmd


Expand All @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

@ShaneHarvey ShaneHarvey Jul 22, 2020

Choose a reason for hiding this comment

The 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 sort.

else:
return_document["updates"][0][key] = value
if return_document["updates"][0].get("hint", True) == {}:
del return_document["updates"][0]["hint"]
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you post those failures here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ran 106 tests in 11.719s

FAILED (failures=1, skipped=42)
SON([('update', 'test'), ('updates', [{'q': {'_id': 1}, 'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}], 'upsert': False, 'hint': {}, 'multi': False}])])
SON([('update', 'test'), ('ordered', True), ('lsid', {'id': Binary(b'\xca\xa9L\x9d\x84\x86L\xc7\x9f\xfa\x8cE4\xa8Kq', 4)}), ('txnNumber', 1), ('$clusterTime', {'clusterTime': Timestamp(1595449666, 16), 'signature': {'hash': b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00', 'keyId': 0}}), ('$db', 'crud-tests'), ('$readPreference', {'mode': 'primary'}), ('updates', [SON([('q', {'_id': 1}), ('u', [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}]), ('multi', False), ('upsert', False)])])])


[{'multi': False,
  'q': {'_id': 1},
  'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}],
  'upsert': False}] != [{'hint': {},
  'multi': False,
  'q': {'_id': 1},
  'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}],
  'upsert': False}]

self.command_document = convert_to_camelcase(return_document)

@property
Expand Down Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to check for None too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Expand Down Expand Up @@ -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 = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this correct? What about all the other find command options? Like:

coll.find({}, limit=10)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a test for coll.find({}, limit=10) to ensure that the limit field (and other command args) are not removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"]
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

    command_document = {
        "findAndModify": collection_name,
        "query": filter,
        "new":  return_document,
        "update": replacement,
    }
    collation = validate_collation_or_none(kwargs.pop('collation', None))
    if collation is not None:
        cmd["collation"] = collation
    if projection is not None:
        cmd["fields"] = helpers._fields_list_to_dict(projection,
                                                     "projection")
    if sort is not None:
        cmd["sort"] = helpers._index_document(sort)
    if upsert is not None:
        common.validate_boolean("upsert", upsert)
        cmd["upsert"] = upsert
    if hint is not None:
        if not isinstance(hint, str):
            hint = helpers._index_document(hint)
        cmd['hint'] = hint
    if array_filters is not None:
        cmd["arrayFilters"] = array_filters

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 collation or hint), and sometimes the python argument name is different from the command argument name (like replacement -> update).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand All @@ -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
Expand Down
53 changes: 30 additions & 23 deletions pymongoexplain/explainable_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {}
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 ({}).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand All @@ -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,
Expand All @@ -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())
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is exclude_keys?

Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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})
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)

Expand All @@ -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)

Expand Down
Loading