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.

Sign up for GitHub

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

feat: Allow user to send custom headers in http client #756

Closed
wants to merge 1 commit into from
Closed

feat: Allow user to send custom headers in http client #756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions client/http-client/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,16 @@
// IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.

use std::str::FromStr;
use std::sync::Arc;
use std::time::Duration;

use crate::transport::HttpTransportClient;
use crate::types::{ErrorResponse, Id, NotificationSer, ParamsSer, RequestSer, Response};
use async_trait::async_trait;
use hyper::header::{HeaderName, InvalidHeaderName};
use hyper::http::HeaderValue;
use hyper::HeaderMap;
use jsonrpsee_core::client::{CertificateStore, ClientT, IdKind, RequestIdManager, Subscription, SubscriptionClientT};
use jsonrpsee_core::{Error, TEN_MB_SIZE_BYTES};
use jsonrpsee_types::error::CallError;
Expand All @@ -44,6 +48,7 @@ pub struct HttpClientBuilder {
max_concurrent_requests: usize,
certificate_store: CertificateStore,
id_kind: IdKind,
custom_headers: HeaderMap,
}

impl HttpClientBuilder {
Expand Down Expand Up @@ -77,10 +82,20 @@ impl HttpClientBuilder {
self
}

/// Adds a custom header, which is sent with every request
Copy link
Member

@niklasad1 niklasad1 May 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

pub fn add_header(mut self, key: &str, value: &str) -> Result<Self, Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@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?

Copy link
Member

@niklasad1 niklasad1 May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@jsdw jsdw May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

self.custom_headers.append(
HeaderName::from_str(key).map_err(|_| Error::InvalidHeaders)?,
HeaderValue::from_str(value).map_err(|_| Error::InvalidHeaders)?,
);
Ok(self)
}

/// Build the HTTP client with target to connect to.
pub fn build(self, target: impl AsRef<str>) -> Result<HttpClient, Error> {
let transport = HttpTransportClient::new(target, self.max_request_body_size, self.certificate_store)
.map_err(|e| Error::Transport(e.into()))?;
let transport =
HttpTransportClient::new(target, self.max_request_body_size, self.certificate_store, self.custom_headers)
.map_err(|e| Error::Transport(e.into()))?;
Ok(HttpClient {
transport,
id_manager: Arc::new(RequestIdManager::new(self.max_concurrent_requests, self.id_kind)),
Expand All @@ -97,6 +112,7 @@ impl Default for HttpClientBuilder {
max_concurrent_requests: 256,
certificate_store: CertificateStore::Native,
id_kind: IdKind::Number,
custom_headers: HeaderMap::new(),
}
}
}
Expand Down
19 changes: 13 additions & 6 deletions client/http-client/src/transport.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
// the JSON-RPC request id to a value that might have already been used.

use hyper::client::{Client, HttpConnector};
use hyper::Uri;
use hyper::{HeaderMap, Uri};
use jsonrpsee_core::client::CertificateStore;
use jsonrpsee_core::error::GenericTransportError;
use jsonrpsee_core::http_helpers;
Expand Down Expand Up @@ -43,6 +43,8 @@ pub struct HttpTransportClient {
client: HyperClient,
/// Configurable max request body size
max_request_body_size: u32,
/// Custom headers sent with every request
custom_headers: HeaderMap,
}

impl HttpTransportClient {
Expand All @@ -51,6 +53,7 @@ impl HttpTransportClient {
target: impl AsRef<str>,
max_request_body_size: u32,
cert_store: CertificateStore,
custom_headers: hyper::HeaderMap,
) -> Result<Self, Error> {
let target: Uri = target.as_ref().parse().map_err(|e| Error::Url(format!("Invalid URL: {}", e)))?;
if target.port_u16().is_none() {
Expand Down Expand Up @@ -84,7 +87,7 @@ impl HttpTransportClient {
return Err(Error::Url(err.into()));
}
};
Ok(Self { target, client, max_request_body_size })
Ok(Self { target, client, max_request_body_size, custom_headers })
}

async fn inner_send(&self, body: String) -> Result<hyper::Response<hyper::Body>, Error> {
Expand All @@ -94,11 +97,15 @@ impl HttpTransportClient {
return Err(Error::RequestTooLarge);
}

let req = hyper::Request::post(&self.target)
let mut builder = hyper::Request::post(&self.target)
.header(hyper::header::CONTENT_TYPE, hyper::header::HeaderValue::from_static(CONTENT_TYPE_JSON))
.header(hyper::header::ACCEPT, hyper::header::HeaderValue::from_static(CONTENT_TYPE_JSON))
.body(From::from(body))
.expect("URI and request headers are valid; qed");
.header(hyper::header::ACCEPT, hyper::header::HeaderValue::from_static(CONTENT_TYPE_JSON));

for (h, v) in self.custom_headers.iter() {
builder = builder.header(h, v);
}

let req = builder.body(From::from(body)).expect("URI and request headers are valid; qed");

let response = self.client.request(req).await.map_err(|e| Error::Http(Box::new(e)))?;
if response.status().is_success() {
Expand Down
Loading