Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 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
2 changes: 2 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ language: python
services:
- mongodb

install: python -m pip install https://github.com/mongodb/mongo-python-driver/archive/3.11.0rc0.tar.gz

python:
- 3.5
- 3.6
Expand Down
44 changes: 37 additions & 7 deletions pymongoexplain/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

from bson.son import SON
from pymongo.collection import Collection
from pymongo.helpers import _index_document
from .utils import convert_to_camelcase


Expand All @@ -39,18 +40,25 @@ 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


class UpdateCommand(BaseCommand):
def __init__(self, collection: Collection, filter, update,
kwargs):
super().__init__(collection.name)
return_document = {"updates":[{"q": filter, "u": update}]}
for key in kwargs:
value = kwargs[key]
return_document = {
"updates":[{"q": filter, "u": update}]
}
for key, value in kwargs.items():
if key == "bypass_document_validation":
return_document[key] = value
elif key == "hint":
if value is not {} and value is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need the {} check here. It's invalid for hint to be {}:

>>>> client.t.t.update_one({}, {'$set': {'a':1}}, hint={})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 1024, in update_one
    hint=hint, session=session),
  File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 870, in _update_retryable
    _update, session)
  File "/Users/shane/git/mongo-python-driver/pymongo/mongo_client.py", line 1497, in _retryable_write
    return self._retry_with_session(retryable, func, s, None)
  File "/Users/shane/git/mongo-python-driver/pymongo/mongo_client.py", line 1383, in _retry_with_session
    return self._retry_internal(retryable, func, session, bulk)
  File "/Users/shane/git/mongo-python-driver/pymongo/mongo_client.py", line 1415, in _retry_internal
    return func(session, sock_info, retryable)
  File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 866, in _update
    retryable_write=retryable_write)
  File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 806, in _update
    hint = helpers._index_document(hint)
  File "/Users/shane/git/mongo-python-driver/pymongo/helpers.py", line 87, in _index_document
    "mean %r?" % list(iteritems(index_list)))
TypeError: passing a dict to sort/create_index/hint is not allowed - use a list of tuples instead. did you mean []?

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.

return_document["updates"][0]["hint"] = value if \
isinstance(value, str) else _index_document(value)
else:
return_document["updates"][0][key] = value
self.command_document = convert_to_camelcase(return_document)
Expand Down Expand Up @@ -81,7 +89,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 All @@ -108,6 +121,8 @@ class FindCommand(BaseCommand):
def __init__(self, collection: Collection,
kwargs):
super().__init__(collection.name)
if kwargs["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.

This looks like a bug (or it's masking a bug). Let's remove this code and address the failing test in a different way. Please post the test failure here.

For reference this is how pymongo generates a find command:
https://github.com/mongodb/mongo-python-driver/blob/3.11.0rc0/pymongo/message.py#L181-L217

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought that might be a bug because it doesn't really make sense. Removing that doesn't seem to cause any additional test failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The three different tests that fail are below, they are in the format of <the command we constructed>, <the command that was expected>

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

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 the entire failed test output though? Like this:

FAIL: test_keyword_arg_defaults (test.test_client.ClientUnitTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/shane/git/mongo-python-driver/test/test_client.py", line 128, in test_keyword_arg_defaults
    self.assertEqual(20.1, pool_opts.connect_timeout)
AssertionError: 20.1 != 20.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Error
Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
    yield
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 676, in run
    self._callTestMethod(testMethod)
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
    method()
  File "/Users/julius/pymongoexplain/test/__init__.py", line 442, in wrap
    return f(*args, **kwargs)
  File "/Users/julius/pymongoexplain/test/test_crud_v2.py", line 62, in run_scenario
    self.run_scenario(scenario_def, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 615, in run_scenario
    self.run_test_ops(sessions, collection, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 525, in run_test_ops
    self.run_operations(sessions, collection, test['operations'])
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 431, in run_operations
    result = self.run_operation(sessions, collection, op.copy())
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 374, in run_operation
    self._compare_command_dicts(wrapped_collection.last_cmd_payload,
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 269, in _compare_command_dicts
    self.assertEqual(ours[key], theirs[key])
KeyError: 'find
SON([('find', 'test_find_allowdiskuse'), ('allowDiskUse', True), ('filter', {})])
{}

Error
Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
    yield
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 676, in run
    self._callTestMethod(testMethod)
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
    method()
  File "/Users/julius/pymongoexplain/test/__init__.py", line 442, in wrap
    return f(*args, **kwargs)
  File "/Users/julius/pymongoexplain/test/test_crud_v2.py", line 62, in run_scenario
    self.run_scenario(scenario_def, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 615, in run_scenario
    self.run_test_ops(sessions, collection, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 525, in run_test_ops
    self.run_operations(sessions, collection, test['operations'])
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 431, in run_operations
    result = self.run_operation(sessions, collection, op.copy())
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 374, in run_operation
    self._compare_command_dicts(wrapped_collection.last_cmd_payload,
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 269, in _compare_command_dicts
    self.assertEqual(ours[key], theirs[key])
KeyError: 'find'
SON([('find', 'test_find_allowdiskuse'), ('allowDiskUse', False), ('filter', {})])
{}

Error
Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
    yield
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 676, in run
    self._callTestMethod(testMethod)
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
    method()
  File "/Users/julius/pymongoexplain/test/__init__.py", line 442, in wrap
    return f(*args, **kwargs)
  File "/Users/julius/pymongoexplain/test/test_crud_v2.py", line 62, in run_scenario
    self.run_scenario(scenario_def, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 615, in run_scenario
    self.run_test_ops(sessions, collection, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 525, in run_test_ops
    self.run_operations(sessions, collection, test['operations'])
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 431, in run_operations
    result = self.run_operation(sessions, collection, op.copy())
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 374, in run_operation
    self._compare_command_dicts(wrapped_collection.last_cmd_payload,
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 269, in _compare_command_dicts
    self.assertEqual(ours[key], theirs[key])
KeyError: 'find'

for key, value in kwargs.items():
self.command_document[key] = value
self.command_document = convert_to_camelcase(self.command_document)
Expand All @@ -116,12 +131,21 @@ def __init__(self, collection: Collection,
def command_name(self):
return "find"


class FindAndModifyCommand(BaseCommand):
def __init__(self, collection: Collection,
kwargs):
super().__init__(collection.name)
for key, value in kwargs.items():
self.command_document[key] = value
if key == "update" and kwargs.get("replacement", None) is not None:
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 you can remove all the "replacement" references here because we never pass "replacement" through kwargs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replacement is passed through kwargs for 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.

Nevermind, I was confused. Fixed it now.

continue
if key == "hint":
self.command_document["hint"] = value if \
isinstance(value, str) else _index_document(value)
elif key == "replacement":
self.command_document["update"] = value
else:
self.command_document[key] = value
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 _index_document(value)
else:
self.command_document[key] = value

self.command_document = convert_to_camelcase(self.command_document)

@property
Expand Down
23 changes: 15 additions & 8 deletions pymongoexplain/explainable_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,25 @@
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
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
Expand Down Expand Up @@ -63,7 +68,7 @@ def distinct(self, key: str, filter: Document=None, session=None, **kwargs):

def aggregate(self, pipeline: List[Document], session=None, **kwargs):
command = AggregateCommand(self.collection, pipeline, session,
{},kwargs)
{}, kwargs)
return self._explain_command(command)

def estimated_document_count(self,
Expand All @@ -76,7 +81,7 @@ def count_documents(self, filter: Document, session=None,
**kwargs):

command = AggregateCommand(self.collection, [{'$match': filter},
{'$group': {'n': {'$sum': 1}, '_id': 1}}],
{'$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?

return self._explain_command(command)
Expand All @@ -95,7 +100,7 @@ def delete_many(self, filter: Document, collation=None,
limit = 0
kwargs["session"] = session
command = DeleteCommand(self.collection, filter, limit, collation,
kwargs)
kwargs)
return self._explain_command(command)

def watch(self, pipeline: Document = None, full_document: Document = None,
Expand Down Expand Up @@ -124,7 +129,8 @@ def find(self, filter: Document = None,
kwargs.update(locals())
del kwargs["self"], kwargs["kwargs"]
command = FindCommand(self.collection,
kwargs)
kwargs)

return self._explain_command(command)

def find_one(self, filter: Document = None, **kwargs: Dict[str,
Expand All @@ -149,29 +155,30 @@ def find_one_and_delete(self, filter: Document, projection: list = None,
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,
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,
Expand Down
4 changes: 2 additions & 2 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
'console_scripts': [
'pymongoexplain=pymongoexplain.cli_explain:cli_explain'],
},
tests_require=["pymongo==3.10.1"],
install_requires=['pymongo==3.10.1'],
tests_require=["pymongo==3.11.0rc0"],
install_requires=['pymongo==3.11.0rc0'],
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 this should be pymongo>=3.11

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.

python_requires='>=3.5',
license="Apache License, Version 2.0",
classifiers=[
Expand Down
Loading