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

Provide with_header method on client #108

Merged
merged 1 commit into from
Aug 7, 2024
Merged

Conversation

jongiddy
Copy link
Contributor

This change allows arbitrary headers to be included in requests to ClickHouse.

It uses HeaderName and HeaderValue which is efficient for use with hyper. The types are provided by the http crate to allow support for other HTTP client crates to use these types without requiring the full hyper crate.

There are other ways to provide this capability, including unsealing the HttpClient trait to allow more implementations. However, this change provides a limited additional capability until a public interface for HttpClient is decided.

Fixes #98

@slvrtrn
Copy link
Contributor

slvrtrn commented Aug 2, 2024

In the hyper documentation, they use plain strings as headers names/values: https://docs.rs/hyper/latest/hyper/struct.Request.html#examples

It uses HeaderName and HeaderValue which is efficient for use with hyper.

If there are optimizations made by HeaderName, and the custom headers will likely use some non-standard names (X-My-Proxy-Auth and similar), does it make sense to add http as a dependency, or maybe we can get away with just &str?

From https://docs.rs/http/latest/http/header/struct.HeaderName.html#representation:

HeaderName represents standard header names using an enum, as such they will not require an allocation for storage. All custom header names are lower cased upon conversion to a HeaderName value.

As I understand it, the custom header names are not represented as enums.

src/query.rs Show resolved Hide resolved
@loyd
Copy link
Collaborator

loyd commented Aug 4, 2024

LGTM, let's also pass to INSERTs

src/lib.rs Outdated
/// ```
pub fn with_header(
mut self,
name: impl Into<HeaderName>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can accept impl TryInto to make it possible to pass strings. Downside here is that we cannot use HeaderMap and should use HashMap<Result<_>, Result<_>> instead =(

@CLAassistant
Copy link

CLAassistant commented Aug 5, 2024

CLA assistant check
All committers have signed the CLA.

@jongiddy
Copy link
Contributor Author

jongiddy commented Aug 5, 2024

I rebased the code on the latest version and rewrote it to use plain Strings without requiring the http crate. The client headers now follow a similar structure to client options.

@loyd
Copy link
Collaborator

loyd commented Aug 7, 2024

LGTM, but need to fix rustfmt warnings

This allows arbitrary headers to be included in requests to ClickHouse.
@loyd loyd merged commit 76d6417 into ClickHouse:master Aug 7, 2024
5 checks passed
@loyd
Copy link
Collaborator

loyd commented Aug 7, 2024

Merged, thanks for your contribution!

loyd added a commit that referenced this pull request Aug 7, 2024
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.

Support adding extra headers
4 participants