plusplusjiajia commented on code in PR #2815:
URL: https://github.com/apache/iceberg-rust/pull/2815#discussion_r3600574694
##########
crates/catalog/rest/src/client.rs:
##########
@@ -58,189 +51,58 @@ impl Debug for HttpClient {
impl HttpClient {
/// Create a new http client.
- pub fn new(cfg: &RestCatalogConfig) -> Result<Self> {
- let extra_headers = cfg.extra_headers()?;
+ pub async fn new(cfg: &RestCatalogConfig) -> Result<Self> {
+ let auth_manager = cfg.resolve_auth_manager()?;
+ let session = auth_manager.init_session().await?;
Ok(HttpClient {
client: cfg.client().unwrap_or_default(),
- token: Mutex::new(cfg.token()),
- token_endpoint: cfg.get_token_endpoint(),
- credential: cfg.credential(),
- extra_headers,
- extra_oauth_params: cfg.extra_oauth_params(),
+ extra_headers: cfg.extra_headers()?,
disable_header_redaction: cfg.disable_header_redaction(),
+ auth_manager,
+ session,
})
}
/// Update the http client with new configuration.
///
/// If cfg carries new value, we will use cfg instead.
/// Otherwise, we will keep the old value.
- pub fn update_with(self, cfg: &RestCatalogConfig) -> Result<Self> {
+ ///
+ /// The auth manager is kept; it derives a new session from the merged
+ /// properties (carrying over state such as a cached token).
+ pub async fn update_with(self, cfg: &RestCatalogConfig) -> Result<Self> {
let extra_headers = (!cfg.extra_headers()?.is_empty())
.then(|| cfg.extra_headers())
.transpose()?
.unwrap_or(self.extra_headers);
+ let session =
self.auth_manager.catalog_session(&cfg.auth_props()).await?;
Ok(HttpClient {
client: cfg.client().unwrap_or(self.client),
- token: Mutex::new(cfg.token().or_else(|| self.token.into_inner())),
- token_endpoint: if !cfg.get_token_endpoint().is_empty() {
- cfg.get_token_endpoint()
- } else {
- self.token_endpoint
- },
- credential: cfg.credential().or(self.credential),
extra_headers,
- extra_oauth_params: if !cfg.extra_oauth_params().is_empty() {
- cfg.extra_oauth_params()
- } else {
- self.extra_oauth_params
- },
disable_header_redaction: cfg.disable_header_redaction(),
+ auth_manager: self.auth_manager,
+ session,
})
}
- /// This API is testing only to assert the token.
+ /// The session authenticating requests in the current phase.
+ pub(crate) fn session(&self) -> &Arc<dyn AuthSession> {
+ &self.session
+ }
+
+ /// This API is testing only to assert the bearer token the session uses.
#[cfg(test)]
pub(crate) async fn token(&self) -> Option<String> {
let mut req = self
- .request(Method::GET, &self.token_endpoint)
+ .request(Method::GET, "http://localhost/unused";)
Review Comment:
@CTTY It's a test-only shim: `HttpClient` no longer holds the token itself,
so the `#[cfg(test)] token()` helper builds a throwaway request (never sent —
`authenticate` only mutates headers) and reads the bearer back from the
`Authorization` header. I'll replace it in the split with assertions on the
headers the mock server observes.
--
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]
