jonbannister commented on code in PR #2543:
URL: https://github.com/apache/iceberg-python/pull/2543#discussion_r2387158490
##########
pyiceberg/io/fsspec.py:
##########
@@ -95,38 +97,60 @@
from botocore.awsrequest import AWSRequest
-def s3v4_rest_signer(properties: Properties, request: "AWSRequest", **_: Any)
-> "AWSRequest":
- signer_url = properties.get(S3_SIGNER_URI, properties[URI]).rstrip("/") #
type: ignore
- signer_endpoint = properties.get(S3_SIGNER_ENDPOINT,
S3_SIGNER_ENDPOINT_DEFAULT)
+class S3RequestSigner(abc.ABC):
+ """Abstract base class for S3 request signers."""
- signer_headers = {}
- if token := properties.get(TOKEN):
- signer_headers = {"Authorization": f"Bearer {token}"}
- signer_headers.update(get_header_properties(properties))
+ properties: Properties
- signer_body = {
- "method": request.method,
- "region": request.context["client_region"],
- "uri": request.url,
- "headers": {key: [val] for key, val in request.headers.items()},
- }
+ def __init__(self, properties: Properties) -> None:
+ self.properties = properties
+
+ @abc.abstractmethod
+ def __call__(self, request: "AWSRequest", **_: Any) -> None:
+ pass
+
+
+class S3V4RestSigner(S3RequestSigner):
+ """An S3 request signer that uses an external REST signing service to sign
requests."""
+
+ session: requests.Session
- response = requests.post(f"{signer_url}/{signer_endpoint.strip()}",
headers=signer_headers, json=signer_body)
- try:
- response.raise_for_status()
- response_json = response.json()
- except HTTPError as e:
- raise SignError(f"Failed to sign request {response.status_code}:
{signer_body}") from e
+ def __init__(self, properties: Properties) -> None:
+ super().__init__(properties)
+ self.session = requests.Session()
Review Comment:
minor: this could be prefixed with an `_`
##########
pyiceberg/io/fsspec.py:
##########
@@ -95,38 +97,60 @@
from botocore.awsrequest import AWSRequest
-def s3v4_rest_signer(properties: Properties, request: "AWSRequest", **_: Any)
-> "AWSRequest":
- signer_url = properties.get(S3_SIGNER_URI, properties[URI]).rstrip("/") #
type: ignore
- signer_endpoint = properties.get(S3_SIGNER_ENDPOINT,
S3_SIGNER_ENDPOINT_DEFAULT)
+class S3RequestSigner(abc.ABC):
+ """Abstract base class for S3 request signers."""
- signer_headers = {}
- if token := properties.get(TOKEN):
- signer_headers = {"Authorization": f"Bearer {token}"}
- signer_headers.update(get_header_properties(properties))
+ properties: Properties
- signer_body = {
- "method": request.method,
- "region": request.context["client_region"],
- "uri": request.url,
- "headers": {key: [val] for key, val in request.headers.items()},
- }
+ def __init__(self, properties: Properties) -> None:
+ self.properties = properties
+
+ @abc.abstractmethod
+ def __call__(self, request: "AWSRequest", **_: Any) -> None:
+ pass
+
+
+class S3V4RestSigner(S3RequestSigner):
+ """An S3 request signer that uses an external REST signing service to sign
requests."""
+
+ session: requests.Session
- response = requests.post(f"{signer_url}/{signer_endpoint.strip()}",
headers=signer_headers, json=signer_body)
- try:
- response.raise_for_status()
- response_json = response.json()
- except HTTPError as e:
- raise SignError(f"Failed to sign request {response.status_code}:
{signer_body}") from e
+ def __init__(self, properties: Properties) -> None:
+ super().__init__(properties)
+ self.session = requests.Session()
- for key, value in response_json["headers"].items():
- request.headers.add_header(key, ", ".join(value))
+ def __call__(self, request: "AWSRequest", **_: Any) -> None:
Review Comment:
Is it safe to change this from `-> "AWSRequest"` to `-> None`?
--
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]
