Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/contributors.rst
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,4 @@ The following is a list of people who have contributed to
- Dainis Gorbunovs (DainisGorbunovs)
- Iris Ho (sleepyStick)
- Stephan Hof (stephan-hof)
- Casey Clements (caseyclements)
21 changes: 14 additions & 7 deletions pymongo/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import inspect
import warnings
from collections import OrderedDict, abc
from difflib import get_close_matches
from typing import (
TYPE_CHECKING,
Any,
Expand Down Expand Up @@ -162,9 +163,12 @@ def clean_node(node: str) -> tuple[str, int]:
return host.lower(), port


def raise_config_error(key: str, dummy: Any) -> NoReturn:
def raise_config_error(key: str, suggestions: Optional[list] = None) -> NoReturn:
"""Raise ConfigurationError with the given key name."""
raise ConfigurationError(f"Unknown option {key}")
msg = f"Unknown option: {key}."
if suggestions:
msg += f" Did you mean one of ({', '.join(suggestions)}) or maybe a camelCase version of one? Refer to docstring."
raise ConfigurationError(msg)


# Mapping of URI uuid representation options to valid subtypes.
Expand Down Expand Up @@ -810,14 +814,18 @@ def validate_auth_option(option: str, value: Any) -> tuple[str, Any]:
"""Validate optional authentication parameters."""
lower, value = validate(option, value)
if lower not in _AUTH_OPTIONS:
raise ConfigurationError(f"Unknown authentication option: {option}")
raise ConfigurationError(f"Unknown option: {option}. Must be in {_AUTH_OPTIONS}")
return option, value


def validate(option: str, value: Any) -> tuple[str, Any]:
"""Generic validation function."""
lower = option.lower()
validator = VALIDATORS.get(lower, raise_config_error)
try:
validator = VALIDATORS[lower]
except KeyError:
suggestions = get_close_matches(lower, VALIDATORS, cutoff=0.2)
raise_config_error(option, suggestions)
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about refactoring this with the code block below? It seems like they are essentially identical besides the VALIDATORS vs URI_OPTIONS_VALIDATOR_MAP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a note /hint on camelCase and suggested referring to docstring.
I've also opened a ticket to explore rationalizing the keys to kwargs: https://jira.mongodb.org/browse/PYTHON-4056

Copy link
Member

@ShaneHarvey ShaneHarvey Nov 21, 2023

Choose a reason for hiding this comment

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

Thanks I'll resolve the other comment about camelCase but this comment is still todo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi. If the original intent hasn't been resolved, would you please elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

Can you refactor this to remove the duplicated code so that there's only one call to try/except + get_close_matches/raise_config_error, eg:

def _get_validator(name, validators): lower = name.lower() try: return validators[lower] except KeyError: suggestions = get_close_matches(lower, validators, cutoff=0.2) raise_config_error(name, suggestions)
value = validator(option, value)
return option, value

Expand Down Expand Up @@ -856,15 +864,14 @@ def get_setter_key(x: str) -> str:
for opt, value in options.items():
normed_key = get_normed_key(opt)
try:
validator = URI_OPTIONS_VALIDATOR_MAP.get(normed_key, raise_config_error)
Copy link
Member

@ShaneHarvey ShaneHarvey Nov 29, 2023

Choose a reason for hiding this comment

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

This code should still be using URI_OPTIONS_VALIDATOR_MAP but by calling validate() it now uses VALIDATORS. This is why I suggested the _get_validator function in my earlier comment.

value = validator(opt, value) # noqa: PLW2901
_, validated = validate(normed_key, value)
except (ValueError, TypeError, ConfigurationError) as exc:
if warn:
warnings.warn(str(exc), stacklevel=2)
else:
raise
else:
validated_options[get_setter_key(normed_key)] = value
validated_options[get_setter_key(normed_key)] = validated
return validated_options


Expand Down
9 changes: 9 additions & 0 deletions test/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import datetime
import gc
import os
import re
import signal
import socket
import struct
Expand Down Expand Up @@ -535,6 +536,14 @@ def test_client_options(self):
self.assertIsInstance(c.options.retry_writes, bool)
self.assertIsInstance(c.options.retry_reads, bool)

def test_validate_suggestion(self):
"""Validate kwargs in constructor."""
for typo in ["auth", "Auth", "AUTH"]:
expected = f"Unknown option: {typo}. Did you mean one of (authsource, authmechanism, authoidcallowedhosts) or maybe a camelCase version of one? Refer to docstring."
expected = re.escape(expected)
with self.assertRaisesRegex(ConfigurationError, expected):
MongoClient(**{typo: "standard"}) # type: ignore[arg-type]


class TestClient(IntegrationTest):
def test_multiple_uris(self):
Expand Down
6 changes: 6 additions & 0 deletions test/test_uri_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,12 @@ def test_split_options(self):
self.assertEqual({"authsource": "foobar"}, split_options("authSource=foobar"))
self.assertEqual({"maxpoolsize": 50}, split_options("maxpoolsize=50"))

# Test suggestions given when invalid kwarg passed

expected = r"Unknown option: auth. Did you mean one of \(authsource, authmechanism, authoidcallowedhosts\) or maybe a camelCase version of one\? Refer to docstring."
with self.assertRaisesRegex(ConfigurationError, expected):
split_options("auth=GSSAPI")

def test_parse_uri(self):
self.assertRaises(InvalidURI, parse_uri, "http://foobar.com")
self.assertRaises(InvalidURI, parse_uri, "http://foo@foobar.com")
Expand Down