From 2e4397c478207ab3ac0b3aa9ae6a004f4db691e6 Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Fri, 10 Nov 2023 10:44:25 -0800 Subject: [PATCH] S3ResponseError (#522) Issue: The exceptions delivered from the S3Client were lacking information like the HTTP response code, headers, & body. You could get the extra information from the on_done callback, but it would be nice to get it from the exception, so you could use the request.finished_future instead of the on_done callback if you wanted. Description of changes: Add new error type: awscrt.s3.S3ResponseError, which inherits from awscrt.exceptions.AwsCrtError. If we have information about the failed request, then use an S3ResponseError instead. --- awscrt/s3.py | 51 +++++++++++++++++++++++++++++++++------ source/auth_credentials.c | 2 +- source/s3_client.c | 2 ++ test/test_s3.py | 8 +++++- 4 files changed, 53 insertions(+), 10 deletions(-) diff --git a/awscrt/s3.py b/awscrt/s3.py index d0f559994..6ba298884 100644 --- a/awscrt/s3.py +++ b/awscrt/s3.py @@ -15,7 +15,7 @@ import awscrt.exceptions from dataclasses import dataclass import threading -from typing import Optional +from typing import List, Optional, Tuple from enum import IntEnum @@ -454,6 +454,35 @@ def cancel(self): _awscrt.s3_meta_request_cancel(self) +class S3ResponseError(awscrt.exceptions.AwsCrtError): + ''' + An error response from S3. + + Subclasses :class:`awscrt.exceptions.AwsCrtError`. + + Attributes: + status_code (int): HTTP response status code. + headers (list[tuple[str, str]]): Headers from HTTP response. + body (Optional[bytes]): Body of HTTP response (if any). + This is usually XML. It may be None in the case of a HEAD response. + code (int): CRT error code. + name (str): CRT error name. + message (str): CRT error message. + ''' + + def __init__(self, *, + code: int, + name: str, + message: str, + status_code: List[Tuple[str, str]] = None, + headers: List[Tuple[str, str]] = None, + body: Optional[bytes] = None): + super().__init__(code, name, message) + self.status_code = status_code + self.headers = headers + self.body = body + + class _S3ClientCore: ''' Private class to keep all the related Python object alive until C land clean up for S3Client @@ -517,13 +546,19 @@ def _on_finish(self, error_code, status_code, error_headers, error_body): error = None if error_code: error = awscrt.exceptions.from_code(error_code) - if error_body: - # TODO The error body is XML, will need to parse it to something prettier. - try: - extra_message = ". Body from error request is: " + str(error_body) - error.message = error.message + extra_message - except BaseException: - pass + + # If the failure was due to a response, make it into an S3ResponseError. + # When failure is due to a response, its headers are always included. + if isinstance(error, awscrt.exceptions.AwsCrtError) \ + and status_code is not None \ + and error_headers is not None: + error = S3ResponseError( + code=error.code, + name=error.name, + message=error.message, + status_code=status_code, + headers=error_headers, + body=error_body) self._finished_future.set_exception(error) else: self._finished_future.set_result(None) diff --git a/source/auth_credentials.c b/source/auth_credentials.c index 66b1afa8e..85f9a3a08 100644 --- a/source/auth_credentials.c +++ b/source/auth_credentials.c @@ -618,7 +618,7 @@ static int s_credentials_provider_delegate_get_credentials( PyGILState_Release(state); if (!native_credentials) { - return aws_raise_error(AWS_ERROR_CRT_CALLBACK_EXCEPTION); + return aws_raise_error(AWS_AUTH_CREDENTIALS_PROVIDER_DELEGATE_FAILURE); } callback(native_credentials, AWS_ERROR_SUCCESS, callback_user_data); diff --git a/source/s3_client.c b/source/s3_client.c index 33bb17b29..9309fa5de 100644 --- a/source/s3_client.c +++ b/source/s3_client.c @@ -125,6 +125,8 @@ PyObject *aws_py_s3_cross_process_lock_acquire(PyObject *self, PyObject *args) { } PyObject *aws_py_s3_cross_process_lock_release(PyObject *self, PyObject *args) { + (void)self; + PyObject *lock_capsule; /* O */ if (!PyArg_ParseTuple(args, "O", &lock_capsule)) { diff --git a/test/test_s3.py b/test/test_s3.py index 8602a8a14..e0ee0b144 100644 --- a/test/test_s3.py +++ b/test/test_s3.py @@ -20,6 +20,7 @@ S3ChecksumLocation, S3Client, S3RequestType, + S3ResponseError, CrossProcessLock, create_default_s3_signing_config, ) @@ -288,6 +289,8 @@ def _validate_successful_response(self, is_put_object): self.assertEqual(self.done_status_code, self.response_status_code, "status-code from on_done doesn't match code from on_headers") self.assertIsNone(self.done_error) + self.assertIsNone(self.done_error_headers) + self.assertIsNone(self.done_error_body) headers = HttpHeaders(self.response_headers) self.assertIsNone(headers.get("Content-Range")) body_length = headers.get("Content-Length") @@ -596,12 +599,15 @@ def test_multipart_upload_with_invalid_request(self): self._test_s3_put_get_object(request, S3RequestType.PUT_OBJECT, "AWS_ERROR_S3_INVALID_RESPONSE_STATUS") # check that data from on_done callback came through correctly - self.assertIsNotNone(self.done_error) + self.assertIsInstance(self.done_error, S3ResponseError) self.assertEqual(self.done_status_code, 400) + self.assertEqual(self.done_error.status_code, 400) self.assertIsNotNone(self.done_error_headers) self.assertTrue(any(h[0].lower() == 'x-amz-request-id' for h in self.done_error_headers)) + self.assertListEqual(self.done_error_headers, self.done_error.headers) self.assertIsNotNone(self.done_error_body) self.assertTrue(b"InvalidDigest" in self.done_error_body) + self.assertEqual(self.done_error_body, self.done_error.body) put_body_stream.close()