CTTY commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3962771293
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props:
&HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request
can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
Storing catalog session here is fine but feels a bit excessive. Why not
build contextual sessions in the catalog, and then pass them to
`RestClient::query_catalog`?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -687,13 +724,15 @@ impl Catalog for RestCatalog {
/// REST catalog implementation of [`SessionCatalog`].
///
-/// Each operation accepts a [`SessionContext`]. REST configuration,
authentication sessions,
-/// and the HTTP client are initialized lazily once per catalog and shared
across all operations.
+/// Each operation accepts a [`SessionContext`]. REST configuration, the auth
manager, the catalog
+/// authentication session, and the HTTP client are initialized lazily once
per catalog. Each
+/// REST request uses the contextual authentication session returned by the
manager.
#[derive(Debug)]
pub struct RestSessionCatalog {
- /// Injected through [`RestSessionCatalogBuilder::with_auth_manager`];
otherwise
- /// one is resolved from `rest.auth.type` when the client is built.
- auth_manager: Option<Arc<dyn AuthManager>>,
+ /// Builder-supplied override retained so lazy client initialization can
+ /// clone it into the runtime state. When absent, a manager is resolved
from
+ /// `rest.auth.type` during initialization.
+ auth_manager_override: Option<Arc<dyn AuthManager>>,
Review Comment:
naming is hard and I like the new name more!
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -829,9 +869,15 @@ impl RestSessionCatalog {
/// Issue a `HEAD` request to `url` and interpret it as an existence check:
/// `2xx` means it exists, `404` means it doesn't.
- async fn check_exists_via_head(&self, client: &RestClient, url: String) ->
Result<bool> {
+ async fn check_exists_via_head(
+ &self,
+ context: &SessionContext,
Review Comment:
Same here, seems like we can pass the pre-built contextual session to this
function directly
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -743,7 +782,8 @@ impl RestSessionCatalog {
}
let request = HttpRequest::build(request_builder)?;
- let http_response = client.query_catalog(request).await?;
+ let session = client.contextual_session(context).await?;
Review Comment:
We can build the contextual session using the auth manager here
--
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]
