PYTHON-2453 Add MongoDB Versioned API #536

ShaneHarvey · 2020-12-18T01:39:13Z

This PR adds support for the MongoDB Versioned API version 1: https://github.com/mongodb/specifications/blob/784857f/source/versioned-api/versioned-api.rst

TODO:

Add support for the server_api MongoClient option.
Send api version command arguments with all commands except getMore and commands that are continuing a multi-document transaction.
Implement spec tests via the unified format. (Tested locally so far with a server started with mongod --setParameter "requireApiVersion=1"). See: https://github.com/mongodb/specifications/tree/784857f548747ee6f894a5b6f7b6e5bb89103709/source/versioned-api/tests
Add documentation page for pymongo.server_api.

prashantmital

Can you edit the docstring for Database.command to specify that users must not explicitly add the serverApi key to the command document? This is mandated by the spec: https://github.com/mongodb/specifications/blob/master/source/versioned-api/versioned-api.rst#generic-command-helper
How does this implementation ensure that serverApi params don't get added to getMore commands?

.evergreen/config.yml

prashantmital · 2021-01-06T02:00:04Z

pymongo/pool.py

+        if (session and session.in_transaction and
+                not session._starting_transaction):
+            return
+        if self.opts.server_api:


To make this more explicitly adhere to the spec, I think we should use if self.opts.server_api is not None. Though the enum doesn't have any such value, the above comparison could evaluate to false if the value is set to something which evaluates to a bool false.

self.opts.server_api is either None or an instance of ServerApi so I'm not sure I follow.

My point here was that since the spec asks us to check if the user passed a value at all, that's exactly what we should do. Since None is the default for this kwarg, if <kwarg name> is not None is a (more) explicit check for whether it contains its default value.
It doesn't really affect behavior so if you prefer it the way it is, leave it.

If a user passes something that's not a ServerApi they will get an error when creating the client:

>>> MongoClient(server_api=False) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "pymongo/mongo_client.py", line 678, in __init__ keyword_opts.cased_key(k), v) for k, v in keyword_opts.items())) File "pymongo/mongo_client.py", line 678, in <genexpr> keyword_opts.cased_key(k), v) for k, v in keyword_opts.items())) File "pymongo/common.py", line 764, in validate value = validator(option, value) File "pymongo/common.py", line 537, in validate_server_api_or_none raise TypeError("%s must be an instance of ServerApi" % (option,)) TypeError: server_api must be an instance of ServerApi

prashantmital · 2021-01-06T03:27:46Z

pymongo/server_api.py

+    """Server API version "1"."""
+
+
+class ServerApi(object):


What do you think about declaring this as an overridden namedtuple instance much like CodecOptions? If you think that's a good idea, I think this would also benefit from having a with_options method like CodecOptions.

I don't see much value in making this a namedtuple. CodecOptions inherits from namedtuple to make it easier to work with in the C extensions. I also see no reason to add a with_options. The intended use case is to create this class once.

One benefit would be immuatability. I believe the spec mandates that this be an immutable object.

This class is already effectively immutable because there are no public attributes which can be mutated.

pymongo/server_api.py

prashantmital · 2021-01-06T03:39:47Z

test/__init__.py

@@ -751,6 +764,8 @@ def sanitize_cmd(cmd):
    cp.pop('$db', None)
    cp.pop('$readPreference', None)
    cp.pop('lsid', None)
+    # Versioned api parameters
+    cp.pop('apiVersion', None)


Why remove the serverApi as opped to skipping the monitoring tests when testing against versioned API?

If I recall, lot's of tests failed because of this. What do you think of:

Suggested change

cp.pop('apiVersion', None)

if MONGODB_API_VERSION:

cp.pop('apiVersion', None)

ShaneHarvey

Thanks for the review. I'll make the requested changes soon. Responding to a few of the comments now.

How does this implementation ensure that serverApi params don't get added to getMore commands?

We don't add serverApi to getMore commands because ... well... we don't add them. The pymongo.message._GetMore class does not call sock_info.add_server_api(command, session). The tests also verify this behavior.

ShaneHarvey · 2021-01-06T20:30:58Z

pymongo/pool.py

+        if (session and session.in_transaction and
+                not session._starting_transaction):
+            return
+        if self.opts.server_api:


self.opts.server_api is either None or an instance of ServerApi so I'm not sure I follow.

ShaneHarvey · 2021-01-06T20:34:16Z

pymongo/server_api.py

+    """Server API version "1"."""
+
+
+class ServerApi(object):


I don't see much value in making this a namedtuple. CodecOptions inherits from namedtuple to make it easier to work with in the C extensions. I also see no reason to add a with_options. The intended use case is to create this class once.

ShaneHarvey · 2021-01-06T20:37:07Z

test/__init__.py

@@ -751,6 +764,8 @@ def sanitize_cmd(cmd):
    cp.pop('$db', None)
    cp.pop('$readPreference', None)
    cp.pop('lsid', None)
+    # Versioned api parameters
+    cp.pop('apiVersion', None)


If I recall, lot's of tests failed because of this. What do you think of:

Suggested change

cp.pop('apiVersion', None)

if MONGODB_API_VERSION:

cp.pop('apiVersion', None)

ShaneHarvey

Can you edit the docstring for Database.command to specify that users must not explicitly add the serverApi key to the command document?

Done.

.evergreen/config.yml

pymongo/server_api.py

prashantmital · 2021-01-07T08:27:37Z

pymongo/pool.py

+        if (session and session.in_transaction and
+                not session._starting_transaction):
+            return
+        if self.opts.server_api:


My point here was that since the spec asks us to check if the user passed a value at all, that's exactly what we should do. Since None is the default for this kwarg, if <kwarg name> is not None is a (more) explicit check for whether it contains its default value.
It doesn't really affect behavior so if you prefer it the way it is, leave it.

prashantmital · 2021-01-07T08:28:17Z

pymongo/server_api.py

+    """Server API version "1"."""
+
+
+class ServerApi(object):


One benefit would be immuatability. I believe the spec mandates that this be an immutable object.

prashantmital · 2021-01-07T08:30:00Z

test/__init__.py

        if COMPRESSORS:
            self.default_client_options["compressors"] = COMPRESSORS
+        if MONGODB_API_VERSION:


Instead, should we check for REQUIRE_API_VERSION and then instantiate the server_api object using MONGODB_API_VERSION?

This is contingent on us keeping the REQUIRE_API_VERSION envvar. Seems like we might not need it?

See my comment above about the difference between the two variables: https://github.com/mongodb/mongo-python-driver/pull/536/files/cfd91bd63ec0dcfb2becdeb2a6481861caf0af7e#r553177142

prashantmital · 2021-01-07T08:31:42Z

.evergreen/config.yml

+        display_name: "requireApiVersion1"
+        tags: [ "requireApiVersion_tag" ]
+        variables:
+          REQUIRE_API_VERSION: "1"


What is the REQUIRE_API_VERSION variable used for? I don't see it used anywhere.

We need both variables. REQUIRE_API_VERSION tells drivers-evg-tools to start the cluster with the requireApiVersion parameter. MONGODB_API_VERSION tells the test suite which ServerApi.version to use. When we eventually test version 2, we'll need to use:

REQUIRE_API_VERSION: "1" MONGODB_API_VERSION: "2"

Ah, I see! That's certainly going to be hard to figure out for a newcomer. Can we add a comment that REQUIRE_API_VERSION is used by cluster orchestration scripts in drivers-evergreen-tools?

prashantmital

One suggested addition of a comment. Otherwise, LGTM

ShaneHarvey · 2021-01-12T01:05:06Z

Now that these changes were approved I'm going to rebase to fix the merge conflicts.

Support Unified Test Format version 1.1 (serverParameters in runOnRequirements)

…r improvements

Add pymongo.server_api.ServerApi and the MongoClient server_api option. Support Unified Test Format version 1.1 (serverParameters in runOnRequirements) Skip dropRole tests due to SERVER-53499. (cherry picked from commit ac2f506)

ShaneHarvey force-pushed the PYTHON-2453 branch from dfcf2cc to 30a43fe Compare December 22, 2020 23:13

ShaneHarvey marked this pull request as ready for review December 23, 2020 04:16

ShaneHarvey requested a review from prashantmital December 23, 2020 04:16

prashantmital suggested changes Jan 6, 2021

View reviewed changes

ShaneHarvey commented Jan 6, 2021

View reviewed changes

.evergreen/config.yml Outdated Show resolved Hide resolved

.evergreen/config.yml Show resolved Hide resolved

pymongo/server_api.py Show resolved Hide resolved

ShaneHarvey requested a review from prashantmital January 6, 2021 23:51

prashantmital suggested changes Jan 7, 2021

View reviewed changes

ShaneHarvey requested a review from prashantmital January 11, 2021 22:09

prashantmital approved these changes Jan 12, 2021

View reviewed changes

ShaneHarvey added 11 commits January 11, 2021 17:07

PYTHON-2453 Add MongoDB Versioned API

63432d0

Support Unified Test Format version 1.1 (serverParameters in runOnRequirements)

PYTHON-2453 Skip failing estimatedDocumentCount test DRIVERS-1437

c59a9b4

PYTHON-2453 Add requireApiVersion tests

9bf52bf

PYTHON-2453 Fix various test failures with requireApiVersion=1

cde00e3

PYTHON-2453 ServerApi is only supported on MongoDB 4.7+

9d77f97

PYTHON-2453 Skip dropRole tests due to SERVER-53499

66a1ada

PYTHON-2453 Correctly add ServerApi to all w=0 writes

99041b7

Remove cruft from evg config, fix require_no_api_version

d58596c

PYTHON-2453 Add documentation

a6002d3

PYTHON-2453 Document db.command interaction, requireApiVersion1, othe…

3db2688

…r improvements

PYTHON-2453 comment REQUIRE_API_VERSION and MONGODB_API_VERSION

3fa1f93

ShaneHarvey force-pushed the PYTHON-2453 branch from 7828c14 to 3fa1f93 Compare January 12, 2021 01:12

ShaneHarvey merged commit ac2f506 into mongodb:master Jan 12, 2021

ShaneHarvey deleted the PYTHON-2453 branch January 12, 2021 02:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PYTHON-2453 Add MongoDB Versioned API #536

PYTHON-2453 Add MongoDB Versioned API #536

ShaneHarvey commented Dec 18, 2020 •

edited

Loading

prashantmital left a comment

prashantmital Jan 6, 2021

ShaneHarvey Jan 6, 2021

prashantmital Jan 7, 2021

ShaneHarvey Jan 7, 2021

prashantmital Jan 6, 2021

ShaneHarvey Jan 6, 2021

prashantmital Jan 7, 2021

ShaneHarvey Jan 7, 2021

prashantmital Jan 6, 2021

ShaneHarvey Jan 6, 2021

ShaneHarvey left a comment

ShaneHarvey Jan 6, 2021

ShaneHarvey Jan 6, 2021

ShaneHarvey Jan 6, 2021

ShaneHarvey left a comment

prashantmital Jan 7, 2021

prashantmital Jan 7, 2021

prashantmital Jan 7, 2021

prashantmital Jan 7, 2021

ShaneHarvey Jan 7, 2021

prashantmital Jan 7, 2021

ShaneHarvey Jan 7, 2021

prashantmital Jan 12, 2021

ShaneHarvey Jan 12, 2021

prashantmital left a comment

ShaneHarvey commented Jan 12, 2021

	cp.pop('apiVersion', None)
	if MONGODB_API_VERSION:
	cp.pop('apiVersion', None)

PYTHON-2453 Add MongoDB Versioned API #536

PYTHON-2453 Add MongoDB Versioned API #536

Conversation

ShaneHarvey commented Dec 18, 2020 • edited Loading

prashantmital left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashantmital left a comment

Choose a reason for hiding this comment

ShaneHarvey commented Jan 12, 2021

ShaneHarvey commented Dec 18, 2020 •

edited

Loading