You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "whynick1 (via GitHub)" <gi...@apache.org> on 2024/03/26 05:43:53 UTC
[PR] feat: Support customized header in Rest catalog client [iceberg-rust]
whynick1 opened a new pull request, #306:
URL: https://github.com/apache/iceberg-rust/pull/306
resolve https://github.com/apache/iceberg-rust/issues/292
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "whynick1 (via GitHub)" <gi...@apache.org>.
whynick1 commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1541494804
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -963,6 +993,76 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
@Fokko Can we address that using a separate PR? Because this PR is to resolve https://github.com/apache/iceberg-rust/issues/292. If you can help file an issue (to support `rest.`), I will be happy to work on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#issuecomment-2024066123
Thanks for @whynick1 for working on this. Thanks @liurenjie1024 @Fokko for the reviews!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1540025945
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -956,6 +983,68 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1541552671
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -963,6 +993,76 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
Checked the Java and Python's implementation. Both are without `rest.` for headers. I'd be nice to have a `rest.` prefix. It doesn't seem a blocker to me though.
pyIceberg doc from https://github.com/apache/iceberg-python/pull/467:
```yaml
catalog:
default:
uri: http://rest-catalog/ws/
credential: t-1234:secret
header.content-type: application/vnd.api+json
```
Java config example
```
spark.sql.catalog.my-catalog.header.X-Catalog-Name=default
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "whynick1 (via GitHub)" <gi...@apache.org>.
whynick1 commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1540153323
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -956,6 +983,68 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
Addressed with f16f1a992ccc2d32f3a55fec23d75fb50a755640. cc @flyrain
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "whynick1 (via GitHub)" <gi...@apache.org>.
whynick1 commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1541504517
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -103,8 +104,7 @@ impl RestCatalogConfig {
])
}
- fn try_create_rest_client(&self) -> Result<HttpClient> {
- // TODO: We will add ssl config, sigv4 later
+ fn get_default_headers(&self) -> Result<HeaderMap> {
Review Comment:
Rename it `http_headers()`. 9ab76786ce65c73f6ee691a5542e57a54bdc420e
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "whynick1 (via GitHub)" <gi...@apache.org>.
whynick1 commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1538626596
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -130,6 +130,33 @@ impl RestCatalogConfig {
);
}
+ for (key, value) in self.props.iter() {
+ if let Some(stripped_key) = key.strip_prefix("header.") {
+ 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.get_default_headers()?;
Review Comment:
Refactor code into a new function `get_default_headers` for easier unit testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "whynick1 (via GitHub)" <gi...@apache.org>.
whynick1 commented on PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#issuecomment-2019455618
@flyrain @liurenjie1024 Please take a look. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "whynick1 (via GitHub)" <gi...@apache.org>.
whynick1 commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1540191330
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -956,6 +983,68 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
> We can fix it here, and file issues in python/java repo
Filed issue https://github.com/apache/iceberg-python/issues/550. Feel free to assign to me. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "whynick1 (via GitHub)" <gi...@apache.org>.
whynick1 commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1539932487
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -956,6 +983,68 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
@flyrain I had the same question, so I end up reference [how customize header is supported in iceberg-python](https://github.com/apache/iceberg-python/blob/main/pyiceberg/catalog/rest.py#L482-L491), which allows overwrite default header.
I am happy to address that. To just confirm, pros like `header.x-client-version` (conflict with default header) should be ignored instead of overwrite default header. Is that correct? If so, should we also update python client?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1540327433
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -103,8 +104,7 @@ impl RestCatalogConfig {
])
}
- fn try_create_rest_client(&self) -> Result<HttpClient> {
- // TODO: We will add ssl config, sigv4 later
+ fn get_default_headers(&self) -> Result<HeaderMap> {
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain merged PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1539833388
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -956,6 +983,68 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1541552671
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -963,6 +993,76 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
Checked the Java and Python's implementation. Both are without `rest.` for headers. It'd be nice to have a `rest.` prefix. It doesn't seem a blocker to me though.
pyIceberg doc from https://github.com/apache/iceberg-python/pull/467:
```yaml
catalog:
default:
uri: http://rest-catalog/ws/
credential: t-1234:secret
header.content-type: application/vnd.api+json
```
Java config example
```
spark.sql.catalog.my-catalog.header.X-Catalog-Name=default
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1541090474
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -963,6 +993,76 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
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?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -956,6 +983,68 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
We don't want to override default headers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1540327945
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -103,8 +104,7 @@ impl RestCatalogConfig {
])
}
- fn try_create_rest_client(&self) -> Result<HttpClient> {
- // TODO: We will add ssl config, sigv4 later
+ fn get_default_headers(&self) -> Result<HeaderMap> {
Review Comment:
cc @whynick1 Would you mind to do this rename in this pr or in following prs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "whynick1 (via GitHub)" <gi...@apache.org>.
whynick1 commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1538626859
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -956,6 +983,68 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
`header.content-type` (with `header.` prefix) is a custom header. This unit test will verify the `content-type` is in `get_default_headers()`, which will be used to create rest catalog client.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#issuecomment-2020044992
cc @flyrain PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1540253733
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -103,8 +104,7 @@ impl RestCatalogConfig {
])
}
- fn try_create_rest_client(&self) -> Result<HttpClient> {
- // TODO: We will add ssl config, sigv4 later
+ fn get_default_headers(&self) -> Result<HeaderMap> {
Review Comment:
Minor: we may simplify the name to `headers()` or `http_headers()`, this is consistent with other function name like `table_endpoint`, `url_prefixed`, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "whynick1 (via GitHub)" <gi...@apache.org>.
whynick1 commented on code in PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#discussion_r1539932487
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -956,6 +983,68 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_default_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.get_default_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_get_default_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(),
Review Comment:
@flyrain I had the same question, so I end up reference [how customize header is supported in iceberg-python](https://github.com/apache/iceberg-python/blob/main/pyiceberg/catalog/rest.py#L482-L491), which allows overwrite default header.
I am happy to address that. To just confirm, props like `header.x-client-version` (conflict with default header) should be ignored instead of overwrite default header. Is that correct? If so, should we also update python client?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Support customized header in Rest catalog client [iceberg-rust]
Posted by "whynick1 (via GitHub)" <gi...@apache.org>.
whynick1 commented on PR #306:
URL: https://github.com/apache/iceberg-rust/pull/306#issuecomment-2021248780
Note: CI check failed due to a known issue https://github.com/apache/iceberg-rust/issues/307, which has been fixed. Rerun build should success.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org