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.

Sign up for GitHub

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

Jump to bottom

Call current user API v1/user during sdk client initialization. #33

Closed
wants to merge 9 commits into from
Closed

Call current user API v1/user during sdk client initialization. #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
61cb261
Call current user API `v1/user` during sdk client initialization.
dbkegley Feb 20, 2024
2460459
Merge branch 'main' of github.com:posit-dev/posit-sdk-py into kegs/cu…
dbkegley Feb 20, 2024
fdc7913
Always call users api
dbkegley Feb 20, 2024
3d35dbc
Fix client tests
dbkegley Feb 20, 2024
ecb0e8d
Remove endpoints.py and tests
dbkegley Feb 20, 2024
89b7afc
merge: resolves conflicts (#40)
tdstein Feb 21, 2024
8c5beb9
Merge remote-tracking branch 'origin/main' into kegs/current-user
tdstein Feb 21, 2024
59f1c75
Merge branch 'main' into kegs/current-user
tdstein Feb 21, 2024
3246cc3
fix: url resolution
tdstein Feb 21, 2024
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
21 changes: 15 additions & 6 deletions src/posit/connect/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@

from .auth import Auth
from .config import Config
from .resources import CachedResources
from .users import Users, User
from .users import CachedUsers, Users, User


class Client:
Expand All @@ -32,14 +31,24 @@ def __init__(
session.auth = Auth(config=self.config)
# Add error handling hooks to the session.
session.hooks["response"].append(hooks.handle_errors)

# Initialize the Users instance.
self.users: CachedResources[User] = Users(config=self.config, session=session)
# Store the Session object.
self.session = session

# Place to cache the server settings
# Internal properties for storing public resources
self.server_settings = None
self._current_user: Optional[User] = None

@property
def me(self) -> User:
if self._current_user is None:
url = urls.append_path(self.config.url, "v1/user")
response = self.session.get(url)
self._current_user = User(**response.json())
return self._current_user

@property
def users(self) -> CachedUsers:
return Users(client=self)
Comment on lines +49 to +51
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to instantiate a new instance every time? The Users class is designed to fetch from the server once, and then use cached results for subsequent iterations.

There are pros and cons to fetching from the server every time.

We currently don't have any methods that mutate the server, but when we do, my thought is to invalidate the cache on methods that mutate (post, delete, put). This would trigger a fresh fetch on the next iteration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, passing self to Users instead of the session and config seems odd. This makes client.users.client.users.client.users valid

If the goal is to reduce the boilerplate of passing two variables down to one, I would prefer creating a DTO object that wraps session and config. But, I still think that is overkill at this stage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Users class is designed to fetch from the server once, and then use cached results for subsequent iterations.

I think instantiating from Client every time gives you the behavior you want. If you (the SDK user) want to use the cached results, do something like all_users = client.users.find() and work with all_users as much as you want. But some actions you'll want to do will modify the collection on the server, and you'll want a way to refresh. So then you go to client.users again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, passing self to Users instead of the session and config seems odd. This makes client.users.client.users.client.users valid

True, but IME it's useful to be able to get back to the original connection object from wherever you are in the API.

If the goal is to reduce the boilerplate of passing two variables down to one, I would prefer creating a DTO object that wraps session and config. But, I still think that is overkill at this stage.

Isn't Client already the object that wraps session and config?

Copy link
Collaborator Author

@dbkegley dbkegley Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't really have any strong opinion about how this happens. My only requirement at this stage is that there is some way to get information about the currently authenticated user from the v1/user endpoint. I need this information to be available in the OAuthIntegration Resource.

I'm fine with changing to approach to use DI instead if that's preferred. For example:

def OAuthIntegration(Resource):
  def __init__(self, current_user: CurrentUser[Resource]):
    ...
    
  def get_credentials():
    guid = self.current_user.get().get('guid')

Copy link
Collaborator

Choose a reason for hiding this comment

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

True, but IME it's useful to be able to get back to the original connection object from wherever you are in the API.

session holds the connection here.

Isn't Client already the object that wraps session and config?

Yes, but circular references should generally be avoided.


@property
def connect_version(self):
Expand Down
28 changes: 25 additions & 3 deletions src/posit/connect/client_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,31 @@ def test_init(
MockAuth.assert_called_once_with(config=MockConfig.return_value)
MockConfig.assert_called_once_with(api_key=api_key, url=url)
MockSession.assert_called_once()
MockUsers.assert_called_once_with(
config=MockConfig.return_value, session=MockSession.return_value
)

def test_users(
self,
MockUsers: MagicMock,
):
api_key = "foobar"
url = "http://foo.bar/__api__"
client = Client(api_key=api_key, url=url)
client.users
MockUsers.assert_called_once_with(client=client)

@patch("posit.connect.client.Session")
@patch("posit.connect.client.User")
def test_me(
self,
User: MagicMock,
Session: MagicMock,
):
api_key = "foobar"
url = "http://foo.bar/__api__"
client = Client(api_key=api_key, url=url)
User.assert_not_called()
assert client._current_user is None
client.me
User.assert_called_once()

def test__del__(self, MockAuth, MockConfig, MockSession, MockUsers):
api_key = "foobar"
Expand Down
23 changes: 10 additions & 13 deletions src/posit/connect/users.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
from __future__ import annotations

from datetime import datetime
from typing import Iterator, Callable, List
from typing import Iterator, Callable, List, TYPE_CHECKING

from requests import Session
if TYPE_CHECKING:
from .client import Client
tdstein marked this conversation as resolved.
Show resolved Hide resolved

from . import urls

from .config import Config
from .resources import Resources, Resource, CachedResources

# The maximum page size supported by the API.
Expand Down Expand Up @@ -43,17 +43,14 @@ def get(self, id: str) -> User:


class Users(CachedUsers, Resources[User]):
def __init__(
self, config: Config, session: Session, *, page_size=_MAX_PAGE_SIZE
) -> None:
def __init__(self, client: Client, *, page_size=_MAX_PAGE_SIZE) -> None:
if page_size > _MAX_PAGE_SIZE:
raise ValueError(
f"page_size must be less than or equal to {_MAX_PAGE_SIZE}"
)

super().__init__(config.url)
self.config = config
self.session = session
super().__init__(client.config.url)
self.client = client
self.page_size = page_size

def fetch(self, index) -> tuple[Iterator[User] | None, bool]:
Expand All @@ -68,9 +65,9 @@ def fetch(self, index) -> tuple[Iterator[User] | None, bool]:
# Define query parameters for pagination.
params = {"page_number": page_number, "page_size": self.page_size}
# Create the URL for the endpoint.
url = urls.append_path(self.config.url, "v1/users")
url = urls.append_path(self.client.config.url, "v1/users")
# Send a GET request to the endpoint with the specified parameters.
response = self.session.get(url, params=params)
response = self.client.session.get(url, params=params)
# Convert response to dict
json: dict = dict(response.json())
# Parse the JSON response and extract the results.
Expand All @@ -82,6 +79,6 @@ def fetch(self, index) -> tuple[Iterator[User] | None, bool]:
return (users, exhausted)

def get(self, id: str) -> User:
url = urls.append_path(self.config.url, f"v1/users/{id}")
response = self.session.get(url)
url = urls.append_path(self.client.config.url, f"v1/users/{id}")
response = self.client.session.get(url)
return User(**response.json())
14 changes: 4 additions & 10 deletions src/posit/connect/users_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,12 @@


@pytest.fixture
def mock_config():
with patch("posit.connect.users.Config") as mock:
yield mock.return_value


@pytest.fixture
def mock_session():
with patch("posit.connect.users.Session") as mock:
def mock_client():
with patch("posit.connect.client.Client") as mock:
yield mock.return_value


class TestUsers:
def test_init(self, mock_config, mock_session):
def test_init(self, mock_client):
with pytest.raises(ValueError):
Users(mock_config, mock_session, page_size=9999)
Users(mock_client, page_size=9999)
2 changes: 1 addition & 1 deletion tinkering.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
from posit.connect import Client

with Client() as client:
print(client.me)
print(client.get("v1/users"))
print(client.users.get("f55ca95d-ce52-43ed-b31b-48dc4a07fe13"))

users = client.users
users = users.find(lambda user: user["first_name"].startswith("T"))
users = users.find(lambda user: user["last_name"].startswith("S"))
Expand Down
Loading