Skip to content

feat: adds boilerplate for config with factories. #1

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

Merged
merged 5 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .coveragerc
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[report]
fail_under = 100
1 change: 1 addition & 0 deletions Justfile
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ clean:
#!/usr/bin/env bash
{{ OPTIONS }}

find . -name "*.egg-info" -exec rm -rf {} +
find . -name "*.pyc" -exec rm -f {} +
find . -name "__pycache__" -exec rm -rf {} +
rm -rf\
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ classifiers = [
]
dynamic = ["version"]
dependencies = [
"requests==2.31.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will sync vs async be a concern? I know httpx supports both but I don't know how important that might be for users of this client

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure. I started with requests since it is the most popular HTTP library after urllib3. And the simplest to use, in my opinion.

Are there situations when calling the Connect API where asynchronous design becomes necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

pyshiny (and other fastapi-base apps) rely heavily on python's asyncio apis. This has come up in the databricks sql client as well. There are a few justifications for the request for async in this issue that might also be applicable for our client.

databricks/databricks-sql-python#176

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! httpx looks great, but it's marked as beta. Do you have opinions on aiohttp? That seems like the most stable library to bring in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't used it but that sounds like a good option to me. FWIW Joe also recommended aiohttp when we were asking him some questions about async shiny: https://positpbc.slack.com/archives/C03HH2ZUU/p1702668760303599

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm skeptical that we'll want to bother with async queries against the Connect API

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we expect the sdk to be used from inside Connect-hosted content like fastapi then a non-blocking api could be important. Based off of Joe's comment from the slack thread, it sounds like request's blocking IO is a problem for uvicorn apps.

uvicorn is single threaded, so requests.get(...) blocks any traffic from being served.

We should test this to confirm the behavior but if all other requests are blocked while we're waiting for a request to the Connect API to resolve then that could be a blocker for obtaining an access token inside a piece of content.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could someone write a uvicorn application that async wraps calls to this library?

I also wouldn't want to force a consumer to set up asyncio and into await'ing everything.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could someone write a uvicornapplication that async wraps calls to this library?

Sure, probably. Let's table this for now. I think this is something that warrants more discussion though. We should test what it feels like to use this SDK from different content types in Connect. Especially single-threaded uvicorn content like pyshiny and fastapi. I'm interested to see whether the content can service multiple requests at the same time without using an async requests library

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, let's defer, this feels like YAGNI in PR #1. If we find out that we are bottlenecking because of this, we should totally revisit.

"requests>=2.31.0,<3"
]

[project.optional-dependencies]
Expand Down
11 changes: 11 additions & 0 deletions src/posit/auth.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from requests import PreparedRequest
from requests.auth import AuthBase


class Auth(AuthBase):
def __init__(self, key) -> None:
self.key = key

def __call__(self, r: PreparedRequest) -> PreparedRequest:
r.headers["Authorization"] = f"Key {self.key}"
return r
13 changes: 13 additions & 0 deletions src/posit/auth_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from unittest.mock import Mock

from .auth import Auth


class TestAuth:
def test_auth_headers(self):
key = "foobar"
auth = Auth(key=key)
r = Mock()
r.headers = {}
auth(r)
assert r.headers == {"Authorization": f"Key {key}"}
22 changes: 22 additions & 0 deletions src/posit/client.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
from requests import Session
from typing import Optional

from .auth import Auth
from .config import ConfigBuilder


class Client:
def __init__(
self, endpoint: Optional[str] = None, api_key: Optional[str] = None
) -> None:
builder = ConfigBuilder()
builder.set_api_key(api_key)
builder.set_endpoint(endpoint)
self._config = builder.build()
self._session = Session()
self._session.auth = Auth(self._config.api_key)

def get(self, endpoint: str, *args, **kwargs): # pragma: no cover
return self._session.request(
"GET", f"{self._config.endpoint}/{endpoint}", *args, **kwargs
)
26 changes: 26 additions & 0 deletions src/posit/client_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
from unittest.mock import MagicMock, Mock, patch

from .client import Client


class TestClient:
@patch("posit.client.Session")
@patch("posit.client.ConfigBuilder")
@patch("posit.client.Auth")
def test_init(self, Auth: MagicMock, ConfigBuilder: MagicMock, Session: MagicMock):
api_key = "foobar"
endpoint = "http://foo.bar"
config = Mock()
config.api_key = api_key
builder = ConfigBuilder.return_value
builder.set_api_key = Mock()
builder.set_endpoint = Mock()
builder.build = Mock(return_value=config)
client = Client(api_key=api_key, endpoint=endpoint)
ConfigBuilder.assert_called_once()
builder.set_api_key.assert_called_once_with(api_key)
builder.set_endpoint.assert_called_once_with(endpoint)
builder.build.assert_called_once()
Session.assert_called_once()
Auth.assert_called_once_with(api_key)
assert client._config == config
54 changes: 54 additions & 0 deletions src/posit/config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import os

from abc import ABC, abstractmethod
from dataclasses import dataclass
from typing import List, Optional


@dataclass
class Config:
api_key: Optional[str] = None
endpoint: Optional[str] = None


class ConfigProvider(ABC):
@abstractmethod
def get_value(self, key: str) -> Optional[str]:
raise NotImplementedError # pragma: no cover


class EnvironmentConfigProvider(ConfigProvider):
def get_value(self, key: str) -> Optional[str]:
if key == "api_key":
return os.environ.get("CONNECT_API_KEY")

if key == "endpoint":
return os.environ.get("CONNECT_ENDPOINT")

return None


class ConfigBuilder:
def __init__(
self, providers: List[ConfigProvider] = [EnvironmentConfigProvider()]
) -> None:
self._config = Config()
self._providers = providers

def build(self) -> Config:
for key in Config.__annotations__:
if not getattr(self._config, key):
setattr(
self._config,
key,
next(
(provider.get_value(key) for provider in self._providers), None
),
)
return self._config

def set_api_key(self, api_key: Optional[str]):
self._config.api_key = api_key

def set_endpoint(self, endpoint: Optional[str]):
self._config.endpoint = endpoint
46 changes: 46 additions & 0 deletions src/posit/config_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
from unittest.mock import Mock, patch

from .config import Config, ConfigBuilder, EnvironmentConfigProvider


class TestEnvironmentConfigProvider:
@patch.dict("os.environ", {"CONNECT_API_KEY": "foobar"})
def test_get_api_key(self):
provider = EnvironmentConfigProvider()
api_key = provider.get_value("api_key")
assert api_key == "foobar"

@patch.dict("os.environ", {"CONNECT_ENDPOINT": "http://foo.bar"})
def test_get_endpoint(self):
provider = EnvironmentConfigProvider()
endpoint = provider.get_value("endpoint")
assert endpoint == "http://foo.bar"

def test_get_value_miss(self):
provider = EnvironmentConfigProvider()
value = provider.get_value("foobar")
assert value == None


class TestConfigBuilder:
def test_build(self):
builder = ConfigBuilder()
assert builder._config == Config()

def test_build_with_provider(self):
provider = Mock()
provider.get_value = Mock()
builder = ConfigBuilder([provider])
builder.build()
for key in Config.__annotations__:
provider.get_value.assert_any_call(key)

def test_set_api_key(self):
builder = ConfigBuilder()
builder.set_api_key("foobar")
assert builder._config.api_key == "foobar"

def test_set_endpoint(self):
builder = ConfigBuilder()
builder.set_endpoint("http://foo.bar")
assert builder._config.endpoint == "http://foo.bar"
2 changes: 0 additions & 2 deletions src/posit/say_hello.py

This file was deleted.

Empty file removed tests/__init__.py
Empty file.
9 changes: 0 additions & 9 deletions tests/test_say_hello.py

This file was deleted.