From 9c12763e23941d93cf8e29b862d605fc35363f84 Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Wed, 22 Nov 2023 15:03:04 -0800 Subject: [PATCH] Add S3ResponseError.operation_name (#530) --- awscrt/s3.py | 43 +++++++++++++++++++++++++++++++++++----- crt/aws-c-mqtt | 2 +- crt/aws-c-s3 | 2 +- crt/aws-lc | 2 +- source/s3_meta_request.c | 16 ++++++++++++--- test/test_s3.py | 27 +++++++++++++++++++++++-- 6 files changed, 79 insertions(+), 13 deletions(-) diff --git a/awscrt/s3.py b/awscrt/s3.py index 7e5574641..125a6d2c4 100644 --- a/awscrt/s3.py +++ b/awscrt/s3.py @@ -192,7 +192,7 @@ class S3Client(NativeResource): def __init__( self, *, - bootstrap, + bootstrap=None, region, tls_mode=None, signing_config=None, @@ -264,6 +264,7 @@ def make_request( *, type, request, + operation_name=None, recv_filepath=None, send_filepath=None, signing_config=None, @@ -284,6 +285,11 @@ def make_request( request (HttpRequest): The overall outgoing API request for S3 operation. If the request body is a file, set send_filepath for better performance. + operation_name(Optional[str]): Optional S3 operation name (e.g. "CreateBucket"). + This will only be used when `type` is :attr:`~S3RequestType.DEFAULT`; + it is automatically populated for other types. + This name is used to fill out details in metrics and error reports. + recv_filepath (Optional[str]): Optional file path. If set, the response body is written directly to a file and the `on_body` callback is not invoked. This should give better @@ -342,6 +348,16 @@ def make_request( side sent an unsuccessful response, the body of the response is provided here. Else None will be returned. + * `error_operation_name` (Optional[str]): If request failed + because server side sent and unsuccessful response, this + is the name of the S3 operation it was responding to. + For example, if a :attr:`~S3RequestType.PUT_OBJECT` fails + this could be "PutObject", "CreateMultipartUpload", "UploadPart", + "CompleteMultipartUpload", or others. For :attr:`~S3RequestType.DEFAULT`, + this is the `operation_name` passed to :meth:`S3Client.make_request()`. + This will be None if the request failed for another reason, + or the S3 operation name is unknown. + * `status_code` (Optional[int]): HTTP response status code (if available). If request failed because server side sent an unsuccessful response, this is its status code. If the operation was successful, @@ -364,6 +380,7 @@ def make_request( client=self, type=type, request=request, + operation_name=operation_name, recv_filepath=recv_filepath, send_filepath=send_filepath, signing_config=signing_config, @@ -398,6 +415,7 @@ def __init__( client, type, request, + operation_name=None, recv_filepath=None, send_filepath=None, signing_config=None, @@ -445,6 +463,7 @@ def __init__( client, request, type, + operation_name, signing_config, credential_provider, recv_filepath, @@ -474,6 +493,12 @@ class S3ResponseError(awscrt.exceptions.AwsCrtError): 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. + operation_name (Optional[str]): Name of the S3 operation that failed (if known). + For example, if a :attr:`~S3RequestType.PUT_OBJECT` fails + this could be "PutObject", "CreateMultipartUpload", "UploadPart", + "CompleteMultipartUpload", or others. For :attr:`~S3RequestType.DEFAULT`, + this is the `operation_name` passed to :meth:`S3Client.make_request()`. + If the S3 operation name is unknown, this will be None. code (int): CRT error code. name (str): CRT error name. message (str): CRT error message. @@ -485,11 +510,13 @@ def __init__(self, *, message: str, status_code: List[Tuple[str, str]] = None, headers: List[Tuple[str, str]] = None, - body: Optional[bytes] = None): + body: Optional[bytes] = None, + operation_name: Optional[str] = None): super().__init__(code, name, message) self.status_code = status_code self.headers = headers self.body = body + self.operation_name = operation_name class _S3ClientCore: @@ -559,7 +586,7 @@ def _on_body(self, chunk, offset): def _on_shutdown(self): self._shutdown_event.set() - def _on_finish(self, error_code, status_code, error_headers, error_body): + def _on_finish(self, error_code, status_code, error_headers, error_body, error_operation_name): # If C layer gives status_code 0, that means "unknown" if status_code == 0: status_code = None @@ -582,12 +609,18 @@ def _on_finish(self, error_code, status_code, error_headers, error_body): message=error.message, status_code=status_code, headers=error_headers, - body=error_body) + body=error_body, + operation_name=error_operation_name) self._finished_future.set_exception(error) else: self._finished_future.set_result(None) if self._on_done_cb: - self._on_done_cb(error=error, error_headers=error_headers, error_body=error_body, status_code=status_code) + self._on_done_cb( + error=error, + error_headers=error_headers, + error_body=error_body, + error_operation_name=error_operation_name, + status_code=status_code) def _on_progress(self, progress): if self._on_progress_cb: diff --git a/crt/aws-c-mqtt b/crt/aws-c-mqtt index 5d198cf2d..6d36cd372 160000 --- a/crt/aws-c-mqtt +++ b/crt/aws-c-mqtt @@ -1 +1 @@ -Subproject commit 5d198cf2d09b19bb18bf03e4425831a246d0a391 +Subproject commit 6d36cd3726233cb757468d0ea26f6cd8dad151ec diff --git a/crt/aws-c-s3 b/crt/aws-c-s3 index f961971f3..cc6ba346b 160000 --- a/crt/aws-c-s3 +++ b/crt/aws-c-s3 @@ -1 +1 @@ -Subproject commit f961971f3851782d40126f0597f007c73ba4ae0c +Subproject commit cc6ba346b55ef012f7131d98dcc68e16acc16d95 diff --git a/crt/aws-lc b/crt/aws-lc index a8d06de79..80f3f3324 160000 --- a/crt/aws-lc +++ b/crt/aws-lc @@ -1 +1 @@ -Subproject commit a8d06de79e405692ac06fe17163626eaab515e4e +Subproject commit 80f3f3324e75737d43af3052b270fd2ffa169d29 diff --git a/source/s3_meta_request.c b/source/s3_meta_request.c index 0907a075c..56b853ef0 100644 --- a/source/s3_meta_request.c +++ b/source/s3_meta_request.c @@ -254,15 +254,22 @@ static void s_s3_request_on_finish( if (meta_request_result->error_response_body) { error_body = *(meta_request_result->error_response_body); } + + const char *operation_name = NULL; + if (meta_request_result->error_response_operation_name != NULL) { + operation_name = aws_string_c_str(meta_request_result->error_response_operation_name); + } + result = PyObject_CallMethod( request_binding->py_core, "_on_finish", - "(iiOy#)", + "(iiOy#s)", error_code, meta_request_result->response_status, header_list ? header_list : Py_None, (const char *)(error_body.buffer), - (Py_ssize_t)error_body.len); + (Py_ssize_t)error_body.len, + operation_name); if (result) { Py_DECREF(result); @@ -356,6 +363,7 @@ PyObject *aws_py_s3_client_make_meta_request(PyObject *self, PyObject *args) { PyObject *s3_client_py; /* O */ PyObject *http_request_py; /* O */ int type; /* i */ + const char *operation_name; /* z */ PyObject *signing_config_py; /* O */ PyObject *credential_provider_py; /* O */ const char *recv_filepath; /* z */ @@ -367,11 +375,12 @@ PyObject *aws_py_s3_client_make_meta_request(PyObject *self, PyObject *args) { PyObject *py_core; /* O */ if (!PyArg_ParseTuple( args, - "OOOiOOzzs#iipO", + "OOOizOOzzs#iipO", &py_s3_request, &s3_client_py, &http_request_py, &type, + &operation_name, &signing_config_py, &credential_provider_py, &recv_filepath, @@ -451,6 +460,7 @@ PyObject *aws_py_s3_client_make_meta_request(PyObject *self, PyObject *args) { struct aws_s3_meta_request_options s3_meta_request_opt = { .type = type, + .operation_name = aws_byte_cursor_from_c_str(operation_name), .message = http_request, .signing_config = signing_config, .checksum_config = &checksum_config, diff --git a/test/test_s3.py b/test/test_s3.py index 67e5a8c89..4116477ec 100644 --- a/test/test_s3.py +++ b/test/test_s3.py @@ -248,6 +248,7 @@ def setUp(self): self.done_status_code = None self.done_error_headers = None self.done_error_body = None + self.done_error_operation_name = None self.files = FileCreator() self.temp_put_obj_file_path = self.files.create_file_with_size("temp_put_obj_10mb", 10 * MB) @@ -282,10 +283,11 @@ def _on_request_headers(self, status_code, headers, **kargs): def _on_request_body(self, chunk, offset, **kargs): self.received_body_len = self.received_body_len + len(chunk) - def _on_request_done(self, error, error_headers, error_body, status_code, **kwargs): + def _on_request_done(self, error, error_headers, error_body, error_operation_name, status_code, **kwargs): self.done_error = error self.done_error_headers = error_headers self.done_error_body = error_body + self.done_error_operation_name = error_operation_name self.done_status_code = status_code def _on_progress(self, progress): @@ -298,6 +300,7 @@ def _validate_successful_response(self, is_put_object): self.assertIsNone(self.done_error) self.assertIsNone(self.done_error_headers) self.assertIsNone(self.done_error_body) + self.assertIsNone(self.done_error_operation_name) headers = HttpHeaders(self.response_headers) self.assertIsNone(headers.get("Content-Range")) body_length = headers.get("Content-Length") @@ -607,7 +610,7 @@ def test_multipart_put_object_cancel(self): def test_put_object_quick_cancel(self): return self._put_object_cancel_helper(False) - def test_multipart_upload_with_invalid_request(self): + def test_singlepart_upload_with_invalid_request(self): # send upload with incorrect Content-MD5 # need to do single-part upload so the Content-MD5 header is sent along as-is. content_length = 100 @@ -625,11 +628,31 @@ def test_multipart_upload_with_invalid_request(self): 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.assertEqual(self.done_error_operation_name, "PutObject") + self.assertEqual(self.done_error_operation_name, self.done_error.operation_name) self.assertTrue(b"InvalidDigest" in self.done_error_body) self.assertEqual(self.done_error_body, self.done_error.body) put_body_stream.close() + def test_default_request_failure(self): + # send invalid DEFAULT S3Request + # ensure error info (including custom operation_name) comes through correctly + s3_client = S3Client(region=self.region) + + # send invalid request to S3. + http_request = HttpRequest(method="GET", path="/obviously-invalid-path-object-does-not-exist") + http_request.headers.add("host", self._build_endpoint_string(self.region, self.bucket_name)) + http_request.headers.add("content-length", "0") + s3_request = s3_client.make_request( + type=S3RequestType.DEFAULT, + request=http_request, + operation_name="MyNewOperationName") + + exception = s3_request.finished_future.exception(self.timeout) + self.assertIsInstance(exception, S3ResponseError) + self.assertEqual(exception.operation_name, "MyNewOperationName") + def test_on_headers_callback_failure(self): def _explode(**kwargs): raise RuntimeError("Error in on_headers callback")