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.

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

Design by wishful thinking for Connect SDK #16

Merged
merged 6 commits into from
Feb 14, 2024
Merged
Changes from all commits
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
82 changes: 82 additions & 0 deletions src/posit/connect/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
## Posit Connect SDK

> Note: this is design-by-wishful-thinking, not how things actually work today.
> To discuss or propose changes, open a PR suggesting new language.
### Connecting

To get started, import the Connect `Client` and create a connection. You can specify the `endpoint` for your Connect server URL and your `api_key`; if not specified, they'll be pulled from the environment (`CONNECT_SERVER` and `CONNECT_API_KEY`).

It is expected that `Client()` just works from within any Posit product's environment (Workbench, Connect, etc.), either by API key and prior system configuration, or by some means of identity federation.

```
from posit.connect import Client

con = Client()
```
Comment on lines +11 to +15
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
```
from posit.connect import Client
con = Client()
```
from posit.connect import create_client
with create_client() as client:
client...

This helps with resource management.

@contextmanager
def create_client(
    api_key: Optional[str] = None, endpoint: Optional[str] = None
) -> Generator[Client, None, None]:
    """Creates a new :class:`Client` instance

    Keyword Arguments:
        api_key -- an api_key for authentication (default: {None})
        endpoint -- a base api endpoint (url) (default: {None})

    Returns:
        A :class:`Client` instance
    """
    client = Client(api_key=api_key, endpoint=endpoint)
    try:
        yield client
    finally:
        del 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.

What resource do you expect client to be holding onto?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, it holds a requests.Session object, which has a close method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there anything that needs to be cleaned up that won't be handled on exit?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just the requests.Session instance that needs to be released. Otherwise, the GC should handle most scenarios.


### Collections and entities

Many resources in the SDK refer to *collections* of *entities* or records in Connect.

All of the general collections can be referenced as properties of the Client object (e.g. `client.content`, `client.users`). Some collections belong to a single entity and are referenced from them similarly (e.g. `content_item.permissions`).

All collections are iterable objects with all read-only List-like methods implemented. They also have the following methods:

* `.find()`: returns another iterable collection object.
* Calling `.find()` with no arguments retrieves all available entities
* If no entities match the query, `.find()` returns a length-0 collection.
* Iterating over a collection without having first called `find()` is equivalent to having queried for all.
* `find()` should use query-based REST APIs where existing, and fall back to retrieving all and filtering client-side where those APIs do not (yet) exist.
* Should `collection.find().find()` work? Probably.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, and the second call should operate on in-memory/cached results.

* `.get(guid)` method that returns a single entity by id. If one is not found, it raises `NotFoundError`
* `.find_one()` is a convenience method that queries with `.find()` and returns a single entity
* If more than one entity match the query, `.find_one()` returns the first
* If no entities match, `.find_one()` returns `None`
* If you need stricter behavior (e.g. you want to be sure that one and only one entity are returned by your query), use `.find()` or `.get()`.
* `.to_pandas()` materializes the collection in a pandas `DataFrame`.
* pandas is not a required dependency of the SDK. `.to_pandas()` should try to import inside the method.

The `.find()` and `.find_one()` methods use named arguments rather than accepting a dict so that IDE tab completion can work.

Collections should handle all API reponse pagination invisibly so that the Python user doesn't need to worry about pages.

Entities have methods that are appropriate to them. Fields in the entity bodies can be accessed as properties.

```
for st in con.content.find(app_mode="streamlit"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding the .find(app_mode="streamline") vs .find({ "app_mode": "streamline"}). I'm unsure what it looks like to support API differences between Connect versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are many ways to address this and we don't need to solve it now. But, I would like to note it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO we make the Python interface whatever we think it should be, and internally map parameters to whatever the connect api/version uses.

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 added some language about this below, LMK what you think.

print(st.title)

my_app = con.content.get("1234-5678-90ab-cdef")
for perm in my_app.permissions:
print(perm.role)
```

### Mapping to HTTP request methods

Entities have an `.update()` method that maps to a `PATCH` request. `.delete()` is `DELETE`.

```
my_app.update(title="Quarterly Analysis of Team Velocity")
my_app.permissions.find_one(email="[email protected]").update(role="owner")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do like this quite a bit. +1 for find_one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Assuming that find_one(kwargs) is find(kwargs)[0], should it error if find() returns len != 1?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've associated find with an Optional return value in other languages. (e.g., go find this thing if you can; otherwise, let me know you came up empty-handed.)

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 restored .find_one() and more explicitly described behavior, LMK if that's what you had in mind.

my_app.permissions.find_one(email="[email protected]").delete()
```

Collections have a `.create()` method that maps to `POST` to create a new entity. It may be aliased to other verbs as appropriate for the entity.

```
my_app.permissions.add(email="[email protected]", role="viewer")
```

### Field/attribute naming

The Python SDK should present the interface we wish we had, and we can evolve the REST API to match that over time. It is the adapter layer that allows us to evolve the Connect API more freely.

Naming of fields and arguments in collection and entity methods should be standardized across entity types for consistency, even if this creates a gap between our current REST API specification.

As a result, the SDK takes on the burden of smoothing over the changes in the Connect API over time. Each collection and entity class may need its own adapter methods that take the current Python SDK field names and maps to the values for the version of the Connect server being used when passing to the HTTP methods.

Entity `.to_dict()` methods likewise present the names and values in the Python interface, which may not map to the actual HTTP response body JSON. There should be some other way to access the raw response body.

### Lower-level HTTP interface

The client object has `.get`, `.post`, etc. methods that pass arguments through to the `requests` methods, accepting URL paths relative to the API root and including the necessary authorization. These are invoked inside the collection and entity action methods, and they are also available for users to call directly, whether because there are API resources we haven't wrapped in Pythonic methods yet, or because they are simpler RPC-style endpoints that just need to be hit directly.
Loading