From c91c77b62fdb2419454a8ccad8b809699d13ab05 Mon Sep 17 00:00:00 2001 From: tdstein Date: Fri, 12 Apr 2024 10:43:30 -0400 Subject: [PATCH] refactor: reduces complexity of urls module. --- .vscode/settings.json | 2 +- pyproject.toml | 2 +- src/posit/connect/bundles.py | 6 +-- src/posit/connect/client.py | 12 ++--- src/posit/connect/config.py | 3 +- src/posit/connect/content.py | 10 ++-- src/posit/connect/me.py | 2 +- src/posit/connect/oauth.py | 2 +- src/posit/connect/permissions.py | 10 ++-- src/posit/connect/urls.py | 90 ++++++++++++++++++++------------ src/posit/connect/users.py | 10 ++-- src/posit/connect/visits.py | 4 +- tests/posit/connect/test_urls.py | 49 +++++++++-------- 13 files changed, 115 insertions(+), 87 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 4deef7c8..4169db45 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -7,4 +7,4 @@ "cSpell.words": [ "mypy" ] -} \ No newline at end of file +} diff --git a/pyproject.toml b/pyproject.toml index 2dd030c8..1f2cc35a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -42,7 +42,7 @@ ignore = [ # NumPy style docstring convention with noted exceptions. # https://docs.astral.sh/ruff/faq/#does-ruff-support-numpy-or-google-style-docstrings # - # This docstring style works with [quarotdoc](https://machow.github.io/quartodoc/get-started/overview.html). + # This docstring style works with [quartodoc](https://machow.github.io/quartodoc/get-started/overview.html). # 'D101', # TODO(#135) implement docstring for public class 'D103', # TODO(#135) implement docstring for public functions diff --git a/src/posit/connect/bundles.py b/src/posit/connect/bundles.py index 3d876283..1691b885 100644 --- a/src/posit/connect/bundles.py +++ b/src/posit/connect/bundles.py @@ -94,7 +94,7 @@ def metadata(self) -> BundleMetadata: def delete(self) -> None: path = f"v1/content/{self.content_guid}/bundles/{self.id}" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) self.session.delete(url) @@ -105,7 +105,7 @@ def __init__(self, config: Config, session: Session, content_guid: str) -> None: def find(self) -> List[Bundle]: path = f"v1/content/{self.content_guid}/bundles" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) response = self.session.get(url) results = response.json() return [ @@ -123,7 +123,7 @@ def find_one(self) -> Bundle | None: def get(self, id: str) -> Bundle: path = f"v1/content/{self.content_guid}/bundles/{id}" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) response = self.session.get(url) result = response.json() return Bundle(self.config, self.session, **result) diff --git a/src/posit/connect/client.py b/src/posit/connect/client.py index d449716c..75b1b2e8 100644 --- a/src/posit/connect/client.py +++ b/src/posit/connect/client.py @@ -136,7 +136,7 @@ def request(self, method: str, path: str, **kwargs) -> Response: ------- Response: A requests.Response object. """ - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) return self.session.request(method, url, **kwargs) def get(self, path: str, **kwargs) -> Response: @@ -152,7 +152,7 @@ def get(self, path: str, **kwargs) -> Response: Response: A requests.Response object. """ - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) return self.session.get(url, **kwargs) def post(self, path: str, **kwargs) -> Response: @@ -168,7 +168,7 @@ def post(self, path: str, **kwargs) -> Response: Response: A requests.Response object. """ - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) return self.session.post(url, **kwargs) def put(self, path: str, **kwargs) -> Response: @@ -184,7 +184,7 @@ def put(self, path: str, **kwargs) -> Response: Response: A requests.Response object. """ - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) return self.session.put(url, **kwargs) def patch(self, path: str, **kwargs) -> Response: @@ -200,7 +200,7 @@ def patch(self, path: str, **kwargs) -> Response: Response: A requests.Response object. """ - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) return self.session.patch(url, **kwargs) def delete(self, path: str, **kwargs) -> Response: @@ -216,5 +216,5 @@ def delete(self, path: str, **kwargs) -> Response: Response: A requests.Response object. """ - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) return self.session.delete(url, **kwargs) diff --git a/src/posit/connect/config.py b/src/posit/connect/config.py index c82c4260..c7574be3 100644 --- a/src/posit/connect/config.py +++ b/src/posit/connect/config.py @@ -56,5 +56,4 @@ def __init__( self, api_key: Optional[str] = None, url: Optional[str] = None ) -> None: self.api_key = api_key or _get_api_key() - self.url = urls.server_to_api_url(url or _get_url()) - urls.validate(self.url) + self.url = urls.create(url or _get_url()) diff --git a/src/posit/connect/content.py b/src/posit/connect/content.py index 435cc3c5..7eeeba2e 100644 --- a/src/posit/connect/content.py +++ b/src/posit/connect/content.py @@ -215,7 +215,7 @@ def tags(self) -> List[dict]: def delete(self) -> None: """Delete the content item.""" path = f"v1/content/{self.guid}" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) self.session.delete(url) @overload @@ -287,14 +287,14 @@ def update(self, *args, **kwargs) -> None: def update(self, *args, **kwargs) -> None: """Update the content item.""" body = dict(*args, **kwargs) - url = urls.append_path(self.config.url, f"v1/content/{self.guid}") + url = urls.append(self.config.url, f"v1/content/{self.guid}") response = self.session.patch(url, json=body) super().update(**response.json()) class Content(Resources): def __init__(self, config: Config, session: Session) -> None: - self.url = urls.append_path(config.url, "v1/content") + self.url = urls.append(config.url, "v1/content") self.config = config self.session = session @@ -390,7 +390,7 @@ def create(self, *args, **kwargs) -> ContentItem: """ body = dict(*args, **kwargs) path = "v1/content" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) response = self.session.post(url, json=body) return ContentItem(self.config, self.session, **response.json()) @@ -531,6 +531,6 @@ def get(self, guid: str) -> ContentItem: ------- ContentItem """ - url = urls.append_path(self.url, guid) + url = urls.append(self.url, guid) response = self.session.get(url) return ContentItem(self.config, self.session, **response.json()) diff --git a/src/posit/connect/me.py b/src/posit/connect/me.py index 1d04a42b..ac8adf46 100644 --- a/src/posit/connect/me.py +++ b/src/posit/connect/me.py @@ -18,6 +18,6 @@ def get(config: Config, session: requests.Session) -> User: ------- User: The current user. """ - url = urls.append_path(config.url, "v1/user") + url = urls.append(config.url, "v1/user") response = session.get(url) return User(config, session, **response.json()) diff --git a/src/posit/connect/oauth.py b/src/posit/connect/oauth.py index 958c4089..22ad49b9 100644 --- a/src/posit/connect/oauth.py +++ b/src/posit/connect/oauth.py @@ -15,7 +15,7 @@ class Credentials(TypedDict, total=False): class OAuthIntegration: def __init__(self, config: Config, session: Session) -> None: - self.url = urls.append_path(config.url, "v1/oauth/integrations/credentials") + self.url = urls.append(config.url, "v1/oauth/integrations/credentials") self.config = config self.session = session diff --git a/src/posit/connect/permissions.py b/src/posit/connect/permissions.py index 4d02fd1d..d034052d 100644 --- a/src/posit/connect/permissions.py +++ b/src/posit/connect/permissions.py @@ -34,7 +34,7 @@ def role(self) -> str: def delete(self) -> None: """Delete the permission.""" path = f"v1/content/{self.content_guid}/permissions/{self.id}" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) self.session.delete(url) @overload @@ -62,7 +62,7 @@ def update(self, *args, **kwargs) -> None: } body.update(*args, **kwargs) path = f"v1/content/{self.content_guid}/permissions/{self.id}" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) response = self.session.put( url, json=body, @@ -120,7 +120,7 @@ def create(self, *args, **kwargs) -> Permission: ... body = dict(*args, **kwargs) path = f"v1/content/{self.content_guid}/permissions" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) response = self.session.post(url, json=body) return Permission(self.config, self.session, **response.json()) @@ -133,7 +133,7 @@ def find(self, *args, **kwargs) -> List[Permission]: """ body = dict(*args, **kwargs) path = f"v1/content/{self.content_guid}/permissions" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) response = self.session.get(url, json=body) results = response.json() return [Permission(self.config, self.session, **result) for result in results] @@ -161,6 +161,6 @@ def get(self, id: str) -> Permission: Permission """ path = f"v1/content/{self.content_guid}/permissions/{id}" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) response = self.session.get(url) return Permission(self.config, self.session, **response.json()) diff --git a/src/posit/connect/urls.py b/src/posit/connect/urls.py index 4d177462..79cf5763 100644 --- a/src/posit/connect/urls.py +++ b/src/posit/connect/urls.py @@ -4,38 +4,43 @@ from urllib.parse import urlsplit, urlunsplit +Url = str -def server_to_api_url(url: str) -> str: - """ - Fixes the given URL by appending '__api__' if it doesn't already end with it. - Args: - url (str): The URL to fix. - - Returns - ------- - str: The fixed URL. - """ - url = url.rstrip("/") - if not url.endswith("__api__"): - return append_path(url, "__api__") - return url +def create(url: str) -> Url: + """Create a Url. + Asserts that the Url is a proper Posit Connect endpoint. The path '__api__' is appended to the Url if it isn't already present. -def validate(url: str) -> None: - """ - Check if the given URL is valid. - - Args: - url (str): The URL to be validated. + Parameters + ---------- + url : str + The original Url. Returns ------- - bool: True if the URL is valid, False otherwise. + Url + The validated and formatted Url. Raises ------ - ValueError: If the URL is missing a scheme or is not absolute. + ValueError + The Url is missing a scheme. + ValueError + The Url is missing a network location (i.e., a domain name). + + Examples + -------- + >>> urls.create( + ... "http://example.com" + ... ) + http://example.com/__api__ + + >>> urls.create( + ... "http://example.com/__api__" + ... ) + http://example.com/__api__ + """ split = urlsplit(url, allow_fragments=False) if not split.scheme: @@ -48,24 +53,45 @@ def validate(url: str) -> None: f"url must be absolute (e.g., http://example.com/__api__): {url}" ) + url = url.rstrip("/") + if not url.endswith("__api__"): + url = append(url, "__api__") + + return url -def append_path(url: str, path: str) -> str: - """ - Appends a path to a URL. - Args: - url (str): The original URL. - path (str): The path to append. +def append(url: Url, path: str) -> Url: + """Append a path to a Url. + + Parameters + ---------- + url : Url + A valid Url. + path : str + A valid Url path. Returns ------- - str: The modified URL with the appended path. + Url + The original Url with the path appended to the end. + + Examples + -------- + >>> url = urls.create( + ... "http://example.com/__api__" + ... ) + >>> urls.append( + ... url, "path" + ... ) + http://example.com/__api__/path """ - # See https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlsplit - split = urlsplit(url, allow_fragments=False) # Removes leading '/' from path to avoid double slashes. path = path.lstrip("/") - # Prepend the path with the existing URL path. + # Removes trailing '/' from path to avoid double slashes. + path = path.rstrip("/") + # See https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlsplit + split = urlsplit(url, allow_fragments=False) + # Append the path to unmodified Url path. path = posixpath.join(split.path, path) # See https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlunsplit return urlunsplit((split.scheme, split.netloc, path, split.query, None)) diff --git a/src/posit/connect/users.py b/src/posit/connect/users.py index 80da9138..31d546bc 100644 --- a/src/posit/connect/users.py +++ b/src/posit/connect/users.py @@ -75,7 +75,7 @@ def lock(self, *, force: bool = False): raise RuntimeError( "You cannot lock your own account. Set force=True to override this behavior." ) - url = urls.append_path(self.config.url, f"v1/users/{self.guid}/lock") + url = urls.append(self.config.url, f"v1/users/{self.guid}/lock") body = {"locked": True} self.session.post(url, json=body) super().update(locked=True) @@ -88,7 +88,7 @@ def unlock(self): ------- None """ - url = urls.append_path(self.config.url, f"v1/users/{self.guid}/lock") + url = urls.append(self.config.url, f"v1/users/{self.guid}/lock") body = {"locked": False} self.session.post(url, json=body) super().update(locked=False) @@ -146,14 +146,14 @@ def update(self, *args, **kwargs) -> None: None """ body = dict(*args, **kwargs) - url = urls.append_path(self.config.url, f"v1/users/{self.guid}") + url = urls.append(self.config.url, f"v1/users/{self.guid}") response = self.session.put(url, json=body) super().update(**response.json()) class Users(Resources): def __init__(self, config: Config, session: requests.Session) -> None: - self.url = urls.append_path(config.url, "v1/users") + self.url = urls.append(config.url, "v1/users") self.config = config self.session = session @@ -206,7 +206,7 @@ def find_one(self, *args, **kwargs) -> User | None: return next(users, None) def get(self, id: str) -> User: - url = urls.append_path(self.config.url, f"v1/users/{id}") + url = urls.append(self.config.url, f"v1/users/{id}") response = self.session.get(url) return User( config=self.config, diff --git a/src/posit/connect/visits.py b/src/posit/connect/visits.py index 70b6012f..4854ac1d 100644 --- a/src/posit/connect/visits.py +++ b/src/posit/connect/visits.py @@ -141,7 +141,7 @@ def find(self, *args, **kwargs) -> List[Visit]: params = rename_params(params) path = "/v1/instrumentation/content/visits" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) paginator = CursorPaginator(self.session, url, params=params) results = paginator.fetch_results() return [ @@ -201,7 +201,7 @@ def find_one(self, *args, **kwargs) -> Visit | None: params = dict(*args, **kwargs) params = rename_params(params) path = "/v1/instrumentation/content/visits" - url = urls.append_path(self.config.url, path) + url = urls.append(self.config.url, path) paginator = CursorPaginator(self.session, url, params=params) pages = paginator.fetch_pages() results = (result for page in pages for result in page.results) diff --git a/tests/posit/connect/test_urls.py b/tests/posit/connect/test_urls.py index c0310feb..a980013d 100644 --- a/tests/posit/connect/test_urls.py +++ b/tests/posit/connect/test_urls.py @@ -1,34 +1,37 @@ import pytest -from posit.connect.urls import append_path, server_to_api_url, validate +from posit.connect import urls -def test_append_path(): - assert append_path("http://foo.bar/__api__", "baz") == "http://foo.bar/__api__/baz" +class TestCreate: + def test(self): + url = "http://example.com/__api__" + assert urls.create(url) == url + def test_append_path(self): + assert urls.create("http://example.com/") == "http://example.com/__api__" -def test_append_path_with_leading_slash(): - assert append_path("http://foo.bar/__api__", "/baz") == "http://foo.bar/__api__/baz" + def test_missing_scheme(self): + with pytest.raises(ValueError): + urls.create("example.com") + def test_missing_netloc(self): + with pytest.raises(ValueError): + urls.create("http://") -def test_fix_with_correct_url(): - assert server_to_api_url("http://foo.bar/__api__") == "http://foo.bar/__api__" - assert server_to_api_url("http://foo.bar/__api__/") == "http://foo.bar/__api__" +class TestAppend: + def test(self): + url = "http://example.com/__api__" + url = urls.create(url) + assert urls.append(url, "path") == "http://example.com/__api__/path" -def test_fix_without_path(): - assert server_to_api_url("http://foo.bar") == "http://foo.bar/__api__" + def test_slash_prefix(self): + url = "http://example.com/__api__" + url = urls.create(url) + assert urls.append(url, "/path") == "http://example.com/__api__/path" - -def test_fix_with_proxy_path(): - assert server_to_api_url("http://foo.bar/baz") == "http://foo.bar/baz/__api__" - - -def test_validate_without_scheme(): - with pytest.raises(ValueError): - validate("foo.bar/__api__") - - -def test_validate_without_netloc(): - with pytest.raises(ValueError): - validate("http:///__api__") + def test_slash_suffix(self): + url = "http://example.com/__api__" + url = urls.create(url) + assert urls.append(url, "path/") == "http://example.com/__api__/path"