feat: Allow user to send custom headers in http client #756
Conversation
client/http-client/src/client.rs
Outdated
| @@ -77,10 +79,17 @@ impl HttpClientBuilder { | |||
| self | |||
| } | |||
|
|
|||
| /// | |||
| pub fn custom_headers(mut self, headers: HeaderMap) -> Self { | |||
There was a problem hiding this comment.
I would rather have a more unified API for the WS client and HTTP client, so if you could add a similar API as https://github.com/paritytech/jsonrpsee/blob/master/client/ws-client/src/lib.rs#L125-#L128 I would be happy with it.
The reason why is to avoid to require users to use http::HeaderMap however we could also re-export HeaderMap but I'm in favor of the WS client API for configuring custom headers.
There was a problem hiding this comment.
sorry, I think it would make more sense to return an error instead of unwrap these might fail for example if HeaderName is uppercase and similar.
in addition an already existing header will just be overwritten with append.
so might be better what you did initially with HeaderMap for simplicity and change the WS client API instead.
There was a problem hiding this comment.
I Added a custom error variant, and made the method fallible. I think this is the nicest we can make this without exposing extraneous APIs.
I also would like to keep the ability to overwrite default headers, what if user wants to send Content-Type: application/json+jsonrpc instead of the default Content-type: application/json
There was a problem hiding this comment.
makes sense,
I think the docs should explain that you can overwrite the default headers too perhaps the doc tests/examples regarding that would awesome :)
semtexzv
force-pushed
the
master
branch
2 times, most recently
from
9670a7c to
397a71f
Compare
| @@ -77,10 +82,20 @@ impl HttpClientBuilder { | |||
| self | |||
| } | |||
|
|
|||
| /// Adds a custom header, which is sent with every request | |||
There was a problem hiding this comment.
can you add some additional docs that explains that "dupllicate" keys will be overwritten by this method?
in addition the docs should explain that you can overwrite the default headers too perhaps the doc tests/examples regarding that would awesome :)
| #[error("Invalid http headers provided")] | ||
| InvalidHeaders, |
There was a problem hiding this comment.
| #[error("Invalid http headers provided")] | |
| InvalidHeaders, | |
| #[error("Invalid http header provided")] | |
| InvalidHeader, |
(just because this error is emitted when one attempt to set a single header)
| @@ -77,10 +82,20 @@ impl HttpClientBuilder { | |||
| self | |||
| } | |||
|
|
|||
| /// Adds a custom header, which is sent with every request | |||
| pub fn add_header(mut self, key: &str, value: &str) -> Result<Self, Error> { | |||
There was a problem hiding this comment.
@niklasad1 in the WS client, we accept &str header names and values and just return Self. Is there any reason why we shouldn't have the same API here do you think?
There was a problem hiding this comment.
it's because two different http libraries are used, soketto is using to httparse::Header which isn't as pedantic as hyper::HeaderMap is.
that's why I was leaning into exposing fn add_headers(header: HeaderMap) -> Self for both instead which makes things more uniform and quite obvious how it works.
There was a problem hiding this comment.
Ah I see! I guess the issue there is that the ws-client doesn't (afaict) depend on hyper, so it would be a shame to add it as a required dependency just for that type?
There was a problem hiding this comment.
looks like https://docs.rs/http/latest/http/index.html is sufficient which doesn't really have much deps at all.
However, the better fix is probably to use http directly in soketto otherwise we need to convert it to httparse::Header anyway.
There was a problem hiding this comment.
Mmm that makes sense!
So regardless of whether we merge this, perhaps we should create another issue/PR to migrate soketto to using http so that we can standardise on this interface (and on what we're using for headers across the two clients, which would be nice)
There was a problem hiding this comment.
I'm happy with this once fmt and such are fixed, but as @niklasad1 suggested I think it'd be good to move to using something like https://docs.rs/http/latest/http/header/struct.HeaderMap.html consistently across clients and then we can expose a more consistent API for doing this, too.
|
@semtexzv are you planning on finishing this or shall we take it over? It's a useful PR for sure. |
|
Hey, sorry. I moved on to custom implementation of jsonrpc, feel free to take over. |
|
Superseeded by #814 |
As a user, I want to use authorization for some requests, or implement cache-control on top of my RPCs to bitcoin nodes.
This seems like a simplest way to achieve that.