-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: adds get_user
and get_current_user
methods.
#7
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,36 @@ | ||
from requests import Session | ||
from typing import Optional | ||
|
||
from . import hooks | ||
|
||
from .auth import Auth | ||
from .config import ConfigBuilder | ||
from .users import Users | ||
|
||
|
||
class Client: | ||
users: Users | ||
|
||
def __init__( | ||
self, endpoint: Optional[str] = None, api_key: Optional[str] = None | ||
self, | ||
api_key: Optional[str] = None, | ||
endpoint: Optional[str] = None, | ||
) -> None: | ||
builder = ConfigBuilder() | ||
builder.set_api_key(api_key) | ||
builder.set_endpoint(endpoint) | ||
if api_key: | ||
builder.set_api_key(api_key) | ||
if endpoint: | ||
builder.set_endpoint(endpoint) | ||
self._config = builder.build() | ||
|
||
if self._config.api_key is None: | ||
raise ValueError("Invalid value for 'api_key': Must be a non-empty string.") | ||
if self._config.endpoint is None: | ||
raise ValueError( | ||
"Invalid value for 'endpoint': Must be a non-empty string." | ||
) | ||
|
||
self._session = Session() | ||
self._session.hooks["response"].append(hooks.handle_errors) | ||
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 | ||
) | ||
self.users = Users(self._config.endpoint, self._session) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
class ClientError(Exception): | ||
def __init__( | ||
self, error_code: int, error_message: str, http_status: int, http_message: str | ||
): | ||
self.error_code = error_code | ||
self.error_message = error_message | ||
self.http_status = http_status | ||
self.http_message = http_message | ||
super().__init__( | ||
f"{error_message} (Error Code: {error_code}, HTTP Status: {http_status} {http_message})" | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import pytest | ||
|
||
from .errors import ClientError | ||
|
||
|
||
class TestClientError: | ||
def test(self): | ||
error_code = 0 | ||
error_message = "foo" | ||
http_status = 404 | ||
http_message = "Foo Bar" | ||
with pytest.raises( | ||
ClientError, match=r"foo \(Error Code: 0, HTTP Status: 404 Foo Bar\)" | ||
): | ||
raise ClientError( | ||
error_code=error_code, | ||
error_message=error_message, | ||
http_status=http_status, | ||
http_message=http_message, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
from http.client import responses | ||
from requests import Response | ||
|
||
from .errors import ClientError | ||
|
||
|
||
def handle_errors(response: Response, *args, **kwargs) -> Response: | ||
if response.status_code >= 400 and response.status_code < 500: | ||
data = response.json() | ||
error_code = data["code"] | ||
message = data["error"] | ||
http_status = response.status_code | ||
http_status_message = responses[http_status] | ||
raise ClientError(error_code, message, http_status, http_status_message) | ||
return response |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import pytest | ||
|
||
from unittest.mock import Mock | ||
|
||
from .hooks import handle_errors | ||
|
||
|
||
class TestHandleErrors: | ||
def test(self): | ||
response = Mock() | ||
response.status_code = 200 | ||
assert handle_errors(response) == response | ||
|
||
def test_client_error(self): | ||
response = Mock() | ||
response.status_code = 400 | ||
response.json = Mock() | ||
response.json.return_value = {"code": 0, "error": "foobar"} | ||
with pytest.raises(Exception): | ||
handle_errors(response) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import os | ||
|
||
from requests import Session, Response | ||
|
||
|
||
class Users: | ||
def __init__(self, endpoint: str, session: Session) -> None: | ||
self._endpoint = endpoint | ||
self._session = session | ||
|
||
def get_user(self, user_id: str) -> Response: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We haven't had the design discussion on this, so take this is my opinion/taste on the matter, not strictly PR feedback. IMO it would be nice to have a common pattern to our collections of entities, like instead of
etc. Not attached to a particular verb per se, just that each collection has the same form, and the parameter names align. WTYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this model as well. It's been a while, but I recall using the ActiveRecord ORM pattern in Ruby-on-Rails. But, the advantage there was mapping relationships down to the SQL query layer for query optimization. We won't be able to do that here. Do you have any prior art I could take a look at? Let's see if we can get some consensus on a design pattern here. I'm too close to the implementation details on this one to know what our users would most enjoy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this isn't quite a SQL ORM. The model that came to mind first for me was Other models we could look at could be pygithub, boto, [...] not an exhaustive list, just trying to think of other projects that users of this SDK may be familiar with and that have a similar objective--allow users to explore and manage collections of entities. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
endpoint = os.path.join(self._endpoint, "v1/users", user_id) | ||
return self._session.get(endpoint) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what does this return exactly? Looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct. Yes, we'll want something else. The most straightforward idea is to return the response body to the user on request. But I've been thinking through other ideas, like...
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
def get_current_user(self) -> Response: | ||
endpoint = os.path.join(self._endpoint, "v1/user") | ||
return self._session.get(endpoint) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
from unittest.mock import Mock | ||
|
||
from .users import Users | ||
|
||
|
||
class TestUsers: | ||
def test_get_user(self): | ||
session = Mock() | ||
session.get = Mock(return_value={}) | ||
users = Users(endpoint="http://foo.bar/", session=session) | ||
response = users.get_user(user_id="foo") | ||
assert response == {} | ||
session.get.assert_called_once_with("http://foo.bar/v1/users/foo") | ||
|
||
def test_get_current_user(self): | ||
session = Mock() | ||
session.get = Mock(return_value={}) | ||
users = Users(endpoint="http://foo.bar/", session=session) | ||
response = users.get_current_user() | ||
assert response == {} | ||
session.get.assert_called_once_with("http://foo.bar/v1/user") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
from posit.client import Client | ||
|
||
client = Client() | ||
res = client.users.get_current_user() | ||
print(res.json()) | ||
Comment on lines
+1
to
+5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the example shown in the Pull Request overview. You can invoke it as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good (soon, not here) to start thinking about things like quartodoc or other ways to structure code examples that are runable... and what example server we might want to run against for that, if at all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I like this idea a lot. I've used lots of Sphinx. The I also had the thought to create a separate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm making a strategic decision here to perform validation in the thing that needs the property. In this case, the
Client
needs theapi_key
and theendpoint
.I could argue that this should go in the
ConfigBuilder#build
method, but since I expect additional methods to consume Config, it makes sense to me to lay SoC here.There was a problem hiding this comment.
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 see the value of this ConfigBuilder factory abstraction. If
Config
needs to exist apart fromClient
, I'd more expect that the__init__
method for Config (and any subclasses thereof) would take the arguments it needs. Not that I'm the arbiter of it, but that feels more Pythonic to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr I'm planning a change to simplify this.
The crux of the idea is to provide some flexibility in how configuration attributes can be set. For example, you may want to set the endpoint URL inline or via
CONNECT_SERVER
. There may be other locations where configuration may come from, such as environment files, secret managers, and distributed stores, to name a few. TheConfigProvider
definition provides this functionality.The
ConfigBuilder#build
iterates through the providers in some priority order to decide on the in-memory value. Right now the priority is "in-line > environment property"But who am I kidding? I over-engineered the mess out of that file, and I had fun doing it...
I'll see if simplifying it reduces the validation burden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW what I have in mind is something along the lines of https://github.com/databricks/databricks-sdk-py/blob/main/databricks/sdk/__init__.py#L89-L143, though with fewer arguments. Basically:
(could even omit kwargs for now since I don't think there's any other way today)
That way there could be future subclasses of Config, and those could be either provided explicitly as
config
, or we could wire up more logic in the Config init or whatever classmethod to detect.