Rectify the condition and add a test

4 files changed, 28 insertions, 19 deletions

diff --git a/src/core/ext/filters/http/client/http_client_filter.cc b/src/core/ext/filters/http/client/http_client_filter.cc
index f44dc032a7..91fa163fec 100644
--- a/src/core/ext/filters/http/client/http_client_filter.cc
+++ b/src/core/ext/filters/http/client/http_client_filter.cc
@@ -79,7 +79,12 @@ struct channel_data {
 static grpc_error* client_filter_incoming_metadata(grpc_call_element* elem,
                                                    grpc_metadata_batch* b) {
   if (b->idx.named.status != nullptr) {
-    if (grpc_mdelem_eq(b->idx.named.status->md, GRPC_MDELEM_STATUS_200)) {
+    /* If both gRPC status and HTTP status are provided in the response, we
+     * should prefer the gRPC status code, as mentioned in
+     * https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md.
+     */
+    if (b->idx.named.grpc_status != nullptr ||
+        grpc_mdelem_eq(b->idx.named.status->md, GRPC_MDELEM_STATUS_200)) {
       grpc_metadata_batch_remove(b, b->idx.named.status);
     } else {
       char* val = grpc_dump_slice(GRPC_MDVALUE(b->idx.named.status->md),
diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc
index 205fb8c370..1e491d2ef8 100644
--- a/src/core/ext/transport/chttp2/transport/parsing.cc
+++ b/src/core/ext/transport/chttp2/transport/parsing.cc
@@ -393,7 +393,6 @@ error_handler:
 static void free_timeout(void* p) { gpr_free(p); }
 
 static void on_initial_header(void* tp, grpc_mdelem md) {
-  gpr_log(GPR_INFO, "on initial header");
   GPR_TIMER_SCOPE("on_initial_header", 0);
 
   grpc_chttp2_transport* t = static_cast<grpc_chttp2_transport*>(tp);
@@ -476,7 +475,6 @@ static void on_initial_header(void* tp, grpc_mdelem md) {
 }
 
 static void on_trailing_header(void* tp, grpc_mdelem md) {
-  gpr_log(GPR_INFO, "on_trailing_header");
   GPR_TIMER_SCOPE("on_trailing_header", 0);
 
   grpc_chttp2_transport* t = static_cast<grpc_chttp2_transport*>(tp);
diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc
index 496de1150a..b07c4d6c10 100644
--- a/src/core/lib/surface/call.cc
+++ b/src/core/lib/surface/call.cc
@@ -685,10 +685,10 @@ static void cancel_with_status(grpc_call* c, grpc_status_code status,
 }
 
 static void set_final_status(grpc_call* call, grpc_error* error) {
-  //if (grpc_call_error_trace.enabled()) {
+  if (grpc_call_error_trace.enabled()) {
     gpr_log(GPR_DEBUG, "set_final_status %s", call->is_client ? "CLI" : "SVR");
     gpr_log(GPR_DEBUG, "%s", grpc_error_string(error));
-  //}
+  }
   if (call->is_client) {
     grpc_error_get_status(error, call->send_deadline,
                           call->final_op.client.status,
diff --git a/test/core/end2end/tests/filter_status_code.cc b/test/core/end2end/tests/filter_status_code.cc
index 447ff520ee..5ffc3d00a3 100644
--- a/test/core/end2end/tests/filter_status_code.cc
+++ b/test/core/end2end/tests/filter_status_code.cc
@@ -16,6 +16,14 @@
  *
  */
 
+/* This test verifies -
+ * 1) grpc_call_final_info passed to the filters on destroying a call contains
+ * the proper status.
+ * 2) If the response has both an HTTP status code and a gRPC status code, then
+ * we should prefer the gRPC status code as mentioned in
+ * https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md
+ */
+
 #include "test/core/end2end/end2end_tests.h"
 
 #include <limits.h>
@@ -249,20 +257,18 @@ typedef struct final_status_data {
   grpc_call_stack* call;
 } final_status_data;
 
-static void start_transport_stream_op_batch(grpc_call_element *elem,
-                                      grpc_transport_stream_op_batch *op) {
+static void server_start_transport_stream_op_batch(
+    grpc_call_element* elem, grpc_transport_stream_op_batch* op) {
   auto* data = static_cast<final_status_data*>(elem->call_data);
-  if(data->call == g_server_call_stack) {
-    gpr_log(GPR_INFO, "here");
-  }
-  if(op->send_initial_metadata) {
-    auto *batch = op->payload->send_initial_metadata.send_initial_metadata;
-    gpr_log(GPR_INFO, "init %p %p", batch->idx.named.status, batch->idx.named.grpc_status);
-    grpc_metadata_batch_substitute(batch, batch->idx.named.status, GRPC_MDELEM_STATUS_404);
-  }
-  if(op->send_trailing_metadata) {
-    auto *batch = op->payload->send_trailing_metadata.send_trailing_metadata;
-    gpr_log(GPR_INFO, "trai %p %p", batch->idx.named.status, batch->idx.named.grpc_status);
+  if (data->call == g_server_call_stack) {
+    if (op->send_initial_metadata) {
+      auto* batch = op->payload->send_initial_metadata.send_initial_metadata;
+      if (batch->idx.named.status != nullptr) {
+        /* Replace the HTTP status with 404 */
+        grpc_metadata_batch_substitute(batch, batch->idx.named.status,
+                                       GRPC_MDELEM_STATUS_404);
+      }
+    }
   }
   grpc_call_next_op(elem, op);
 }
@@ -325,7 +331,7 @@ static const grpc_channel_filter test_client_filter = {
     "client_filter_status_code"};
 
 static const grpc_channel_filter test_server_filter = {
-    start_transport_stream_op_batch,
+    server_start_transport_stream_op_batch,
     grpc_channel_next_op,
     sizeof(final_status_data),
     init_call_elem,