gabeiglio commented on code in PR #3751:
URL: https://github.com/apache/iceberg-python/pull/3751#discussion_r3913886325
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -827,37 +812,50 @@ def add_headers(self, request: PreparedRequest, **kwargs:
Any) -> None: # pylin
def _response_to_table(self, identifier_tuple: tuple[str, ...],
table_response: TableResponse) -> Table:
# Per Iceberg spec: storage-credentials take precedence over config
- credential_config = self._resolve_storage_credentials(
- table_response.storage_credentials,
table_response.metadata_location
+ credential_config =
resolve_storage_credentials(table_response.storage_credentials,
table_response.metadata_location)
+ io = self._load_file_io(
+ {**table_response.metadata.properties, **table_response.config,
**credential_config},
+ table_response.metadata_location,
)
+ self._attach_credentials_provider(io, identifier_tuple,
table_response.storage_credentials)
return Table(
identifier=identifier_tuple,
metadata_location=table_response.metadata_location, # type: ignore
metadata=table_response.metadata,
- io=self._load_file_io(
- {**table_response.metadata.properties,
**table_response.config, **credential_config},
- table_response.metadata_location,
- ),
+ io=io,
catalog=self,
config=table_response.config,
)
def _response_to_staged_table(self, identifier_tuple: tuple[str, ...],
table_response: TableResponse) -> StagedTable:
# Per Iceberg spec: storage-credentials take precedence over config
- credential_config = self._resolve_storage_credentials(
- table_response.storage_credentials,
table_response.metadata_location
+ credential_config =
resolve_storage_credentials(table_response.storage_credentials,
table_response.metadata_location)
+ io = self._load_file_io(
+ {**table_response.metadata.properties, **table_response.config,
**credential_config},
+ table_response.metadata_location,
)
+ self._attach_credentials_provider(io, identifier_tuple,
table_response.storage_credentials)
return StagedTable(
identifier=identifier_tuple,
metadata_location=table_response.metadata_location, # type: ignore
metadata=table_response.metadata,
- io=self._load_file_io(
- {**table_response.metadata.properties,
**table_response.config, **credential_config},
- table_response.metadata_location,
- ),
+ io=io,
catalog=self,
)
+ def _attach_credentials_provider(
+ self, io: FileIO, identifier: str | Identifier, storage_credentials:
list[StorageCredential]
+ ) -> None:
+ """Attach a CredentialsProvider to io if credential refresh is enabled
and credentials were vended.
+
+ The refresh callback returns the full LoadCredentialsResponse so the
provider can re-run
+ longest-prefix matching against the freshly vended credentials.
+ """
+ if storage_credentials and property_as_bool(self.properties,
REFRESH_CREDENTIALS_ENABLED, False):
+ io.set_credentials_provider(
Review Comment:
Good catch, this is a real blocker for this. If we want to keep the concern
of refreshing the credentials by calling the catalog.load_credentials then we
cannot reliable pickle this object. Specially this we would need to pickle
AuthManagers and its locks and it would become a very fragile and prone to
break.
I bellieve we should re-visit allowing the credential provider make the API
call itself so there is no reference to any lambdas or catalog object dependency
--
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]
