refactor: REST Catalog implementation

[connortsui20]

Followup of #962

#962 Introduced a bug where it is not some of the methods allow for both StatusCode::OK and StatusCode::NO_CONTENT as success cases, when in reality it should be one or the other (this was me, sorry about that).

This PR attempts to unify the 3 different types of response helpers that essentially all do the exact same thing slightly differently. The main addition here is a function query_catalog:

    // Queries the Iceberg REST catalog with the given `Request` and a provided handler.
    pub async fn query_catalog<R, H, Fut>(&self, mut request: Request, handler: H) -> Result<R>
    where
        R: DeserializeOwned,
        H: FnOnce(Response) -> Fut,
        Fut: Future<Output = Result<R>>,
    {
        self.authenticate(&mut request).await?;

        let response = self.client.execute(request).await?;

        handler(response).await
    }

By allowing each Catalog method to specify how they want to handle the responses, it gets much finer control on the success/error cases as well as the error messages. Previously, there were 3 functions that all did similar things:

    pub async fn query<R: DeserializeOwned, E: DeserializeOwned + Into<Error>>(
        &self,
        mut request: Request,
    ) -> Result<R> {

    pub async fn execute<E: DeserializeOwned + Into<Error>>(
        &self,
        mut request: Request,
    ) -> Result<()> {

    pub async fn do_execute<R, E: DeserializeOwned + Into<Error>>(
        &self,
        mut request: Request,
        handler: impl FnOnce(&Response) -> Option<R>,
    ) -> Result<R> {

I'm also somewhat using this as a chance to refactor some other parts of this crate, mainly documentation and examples.

@Xuanwo It would be great if I could get feedback on some of these proposed changes before I keep going!

[liurenjie1024]

Thanks @connortsui20 for this pr, LGTM! Let's wait for a moment to have more people to take a review on this.

[connortsui20]

@Xuanwo could you take a look at the latest commit and see if it makes sense?

[Xuanwo]

@Xuanwo could you take a look at the latest commit and see if it makes sense?

Hi, I'm still not in favor of the handler style. The stack size of my brain is not sufficient to handle the extra handler (sorry for that). Maybe we could consider the following?

let resp: Response = client.query_catalog(request).await?;
if response.status() == StatusCode::OK {
    deserialize_catalog_response::<CatalogConfig>(response).await
} else {
    Err(deserialize_unexpected_catalog_error(response).await)
}

[connortsui20]

@Xuanwo could you take a look at the latest commit and see if it makes sense?

Hi, I'm still not in favor of the handler style. The stack size of my brain is not sufficient to handle the extra handler (sorry for that). Maybe we could consider the following?

let resp: Response = client.query_catalog(request).await?;
if response.status() == StatusCode::OK {
    deserialize_catalog_response::<CatalogConfig>(response).await
} else {
    Err(deserialize_unexpected_catalog_error(response).await)
}

Okay, I'll make that change and then we can reevaluate if it makes more sense to delay any serialization errors + extra boilerplate. You're probably right by the way, I guess I just think about this weirdly.

[connortsui20]

@Xuanwo ready for review! I think that this version is a lot more boilerplate but that should be fine as it is more or less linearly readable.

[Xuanwo]

Thank you @connortsui20 for working on this. I love this version.

[Xuanwo]

Inviting @liurenjie1024 for another look.

[Xuanwo]

I'll merge this PR in three days if there are no further objections. I believe it's on the right track, and additional polish can be made on top of it.