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

custom client + parse macro + signature check functions #44

Merged
merged 28 commits into from
Aug 9, 2021

Conversation

JakubKoralewski
Copy link
Contributor

Let me know what you think when you get the chance.

@JakubKoralewski
Copy link
Contributor Author

Here's how I implemented a RequestBuilder for async:

struct AsyncRequestBuilder {
    inner: RequestBuilder
}

impl RequestBuildah for AsyncRequestBuilder {
    type Error = anyhow::Error;
    type ReturnValue = tokio::task::JoinHandle<anyhow::Result<String>>;

    fn new(method: Method, url: &'_ str) -> Self {
        Self {
            inner: CLIENT.request(method, url)
        }
    }

    fn body(mut self, b: String) -> Self {
        self.inner = self.inner.body(b);

        self
    }

    fn header<K, V>(mut self, key: K, val: V) -> Self where HeaderName: TryFrom<K>, HeaderValue: TryFrom<V>, <HeaderName as TryFrom<K>>::Error: Into<Error>, <HeaderValue as TryFrom<V>>::Error: Into<Error> {
        self.inner = self.inner.header(key, val);

        self
    }

    fn send(self) -> Result<Self::ReturnValue, Self::Error> {
        Ok(tokio::spawn(async {
             Ok(
                 self.inner
                    .send_with_retry(100, ExponentialBackoff::default()).await?
                    .error_for_status()?
                    .text()
                    .await?
             )
        }))
    }
}

src/lib.rs Outdated

fn new(method: http::Method, url: impl IntoUrl) -> Self;
fn new(method: http::Method, url: &'_ str) -> Self;
Copy link
Contributor Author

@JakubKoralewski JakubKoralewski Jul 10, 2021

Choose a reason for hiding this comment

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

I'm not very happy with this signature, as to achieve connection pooling it requires you to use lazy_static type of solution. Ideally I would like to add a third argument to fn new to be able to pass here a reqwest::Client type of variable to clone it for reusing the connection pool. I think adding a third associated type ClientBuilder or sth would be a good idea.

@JakubKoralewski
Copy link
Contributor Author

JakubKoralewski commented Jul 11, 2021

Here I added the ability to pass in a client builder:

1e4820f (#1)

I'm not sure if this should be added as for the default case it requries passsing a reference to an empty tuple. This is because lazy_static is being used for the DefaultRequestBuilder anyway.

EDIT : I merged it in.

@JakubKoralewski JakubKoralewski marked this pull request as ready for review July 14, 2021 08:51
@JakubKoralewski
Copy link
Contributor Author

made fn encode public cause I needed it available for my tests

@JakubKoralewski
Copy link
Contributor Author

sorry I'm piling on like that, this PR is not really a WIP anymore, I'd have opened new PRs if this one got merged

@JakubKoralewski JakubKoralewski changed the title custom client + parse macro custom client + parse macro + signature check functions Jul 15, 2021
@JakubKoralewski
Copy link
Contributor Author

Closes #29

@JakubKoralewski
Copy link
Contributor Author

sorry I'm piling on like that, this PR is not really a WIP anymore, I'd have opened new PRs if this one got merged

now it's ready 😅

Copy link
Owner

@gifnksm gifnksm left a comment

Choose a reason for hiding this comment

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

Thanks for the great pull request.
I would love to merge them.

I've made review comments.
If you have any questions, please feel free to ask.

The source code in the example directory has not been reviewed yet, as it may need to be modified due to changes in src/lib.rs.

Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@JakubKoralewski
Copy link
Contributor Author

The examples seem to be failing with a 404 response. I'm not investigating that.

Made changes I found actionable, waiting for response for others.

@gifnksm
Copy link
Owner

gifnksm commented Aug 9, 2021

The examples seem to be failing with a 404 response. I'm not investigating that.

I'm going to fix the problem in example, so ignore it.

Made changes I found actionable, waiting for response for others.

I reviewed it again (sorry for being late).
The fix I suggested has been committed to this branch.
You can merge it as it is, or you can modify it.

@JakubKoralewski
Copy link
Contributor Author

I run miri with the following command cargo test miri parse that ran all the tests involving the parse_query_string function and got no warnings or errors!

@gifnksm gifnksm merged commit 6755cf7 into gifnksm:master Aug 9, 2021
@gifnksm
Copy link
Owner

gifnksm commented Aug 9, 2021

Thank you!

@JakubKoralewski JakubKoralewski deleted the custom-client branch August 9, 2021 17:09
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