Add uri options tests #414

saghm · 2018-11-13T20:17:10Z

No description provided.

prashantmital

The presence of null-valued keys in the hosts and auth object arrays seems unnecessary and makes changes to the connection-string test-runner necessary. Can you comment on why/if we need to keep these keys?

source/uri-options/tests/README.rst

source/uri-options/tests/single-threaded-options.json

prashantmital

It seems there are some inconsistencies/ambiguities with the validation applied to the URI options during parsing. While we do expect incorrectly formatted readPreferenceTags to raise warnings, inadmissiblereadPreference values are not expected to do the same. Similarly, it is unclear whether any warnings are raised when the user passes cert files that don't exist/are not readable.
I think we should validate all options against their 'Accepted values' from the spec. If that validation fails, we should raise a warning.

source/uri-options/tests/read-preference-options.json

source/uri-options/tests/tls-options.json

source/uri-options/tests/connection-options.yml

source/uri-options/tests/connection-options.json

source/uri-options/tests/tls-options.json

prashantmital · 2018-11-16T23:12:31Z

@saghm can we add another test to test the 'last entry wins' parsing logic? For instance, zlibcompression=3&zlibCompressionLevel=4&zlibCompressionLevel=8 should result in zlibCompressionLevel=8. This would also nail down the warning behavior (should drivers raise a warning?) for this case.

saghm · 2018-11-16T23:23:50Z

The connection string spec is pretty explicit about not mandating a specific mechanism to resolve duplicate keys, and I'm not convinced that we should change that.

… time

source/uri-options/tests/tls-options.json

prashantmital · 2018-12-04T21:15:24Z

Python POC has been updated with the new TLS option names.

source/uri-options/tests/README.rst

source/uri-options/tests/concern-options.json

source/uri-options/tests/connection-options.yml

source/uri-options/tests/connection-pool-options.yml

source/uri-options/tests/read-preference-options.yml

source/uri-options/tests/single-threaded-options.json

source/uri-options/tests/tls-options.yml

jmikola

Leaving a LGTM, as I'm out sick today and don't want to hold things up further. If you'd like to address the last couple of comments I made (re: invalid tests for numeric and boolean options), feel free to do so and resolve the comment threads. If not, feel free to resolve them just the same and proceed without any changes.

source/uri-options/tests/connection-options.yml

source/uri-options/tests/tls-options.yml

prashantmital · 2018-12-19T19:34:25Z

source/uri-options/tests/auth-options.json

+        "authmechanism": "GSSAPI",
+        "authMechanismProperties": {
+          "SERVICE_NAME": "other",
+          "CANONICALIZE_HOST_NAME": "true"


Why is this string "true" instead of boolean true?

I wasn't sure if we should be prescriptive about how/when drivers parse the authMechanismProperties from strings. Since it's a lot easier to go from the boolean to the string than from the string to the boolean though, I'll update this to use the boolean.

prashantmital · 2018-12-19T19:54:10Z

source/uri-options/tests/compression-options.yml

+        hosts: ~
+        auth: ~
+        options:
+            compressors: "zlib"


Since compressors is a

comma separated list of strings, e.g. “snappy,zlib”

I think this should be 1-member list with "zlib" as its only member.

Nevermind, I suppose this isn't necessary (since it could just be a string with comma-separated values).

I agree it should be a list, and there should be another test that contains multiple compressors.

I'll add these changes

prashantmital · 2018-12-19T20:03:05Z

source/uri-options/tests/tls-options.json

+        "tls": true,
+        "tlsCAFile": "ca.pem",
+        "tlsCertificateKeyFile": "cert.pem",
+        "tlsCertificateKeyPassword": "hunter2"


This should be "tlsCertificateKeyFilePassword".

prashantmital · 2018-12-19T20:05:54Z

source/uri-options/tests/read-preference-options.json

+      "options": {
+        "readPreference": "primaryPreferred",
+        "readPreferenceTags": [
+          "dc:ny,rack",


This does not reflect the rack:1 from the YAML file. You might need to regenerate the JSONs.

Forgot to update the JSON files before pushing; I'll do that now

source/uri-options/tests/connection-options.yml

saghm · 2019-01-09T23:06:11Z

@prashantmital are these working with the Python driver now?

jyemin · 2019-01-10T01:11:45Z

source/uri-options/tests/README.rst

+=================
+
+Testing whether a URI is valid or not requires testing whether URI parsing (or
+MongoClient construction) causes a warning due to a URI option being valid and asserting that the 


Should that be "being invalid"?

Yep, good catch!

jyemin · 2019-01-10T13:47:42Z

source/uri-options/tests/auth-options.json

+{
+  "tests": [
+    {
+      "description": "Valid auth options are parsed correctly",


I don't understand this test. It's saying that you should be able to specify all the auth options, but if there's no user name, auth should be disabled (but the auth properties should still be accessible)?

Java driver doesn't support this, as the ConnectionString returns a fully formed MongoCredential, not individual properties?

Would it go against the purpose of this test to add a username to the URI?

I hadn't considered that drivers might parse all options into a single object at once; I can add a username to this test.

source/uri-options/tests/compression-options.yml

jyemin · 2019-01-10T13:55:09Z

source/uri-options/tests/tls-options.yml

+        warning: true
+        hosts: ~
+        auth: ~
+        options: ~


You'll make it easier for test runner writers if options is always a document, even if sometimes empty, rather than sometimes a document and sometimes null. I see other places where it is an empty document, so let's use that everywhere.

jyemin · 2019-01-10T13:56:41Z

source/uri-options/tests/read-preference-options.yml

+        auth: ~
+        options:
+            readPreference: "primaryPreferred"
+            readPreferenceTags:


Make these match the format used my MongoDB, using documents rather than strings:

- {dc: ny, rack: 1} - {dc: ny}

jyemin · 2019-01-10T13:57:54Z

source/uri-options/tests/compression-options.yml

+        hosts: ~
+        auth: ~
+        options:
+            compressors: "zlib"


I agree it should be a list, and there should be another test that contains multiple compressors.

jyemin · 2019-01-10T14:00:54Z

source/uri-options/tests/single-threaded-options.json

+      "description": "Invalid serverSelectionTryOnce causes a warning",
+      "uri": "mongodb://example.com/?serverSelectionTryOnce=invalid",
+      "valid": true,
+      "warning": false,


Should be warning: true

Oops, thanks

add uri options tests

4e0048b

saghm requested a review from behackett November 13, 2018 20:17

rathisekaran requested a review from prashantmital November 14, 2018 22:28

prashantmital suggested changes Nov 15, 2018

View reviewed changes

source/uri-options/tests/README.rst Show resolved Hide resolved

source/uri-options/tests/single-threaded-options.json Show resolved Hide resolved

saghm added 2 commits November 15, 2018 14:55

set hosts fields to null

f9fd27f

add note about optional tests

68f70a3

prashantmital reviewed Nov 16, 2018

View reviewed changes

source/uri-options/tests/read-preference-options.json Outdated Show resolved Hide resolved

source/uri-options/tests/tls-options.json Show resolved Hide resolved

don't specify behavior of unknown read preference mode

1e29a9a

p-mongo reviewed Nov 16, 2018

View reviewed changes

source/uri-options/tests/connection-options.yml Show resolved Hide resolved

don't specify behavior of unknown auth mechanism

cc6e590

behackett requested changes Nov 16, 2018

View reviewed changes

source/uri-options/tests/connection-options.json Show resolved Hide resolved

source/uri-options/tests/tls-options.json Show resolved Hide resolved

saghm added 2 commits November 16, 2018 18:28

add dummy .pem files for drivers which validate file options at parse…

23af0c4

… time

separate out connection pool options

8341942

prashantmital mentioned this pull request Nov 27, 2018

Implement URI options spec mongodb/mongo-python-driver#386

Merged

remove trailing whitespace

2be7e48

saghm mentioned this pull request Nov 27, 2018

RUBY-1559 Implement uri option spec tests mongodb/mongo-ruby-driver#1170

Merged

prashantmital approved these changes Nov 28, 2018

View reviewed changes

remove "path" from tls option names

27ffa77

prashantmital reviewed Dec 4, 2018

View reviewed changes

source/uri-options/tests/tls-options.json Outdated Show resolved Hide resolved

remove final "Path"

c385b20

jmikola reviewed Dec 5, 2018

View reviewed changes

saghm added 6 commits December 5, 2018 12:32

more code review feedback

371041e

fix read preference description

bfeb58e

update client certificate key option

af548c1

update json files

3fa72d2

update tlsCertificateKeyPassword name

9f7f05c

add test for conflicting insecure tls options

3690a14

jmikola approved these changes Dec 10, 2018

View reviewed changes

saghm added 4 commits December 10, 2018 15:48

more code review feedback

44bf061

fix typo

b28ee9e

fix indentation

917c3b3

remove w=-2 test

42b555d

behackett approved these changes Dec 17, 2018

View reviewed changes

prashantmital reviewed Dec 18, 2018

View reviewed changes

source/uri-options/tests/connection-options.yml Show resolved Hide resolved

add missing options field

55824c9

prashantmital reviewed Dec 18, 2018

View reviewed changes

source/uri-options/tests/tls-options.yml Outdated Show resolved Hide resolved

saghm added 2 commits December 18, 2018 18:03

fix typo in tls options

931f684

small fixes

0d42dbd

prashantmital reviewed Dec 19, 2018

View reviewed changes

source/uri-options/tests/connection-options.yml Show resolved Hide resolved

more fixes

d91db3b

saghm force-pushed the uri-options-spec branch from 171c2e9 to d91db3b Compare December 19, 2018 23:06

jyemin self-requested a review January 10, 2019 12:55

jyemin requested changes Jan 10, 2019

View reviewed changes

code review feedback

5a508de

jyemin approved these changes Jan 14, 2019

View reviewed changes

saghm merged commit 99a5f80 into mongodb:master Jan 14, 2019

Add uri options tests #414

Add uri options tests #414

Uh oh!

Conversation

saghm commented Nov 13, 2018

Uh oh!

prashantmital left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

prashantmital 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prashantmital commented Nov 16, 2018

Uh oh!

saghm commented Nov 16, 2018

Uh oh!

Uh oh!

prashantmital commented Dec 4, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

prashantmital Dec 19, 2018 • 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!

Uh oh!

saghm commented Jan 9, 2019

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!

prashantmital left a comment •

edited

Loading

prashantmital Dec 19, 2018 •

edited

Loading