Skip to content

refactor: Allow Builder to be cloned by removing unnecessary reference and lifetime. #51

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

Merged

Conversation

jshearer
Copy link
Contributor

@jshearer jshearer commented Jul 7, 2023

What kind of change does this PR introduce?

request::Client is already cloneable since it's just an Arc<ClientRef>. So let's get rid of all of the &'a Client stuff and instead just have Builder own a Client, and clone it when needed.

What is the current behavior?

Builder has a &'a Client.

What is the new behavior?

Builder owns its own Client, and therefore is able to be #[derive(Clone)].

Additional context

The impetus for this change is that I wanted to build support for automatically paginating Builders in order to transparently fetch more rows than a single request is able to. Since executing a builder consumes it, I needed a way to keep the original builder around after executing, in order to update its range and make the request for the next page of results.

A much trimmed down example of what this could look like:

async fn api_exec<T>(b: postgrest::Builder) -> anyhow::Result<T>
where
    for<'de> T: serde::Deserialize<'de>,
{
    let req = b.execute().await?;
    let status = resp.status();

    if status.is_success() {
        let v: T = resp.json().await?;
        Ok(v)
    } else {
        let body = resp.text().await?;
        anyhow::bail!("{status}: {body}");
    }
}

async fn api_exec_paginated<T>(b: postgrest::Builder) -> anyhow::Result<Vec<T>>
where
    for<'de> T: serde::Deserialize<'de>,
{
    let mut offset = 0;
    let mut rows: Vec<T> = vec![];

    loop {
        let req = b.clone().range(offset, offset + API_ROW_LIMIT);
        let mut resp = api_exec::<Vec<T>>(req).await?;

        if resp.len() == 0 {
            break;
        }

        offset += resp.len();
        rows.append(&mut resp);
    }

    Ok(rows)
}

A side benefit here is that (at least IMO) this is more ergonomic: no more lifetimes cluttering any struct that needs to hold a Builder

…nce and lifetime. `request::Client` is already cloneable since it's just an `Arc<ClientRef>`. So let's get rid of all of the `&'a Client` stuff and instead just have Builder own a Client, and clone it when needed.
Copy link
Collaborator

@soedirgo soedirgo left a comment

Choose a reason for hiding this comment

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

Thanks! That is a nifty way of doing pagination 💯

@soedirgo soedirgo merged commit f1163c5 into supabase-community:master Jul 7, 2023
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