PYTHON-2823 Allow custom service names with srvServiceName URI option #749

juliusgeo · 2021-09-29T22:29:59Z

No description provided.

behackett

You need to add documentation for the new option in the docstring for MongoClient, and a mention in the changelog.

behackett · 2021-10-04T23:20:35Z

pymongo/srv_resolver.py

        try:
-            results = _resolve('_mongodb._tcp.' + self.__fqdn, 'SRV',
-                               lifetime=self.__connect_timeout)
+            results = _resolve('_'+str(self.__srv)+'._tcp.' + self.__fqdn,


Add spaces around the '+' characters. Also, why do you have to wrap self.__srv in str? Is it being converted to a bytes before it gets here?

I think you are right and we can remove that.

behackett · 2021-10-04T23:21:12Z

pymongo/uri_parser.py

                    options[opt] = val
        if "tls" not in options and "ssl" not in options:
            options["tls"] = True if validate else 'true'
+    #elif not is_srv and options.get("srvServiceName", None) is not None:


Remove the dead code.

This isn't dead code, it was to fix the test that was testing for a configuration to be raised under those conditions. So I restored it.

ShaneHarvey

From a grep through the codebase we also need to pass the new option to the _SrvResolver in pymongo/monitor.py. It's somewhat concerning that the tests pass without this change (but not your fault at all of course). Does the spec change have any test coverage for mongos SRV polling?

ShaneHarvey · 2021-10-05T20:36:36Z

pymongo/mongo_client.py

+        Starting in version 4.0 you can pass a custom SRV service name by
+        using the ``srvServiceName`` option like so::
+
+            MongoClient("mongodb+srv://test22.test.build.10gen.cc/?srvServiceName=customname")


This should be documented at the bottom of the **Other optional parameters can be passed as keyword arguments:** section below. Please also note the new parameter name in the existing versonchanged:: 4.0

ShaneHarvey · 2021-10-05T20:38:11Z

pymongo/srv_resolver.py

        nodes = [
            (maybe_decode(res.target.to_text(omit_final_dot=True)), res.port)
            for res in results]
-


This change is uneeded.

ShaneHarvey · 2021-10-05T20:42:22Z

pymongo/srv_resolver.py

 class _SrvResolver(object):
-    def __init__(self, fqdn, connect_timeout=None):
+    def __init__(self, fqdn,
+                 connect_timeout=None, srv_service_name="mongodb"):


It looks like we should always pass connect_timeout and srv_service_name when creating a _SrvResolver. Can you make both of them required arguments?

So I'm not sure how to get the value of srv_service_name from within monitor.py where _SrvResolver is called, so I don't know how to make them required arguments because I don't know where the values that should go in those arguments are stored.

Nevermind I figured out the plumbing. However, I don't think that connect_timeout is always passed when creating a _SrvResolver.

connect_timeout needs to always be passed. The fact that it's not already is an oversight. Please update SrvMonitor to get the timeout from topology_settings (self._settings).

ShaneHarvey · 2021-10-05T20:43:47Z

pymongo/srv_resolver.py

-            results = _resolve('_mongodb._tcp.' + self.__fqdn, 'SRV',
-                               lifetime=self.__connect_timeout)
+            results = _resolve('_' + self.__srv+'._tcp.' + self.__fqdn,
+                                   'SRV', lifetime=self.__connect_timeout)


Please match the surrounding code style. Second line is indented too much. Spaces around +.

ShaneHarvey · 2021-10-05T20:47:51Z

pymongo/uri_parser.py

-
+    # get the srvservicename from options so that we can pass it to
+    # _SrvResolver
+    # constructor below


This comment is unneeded. Writing useful comments is a tough skill to learn. Usually you want a comment to explain some tricky or non-obvious code (which this is not) or to explain why the code is needed (which is obvious here). Please read this blog for some good guidelines: https://stackoverflow.blog/2021/07/05/best-practices-for-writing-code-comments/

ShaneHarvey · 2021-10-05T20:48:54Z

pymongo/uri_parser.py

                    options[opt] = val
        if "tls" not in options and "ssl" not in options:
            options["tls"] = True if validate else 'true'
+    elif not is_srv and options.get("srvServiceName", None) is not None:


.get("srvServiceName", None) is the same as .get("srvServiceName"). Let's use the second.

ShaneHarvey · 2021-10-05T20:53:48Z

pymongo/uri_parser.py

            options["tls"] = True if validate else 'true'
+    elif not is_srv and options.get("srvServiceName", None) is not None:
+        raise ConfigurationError("You cannot use the srvServiceName option "
+                                 "with non-SRV URIs.")


I'd suggest: "The srvServiceName option is only allowed with 'mongodb+srv://' URIs"

test/srv_seedlist/replica-set/srv-service-name.json

test/uri_options/srv-service-name-option.json

ShaneHarvey · 2021-10-06T17:18:24Z

pymongo/mongo_client.py

+           There is a new parameter for SRV URIs ``srvServiceName`` which
+           can be used like so::
+
+                MongoClient("mongodb+srv://test22.test.build.10gen.cc/?srvServiceName=customname")


We should not repeat the docs here. We should just say:

Added the ``srvServiceName`` URI and keyword argument.

ShaneHarvey · 2021-10-06T17:20:07Z

pymongo/mongo_client.py

+          - Starting in version 4.0 you can pass a custom SRV service name by
+            using the ``srvServiceName`` option like so::
+
+                MongoClient("mongodb+srv://test22.test.build.10gen.cc/?srvServiceName=customname")


Let's move this to the bottom of this list (under unicode_decode_error_handler). Also please match the doc formatting and style of the other options here.

ShaneHarvey · 2021-10-06T17:22:08Z

pymongo/mongo_client.py

                dbase = res["database"] or dbase
                opts = res["options"]
                fqdn = res["fqdn"]
+                srv_service_name = opts.get("srvServiceName", "mongodb")


Please refactor the default "mongodb" value into a global variable. You can add it to common.py next to the other default global settings.

pymongo/mongo_client.py

ShaneHarvey · 2021-10-06T17:27:26Z

pymongo/monitor.py

-            seedlist, ttl = _SrvResolver(self._fqdn).get_hosts_and_min_ttl()
+            seedlist, ttl = _SrvResolver(self._fqdn,
+                                         srv_service_name=self._srv_service_name
+                                         ).get_hosts_and_min_ttl()


The style here would be cleaner like this:

try: resolver = _SrvResolver( self._fqdn, srv_service_name=self._srv_service_name) seedlist, ttl = resolver.get_hosts_and_min_ttl()

ShaneHarvey · 2021-10-06T17:32:52Z

pymongo/srv_resolver.py

 class _SrvResolver(object):
-    def __init__(self, fqdn, connect_timeout=None):
+    def __init__(self, fqdn,
+                 connect_timeout=None, srv_service_name="mongodb"):


connect_timeout needs to always be passed. The fact that it's not already is an oversight. Please update SrvMonitor to get the timeout from topology_settings (self._settings).

ShaneHarvey · 2021-10-06T17:33:44Z

pymongo/srv_resolver.py

-            results = _resolve('_mongodb._tcp.' + self.__fqdn, 'SRV',
-                               lifetime=self.__connect_timeout)
+            results = _resolve('_' + self.__srv + '._tcp.' + self.__fqdn,
+                                'SRV', lifetime=self.__connect_timeout)


The indentation here still looks off, should be:

results = _resolve('_' + self.__srv + '._tcp.' + self.__fqdn, 'SRV', lifetime=self.__connect_timeout)

ShaneHarvey · 2021-10-06T17:35:59Z

doc/changelog.rst

 - The ``hint`` option is now required when using ``min`` or ``max`` queries
  with :meth:`~pymongo.collection.Collection.find`.
 - ``name`` is now a required argument for the :class:`pymongo.driver_info.DriverInfo` class.
+- When providing a URI to the :class:`~pymongo.mongo_client.MongoClient`


Suggest: When providing a "mongodb+srv://" URI to :class:...

ShaneHarvey · 2021-10-06T21:43:33Z

pymongo/common.py

 _MAX_END_SESSIONS = 10000

+# Default value for srvServiceName
+SRV_SERVICE_NAME_DEFAULT = "mongodb"


I'd omit the _DEFAULT because none of the other constants have it.

pymongo/mongo_client.py

ShaneHarvey · 2021-10-06T21:46:05Z

pymongo/mongo_client.py

+          - Starting in version 4.0 you can pass a custom SRV service name by
+            using the ``srvServiceName`` option like so::


This does not match how we document the other options. The line needs to start with:

- `<option name>`: description.

There's also no need to mention the Starting in version 4.0 part.

ShaneHarvey · 2021-10-06T21:57:20Z

pymongo/uri_parser.py

 from urllib.parse import unquote_plus

-from pymongo.common import (
+from pymongo.common import (SRV_SERVICE_NAME_DEFAULT,


This import should be on a new line to match the existing style.

ShaneHarvey · 2021-10-06T21:58:34Z

test/test_client.py

+        client = MongoClient('mongodb+srv://test22.test.build.10gen.cc',
+                             srvServiceName='namefromkwarg')
+        self.assertEqual(client._topology_settings._srv_service_name,
+        'namefromkwarg')


Indentation.

ShaneHarvey · 2021-10-06T21:59:23Z

test/test_client.py


+    def test_service_name_from_kwargs(self):
+        client = MongoClient('mongodb+srv://test22.test.build.10gen.cc',
+                             srvServiceName='namefromkwarg')


Please add connect=False since this test doesn't need to open the topology.

… URI srvServiceName

… selecting between default, kwargs, and URI opts

ShaneHarvey · 2021-10-07T14:58:25Z

test/test_client.py

            with self.assertRaisesRegex(AutoReconnect, expected):
                client.pymongo_test.test.find_one({})

+    def test_service_name_from_kwargs(self):


This needs:

@unittest.skipUnless( _HAVE_DNSPYTHON, "DNS-related tests need dnspython to be installed") def test_service_name_from_kwargs(self):

ShaneHarvey · 2021-10-07T15:00:53Z

test/test_client.py

+             '/?srvServiceName=customname',
+             connect=False)
+        self.assertEqual(client._topology_settings._srv_service_name,
+                         'customname')


We need a test which actually checks that the kwarg overrides the URI. Like this:

client = MongoClient( 'mongodb+srv://user:[email protected]' '/?srvServiceName=shouldBeOverridden', srvServiceName='customname', connect=False) self.assertEqual(client._topology_settings._srv_service_name, 'customname')

ShaneHarvey · 2021-10-07T15:06:07Z

pymongo/uri_parser.py


 def parse_uri(uri, default_port=DEFAULT_PORT, validate=True, warn=False,
-              normalize=True, connect_timeout=None):
+              normalize=True, connect_timeout=None, srv_service_name=None):


Please update the docstring below to reflect the new srv_service_name param.

ShaneHarvey · 2021-10-07T18:30:03Z

pymongo/mongo_client.py

            and decoding of custom types.

          | **Other optional parameters can be passed as keyword arguments:**
-


Please undo this change.

ShaneHarvey · 2021-10-07T18:34:09Z

pymongo/mongo_client.py

            a Unicode-related error occurs during BSON decoding that would
            otherwise raise :exc:`UnicodeDecodeError`. Valid options include
            'strict', 'replace', and 'ignore'. Defaults to 'strict'.
+          - `srvServiceName`: A custom SRV service name. Use it like so::


I suggest:

`srvServiceName`: (string) The SRV service name to use for "mongodb+srv://" URIs. Defaults to "mongodb". Use it like so::

ShaneHarvey · 2021-10-07T18:36:04Z

From a grep through the codebase we also need to pass the new option to the _SrvResolver in pymongo/monitor.py. It's somewhat concerning that the tests pass without this change (but not your fault at all of course). Does the spec change have any test coverage for mongos SRV polling?

I know you've already addressed the option plumbing but do you have any thoughts on the test coverage issue?

juliusgeo · 2021-10-07T19:45:47Z

From a grep through the codebase we also need to pass the new option to the _SrvResolver in pymongo/monitor.py. It's somewhat concerning that the tests pass without this change (but not your fault at all of course). Does the spec change have any test coverage for mongos SRV polling?

I know you've already addressed the option plumbing but do you have any thoughts on the test coverage issue?

I'm not sure about the test coverage. I think that our new tests cover most of the behavior, but I didn't see anything else in the spec tests besides the ones for this PR. I think one thing we could do is maybe check and make sure that the value of self.__srv is the correct value for _SrvResolver, but I'm not sure how to do that.

ShaneHarvey

LGTM!

I'm not sure about the test coverage. ...

Please file a DRIVERS ticket to request test coverage here. The problem is that there is no test coverage for SRV polling with a custom service name.

…mongodb#749)

juliusgeo added 13 commits September 29, 2021 15:28

resync spec tests

4f4ce7f

add srvservicename to dictionary

9a89ed4

might fix some tests

ef4db92

found right spot to get the srvServiceName from options

d88f1eb

fix resolve_uri

fcaf49b

fix formatting

3e63bd5

run tests without changes in uri_parser.py

029f222

make srv_service_name last argument

9fdfb38

add in plumbing to get srv_service_name to _SrvResolver

944786e

fix _SrvResolver call so that keyword arguments are passed that way

90466a4

get default value for srv_service_name

1b48a20

add _ to prefix srvservicename

459f5e7

remove _ from default values

be53d49

behackett requested changes Oct 4, 2021

View reviewed changes

juliusgeo added 6 commits October 4, 2021 16:48

ressurect dead code, add note to changelog

4a8b1d0

change get to pop? might fix

94e44c2

remove test that shouldn't be there

9c93a7a

go back to get

ce2f48b

fix change that shouldn't have been included

ad366dd

add spaces around + and remove str cast

241e105

ShaneHarvey requested changes Oct 5, 2021

View reviewed changes

juliusgeo added 3 commits October 5, 2021 15:33

shane fixes and added plumbing to monitor.py

f415705

fix use before assignment error

70dcb1f

fix keyerror by using get

c148364

ShaneHarvey requested changes Oct 6, 2021

View reviewed changes

juliusgeo added 2 commits October 6, 2021 11:53

shane fixes

57c19be

fixed formatting of docstring

d3b210f

ShaneHarvey requested changes Oct 6, 2021

View reviewed changes

juliusgeo added 2 commits October 6, 2021 16:10

shane fixes, along with moving location of pop so that kwargs overrides

ec58a75

change it so that the behavior will always choose the kwargs over the…

2cbd7ce

… URI srvServiceName

juliusgeo added 4 commits October 6, 2021 17:58

add exact same URI as spec tests into our tests, tweaked the code for…

891446b

… selecting between default, kwargs, and URI opts

more tweaks, went back to None instead of mongodb

f54b120

fixed typo and added more testing

e5c7e9d

remove part of test that is not possible to test at this point in time

b9dc33a

ShaneHarvey requested changes Oct 7, 2021

View reviewed changes

juliusgeo and others added 2 commits October 7, 2021 08:57

shane fixes, add more testing

8a31961

Merge branch 'master' into PYTHON-2823

e593759

juliusgeo requested a review from ShaneHarvey October 7, 2021 18:09

ShaneHarvey requested changes Oct 7, 2021

View reviewed changes

shane fixes

edec74e

juliusgeo requested a review from ShaneHarvey October 7, 2021 22:21

ShaneHarvey approved these changes Oct 8, 2021

View reviewed changes

juliusgeo merged commit 6bb8a1f into mongodb:master Oct 8, 2021

juliusgeo added a commit to juliusgeo/mongo-python-driver that referenced this pull request Apr 5, 2022

PYTHON-2823 Allow custom service names with srvServiceName URI option (…

dc2301c

…mongodb#749)

juliusgeo added a commit to juliusgeo/mongo-python-driver that referenced this pull request Apr 6, 2022

PYTHON-2823 Allow custom service names with srvServiceName URI option (…

4181341

…mongodb#749)

juliusgeo added a commit to juliusgeo/mongo-python-driver that referenced this pull request Apr 7, 2022

PYTHON-2823 Allow custom service names with srvServiceName URI option (…

265cdb0

…mongodb#749)

		- Starting in version 4.0 you can pass a custom SRV service name by
		using the ``srvServiceName`` option like so::

		and decoding of custom types.

		\| Other optional parameters can be passed as keyword arguments:

PYTHON-2823 Allow custom service names with srvServiceName URI option #749

PYTHON-2823 Allow custom service names with srvServiceName URI option #749

Uh oh!

Conversation

juliusgeo commented Sep 29, 2021

Uh oh!

behackett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaneHarvey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaneHarvey Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaneHarvey left a comment •

edited

Loading

ShaneHarvey Oct 5, 2021 •

edited

Loading