Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add setting to configure API base URL #42

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

c-w
Copy link

@c-w c-w commented Dec 11, 2023

This pull request proposes adding a setting to configure the base URL of the PwnedPasswords API, which enables a use-case that I have for using this package together with a self-hosted instance of the API.

Alternatives considered

Technically it's already possible to customize the API endpoint by overriding a class-level variable directly on the PwnedPasswords client as shown in the snippet below, but this approach to me seems unnecessarily brittle and splits configuration of the package into two locations (Django settings and code):

##### current approach

from pwned_passwords_django import api

api.PwnedPasswords.api_endpoint = "http://custom.endpoi.nt/"

Instead, if this pull request is merged, a consumer of this package can simply configure the base URL in the Django settings:

##### proposed approach

PWNED_PASSWORDS = {
    "API_ENDPOINT": "http://custom.endpoi.nt/",
}

Implementation notes

The proposed implementation is a little bit round-about in overriding the value of the existing class-level api_endpoint variable in the constructor. I considered two alternative implementations (discussed below) but believe that there's larger downsides to each of them compared to the proposed implementation.

Alternative 1: access setting in the class-level variable

Upsides:

  • Most minimal change.

Downsides:

  • Accessing settings is now split between constructor and class-initialization.
  • Value is bound early (import time as opposed to run time) which may cause issues when overriding settings.
diff --git a/src/pwned_passwords_django/api.py b/src/pwned_passwords_django/api.py
index 19bdce1..3fdc405 100644
--- a/src/pwned_passwords_django/api.py
+++ b/src/pwned_passwords_django/api.py
@@ -61,7 +61,10 @@ class PwnedPasswords:
 
     """
 
-    api_endpoint: str = "https://api.pwnedpasswords.com/range/"
+    api_endpoint: str = getattr(settings, "PWNED_PASSWORDS", {}).get(
+        "API_ENDPOINT",
+        "https://api.pwnedpasswords.com/range/",
+    )
 
     user_agent: str = (
         f"pwned-passwords-django/{__version__} "

Alternative 2: only initialize the setting in the constructor

Upsides:

  • Avoids potential confusion having api_endpoint be defined on both the class-level and the instance-level.

Downsides:

  • Technically a breaking change as a consumer of the package may depend on api.PwnedPasswords.api_endpoint existing as a class-level variable.
diff --git a/src/pwned_passwords_django/api.py b/src/pwned_passwords_django/api.py
index 19bdce1..3fc1e97 100644
--- a/src/pwned_passwords_django/api.py
+++ b/src/pwned_passwords_django/api.py
@@ -61,8 +61,6 @@ class PwnedPasswords:
 
     """
 
-    api_endpoint: str = "https://api.pwnedpasswords.com/range/"
-
     user_agent: str = (
         f"pwned-passwords-django/{__version__} "
         f"(Python/{sys.version_info.major}.{sys.version_info.minor}.{sys.version_info.micro} "
@@ -81,6 +79,10 @@ class PwnedPasswords:
         self.add_padding = settings_dict.get("ADD_PADDING", True)
         self.client = client or httpx.Client()
         self.async_client = async_client or httpx.AsyncClient()
+        self.api_endpoint = settings_dict.get(
+            "API_ENDPOINT",
+            "https://api.pwnedpasswords.com/range/",
+        )
 
     def _prepare_password(self, password: str) -> typing.Tuple[str, str]:
         """

@ubernostrum
Copy link
Owner

I need to think about this one a bit, not least because I wasn't aware you could run your own instance of this...

@c-w
Copy link
Author

c-w commented Dec 12, 2023

Thanks, let me know if there's any questions or concerns that I can help answer. Another use-case which I have that this PR unlocks is using a fake implementation of the service for example during e2e tests. In unit tests we can use mocks, but in more involved tests that spin up an entire stack, it's nice being able to avoid making live service calls and instead calling a local instance of the service.

@ubernostrum
Copy link
Owner

So, one thing I planned to do anyway for the next release was re-work the test mocks to use the httpx.MockTransport, which I'm already doing in another project (and wrote about recently too). That combined with the ability to pass in a custom HTTP client ought to give you the flexibility you need, and I probably want to think about ways to be able to pass that in to the middleware as a config option too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants