feat: adds get_user and get_current_user methods.
#7
Conversation
Adds a new property to the Client class called 'users', which is an instance of a User class. The User class is responsible for managing requests to the /v1/user and /v1/users endpoints. Additionally, a hook is added to check and parse client errors from Connect.
| from posit.client import Client | ||
|
|
||
| client = Client() | ||
| res = client.users.get_current_user() | ||
| print(res.json()) |
There was a problem hiding this comment.
This is the example shown in the Pull Request overview. You can invoke it as CONNECT_API_KEY=... CONNECT_SERVER=... python tinkering.py
There was a problem hiding this comment.
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.
Yeah, I like this idea a lot. I've used lots of Sphinx. The quartodoc setup looks similar.
I also had the thought to create a separate cookbooks module that's part of an extras package.
| 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." | ||
| ) |
There was a problem hiding this comment.
I'm making a strategic decision here to perform validation in the thing that needs the property. In this case, the Client needs the api_key and the endpoint.
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.
I'm not sure I see the value of this ConfigBuilder factory abstraction. If Config needs to exist apart from Client, 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.
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. The ConfigProvider 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.
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:
class Client(object):
def __init__(self, server=os.getenv("CONNECT_SERVER"), token=os.getenv("CONNECT_API_KEY"), **kwargs, config=None):
if config is None:
config = Config(server, token, **kwargs)
...
(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.
| 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." | ||
| ) |
There was a problem hiding this comment.
I'm not sure I see the value of this ConfigBuilder factory abstraction. If Config needs to exist apart from Client, 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.
| self._endpoint = endpoint | ||
| self._session = session | ||
|
|
||
| def get_user(self, user_id: str) -> Response: |
There was a problem hiding this comment.
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 client.users.get_user(user_id) client.content.get_content(content_id), more like:
client.users.find_one(id, name, email)
client.users.find(**kwargs)
client.content.find_one(id, title)
client.content.find_one(id).permissions.find(**kwargs)
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.
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.
Yeah this isn't quite a SQL ORM. The model that came to mind first for me was pymongo, because we essentially have collections of records that we want to be able to (a) get one from or (b) get something iterable to scan over.
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.
|
|
||
| def get_user(self, user_id: str) -> Response: | ||
| endpoint = os.path.join(self._endpoint, "v1/users", user_id) | ||
| return self._session.get(endpoint) |
There was a problem hiding this comment.
So what does this return exactly? Looks like requests.Response? We probably want something better than that. (Can make a separate issue.)
There was a problem hiding this comment.
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...
- Return something that implements a
to_dateframe(pandas) method. - Convert all responses into standard HATEOAS style that a paginator implementation understands.
| from posit.client import Client | ||
|
|
||
| client = Client() | ||
| res = client.users.get_current_user() | ||
| print(res.json()) |
There was a problem hiding this comment.
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.
Adds a new property to the Client class named
users, which is an instance of a User class. The User class manages requests to the/v1/userand/v1/usersendpoints.Additionally, a hook is added to check and parse client errors from Connect.
Example