ServiceConfig::Create expects non-Null error and better errors

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc

@@ -308,8 +308,11 @@ void AresDnsResolver::OnResolvedLocked(void* arg, grpc_error* error) {
       if (service_config_string != nullptr) {
         GRPC_CARES_TRACE_LOG("resolver:%p selected service config choice: %s",
                              r, service_config_string);
+        grpc_error* service_config_error = GRPC_ERROR_NONE;
         result.service_config =
-            ServiceConfig::Create(service_config_string, nullptr);
+            ServiceConfig::Create(service_config_string, &service_config_error);
+        // Error is currently unused.
+        GRPC_ERROR_UNREF(service_config_error);
       }
       gpr_free(service_config_string);
     }

src/core/ext/filters/client_channel/resolver_result_parsing.cc

@@ -52,7 +52,10 @@ ProcessedResolverResult::ProcessedResolverResult(
     const char* service_config_json = grpc_channel_arg_get_string(
         grpc_channel_args_find(resolver_result->args, GRPC_ARG_SERVICE_CONFIG));
     if (service_config_json != nullptr) {
-      service_config_ = ServiceConfig::Create(service_config_json, nullptr);
+      grpc_error* error = GRPC_ERROR_NONE;
+      service_config_ = ServiceConfig::Create(service_config_json, &error);
+      // Error is currently unused.
+      GRPC_ERROR_UNREF(error);
     }
   } else {
     // Add the service config JSON to channel args so that it's

src/core/ext/filters/client_channel/service_config.cc

@@ -38,30 +38,16 @@ RefCountedPtr<ServiceConfig> ServiceConfig::Create(const char* json,
                                                    grpc_error** error) {
   UniquePtr<char> service_config_json(gpr_strdup(json));
   UniquePtr<char> json_string(gpr_strdup(json));
+  GPR_DEBUG_ASSERT(error != nullptr);
   grpc_json* json_tree = grpc_json_parse_string(json_string.get());
   if (json_tree == nullptr) {
-    if (error != nullptr) {
     *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
         "failed to parse JSON for service config");
-    }
     return nullptr;
   }
-  grpc_error* create_error;
   auto return_value = MakeRefCounted<ServiceConfig>(
-      std::move(service_config_json), std::move(json_string), json_tree,
-      &create_error);
-  if (create_error != GRPC_ERROR_NONE) {
-    if (error != nullptr) {
-      *error = create_error;
-    } else {
-      GRPC_ERROR_UNREF(create_error);
-    }
-    return nullptr;
-  }
-  if (error != nullptr) {
-    *error = GRPC_ERROR_NONE;
-  }
-  return return_value;
+      std::move(service_config_json), std::move(json_string), json_tree, error);
+  return *error == GRPC_ERROR_NONE ? return_value : nullptr;
 }
 
 ServiceConfig::ServiceConfig(UniquePtr<char> service_config_json,
@@ -115,22 +101,26 @@ grpc_error* ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable(
     if (child->key == nullptr) continue;
     if (strcmp(child->key, "name") == 0) {
       if (child->type != GRPC_JSON_ARRAY) {
-        return grpc_error_add_child(error, GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-                                               "name should be of type Array"));
+        error = grpc_error_add_child(error,
+                                     GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+                                         "field:name error:not of type Array"));
+        goto wrap_error;
       }
       for (grpc_json* name = child->child; name != nullptr; name = name->next) {
-        UniquePtr<char> path = ParseJsonMethodName(name);
+        grpc_error* parse_error = GRPC_ERROR_NONE;
+        UniquePtr<char> path = ParseJsonMethodName(name, &parse_error);
         if (path == nullptr) {
-          return GRPC_ERROR_CREATE_FROM_STATIC_STRING("Failed to parse name");
+          error = grpc_error_add_child(error, parse_error);
+        } else {
+          GPR_DEBUG_ASSERT(parse_error == GRPC_ERROR_NONE);
         }
         paths.push_back(std::move(path));
       }
     }
   }
   if (paths.size() == 0) {
-    return grpc_error_add_child(error,
-                                GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-                                    "No names specified in methodConfig"));
+    error = grpc_error_add_child(
+        error, GRPC_ERROR_CREATE_FROM_STATIC_STRING("No names specified"));
   }
   // Add entry for each path.
   for (size_t i = 0; i < paths.size(); ++i) {
@@ -138,6 +128,11 @@ grpc_error* ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable(
     entries[*idx].value = vector_ptr;  // Takes a new ref.
     ++*idx;
   }
+wrap_error:
+  if (error != GRPC_ERROR_NONE) {
+    error = grpc_error_add_child(
+        GRPC_ERROR_CREATE_FROM_STATIC_STRING("field:methodConfig"), error);
+  }
   return error;
 }
 
@@ -150,8 +145,9 @@ grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) {
   for (grpc_json* field = json_tree->child; field != nullptr;
        field = field->next) {
     if (field->key == nullptr) {
-      error = grpc_error_add_child(error, GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-                                              "Illegal key value - NULL"));
+      error =
+          grpc_error_add_child(error, GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+                                          "error:Illegal key value - NULL"));
       continue;
     }
     if (strcmp(field->key, "methodConfig") == 0) {
@@ -159,17 +155,17 @@ grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) {
         GPR_ASSERT(false);
       }
       if (field->type != GRPC_JSON_ARRAY) {
-        return grpc_error_add_child(error,
-                                    GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-                                        "methodConfig is not of type Array"));
+        return grpc_error_add_child(
+            error, GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+                       "field:methodConfig error:not of type Array"));
       }
       for (grpc_json* method = field->child; method != nullptr;
            method = method->next) {
         int count = CountNamesInMethodConfig(method);
         if (count <= 0) {
-          error = grpc_error_add_child(error,
-                                       GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-                                           "No names found in methodConfig"));
+          error = grpc_error_add_child(
+              error, GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+                         "field:methodConfig error:No names found"));
         }
         num_entries += static_cast<size_t>(count);
       }
@@ -184,6 +180,7 @@ grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) {
             error, ParseJsonMethodConfigToServiceConfigObjectsTable(
                        method, entries, &idx));
       }
+      GPR_DEBUG_ASSERT(num_entries == idx);
       break;
     }
   }
@@ -229,24 +226,56 @@ int ServiceConfig::CountNamesInMethodConfig(grpc_json* json) {
   return num_names;
 }
 
-UniquePtr<char> ServiceConfig::ParseJsonMethodName(grpc_json* json) {
-  if (json->type != GRPC_JSON_OBJECT) return nullptr;
+UniquePtr<char> ServiceConfig::ParseJsonMethodName(grpc_json* json,
+                                                   grpc_error** error) {
+  if (json->type != GRPC_JSON_OBJECT) {
+    *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+        "Field name should be of type object");
+    return nullptr;
+  }
   const char* service_name = nullptr;
   const char* method_name = nullptr;
   for (grpc_json* child = json->child; child != nullptr; child = child->next) {
-    if (child->key == nullptr) return nullptr;
-    if (child->type != GRPC_JSON_STRING) return nullptr;
+    if (child->key == nullptr) {
+      *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Child entry with no key");
+      return nullptr;
+    }
+    if (child->type != GRPC_JSON_STRING) {
+      *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+          "Child entry should of type string");
+      return nullptr;
+    }
     if (strcmp(child->key, "service") == 0) {
-      if (service_name != nullptr) return nullptr;  // Duplicate.
-      if (child->value == nullptr) return nullptr;
+      if (service_name != nullptr) {
+        *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+            "field:service error:Multiple entries");
+        return nullptr;  // Duplicate.
+      }
+      if (child->value == nullptr) {
+        *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+            "field:service error:empty value");
+        return nullptr;
+      }
       service_name = child->value;
     } else if (strcmp(child->key, "method") == 0) {
-      if (method_name != nullptr) return nullptr;  // Duplicate.
-      if (child->value == nullptr) return nullptr;
+      if (method_name != nullptr) {
+        *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+            "field:method error:multiple entries");
+        return nullptr;  // Duplicate.
+      }
+      if (child->value == nullptr) {
+        *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+            "field:method error:empty value");
+        return nullptr;
+      }
       method_name = child->value;
     }
   }
-  if (service_name == nullptr) return nullptr;  // Required field.
+  if (service_name == nullptr) {
+    *error =
+        GRPC_ERROR_CREATE_FROM_STATIC_STRING("field:service error:not found");
+    return nullptr;  // Required field.
+  }
   char* path;
   gpr_asprintf(&path, "/%s/%s", service_name,
                method_name == nullptr ? "*" : method_name);

src/core/ext/filters/client_channel/service_config.h

@@ -185,8 +185,9 @@ class ServiceConfig : public RefCounted<ServiceConfig> {
   static int CountNamesInMethodConfig(grpc_json* json);
 
   // Returns a path string for the JSON name object specified by \a json.
-  // Returns null on error.
-  static UniquePtr<char> ParseJsonMethodName(grpc_json* json);
+  // Returns null on error, and stores error in \a error.
+  static UniquePtr<char> ParseJsonMethodName(grpc_json* json,
+                                             grpc_error** error);
 
   // Parses the method config from \a json.  Adds an entry to \a entries for
   // each name found, incrementing \a idx for each entry added.
@@ -209,8 +210,13 @@ class ServiceConfig : public RefCounted<ServiceConfig> {
 
   InlinedVector<UniquePtr<ServiceConfigParsedObject>, kNumPreallocatedParsers>
       parsed_global_service_config_objects_;
+  // A map from the method name to the service config objects vector. Note that
+  // we are using a raw pointer and not a unique pointer so that we can use the
+  // same vector for multiple names.
   RefCountedPtr<SliceHashTable<const ServiceConfigObjectsVector*>>
       parsed_method_service_config_objects_table_;
+  // Storage for all the vectors that are being used in
+  // parsed_method_service_config_objects_table_.
   InlinedVector<UniquePtr<ServiceConfigObjectsVector>, 32>
       service_config_objects_vectors_storage_;
 };
@@ -247,7 +253,10 @@ bool ServiceConfig::ParseJsonMethodConfig(
     if (strcmp(child->key, "name") == 0) {
       if (child->type != GRPC_JSON_ARRAY) return false;
       for (grpc_json* name = child->child; name != nullptr; name = name->next) {
-        UniquePtr<char> path = ParseJsonMethodName(name);
+        grpc_error* error = GRPC_ERROR_NONE;
+        UniquePtr<char> path = ParseJsonMethodName(name, &error);
+        // We are not reporting the error here.
+        GRPC_ERROR_UNREF(error);
         if (path == nullptr) return false;
         paths.push_back(std::move(path));
       }

src/core/ext/filters/client_channel/subchannel.cc

@@ -590,8 +590,11 @@ Subchannel::Subchannel(SubchannelKey* key, grpc_connector* connector,
   const char* service_config_json = grpc_channel_arg_get_string(
       grpc_channel_args_find(args_, GRPC_ARG_SERVICE_CONFIG));
   if (service_config_json != nullptr) {
+    grpc_error* service_config_error = GRPC_ERROR_NONE;
     RefCountedPtr<ServiceConfig> service_config =
-        ServiceConfig::Create(service_config_json, nullptr);
+        ServiceConfig::Create(service_config_json, &service_config_error);
+    // service_config_error is currently unused.
+    GRPC_ERROR_UNREF(service_config_error);
     if (service_config != nullptr) {
       HealthCheckParams params;
       service_config->ParseGlobalParams(HealthCheckParams::Parse, &params);

src/core/ext/filters/message_size/message_size_filter.cc

@@ -319,8 +319,11 @@ static grpc_error* init_channel_elem(grpc_channel_element* elem,
       grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVICE_CONFIG);
   const char* service_config_str = grpc_channel_arg_get_string(channel_arg);
   if (service_config_str != nullptr) {
+    grpc_error* service_config_error = GRPC_ERROR_NONE;
     grpc_core::RefCountedPtr<grpc_core::ServiceConfig> service_config =
-        grpc_core::ServiceConfig::Create(service_config_str, nullptr);
+        grpc_core::ServiceConfig::Create(service_config_str,
+                                         &service_config_error);
+    GRPC_ERROR_UNREF(service_config_error);
     if (service_config != nullptr) {
       chand->method_limit_table = service_config->CreateMethodConfigTable(
           grpc_core::MessageSizeLimits::CreateFromJson);

test/core/client_channel/service_config_test.cc

@@ -46,14 +46,14 @@ class TestParser1 : public ServiceConfigParser {
          field = field->next) {
       if (strcmp(field->key, "global_param") == 0) {
         if (field->type != GRPC_JSON_NUMBER) {
-          *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-              "global_param value type should be a number");
+          *error =
+              GRPC_ERROR_CREATE_FROM_STATIC_STRING(InvalidTypeErrorMessage());
           return nullptr;
         }
         int value = gpr_parse_nonnegative_int(field->value);
         if (value == -1) {
-          *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-              "global_param value type should be non-negative");
+          *error =
+              GRPC_ERROR_CREATE_FROM_STATIC_STRING(InvalidValueErrorMessage());
           return nullptr;
         }
         return UniquePtr<ServiceConfigParsedObject>(
@@ -62,6 +62,14 @@ class TestParser1 : public ServiceConfigParser {
     }
     return nullptr;
   }
+
+  static const char* InvalidTypeErrorMessage() {
+    return "global_param value type should be a number";
+  }
+
+  static const char* InvalidValueErrorMessage() {
+    return "global_param value type should be non-negative";
+  }
 };
 
 class TestParser2 : public ServiceConfigParser {
@@ -76,14 +84,14 @@ class TestParser2 : public ServiceConfigParser {
       }
       if (strcmp(field->key, "method_param") == 0) {
         if (field->type != GRPC_JSON_NUMBER) {
-          *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-              "method_param value type should be a number");
+          *error =
+              GRPC_ERROR_CREATE_FROM_STATIC_STRING(InvalidTypeErrorMessage());
           return nullptr;
         }
         int value = gpr_parse_nonnegative_int(field->value);
         if (value == -1) {
-          *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-              "method_param value type should be non-negative");
+          *error =
+              GRPC_ERROR_CREATE_FROM_STATIC_STRING(InvalidValueErrorMessage());
           return nullptr;
         }
         return UniquePtr<ServiceConfigParsedObject>(
@@ -92,6 +100,14 @@ class TestParser2 : public ServiceConfigParser {
     }
     return nullptr;
   }
+
+  static const char* InvalidTypeErrorMessage() {
+    return "method_param value type should be a number";
+  }
+
+  static const char* InvalidValueErrorMessage() {
+    return "method_param value type should be non-negative";
+  }
 };
 
 class ServiceConfigTest : public ::testing::Test {
@@ -110,7 +126,10 @@ TEST_F(ServiceConfigTest, ErrorCheck1) {
   const char* test_json = "";
   grpc_error* error = GRPC_ERROR_NONE;
   auto svc_cfg = ServiceConfig::Create(test_json, &error);
+  gpr_log(GPR_ERROR, "%s", grpc_error_string(error));
   EXPECT_TRUE(error != GRPC_ERROR_NONE);
+  EXPECT_TRUE(strstr(grpc_error_string(error),
+                     "failed to parse JSON for service config") != nullptr);
 }
 
 TEST_F(ServiceConfigTest, BasicTest1) {
@@ -126,6 +145,7 @@ TEST_F(ServiceConfigTest, ErrorNoNames) {
   auto svc_cfg = ServiceConfig::Create(test_json, &error);
   gpr_log(GPR_ERROR, "%s", grpc_error_string(error));
   EXPECT_TRUE(error != GRPC_ERROR_NONE);
+  EXPECT_TRUE(strstr(grpc_error_string(error), "No names found") != nullptr);
 }
 
 TEST_F(ServiceConfigTest, ErrorNoNamesWithMultipleMethodConfigs) {
@@ -135,6 +155,7 @@ TEST_F(ServiceConfigTest, ErrorNoNamesWithMultipleMethodConfigs) {
   auto svc_cfg = ServiceConfig::Create(test_json, &error);
   gpr_log(GPR_ERROR, "%s", grpc_error_string(error));
   EXPECT_TRUE(error != GRPC_ERROR_NONE);
+  EXPECT_TRUE(strstr(grpc_error_string(error), "No names found") != nullptr);
 }
 
 TEST_F(ServiceConfigTest, ValidMethodConfig) {
@@ -171,6 +192,8 @@ TEST_F(ServiceConfigTest, Parser1ErrorInvalidType) {
   auto svc_cfg = ServiceConfig::Create(test_json, &error);
   gpr_log(GPR_ERROR, "%s", grpc_error_string(error));
   EXPECT_TRUE(error != GRPC_ERROR_NONE);
+  EXPECT_TRUE(strstr(grpc_error_string(error),
+                     TestParser1::InvalidTypeErrorMessage()) != nullptr);
 }
 
 TEST_F(ServiceConfigTest, Parser1ErrorInvalidValue) {
@@ -179,6 +202,8 @@ TEST_F(ServiceConfigTest, Parser1ErrorInvalidValue) {
   auto svc_cfg = ServiceConfig::Create(test_json, &error);
   gpr_log(GPR_ERROR, "%s", grpc_error_string(error));
   EXPECT_TRUE(error != GRPC_ERROR_NONE);
+  EXPECT_TRUE(strstr(grpc_error_string(error),
+                     TestParser1::InvalidValueErrorMessage()) != nullptr);
 }
 
 TEST_F(ServiceConfigTest, Parser2BasicTest) {
@@ -188,6 +213,12 @@ TEST_F(ServiceConfigTest, Parser2BasicTest) {
   grpc_error* error = GRPC_ERROR_NONE;
   auto svc_cfg = ServiceConfig::Create(test_json, &error);
   EXPECT_TRUE(error == GRPC_ERROR_NONE);
+  const auto* const* vector_ptr = svc_cfg->GetMethodServiceConfigObjectsVector(
+      grpc_slice_from_static_string("/TestServ/TestMethod"));
+  ASSERT_TRUE(vector_ptr != nullptr);
+  const auto* vector = *vector_ptr;
+  auto parsed_object = ((*vector)[1]).get();
+  EXPECT_TRUE(static_cast<TestParsedObject1*>(parsed_object)->value() == 5);
 }
 
 TEST_F(ServiceConfigTest, Parser2ErrorInvalidType) {
@@ -197,6 +228,8 @@ TEST_F(ServiceConfigTest, Parser2ErrorInvalidType) {
   grpc_error* error = GRPC_ERROR_NONE;
   auto svc_cfg = ServiceConfig::Create(test_json, &error);
   EXPECT_TRUE(error != GRPC_ERROR_NONE);
+  EXPECT_TRUE(strstr(grpc_error_string(error),
+                     TestParser2::InvalidTypeErrorMessage()) != nullptr);
 }
 
 TEST_F(ServiceConfigTest, Parser2ErrorInvalidValue) {
@@ -206,6 +239,8 @@ TEST_F(ServiceConfigTest, Parser2ErrorInvalidValue) {
   grpc_error* error = GRPC_ERROR_NONE;
   auto svc_cfg = ServiceConfig::Create(test_json, &error);
   EXPECT_TRUE(error != GRPC_ERROR_NONE);
+  EXPECT_TRUE(strstr(grpc_error_string(error),
+                     TestParser2::InvalidValueErrorMessage()) != nullptr);
 }
 
 }  // namespace testing

test/cpp/end2end/grpclb_end2end_test.cc

@@ -542,8 +542,10 @@ class GrpclbEnd2endTest : public ::testing::Test {
     grpc_core::Resolver::Result result;
     result.addresses = CreateLbAddressesFromAddressDataList(address_data);
     if (service_config_json != nullptr) {
+      grpc_error* error = GRPC_ERROR_NONE;
       result.service_config =
-          grpc_core::ServiceConfig::Create(service_config_json);
+          grpc_core::ServiceConfig::Create(service_config_json, &error);
+      GRPC_ERROR_UNREF(error);
     }
     response_generator_->SetResponse(std::move(result));
   }

test/cpp/end2end/xds_end2end_test.cc

@@ -524,8 +524,10 @@ class XdsEnd2endTest : public ::testing::Test {
     grpc_core::Resolver::Result result;
     result.addresses = CreateLbAddressesFromPortList(ports);
     if (service_config_json != nullptr) {
+      grpc_error* error = GRPC_ERROR_NONE;
       result.service_config =
           grpc_core::ServiceConfig::Create(service_config_json);
+      GRPC_ERROR_UNREF(error);
     }
     grpc_arg arg = grpc_core::FakeResolverResponseGenerator::MakeChannelArg(
         lb_channel_response_generator == nullptr
@@ -555,8 +557,10 @@ class XdsEnd2endTest : public ::testing::Test {
     grpc_core::Resolver::Result result;
     result.addresses = CreateLbAddressesFromPortList(ports);
     if (service_config_json != nullptr) {
+      grpc_error* error = GRPC_ERROR_NONE;
       result.service_config =
           grpc_core::ServiceConfig::Create(service_config_json);
+      GRPC_ERROR_UNREF(error);
     }
     if (lb_channel_response_generator == nullptr) {
       lb_channel_response_generator = lb_channel_response_generator_.get();