Directly declare request_type and operation_name, instead of getting …

…them via vtable calls.

In my opinion, the previous approach was complicated, hard to use, and easy to mess up. I didn't realize we weren't handling the case where singlepart operations use DEFAULT request type under the hood. Now our code MUST declare request-type and operation-name up front. It's obvious. It works. Yay.

include/aws/s3/private/s3_default_meta_request.h

@@ -15,6 +15,10 @@ struct aws_s3_meta_request_default {
 
     size_t content_length;
 
+    /* Actual type for the single request (AWS_S3_REQUEST_TYPE_DEFAULT if unknown) */
+    enum aws_s3_request_type request_type;
+
+    /* S3 operation name for the single request (NULL if unknown) */
     struct aws_string *operation_name;
 
     /* Members to only be used when the mutex in the base type is locked. */
@@ -28,10 +32,23 @@ struct aws_s3_meta_request_default {
     } synced_data;
 };
 
-/* Creates a new default meta request. This will send the request as is and pass back the response. */
+/* Creates a new default meta request. This will send the request as is and pass back the response.
+ *
+ * Sometimes, a default meta-request is used for other aws_s3_meta_request_types.
+ * For example, if the request is simple (e.g. single part upload or download),
+ * or if there's a header that's too complex to deal with.
+ * In these cases, request_type and operation_name_override should be set.
+ *
+ * But if the user literally asked for a AWS_S3_META_REQUEST_TYPE_DEFAULT,
+ * the request_type should be AWS_S3_META_REQUEST_TYPE_DEFAULT and
+ * operation_name_override should be NULL (options->operation_name will be used
+ * if it is set).
+ */
 struct aws_s3_meta_request *aws_s3_meta_request_default_new(
     struct aws_allocator *allocator,
     struct aws_s3_client *client,
+    enum aws_s3_request_type request_type,
+    const char *operation_name_override,
     uint64_t content_length,
     bool should_compute_content_md5,
     const struct aws_s3_meta_request_options *options);

include/aws/s3/private/s3_meta_request_impl.h

@@ -117,17 +117,6 @@ struct aws_s3_meta_request_vtable {
 
     /* Pause the given request */
     int (*pause)(struct aws_s3_meta_request *meta_request, struct aws_s3_meta_request_resume_token **resume_token);
-
-    /* Get the type of the aws_s3_request.
-     * Not implemented for DEFAULT type, which doesn't know its aws_s3_request_type enum value */
-    enum aws_s3_request_type (*get_request_type)(const struct aws_s3_request *request);
-
-    /* Get the S3 operation name for the meta-request.
-     * This is only implemented for DEFAULT type, which doesn't know its
-     * aws_s3_request_type enum value, but might know its operation name.
-     * Returns empty string if the name isn't known.
-     * Note that this C-string's lifetime is tied to the meta-request. */
-    const char *(*get_operation_name)(const struct aws_s3_meta_request *meta_request);
 };
 
 /**

include/aws/s3/private/s3_request.h

@@ -160,6 +160,12 @@ struct aws_s3_request {
     /* TODO: this should be a union type to make it clear that this could be one of two enums for puts, and gets. */
     int request_tag;
 
+    /* Actual S3 type for the single request (AWS_S3_REQUEST_TYPE_DEFAULT if unknown) */
+    enum aws_s3_request_type request_type;
+
+    /* S3 operation name for the single request (e.g. "CompleteMultipartUpload") (empty string if unknown) */
+    const char *operation_name;
+
     /* Members of this structure will be repopulated each time the request is sent. If the request fails, and needs to
      * be retried, then the members of this structure will be cleaned up and re-populated on the next send.
      */
@@ -221,11 +227,17 @@ struct aws_s3_request {
 
 AWS_EXTERN_C_BEGIN
 
-/* Create a new s3 request structure with the given options. */
+/* Create a new s3 request structure with the given options.
+ * @param operation_name    Official S3 operation name (e.g. "CreateMultipartUpload").
+ *                          Pass an empty string if unknown.
+ *                          The string's memory must outlive the request (static string, or live on meta_request)
+ */
 AWS_S3_API
 struct aws_s3_request *aws_s3_request_new(
     struct aws_s3_meta_request *meta_request,
     int request_tag,
+    enum aws_s3_request_type request_type,
+    const char *operation_name,
     uint32_t part_number,
     uint32_t flags);
 

include/aws/s3/s3_client.h

@@ -86,9 +86,6 @@ enum aws_s3_meta_request_type {
 
 /**
  * The type of S3 request made. Used by metrics.
- * aws_s3_request_type_operation_name() returns the S3 operation name
- * for types that map (e.g. AWS_S3_REQUEST_TYPE_HEAD_OBJECT -> "HeadObject"),
- * or empty string for types that don't map (e.g. AWS_S3_REQUEST_TYPE_DEFAULT -> "").
  */
 enum aws_s3_request_type {
     /* Same as the original HTTP request passed to aws_s3_meta_request_options */
@@ -98,6 +95,7 @@ enum aws_s3_request_type {
     AWS_S3_REQUEST_TYPE_HEAD_OBJECT,
     AWS_S3_REQUEST_TYPE_GET_OBJECT,
     AWS_S3_REQUEST_TYPE_LIST_PARTS,
+    AWS_S3_REQUEST_TYPE_PUT_OBJECT,
     AWS_S3_REQUEST_TYPE_CREATE_MULTIPART_UPLOAD,
     AWS_S3_REQUEST_TYPE_UPLOAD_PART,
     AWS_S3_REQUEST_TYPE_ABORT_MULTIPART_UPLOAD,
@@ -625,8 +623,8 @@ struct aws_s3_meta_request_result {
 
     /* If meta request failed due to an HTTP error response from S3,
      * this is the name of the S3 operation it was responding to.
-     * For example, if a AWS_S3_META_REQUEST_TYPE_PUT_OBJECT fails, this could be
-     * "CreateMultipartUpload", "UploadPart", "CompleteMultipartUpload", or others.
+     * For example, if a AWS_S3_META_REQUEST_TYPE_PUT_OBJECT fails this could be
+     * "PutObject, "CreateMultipartUpload", "UploadPart", "CompleteMultipartUpload", or others.
      * For AWS_S3_META_REQUEST_TYPE_DEFAULT, this is the same value passed to
      * aws_s3_meta_request_options.operation_name.
      * NULL if the meta request failed for another reason, or the operation name is not known. */
@@ -847,17 +845,6 @@ void aws_s3_init_default_signing_config(
     const struct aws_byte_cursor region,
     struct aws_credentials_provider *credentials_provider);
 
-/**
- * Return operation name for aws_s3_request_type,
- * or empty string if the type doesn't map to an actual operation.
- * For example:
- * AWS_S3_REQUEST_TYPE_HEAD_OBJECT -> "HeadObject"
- * AWS_S3_REQUEST_TYPE_DEFAULT -> ""
- * AWS_S3_REQUEST_TYPE_MAX -> ""
- */
-AWS_S3_API
-const char *aws_s3_request_type_operation_name(enum aws_s3_request_type type);
-
 /**
  * Add a reference, keeping this object alive.
  * The reference must be released when you are done with it, or it's memory will never be cleaned up.
@@ -1020,7 +1007,8 @@ int aws_s3_request_metrics_get_operation_name(
     const struct aws_s3_request_metrics *metrics,
     const struct aws_string **out_operation_name);
 
-/* Get the request type from request metrics. */
+/* Get the request type from request metrics.
+ * If you just need a string, aws_s3_request_metrics_get_operation_name() is more reliable. */
 AWS_S3_API
 void aws_s3_request_metrics_get_request_type(
     const struct aws_s3_request_metrics *metrics,

source/s3_auto_ranged_get.c

@@ -28,8 +28,6 @@ static void s_s3_auto_ranged_get_request_finished(
     struct aws_s3_request *request,
     int error_code);
 
-static enum aws_s3_request_type s_s3_auto_ranged_get_request_type(const struct aws_s3_request *request);
-
 static struct aws_s3_meta_request_vtable s_s3_auto_ranged_get_vtable = {
     .update = s_s3_auto_ranged_get_update,
     .send_request_finish = aws_s3_meta_request_send_request_finish_default,
@@ -39,21 +37,8 @@ static struct aws_s3_meta_request_vtable s_s3_auto_ranged_get_vtable = {
     .finished_request = s_s3_auto_ranged_get_request_finished,
     .destroy = s_s3_meta_request_auto_ranged_get_destroy,
     .finish = aws_s3_meta_request_finish_default,
-    .get_request_type = s_s3_auto_ranged_get_request_type,
 };
 
-static enum aws_s3_request_type s_s3_auto_ranged_get_request_type(const struct aws_s3_request *request) {
-    switch (request->request_tag) {
-        case AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_HEAD_OBJECT:
-            return AWS_S3_REQUEST_TYPE_HEAD_OBJECT;
-        case AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_PART:
-        case AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_INITIAL_MESSAGE:
-            return AWS_S3_REQUEST_TYPE_GET_OBJECT;
-    }
-    AWS_ASSERT(false);
-    return AWS_S3_REQUEST_TYPE_MAX;
-}
-
 static int s_s3_auto_ranged_get_success_status(struct aws_s3_meta_request *meta_request) {
     AWS_PRECONDITION(meta_request);
 
@@ -176,7 +161,9 @@ static bool s_s3_auto_ranged_get_update(
                         request = aws_s3_request_new(
                             meta_request,
                             AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_HEAD_OBJECT,
-                            0,
+                            AWS_S3_REQUEST_TYPE_HEAD_OBJECT,
+                            "HeadObject",
+                            0 /*part_number*/,
                             AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);
 
                         request->discovers_object_size = true;
@@ -197,7 +184,9 @@ static bool s_s3_auto_ranged_get_update(
                     request = aws_s3_request_new(
                         meta_request,
                         AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_PART,
-                        1,
+                        AWS_S3_REQUEST_TYPE_UPLOAD_PART,
+                        "UploadPart",
+                        1 /*part_number*/,
                         AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS | AWS_S3_REQUEST_FLAG_PART_SIZE_RESPONSE_BODY);
 
                     request->ticket = ticket;
@@ -223,7 +212,9 @@ static bool s_s3_auto_ranged_get_update(
                 request = aws_s3_request_new(
                     meta_request,
                     AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_INITIAL_MESSAGE,
-                    0,
+                    AWS_S3_REQUEST_TYPE_GET_OBJECT,
+                    "GetObject",
+                    0 /*part_number*/,
                     AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);
 
                 auto_ranged_get->synced_data.get_without_range_sent = true;
@@ -272,7 +263,9 @@ static bool s_s3_auto_ranged_get_update(
                 request = aws_s3_request_new(
                     meta_request,
                     AWS_S3_AUTO_RANGE_GET_REQUEST_TYPE_PART,
-                    auto_ranged_get->synced_data.num_parts_requested + 1,
+                    AWS_S3_REQUEST_TYPE_GET_OBJECT,
+                    "GetObject",
+                    auto_ranged_get->synced_data.num_parts_requested + 1 /*part_number*/,
                     AWS_S3_REQUEST_FLAG_PART_SIZE_RESPONSE_BODY);
 
                 request->ticket = ticket;

source/s3_auto_ranged_put.c

@@ -294,23 +294,6 @@ static int s_try_init_resume_state_from_persisted_data(
     return AWS_OP_SUCCESS;
 }
 
-static enum aws_s3_request_type s_s3_auto_ranged_put_request_type(const struct aws_s3_request *request) {
-    switch (request->request_tag) {
-        case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_LIST_PARTS:
-            return AWS_S3_REQUEST_TYPE_LIST_PARTS;
-        case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_CREATE_MULTIPART_UPLOAD:
-            return AWS_S3_REQUEST_TYPE_CREATE_MULTIPART_UPLOAD;
-        case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_PART:
-            return AWS_S3_REQUEST_TYPE_UPLOAD_PART;
-        case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_ABORT_MULTIPART_UPLOAD:
-            return AWS_S3_REQUEST_TYPE_ABORT_MULTIPART_UPLOAD;
-        case AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_COMPLETE_MULTIPART_UPLOAD:
-            return AWS_S3_REQUEST_TYPE_COMPLETE_MULTIPART_UPLOAD;
-    }
-    AWS_ASSERT(false);
-    return AWS_S3_REQUEST_TYPE_MAX;
-}
-
 static struct aws_s3_meta_request_vtable s_s3_auto_ranged_put_vtable = {
     .update = s_s3_auto_ranged_put_update,
     .send_request_finish = s_s3_auto_ranged_put_send_request_finish,
@@ -321,7 +304,6 @@ static struct aws_s3_meta_request_vtable s_s3_auto_ranged_put_vtable = {
     .destroy = s_s3_meta_request_auto_ranged_put_destroy,
     .finish = aws_s3_meta_request_finish_default,
     .pause = s_s3_auto_ranged_put_pause,
-    .get_request_type = s_s3_auto_ranged_put_request_type,
 };
 
 /* Allocate a new auto-ranged put meta request */
@@ -480,7 +462,9 @@ static bool s_s3_auto_ranged_put_update(
                 request = aws_s3_request_new(
                     meta_request,
                     AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_LIST_PARTS,
-                    0,
+                    AWS_S3_REQUEST_TYPE_LIST_PARTS,
+                    "ListParts",
+                    0 /*part_number*/,
                     AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);
 
                 auto_ranged_put->synced_data.list_parts_state.started = true;
@@ -494,7 +478,9 @@ static bool s_s3_auto_ranged_put_update(
                 request = aws_s3_request_new(
                     meta_request,
                     AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_LIST_PARTS,
-                    0,
+                    AWS_S3_REQUEST_TYPE_LIST_PARTS,
+                    "ListParts",
+                    0 /*part_number*/,
                     AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);
                 auto_ranged_put->synced_data.list_parts_state.continues = false;
                 goto has_work_remaining;
@@ -510,7 +496,9 @@ static bool s_s3_auto_ranged_put_update(
                 request = aws_s3_request_new(
                     meta_request,
                     AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_CREATE_MULTIPART_UPLOAD,
-                    0,
+                    AWS_S3_REQUEST_TYPE_CREATE_MULTIPART_UPLOAD,
+                    "CreateMultipartUpload",
+                    0 /*part_number*/,
                     AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);
 
                 auto_ranged_put->synced_data.create_multipart_upload_sent = true;
@@ -564,7 +552,9 @@ static bool s_s3_auto_ranged_put_update(
                     request = aws_s3_request_new(
                         meta_request,
                         AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_PART,
-                        0,
+                        AWS_S3_REQUEST_TYPE_UPLOAD_PART,
+                        "UploadPart",
+                        0 /*part_number*/,
                         AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS | AWS_S3_REQUEST_FLAG_PART_SIZE_REQUEST_BODY);
 
                     request->part_number = auto_ranged_put->threaded_update_data.next_part_number;
@@ -610,7 +600,9 @@ static bool s_s3_auto_ranged_put_update(
                 request = aws_s3_request_new(
                     meta_request,
                     AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_COMPLETE_MULTIPART_UPLOAD,
-                    0,
+                    AWS_S3_REQUEST_TYPE_COMPLETE_MULTIPART_UPLOAD,
+                    "CompleteMultipartUpload",
+                    0 /*part_number*/,
                     AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS);
 
                 auto_ranged_put->synced_data.complete_multipart_upload_sent = true;
@@ -675,7 +667,9 @@ static bool s_s3_auto_ranged_put_update(
                 request = aws_s3_request_new(
                     meta_request,
                     AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_ABORT_MULTIPART_UPLOAD,
-                    0,
+                    AWS_S3_REQUEST_TYPE_ABORT_MULTIPART_UPLOAD,
+                    "AbortMultipartUpload",
+                    0 /*part_number*/,
                     AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS | AWS_S3_REQUEST_FLAG_ALWAYS_SEND);
 
                 auto_ranged_put->synced_data.abort_multipart_upload_sent = true;

source/s3_client.c

@@ -1043,7 +1043,14 @@ static struct aws_s3_meta_request *s_s3_client_meta_request_factory_default(
              * TODO: Still need tests to verify that the request of a part is
              * splittable or not */
             if (aws_http_headers_has(initial_message_headers, aws_byte_cursor_from_c_str("partNumber"))) {
-                return aws_s3_meta_request_default_new(client->allocator, client, content_length, false, options);
+                return aws_s3_meta_request_default_new(
+                    client->allocator,
+                    client,
+                    AWS_S3_REQUEST_TYPE_GET_OBJECT,
+                    "GetObject",
+                    content_length,
+                    false /*should_compute_content_md5*/,
+                    options);
             }
 
             return aws_s3_meta_request_auto_ranged_get_new(client->allocator, client, client->part_size, options);
@@ -1092,6 +1099,8 @@ static struct aws_s3_meta_request *s_s3_client_meta_request_factory_default(
                     return aws_s3_meta_request_default_new(
                         client->allocator,
                         client,
+                        AWS_S3_REQUEST_TYPE_PUT_OBJECT,
+                        "PutObject",
                         content_length,
                         client->compute_content_md5 == AWS_MR_CONTENT_MD5_ENABLED &&
                             !aws_http_headers_has(initial_message_headers, g_content_md5_header_name),
@@ -1138,7 +1147,14 @@ static struct aws_s3_meta_request *s_s3_client_meta_request_factory_default(
             return aws_s3_meta_request_copy_object_new(client->allocator, client, options);
         }
         case AWS_S3_META_REQUEST_TYPE_DEFAULT:
-            return aws_s3_meta_request_default_new(client->allocator, client, content_length, false, options);
+            return aws_s3_meta_request_default_new(
+                client->allocator,
+                client,
+                AWS_S3_REQUEST_TYPE_DEFAULT,
+                NULL /*operation_name_override*/,
+                content_length,
+                false /*should_compute_content_md5*/,
+                options);
         default:
             AWS_FATAL_ASSERT(false);
     }