liurenjie1024 commented on code in PR #254:
URL: https://github.com/apache/iceberg-rust/pull/254#discussion_r1520950487
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -113,6 +117,19 @@ impl RestCatalogConfig {
),
]);
+ if let Some(token) = self.props.get("token") {
+ headers.insert(
+ "Authorization",
Review Comment:
Ditto.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -113,6 +117,19 @@ impl RestCatalogConfig {
),
]);
+ if let Some(token) = self.props.get("token") {
Review Comment:
Please make this a constant.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -497,13 +516,56 @@ impl RestCatalog {
client: config.try_create_rest_client()?,
config,
};
-
+ catalog.fetch_access_token().await?;
+ catalog.client = catalog.config.try_create_rest_client()?;
catalog.update_config().await?;
catalog.client = catalog.config.try_create_rest_client()?;
Ok(catalog)
}
+ async fn fetch_access_token(&mut self) -> Result<()> {
Review Comment:
```suggestion
async fn refresh_access_token(&mut self) -> Result<()> {
```
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -144,13 +161,15 @@ impl HttpClient {
.with_source(e)
})?)
} else {
+ let code = resp.status();
let text = resp.bytes().await?;
let e = serde_json::from_slice::<E>(&text).map_err(|e| {
Error::new(
ErrorKind::Unexpected,
"Failed to parse response from rest catalog server!",
)
.with_context("json", String::from_utf8_lossy(&text))
+ .with_context("code", code.to_string())
Review Comment:
Also add it for `execute`?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -497,13 +516,56 @@ impl RestCatalog {
client: config.try_create_rest_client()?,
config,
};
-
+ catalog.fetch_access_token().await?;
+ catalog.client = catalog.config.try_create_rest_client()?;
catalog.update_config().await?;
catalog.client = catalog.config.try_create_rest_client()?;
Ok(catalog)
}
+ async fn fetch_access_token(&mut self) -> Result<()> {
+ if self.config.props.contains_key("token") {
+ return Ok(());
+ }
+ if let Some(credential) = self.config.props.get("credential") {
+ let (client_id, client_secret) = if credential.contains(':') {
+ let (client_id, client_secret) =
credential.split_once(':').unwrap();
+ (Some(client_id), client_secret)
+ } else {
+ (None, credential.as_str())
+ };
+ let mut params = HashMap::with_capacity(4);
+ params.insert("grant_type", "client_credentials");
+ if let Some(client_id) = client_id {
+ params.insert("client_id", client_id);
+ }
+ params.insert("client_secret", client_secret);
+ params.insert("scope", "catalog");
+ let req = self
+ .client
+ .0
+ .post(self.config.get_token_endpoint())
Review Comment:
In fact the auth url could be configurable:
https://github.com/apache/iceberg-python/blob/70342ac83d2d1f121f3ab04c6d7317c8830fdad1/pyiceberg/catalog/rest.py#L306
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -497,13 +516,56 @@ impl RestCatalog {
client: config.try_create_rest_client()?,
config,
};
-
+ catalog.fetch_access_token().await?;
+ catalog.client = catalog.config.try_create_rest_client()?;
catalog.update_config().await?;
catalog.client = catalog.config.try_create_rest_client()?;
Ok(catalog)
}
+ async fn fetch_access_token(&mut self) -> Result<()> {
+ if self.config.props.contains_key("token") {
+ return Ok(());
+ }
Review Comment:
I don't think we should add this check since toke may expire, and we need to
refresh it.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -497,13 +516,56 @@ impl RestCatalog {
client: config.try_create_rest_client()?,
config,
};
-
+ catalog.fetch_access_token().await?;
+ catalog.client = catalog.config.try_create_rest_client()?;
Review Comment:
The python implementation update config first, then recreate client? Is this
required or we can relax? cc @Fokko @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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
