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

Allow storing, extracting installation tokens #436

Closed

Conversation

marcusirgens
Copy link
Contributor

First attempt to handle #435.

Most of the changes in this pull request are from the added test.

I made some breaking changes, which I am happy to discuss or to solve differently. The reasoning behind this is that adding a method on octocrab::Octocrab caused a lot of stutter:

impl Octocrab {
    pub fn installation(&self, id: InstallationId) -> Self { /* ... */ }
    pub async fn installation_and_token(&self, id: InstallationId) -> Result<(Self, InstallationToken)> { /* ... */ }

    // new?
    pub fn installation_with_token(&self, id: InstallationId, token: InstallationToken) -> Self { /* ... */ }
    pub async fn current_installation_token(&self, force_refetch: bool) -> Result<InstallationToken> { /* ... */ }
}

The approach I suggest gives the following API:

impl Octocrab {
    pub fn installation(&self, id: InstallationId) -> InstallationClientBuilder { /* ... */ }
    pub async fn installation_token(&self, force_refetch: bool) -> Result<InstallationToken> { /* ... */ }
}

InstallationClientBuilder

octocrab::Octocrab::installation() now emits an octocrab::InstallationClientBuilder.

Building without a known token (this is probably the most significant breaking change):

Prior to this change:

let installation_client: octocrab::Octocrab = app_client.installation(123.into());

After this change:

let installation_client: octocrab::Octocrab = app_client.installation(123.into()).build();

Building with a known token

Not possible prior to this change.

After this change:

let installation_client: octocrab::Octocrab = app_client
    .installation(123.into())
    .with_token(my_token)
    .build();

Building a client and emitting the token

Prior to this change:

let (installation_client, token): (octocrab::Octocrab, octocrab::models::InstallationToken) = app_client.installation_and_token(123.into).await?;

After this change:

let installation_client: octocrab::Octocrab = app_client.installation(123.into()).build();
let token = installation_client.installation_token(/* force_refetch: */ false).await?;

Fetching the token from the client

The token can now be extracted. Here's the gist of how we're going to use this internally, which allows us to significantly reduce how many requests we send to GitHub.

async fn our_business_logic(app_client: octocrab::Octocrab, token_cache: TokenCache, customer: Customer) -> Result<(), Error> {
    let installation_client = match token_cache.get_installation(customer.id).await? {
        Some((id, token)) => app_client.installation(id).with_token(token).build(),
        None => {
            let id = app_client.apps().get_org_installation(customer.org).await?;
            let installation_client = app_client.installation(id).build();
            token_cache.set_installation(id, installation_client.installation_token(false).await?);
            installation_client
        }
    }

    /* our secret sauce goes here */
}

@marcusirgens
Copy link
Contributor Author

marcusirgens commented Aug 6, 2023

Thinking a bit, an alternative to this could be adding these utility methods on octocrab::AuthState or a wrapper and providing a mutable getter for it:

let crab: octocrab::Octocrab = build_with_app_auth();
let installation_crab = crab.installation(123.into()); // Current API, no changes

let mut auth = installation_crab.auth(); // New API, no BC break
auth.set_installation_token(my_token_getter());

That would allow keeping the existing API while avoiding the stuttering issues, and adds a logical place to operate on the auth state, which is currently not possible,

@XAMPPRocky
Copy link
Owner

Thank you for your issue! Is this still relevant to the current version?

@XAMPPRocky
Copy link
Owner

I'm going close this for now, but feel free to reopen it if it's still relevant.

@XAMPPRocky XAMPPRocky closed this Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants