PYTHON-2957 Support 'let' option for multiple CRUD commands #804
Conversation
pymongo/collection.py
Outdated
| @@ -619,6 +619,9 @@ def _update(self, sock_info, criteria, document, upsert=False, | |||
| command = SON([('update', self.name), | |||
| ('ordered', ordered), | |||
| ('updates', [update_doc])]) | |||
| if let: | |||
| common.validate_is_document_type("let", let) | |||
There was a problem hiding this comment.
Should this be the more generic validate_is_mapping instead?
There was a problem hiding this comment.
Also can we add a test to make sure that validation works?
pymongo/collection.py
Outdated
| @@ -795,6 +798,8 @@ def update_one(self, filter, update, upsert=False, | |||
| MongoDB 4.2 and above. | |||
| - `session` (optional): a | |||
| :class:`~pymongo.client_session.ClientSession`. | |||
| - `let` (optional): Specifies a document with a list of variables | |||
| that can then be accessed using the form `$$<variable_name>`. | |||
There was a problem hiding this comment.
Hmm my first impression is that this is very vague. Are there any docs that we can link to? Or any documentation guidance in the CRUD spec?
There was a problem hiding this comment.
I could link to this: https://docs.mongodb.com/manual/reference/method/db.collection.update/#std-label-db.collection.update-let-example but that also doesn't give much info.
There was a problem hiding this comment.
Can you update this with the docs from the CRUD spec?:
Map of parameter names and values. Values must be constant or closed
expressions that do not reference document fields. Parameters can then be
accessed as variables in an aggregate expression context (e.g. "$$var").
I think this version more clearly describes the api.
| @@ -752,7 +755,7 @@ def replace_one(self, filter, replacement, upsert=False, | |||
| def update_one(self, filter, update, upsert=False, | |||
| bypass_document_validation=False, | |||
| collation=None, array_filters=None, hint=None, | |||
| session=None): | |||
| session=None, let=None): | |||
There was a problem hiding this comment.
For all the helpers that didn't support "let" before, can you add a versionchanged:: 4.1 section?
| @@ -2236,6 +2254,8 @@ def __find_and_modify(self, filter, projection, sort, upsert=None, | |||
| cmd = SON([("findAndModify", self.__name), | |||
| ("query", filter), | |||
| ("new", return_document)]) | |||
| if let: | |||
| cmd["let"] = let | |||
There was a problem hiding this comment.
Missing a validate_is_mapping?
test/test_collection.py
Outdated
| @client_context.require_version_min(5, 0, 0) | ||
| def test_helpers_with_let(self): | ||
| c = self.db.test | ||
| helpers = [("delete_many", ({}, {})), ("delete_one", ({}, {})), |
There was a problem hiding this comment.
Please refactor this to use the methods instead of strings, eg:
helpers = [(c.delete_many,...
It makes it easier to find usages of the method.
test/test_collection.py
Outdated
| for helper, args in helpers: | ||
| with self.assertRaises(TypeError) as cm: | ||
| getattr(c, helper)(*args, let=let) | ||
| self.assertIn("let must be an instance of dict", |
There was a problem hiding this comment.
Let's use self.assertRaisesRegex instead: https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaisesRegex
pymongo/collection.py
Outdated
| @@ -795,6 +798,8 @@ def update_one(self, filter, update, upsert=False, | |||
| MongoDB 4.2 and above. | |||
| - `session` (optional): a | |||
| :class:`~pymongo.client_session.ClientSession`. | |||
| - `let` (optional): Specifies a document with a list of variables | |||
| that can then be accessed using the form `$$<variable_name>`. | |||
There was a problem hiding this comment.
Can you update this with the docs from the CRUD spec?:
Map of parameter names and values. Values must be constant or closed
expressions that do not reference document fields. Parameters can then be
accessed as variables in an aggregate expression context (e.g. "$$var").
I think this version more clearly describes the api.
No description provided.