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

WIP: use reqwest-middleware #252

Closed
wants to merge 1 commit into from

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Nov 22, 2022

https://github.com/TrueLayer/reqwest-middleware brings retry & tracing support.

I havent updated the docs yet; waiting to see if there is interest in this feature.

I see a bunch of libs in https://github.com/oxidecomputer/third-party-api-clients are using reqwest-middleware, and https://github.com/oxidecomputer/reqwest-conditional-middleware

I see https://crates.io/crates/openapitor/0.0.5/dependencies is also using reqwest-middleware, but doesnt have its code published on git.

@jayvdb
Copy link
Contributor Author

jayvdb commented Nov 22, 2022

ping @augustuswm @jessfraz

@augustuswm
Copy link
Contributor

Being able to use middleware in generated clients would be nice, but I'm not sure if it is worth the tradeoff of requiring all clients to use the reqwest_middleware crate.

Also as you noted in the status function, the way error handling is done for middleware has some issues. Internally the Middleware variant of reqwest_middleware::Error carries an anyhow error, which is not ideal for an API client as we lose the ability to provide a structured error back.

If this were an opt-in change I would more onboard, but that does bring along additional complexity to users. I'll defer to @ahl though on if there are others that are interested in a middleware feature.

@DDtKey
Copy link
Contributor

DDtKey commented Nov 28, 2022

I believe it could be optional feature or somehow generalized 🤔
I'd like to use reqwest-middleware, but it make sense to keep ability to use pure client as well

@jayvdb
Copy link
Contributor Author

jayvdb commented Nov 28, 2022

I dont mind making it a CLI-arg opt-in feature.

@ahl
Copy link
Collaborator

ahl commented Dec 5, 2022

We already let consumers effect the generation with pre- and post-hooks -- would that be sufficient? I'm not a fan of having this by default.. in fact, I've been thinking about ripping out reqwest and moving to a lighter weight HTTP client of our own construction.

@jayvdb
Copy link
Contributor Author

jayvdb commented Dec 5, 2022

As this requires improvements to progenitor-client, it does not seem like this would be easy to do with pre- and post-hooks.

Making the codebase able to support either reqwest or reqwest-middleware (with reqwest as default) will lead the way to allowing other http clients.

@ahl
Copy link
Collaborator

ahl commented Dec 27, 2022

Thanks for making this, but after looking into reqwest-middleware more, I don't think we're interested in this at this time.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jan 7, 2024

For anyone interested in using reqwest_middleware, the following will transform the generated client. Sorry it is quite messy.

// Position of "mod types" in the AST
const MOD_TYPES_INDEX: usize = 3;

/// Transform the `Client`, leaving the struct name as `Client`, and
/// replace `Client` inside the code (which was reqwest::Client) to be
/// `ClientWithMiddleware`.
pub fn use_reqwest_middleware(input: &mut syn::File) {
    /// Removes any `reqwest::` prefix.
    struct RemoveReqwestPrefixVisitor;

    impl VisitMut for RemoveReqwestPrefixVisitor {
        fn visit_path_mut(&mut self, path: &mut syn::Path) {
            let mut new_segments = syn::punctuated::Punctuated::new();
            for segment in path.segments.iter_mut() {
                if segment.ident != "reqwest" {
                    new_segments.push(segment.clone());
                }
            }
            path.segments = new_segments;
            syn::visit_mut::visit_path_mut(self, path);
        }
    }

    // Remove "reqwest::" prefix
    let mut rewriter = RemoveReqwestPrefixVisitor {};
    rewriter.visit_file_mut(input);

    // Used to replace all or some occurrences of `from` to `to`.
    struct Renamer {
        from: String,
        to: syn::Ident,
        found: u64,
        max_times: u64,
    }

    impl Fold for Renamer {
        fn fold_ident(&mut self, id: syn::Ident) -> syn::Ident {
            if id == self.from {
                self.found += 1;
                if self.max_times == 0 || self.found <= self.max_times {
                    self.to.clone()
                } else {
                    id
                }
            } else {
                id
            }
        }
    }

    let mut renamer = Renamer {
        to: proc_macro2::Ident::new("ClientWithMiddleware", proc_macro2::Span::call_site()),
        from: "Client".to_string(),
        max_times: 0,
        found: 0,
    };

    for i in MOD_TYPES_INDEX..input.items.len() - 1 {
        // this renames `pub struct Client` as well as all other mentions of `Client`
        let new_item = input.items[i].clone();
        let new_item = renamer.fold_item(new_item.clone());
        // this bit restores `pub struct Client`
        let mut renamer_revert_once = Renamer {
            to: proc_macro2::Ident::new("Client", proc_macro2::Span::call_site()),
            from: "ClientWithMiddleware".to_string(),
            max_times: 1,
            found: 0,
        };
        let new_item = renamer_revert_once.fold_item(new_item.clone());
        match i.cmp(&(MOD_TYPES_INDEX + 2)) {
            Ordering::Equal => {
                let impl_item: syn::ItemImpl = cast!(&new_item, syn::Item::Impl).to_owned();
                let new_impl_item = transform_client_impl(impl_item);
                input.items[i] = syn::Item::Impl(new_impl_item);
            }
            Ordering::Greater => {
                if let syn::Item::Impl(new_impl) = new_item {
                    let no_doc_test: syn::Attribute = parse_quote! {
                        #[cfg(not(doctest))]
                    };
                    let mut new_impl = new_impl.clone();
                    new_impl.attrs.insert(0, no_doc_test);
                    input.items[i] = syn::Item::Impl(new_impl);
                } else {
                    input.items[i] = new_item;
                }
            }
            Ordering::Less => {
                input.items[i] = new_item;
            }
        }
    }
}

// The following replaces the body of `Client::new` to use reqwest_middleware
fn transform_client_impl(input: syn::ItemImpl) -> syn::ItemImpl {
    let mut new_impl = input;
    let mut new_items: Vec<syn::ImplItem> = vec![];
    for item in new_impl.items.iter_mut() {
        match item {
            syn::ImplItem::Fn(m) => {
                let name = m.sig.ident.to_string();
                if name == "new" {
                    let new_block: syn::Block = parse_quote! {
                        {
                            let dur = std::time::Duration::from_secs(15);

                            let client = reqwest_middleware::ClientBuilder::new(
                                reqwest::ClientBuilder::new()
                                    .connect_timeout(dur)
                                    .timeout(dur)
                                    .build()
                                    .unwrap(),
                            )
                            .build();

                            Self::new_with_client(baseurl, client)
                        }
                    };
                    m.block = new_block;
                }
                new_items.push(syn::ImplItem::Fn(m.to_owned()))
            }
            other => new_items.push(other.to_owned()),
        }
    }
    new_impl.items = new_items;
    new_impl
}

After that, re-inject

use reqwest::{header, Body};

and inject

use reqwest_middleware::ClientWithMiddleware;

and in Cargo.toml add

progenitor-client = { git = "https://github.com/franklin-ai/progenitor/", branch = "ros-enhancements-combined" }
reqwest-middleware = "*"

Specifically this is the commit needed for progenitor-client
franklin-ai@9573e42

I can tidy this up and publish it as a separate crate if anyone needs it.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jan 24, 2024

reqwest-middleware enhanced progenitor-client rebased at https://github.com/jayvdb/progenitor/tree/reqwest-middleware-v2

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.

None yet

4 participants