DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4209104209
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -463,29 +481,48 @@ impl RestClient {
let http_client = http_client.update_with(&config)?;
// The manager is handed an unauthenticated client: its own
// requests must not be signed by the session it is deriving.
- let session = auth_manager
+ let catalog_session = auth_manager
.catalog_session(
&http_client.without_auth_session(),
&Self::auth_props(&config),
)
.await?;
Ok(Self {
+ auth_manager,
+ catalog_session,
config,
- http_client: http_client.with_auth_session(session),
+ http_client,
endpoints,
})
}
/// Testing only: the bearer token the catalog session would attach.
#[cfg(test)]
async fn token(&self) -> Option<String> {
- self.http_client.token().await
+ self.http_client
+ .with_auth_session(Arc::clone(&self.catalog_session))
+ .token()
+ .await
+ }
+
+ /// Derives the authentication session for one catalog operation.
+ async fn contextual_session(&self, context: &SessionContext) ->
Result<Arc<dyn AuthSession>> {
+ self.auth_manager
+ .contextual_session(context, Arc::clone(&self.catalog_session))
+ .await
}
- /// Sends `request`, authenticated by the client's session.
- async fn query_catalog(&self, request: HttpRequest) ->
Result<HttpResponse> {
- self.http_client.query_catalog(request).await
+ /// Sends `request` with `session`.
+ async fn query_catalog(
+ &self,
+ session: Arc<dyn AuthSession>,
+ request: HttpRequest,
+ ) -> Result<HttpResponse> {
+ self.http_client
+ .with_auth_session(session)
Review Comment:
The benefit was negligible in my benchmarks but the clone still shows a
design issue: a contextual auth session is now passed with every catalog
request, but the old client that was designed around catalog sessions assumed
they were static for the lifetime of the client.
https://github.com/apache/iceberg-rust/pull/3081/commits/d33821a17f28f6a239bb199a4d50a1fb3a0b7022
and
https://github.com/apache/iceberg-rust/pull/3081/commits/15ef053d8fc27de5258689ec8c9f05112737cd42
get this straight. A session is now passed on every request to `query_catalog`
and the client doesn't hold any sessions anymore.
The catalog's use of the `HttpClient` now also diverges from the
`AuthManager`'s use. The `HttpClient::post_form` method is public for
implementors of auth managers. It's now not using any catalog sessions anymore
because it's meant to derive them. If implementors need to authenticate their
requests, they can still do so with client-level 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: [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]
