-
Notifications
You must be signed in to change notification settings - Fork 448
feat: Support customized header in Rest catalog client #306
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
Changes from all commits
8d691ca
0876420
f16f1a9
252e8f8
9ab7678
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| //! This module contains rest catalog implementation. | ||
|
|
||
| use std::collections::HashMap; | ||
| use std::str::FromStr; | ||
|
|
||
| use async_trait::async_trait; | ||
| use itertools::Itertools; | ||
|
|
@@ -103,8 +104,7 @@ impl RestCatalogConfig { | |
| ]) | ||
| } | ||
|
|
||
| fn try_create_rest_client(&self) -> Result<HttpClient> { | ||
| // TODO: We will add ssl config, sigv4 later | ||
| fn http_headers(&self) -> Result<HeaderMap> { | ||
| let mut headers = HeaderMap::from_iter([ | ||
| ( | ||
| header::CONTENT_TYPE, | ||
|
|
@@ -133,6 +133,36 @@ impl RestCatalogConfig { | |
| ); | ||
| } | ||
|
|
||
| for (key, value) in self.props.iter() { | ||
| if let Some(stripped_key) = key.strip_prefix("header.") { | ||
| // Avoid overwriting default headers | ||
| if !headers.contains_key(stripped_key) { | ||
| headers.insert( | ||
| HeaderName::from_str(stripped_key).map_err(|e| { | ||
| Error::new( | ||
| ErrorKind::DataInvalid, | ||
| format!("Invalid header name: {stripped_key}!"), | ||
| ) | ||
| .with_source(e) | ||
| })?, | ||
| HeaderValue::from_str(value).map_err(|e| { | ||
| Error::new( | ||
| ErrorKind::DataInvalid, | ||
| format!("Invalid header value: {value}!"), | ||
| ) | ||
| .with_source(e) | ||
| })?, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| Ok(headers) | ||
| } | ||
|
|
||
| fn try_create_rest_client(&self) -> Result<HttpClient> { | ||
| // TODO: We will add ssl config, sigv4 later | ||
| let headers = self.http_headers()?; | ||
|
|
||
| Ok(HttpClient( | ||
| Client::builder().default_headers(headers).build()?, | ||
| )) | ||
|
|
@@ -963,6 +993,76 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_http_headers() { | ||
| let server = Server::new_async().await; | ||
| let mut props = HashMap::new(); | ||
| props.insert("credential".to_string(), "client1:secret1".to_string()); | ||
|
|
||
| let config = RestCatalogConfig::builder() | ||
| .uri(server.url()) | ||
| .props(props) | ||
| .build(); | ||
| let headers: HeaderMap = config.http_headers().unwrap(); | ||
|
|
||
| let expected_headers = HeaderMap::from_iter([ | ||
| ( | ||
| header::CONTENT_TYPE, | ||
| HeaderValue::from_static("application/json"), | ||
| ), | ||
| ( | ||
| HeaderName::from_static("x-client-version"), | ||
| HeaderValue::from_static(ICEBERG_REST_SPEC_VERSION), | ||
| ), | ||
| ( | ||
| header::USER_AGENT, | ||
| HeaderValue::from_str(&format!("iceberg-rs/{}", CARGO_PKG_VERSION)).unwrap(), | ||
| ), | ||
| ]); | ||
| assert_eq!(headers, expected_headers); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_http_headers_with_custom_headers() { | ||
| let server = Server::new_async().await; | ||
| let mut props = HashMap::new(); | ||
| props.insert("credential".to_string(), "client1:secret1".to_string()); | ||
| props.insert( | ||
| "header.content-type".to_string(), | ||
| "application/yaml".to_string(), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like the header config will overwrite the default one. Are we going to allow that? We may not overwrite these default headers since it makes less sense. For example, we may never customize ICEBERG_REST_SPEC_VERSION, it should come from the lib itself. In this PR, can we focus on allowing users to add extra headers in this PR. cc @liurenjie1024
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @flyrain I had the same question, so I end up reference how customize header is supported in iceberg-python, which allows overwrite default header. I am happy to address that. To just confirm, props like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can fix it here, and file issues in python/java repo. I assume all default headers should not be overwritten. WDYT? @whynick1 @liurenjie1024 @Fokko
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Filed issue apache/iceberg-python#550. Feel free to assign to me. Thanks.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want to override default headers. |
||
| ); | ||
| props.insert( | ||
| "header.customized-header".to_string(), | ||
| "some/value".to_string(), | ||
| ); | ||
|
|
||
| let config = RestCatalogConfig::builder() | ||
| .uri(server.url()) | ||
| .props(props) | ||
| .build(); | ||
| let headers: HeaderMap = config.http_headers().unwrap(); | ||
|
|
||
| let expected_headers = HeaderMap::from_iter([ | ||
| ( | ||
| header::CONTENT_TYPE, | ||
| HeaderValue::from_static("application/json"), | ||
| ), | ||
| ( | ||
| HeaderName::from_static("x-client-version"), | ||
| HeaderValue::from_static(ICEBERG_REST_SPEC_VERSION), | ||
| ), | ||
| ( | ||
| header::USER_AGENT, | ||
| HeaderValue::from_str(&format!("iceberg-rs/{}", CARGO_PKG_VERSION)).unwrap(), | ||
| ), | ||
| ( | ||
| HeaderName::from_static("customized-header"), | ||
| HeaderValue::from_static("some/value"), | ||
| ), | ||
| ]); | ||
| assert_eq!(headers, expected_headers); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_oauth_with_auth_url() { | ||
| let mut server = Server::new_async().await; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For REST we have the
rest.prefix for REST-specific config, see: https://py.iceberg.apache.org/configuration/We might want to consider it here as well, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fokko Can we address that using a separate PR? Because this PR is to resolve #292. If you can help file an issue (to support
rest.), I will be happy to work on it.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked the Java and Python's implementation. Both are without
rest.for headers. It'd be nice to have arest.prefix. It doesn't seem a blocker to me though.pyIceberg doc from apache/iceberg-python#467:
Java config example