Repository: posit-dev/posit-sdk-py

Issue:
Remove `session` and `url` from the underlying hash table in `Resource`.
During `Resource` initialization, the `session` and `url` are stored as items within the underlying hash table. This results in unexpected behavior when a resource list into a pandas DataFrame.

Try to modify the usage of `session` and `url` so that they exist as internal properties instead of on the underlying table.

Discussion:
github-actions[bot]: Hey there! 👋

We noticed that the title of your pull request doesn't follow the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) specification. To ensure consistency, we kindly ask you to adjust the title accordingly.

Here are the details:

```
Unknown release type "refactor" found in pull request title "refactor: moves session and url to resource attributes". 

Available types:
 - build
 - chore
 - ci
 - docs
 - feat
 - fix
 - perf
 - style
 - test
```
<!-- Sticky Pull Request Commentlint-error -->
github-actions[bot]: # ☂️ Python Coverage
> current status: ✅
## Overall Coverage
| Lines | Covered | Coverage | Threshold | Status |
| :---: | :-----: | :------: | :-------: | :----: |
|  501  |   414   |    83%   |    80%    |   🟢   |
## New Files
No new covered files...
## Modified Files
| File                           | Coverage | Status |
| :----------------------------- | :------: | :----: |
| src/posit/connect/client.py    |   100%   |   🟢   |
| src/posit/connect/content.py   |    73%   |   🟢   |
| src/posit/connect/resources.py |   100%   |   🟢   |
| src/posit/connect/users.py     |    92%   |   🟢   |
| **TOTAL**                      |  **91%** |   🟢   |


> **updated for commit: `d02d85c` by [action](https://github.com/marketplace/actions/python-coverage)🐍**

Patch:
diff --git a/.github/workflows/conventional-commits.yaml b/.github/workflows/conventional-commits.yaml
index f43068f..53f3984 100644
--- a/.github/workflows/conventional-commits.yaml
+++ b/.github/workflows/conventional-commits.yaml
@@ -23,6 +23,7 @@ jobs:
             fix
             perf
             style
+            refactor
             test
       - uses: marocchino/sticky-pull-request-comment@v2
         if: always() && (steps.lint.outputs.error_message != null)
diff --git a/src/posit/connect/client.py b/src/posit/connect/client.py
index 6d9a751..3983c00 100644
--- a/src/posit/connect/client.py
+++ b/src/posit/connect/client.py
@@ -50,7 +50,7 @@ class Client:
     def me(self) -> User:
         url = urls.append_path(self.config.url, "v1/user")
         response = self.session.get(url)
-        return User(**response.json())
+        return User(self.session, url, **response.json())
 
     @property
     def oauth(self) -> OAuthIntegration:
diff --git a/src/posit/connect/content.py b/src/posit/connect/content.py
index 56ec07e..5ce5949 100644
--- a/src/posit/connect/content.py
+++ b/src/posit/connect/content.py
@@ -1,6 +1,6 @@
 from __future__ import annotations
 
-from typing import Any, Callable, List, Optional
+from typing import Callable, List, Optional
 
 from requests import Session
 
@@ -183,9 +183,6 @@ class ContentItem(Resource):
     def id(self) -> str:
         return self.get("id")  # type: ignore
 
-    def __setattr__(self, name: str, value: Any) -> None:
-        raise AttributeError("Cannot set attributes: use update() instead")
-
 
 class Content(Resources[ContentItem]):
     def __init__(self, config: Config, session: Session) -> None:
@@ -197,22 +194,34 @@ class Content(Resources[ContentItem]):
         self, filter: Callable[[ContentItem], bool] = lambda _: True
     ) -> List[ContentItem]:
         results = self.session.get(self.url).json()
-        return [ContentItem(**c) for c in results if filter(ContentItem(**c))]
+        items = (
+            ContentItem(
+                session=self.session,
+                url=urls.append_path(self.url, result["guid"]),
+                **result,
+            )
+            for result in results
+        )
+        return [item for item in items if filter(item)]
 
     def find_one(
         self, filter: Callable[[ContentItem], bool] = lambda _: True
     ) -> ContentItem | None:
         results = self.session.get(self.url).json()
-        for c in results:
-            content_item = ContentItem(**c)
-            if filter(content_item):
-                return content_item
+        for result in results:
+            item = ContentItem(
+                session=self.session,
+                url=urls.append_path(self.url, result["guid"]),
+                **result,
+            )
+            if filter(item):
+                return item
         return None
 
     def get(self, id: str) -> ContentItem:
         url = urls.append_path(self.url, id)
         response = self.session.get(url)
-        return ContentItem(**response.json())
+        return ContentItem(self.session, url, **response.json())
 
     def create(self) -> ContentItem:
         raise NotImplementedError()
diff --git a/src/posit/connect/resources.py b/src/posit/connect/resources.py
index b37b475..5559e31 100644
--- a/src/posit/connect/resources.py
+++ b/src/posit/connect/resources.py
@@ -1,13 +1,22 @@
 import warnings
 
 from abc import ABC, abstractmethod
-from typing import Generic, List, Optional, TypeVar
+from typing import Any, Generic, List, Optional, TypeVar
+
+import requests
 
 
 T = TypeVar("T")
 
 
 class Resource(ABC, dict):
+    def __init__(self, session: requests.Session, url: str, **kwargs):
+        super().__init__(**kwargs)
+        self.session: requests.Session
+        super().__setattr__("session", session)
+        self.url: str
+        super().__setattr__("url", url)
+
     def __getitem__(self, key):
         warnings.warn(
             f"__getitem__ for '{key}' does not support backwards compatibility. Consider using field based access instead: 'instance.{key}'",
@@ -29,6 +38,9 @@ class Resource(ABC, dict):
         )
         return super().__delitem__(key)
 
+    def __setattr__(self, name: str, value: Any) -> None:
+        raise AttributeError("cannot set attributes: use update() instead")
+
 
 class Resources(ABC, Generic[T]):
     @abstractmethod
diff --git a/src/posit/connect/users.py b/src/posit/connect/users.py
index bfabf7d..d61e903 100644
--- a/src/posit/connect/users.py
+++ b/src/posit/connect/users.py
@@ -1,5 +1,5 @@
 from __future__ import annotations
-from typing import Any, Callable, List, Optional
+from typing import Callable, List, Optional
 
 
 from requests import Session
@@ -57,11 +57,8 @@ class User(Resource):
     def locked(self) -> bool:
         return self.get("locked")  # type: ignore
 
-    def __setattr__(self, name: str, value: Any) -> None:
-        raise AttributeError("Cannot set attributes: use update() instead.")
-
     def _update(self, body):
-        self.get("session").patch(self.get("url"), json=body)
+        self.session.patch(self.url, json=body)
         # If the request is successful, update the local object
         super().update(body)
         # TODO(#99): that patch request returns a payload on success,
@@ -107,15 +104,15 @@ class Users(Resources[User]):
         self, filter: Callable[[User], bool] = lambda _: True, page_size=_MAX_PAGE_SIZE
     ) -> List[User]:
         results = Paginator(self.session, self.url, page_size=page_size).get_all()
-        return [
+        users = (
             User(
-                **user,
                 session=self.session,
                 url=urls.append_path(self.url, user["guid"]),
+                **user,
             )
             for user in results
-            if filter(User(**user))
-        ]
+        )
+        return [user for user in users if filter(user)]
 
     def find_one(
         self, filter: Callable[[User], bool] = lambda _: True, page_size=_MAX_PAGE_SIZE
@@ -125,9 +122,9 @@ class Users(Resources[User]):
             result = pager.get_next_page()
             for u in result:
                 user = User(
-                    **u,
                     session=self.session,
                     url=urls.append_path(self.url, u["guid"]),
+                    **u,
                 )
                 if filter(user):
                     return user
@@ -138,9 +135,9 @@ class Users(Resources[User]):
         response = self.session.get(url)
         raw_user = response.json()
         return User(
-            **raw_user,
             session=self.session,
             url=urls.append_path(self.url, raw_user["guid"]),
+            **raw_user,
         )
 
     def create(self) -> User:
diff --git a/tinkering.py b/tinkering.py
index 6054fdf..a1e62f5 100644
--- a/tinkering.py
+++ b/tinkering.py
@@ -1,11 +1,7 @@
 from posit.connect import Client
 
 with Client() as client:
-    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"))
-    user = users.find_one(lambda user: user["user_role"] == "administrator")
-    print(user)
+    client.get("v1/users")
+    client.users.get("f55ca95d-ce52-43ed-b31b-48dc4a07fe13")
+    client.users.find(lambda user: user.first_name.startswith("T"))
+    client.users.find_one(lambda user: user.first_name.startswith("T"))


Test patch:
diff --git a/tests/posit/connect/test_resources.py b/tests/posit/connect/test_resources.py
index 5a8a20c..380eded 100644
--- a/tests/posit/connect/test_resources.py
+++ b/tests/posit/connect/test_resources.py
@@ -1,10 +1,16 @@
+from unittest.mock import Mock
 import pytest
 import warnings
 
 from typing import Any, List, Optional
 
+from requests.sessions import Session as Session
+
 from posit.connect.resources import Resource, Resources
 
+session = Mock()
+url = Mock()
+
 
 class FakeResource(Resource):
     @property
@@ -13,12 +19,20 @@ class FakeResource(Resource):
 
 
 class TestResource:
+    def test_init(self):
+        k = "foo"
+        v = "bar"
+        d = dict({k: v})
+        r = FakeResource(session, url, **d)
+        assert r.session == session
+        assert r.url == url
+
     def test__getitem__(self):
         warnings.filterwarnings("ignore", category=FutureWarning)
         k = "foo"
         v = "bar"
         d = dict({k: v})
-        r = FakeResource(d)
+        r = FakeResource(session, url, **d)
         assert r.__getitem__(k) == d.__getitem__(k)
         assert r[k] == d[k]
 
@@ -27,7 +41,8 @@ class TestResource:
         k = "foo"
         v1 = "bar"
         v2 = "baz"
-        r = FakeResource({k: v1})
+        d = dict({k: v1})
+        r = FakeResource(session, url, **d)
         assert r[k] == v1
         r[k] = v2
         assert r[k] == v2
@@ -36,7 +51,8 @@ class TestResource:
         warnings.filterwarnings("ignore", category=FutureWarning)
         k = "foo"
         v = "bar"
-        r = FakeResource({k: v})
+        d = dict({k: v})
+        r = FakeResource(session, url, **d)
         assert k in r
         assert r[k] == v
         del r[k]
@@ -46,7 +62,7 @@ class TestResource:
         k = "foo"
         v = "bar"
         d = dict({k: v})
-        r = FakeResource(d)
+        r = FakeResource(session, url, **d)
         assert r.foo == v
 
 
diff --git a/tests/posit/connect/test_users.py b/tests/posit/connect/test_users.py
index 5f77b19..9ebfcda 100644
--- a/tests/posit/connect/test_users.py
+++ b/tests/posit/connect/test_users.py
@@ -1,3 +1,4 @@
+from unittest.mock import Mock
 import pandas as pd
 import pytest
 import responses
@@ -9,83 +10,86 @@ from posit.connect.users import User
 
 from .api import load_mock  # type: ignore
 
+session = Mock()
+url = Mock()
+
 
 class TestUser:
     def test_guid(self):
-        user = User()
+        user = User(session, url)
         assert hasattr(user, "guid")
         assert user.guid is None
-        user = User({"guid": "test_guid"})
+        user = User(session, url, guid="test_guid")
         assert user.guid == "test_guid"
 
     def test_email(self):
-        user = User()
+        user = User(session, url)
         assert hasattr(user, "email")
         assert user.email is None
-        user = User({"email": "test@example.com"})
+        user = User(session, url, email="test@example.com")
         assert user.email == "test@example.com"
 
     def test_username(self):
-        user = User()
+        user = User(session, url)
         assert hasattr(user, "username")
         assert user.username is None
-        user = User({"username": "test_user"})
+        user = User(session, url, username="test_user")
         assert user.username == "test_user"
 
     def test_first_name(self):
-        user = User()
+        user = User(session, url)
         assert hasattr(user, "first_name")
         assert user.first_name is None
-        user = User({"first_name": "John"})
+        user = User(session, url, first_name="John")
         assert user.first_name == "John"
 
     def test_last_name(self):
-        user = User()
+        user = User(session, url)
         assert hasattr(user, "last_name")
         assert user.last_name is None
-        user = User({"last_name": "Doe"})
+        user = User(session, url, last_name="Doe")
         assert user.last_name == "Doe"
 
     def test_user_role(self):
-        user = User()
+        user = User(session, url)
         assert hasattr(user, "user_role")
         assert user.user_role is None
-        user = User({"user_role": "admin"})
+        user = User(session, url, user_role="admin")
         assert user.user_role == "admin"
 
     def test_created_time(self):
-        user = User()
+        user = User(session, url)
         assert hasattr(user, "created_time")
         assert user.created_time is None
-        user = User({"created_time": "2022-01-01T00:00:00"})
+        user = User(session, url, created_time="2022-01-01T00:00:00")
         assert user.created_time == "2022-01-01T00:00:00"
 
     def test_updated_time(self):
-        user = User()
+        user = User(session, url)
         assert hasattr(user, "updated_time")
         assert user.updated_time is None
-        user = User({"updated_time": "2022-01-01T00:00:00"})
+        user = User(session, url, updated_time="2022-01-01T00:00:00")
         assert user.updated_time == "2022-01-01T00:00:00"
 
     def test_active_time(self):
-        user = User()
+        user = User(session, url)
         assert hasattr(user, "active_time")
         assert user.active_time is None
-        user = User({"active_time": "2022-01-01T00:00:00"})
+        user = User(session, url, active_time="2022-01-01T00:00:00")
         assert user.active_time == "2022-01-01T00:00:00"
 
     def test_confirmed(self):
-        user = User()
+        user = User(session, url)
         assert hasattr(user, "confirmed")
         assert user.confirmed is None
-        user = User({"confirmed": True})
+        user = User(session, url, confirmed=True)
         assert user.confirmed is True
 
     def test_locked(self):
-        user = User()
+        user = User(session, url)
         assert hasattr(user, "locked")
         assert user.locked is None
-        user = User({"locked": False})
+        user = User(session, url, locked=False)
         assert user.locked is False
 
 
@@ -117,7 +121,7 @@ class TestUsers:
 
         df = pd.DataFrame(all_users)
         assert isinstance(df, pd.DataFrame)
-        assert df.shape == (3, 13)
+        assert df.shape == (3, 11)
         assert df.columns.to_list() == [
             "email",
             "username",
@@ -130,8 +134,6 @@ class TestUsers:
             "confirmed",
             "locked",
             "guid",
-            "session",
-            "url",
         ]
         assert df["username"].to_list() == ["al", "robert", "carlos12"]
 
@@ -263,6 +265,6 @@ class TestUsers:
 
         with pytest.raises(
             AttributeError,
-            match=r"Cannot set attributes: use update\(\) instead",
+            match=r"cannot set attributes: use update\(\) instead",
         ):
             carlos.first_name = "Carlitos"