diff --git a/src/core/ext/filters/client_channel/service_config.cc b/src/core/ext/filters/client_channel/service_config.cc index 360b4cd81bf..64560175804 100644 --- a/src/core/ext/filters/client_channel/service_config.cc +++ b/src/core/ext/filters/client_channel/service_config.cc @@ -33,7 +33,31 @@ namespace grpc_core { -ServiceConfig::ServiceConfigParserList* ServiceConfig::registered_parsers_; +namespace { +typedef InlinedVector, + ServiceConfig::kNumPreallocatedParsers> + ServiceConfigParserList; +ServiceConfigParserList* registered_parsers; + +// Consumes all the errors in the vector and forms a referencing error from +// them. If the vector is empty, return GRPC_ERROR_NONE. +template +grpc_error* CreateErrorFromVector(const char* desc, + InlinedVector* error_list) { + grpc_error* error = GRPC_ERROR_NONE; + if (error_list->size() != 0) { + error = GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( + desc, error_list->data(), error_list->size()); + // Remove refs to all errors in error_list. + for (size_t i = 0; i < error_list->size(); i++) { + GRPC_ERROR_UNREF((*error_list)[i]); + } + error_list->clear(); + } + return error; +} +} // namespace + RefCountedPtr ServiceConfig::Create(const char* json, grpc_error** error) { UniquePtr service_config_json(gpr_strdup(json)); @@ -62,22 +86,38 @@ ServiceConfig::ServiceConfig(UniquePtr service_config_json, "Malformed service Config JSON object"); return; } - *error = ParseGlobalParams(json_tree); - *error = grpc_error_add_child(*error, ParsePerMethodParams(json_tree)); + grpc_error* error_list[2]; + int error_count = 0; + grpc_error* global_error = ParseGlobalParams(json_tree); + grpc_error* local_error = ParsePerMethodParams(json_tree); + if (global_error != GRPC_ERROR_NONE) { + error_list[error_count++] = global_error; + } + if (local_error != GRPC_ERROR_NONE) { + error_list[error_count++] = local_error; + } + if (error_count > 0) { + *error = GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( + "Service config parsing error", error_list, error_count); + GRPC_ERROR_UNREF(global_error); + GRPC_ERROR_UNREF(local_error); + } } grpc_error* ServiceConfig::ParseGlobalParams(const grpc_json* json_tree) { GPR_DEBUG_ASSERT(json_tree_->type == GRPC_JSON_OBJECT); GPR_DEBUG_ASSERT(json_tree_->key == nullptr); - grpc_error* error = GRPC_ERROR_NONE; - for (size_t i = 0; i < registered_parsers_->size(); i++) { + InlinedVector error_list; + for (size_t i = 0; i < registered_parsers->size(); i++) { grpc_error* parser_error = GRPC_ERROR_NONE; auto parsed_obj = - (*registered_parsers_)[i]->ParseGlobalParams(json_tree, &parser_error); - error = grpc_error_add_child(error, parser_error); + (*registered_parsers)[i]->ParseGlobalParams(json_tree, &parser_error); + if (parser_error != GRPC_ERROR_NONE) { + error_list.push_back(parser_error); + } parsed_global_service_config_objects_.push_back(std::move(parsed_obj)); } - return error; + return CreateErrorFromVector("Global Params", &error_list); } grpc_error* ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable( @@ -85,12 +125,14 @@ grpc_error* ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable( SliceHashTable::Entry* entries, size_t* idx) { auto objs_vector = MakeUnique(); - grpc_error* error = GRPC_ERROR_NONE; - for (size_t i = 0; i < registered_parsers_->size(); i++) { + InlinedVector error_list; + for (size_t i = 0; i < registered_parsers->size(); i++) { grpc_error* parser_error = GRPC_ERROR_NONE; auto parsed_obj = - (*registered_parsers_)[i]->ParsePerMethodParams(json, &error); - error = grpc_error_add_child(error, parser_error); + (*registered_parsers)[i]->ParsePerMethodParams(json, &parser_error); + if (parser_error != GRPC_ERROR_NONE) { + error_list.push_back(parser_error); + } objs_vector->push_back(std::move(parsed_obj)); } const auto* vector_ptr = objs_vector.get(); @@ -101,16 +143,15 @@ grpc_error* ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable( if (child->key == nullptr) continue; if (strcmp(child->key, "name") == 0) { if (child->type != GRPC_JSON_ARRAY) { - error = grpc_error_add_child(error, - GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:name error:not of type Array")); + error_list.push_back(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) { grpc_error* parse_error = GRPC_ERROR_NONE; UniquePtr path = ParseJsonMethodName(name, &parse_error); if (path == nullptr) { - error = grpc_error_add_child(error, parse_error); + error_list.push_back(parse_error); } else { GPR_DEBUG_ASSERT(parse_error == GRPC_ERROR_NONE); } @@ -119,21 +160,17 @@ grpc_error* ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable( } } if (paths.size() == 0) { - error = grpc_error_add_child( - error, GRPC_ERROR_CREATE_FROM_STATIC_STRING("No names specified")); + error_list.push_back( + GRPC_ERROR_CREATE_FROM_STATIC_STRING("No names specified")); } // Add entry for each path. for (size_t i = 0; i < paths.size(); ++i) { entries[*idx].key = grpc_slice_from_copied_string(paths[i].get()); - entries[*idx].value = vector_ptr; // Takes a new ref. + entries[*idx].value = vector_ptr; ++*idx; } wrap_error: - if (error != GRPC_ERROR_NONE) { - error = grpc_error_add_child( - GRPC_ERROR_CREATE_FROM_STATIC_STRING("field:methodConfig"), error); - } - return error; + return CreateErrorFromVector("methodConfig", &error_list); } grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) { @@ -141,13 +178,12 @@ grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) { GPR_DEBUG_ASSERT(json_tree_->key == nullptr); SliceHashTable::Entry* entries = nullptr; size_t num_entries = 0; - grpc_error* error = GRPC_ERROR_NONE; + InlinedVector error_list; 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( - "error:Illegal key value - NULL")); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "error:Illegal key value - NULL")); continue; } if (strcmp(field->key, "methodConfig") == 0) { @@ -155,17 +191,15 @@ 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( - "field:methodConfig error:not of type Array")); + error_list.push_back(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( - "field:methodConfig error:No names found")); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:methodConfig error:No names found")); } num_entries += static_cast(count); } @@ -176,9 +210,11 @@ grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) { size_t idx = 0; for (grpc_json* method = field->child; method != nullptr; method = method->next) { - error = grpc_error_add_child( - error, ParseJsonMethodConfigToServiceConfigObjectsTable( - method, entries, &idx)); + grpc_error* error = ParseJsonMethodConfigToServiceConfigObjectsTable( + method, entries, &idx); + if (error != GRPC_ERROR_NONE) { + error_list.push_back(error); + } } GPR_DEBUG_ASSERT(num_entries == idx); break; @@ -190,7 +226,7 @@ grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) { num_entries, entries, nullptr); gpr_free(entries); } - return error; + return CreateErrorFromVector("Method Params", &error_list); } ServiceConfig::~ServiceConfig() { grpc_json_destroy(json_tree_); } @@ -230,50 +266,51 @@ UniquePtr 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"); + "field:name error:type is not 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) { - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Child entry with no key"); + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:name error: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"); + "field:name error:Child entry not of type string"); return nullptr; } if (strcmp(child->key, "service") == 0) { if (service_name != nullptr) { *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:service error:Multiple entries"); + "field:name error: field:service error:Multiple entries"); return nullptr; // Duplicate. } if (child->value == nullptr) { *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:service error:empty value"); + "field:name error: field:service error:empty value"); return nullptr; } service_name = child->value; } else if (strcmp(child->key, "method") == 0) { if (method_name != nullptr) { *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:method error:multiple entries"); + "field:name error: field:method error:multiple entries"); return nullptr; // Duplicate. } if (child->value == nullptr) { *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:method error:empty value"); + "field:name error: field:method error:empty value"); return nullptr; } method_name = child->value; } } if (service_name == nullptr) { - *error = - GRPC_ERROR_CREATE_FROM_STATIC_STRING("field:service error:not found"); + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:name error: field:service error:not found"); return nullptr; // Required field. } char* path; @@ -305,4 +342,19 @@ ServiceConfig::GetMethodServiceConfigObjectsVector(const grpc_slice& path) { return value; } +int ServiceConfig::RegisterParser(UniquePtr parser) { + registered_parsers->push_back(std::move(parser)); + return registered_parsers->size() - 1; +} + +void ServiceConfig::Init() { + GPR_ASSERT(registered_parsers == nullptr); + registered_parsers = New(); +} + +void ServiceConfig::Shutdown() { + Delete(registered_parsers); + registered_parsers = nullptr; +} + } // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/service_config.h b/src/core/ext/filters/client_channel/service_config.h index 9738cc82d30..f205fa2f243 100644 --- a/src/core/ext/filters/client_channel/service_config.h +++ b/src/core/ext/filters/client_channel/service_config.h @@ -150,24 +150,13 @@ class ServiceConfig : public RefCounted { /// registered parser. Each parser is responsible for reading the service /// config json and returning a parsed object. This parsed object can later be /// retrieved using the same index that was returned at registration time. - static int RegisterParser(UniquePtr parser) { - registered_parsers_->push_back(std::move(parser)); - return registered_parsers_->size() - 1; - } + static int RegisterParser(UniquePtr parser); - static void Init() { - GPR_ASSERT(registered_parsers_ == nullptr); - registered_parsers_ = New(); - } + static void Init(); - static void Shutdown() { - Delete(registered_parsers_); - registered_parsers_ = nullptr; - } + static void Shutdown(); private: - typedef InlinedVector, kNumPreallocatedParsers> - ServiceConfigParserList; // So New() can call our private ctor. template friend T* New(Args&&... args); @@ -202,8 +191,6 @@ class ServiceConfig : public RefCounted { SliceHashTable::Entry* entries, size_t* idx); - static ServiceConfigParserList* registered_parsers_; - UniquePtr service_config_json_; UniquePtr json_string_; // Underlying storage for json_tree. grpc_json* json_tree_; diff --git a/test/core/client_channel/service_config_test.cc b/test/core/client_channel/service_config_test.cc index 50e99ccfc10..f364da9cf89 100644 --- a/test/core/client_channel/service_config_test.cc +++ b/test/core/client_channel/service_config_test.cc @@ -16,6 +16,8 @@ * */ +#include + #include #include @@ -110,6 +112,28 @@ class TestParser2 : public ServiceConfigParser { } }; +// This parser always adds errors +class ErrorParser : public ServiceConfigParser { + public: + UniquePtr ParsePerMethodParams( + const grpc_json* json, grpc_error** error) override { + GPR_DEBUG_ASSERT(error != nullptr); + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(MethodError()); + return nullptr; + } + + UniquePtr ParseGlobalParams( + const grpc_json* json, grpc_error** error) override { + GPR_DEBUG_ASSERT(error != nullptr); + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(GlobalError()); + return nullptr; + } + + static const char* MethodError() { return "ErrorParser : methodError"; } + + static const char* GlobalError() { return "ErrorParser : globalError"; } +}; + class ServiceConfigTest : public ::testing::Test { protected: void SetUp() override { @@ -127,7 +151,7 @@ TEST_F(ServiceConfigTest, ErrorCheck1) { 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); + ASSERT_TRUE(error != GRPC_ERROR_NONE); EXPECT_TRUE(strstr(grpc_error_string(error), "failed to parse JSON for service config") != nullptr); } @@ -144,7 +168,7 @@ TEST_F(ServiceConfigTest, ErrorNoNames) { 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); + ASSERT_TRUE(error != GRPC_ERROR_NONE); EXPECT_TRUE(strstr(grpc_error_string(error), "No names found") != nullptr); } @@ -154,7 +178,7 @@ TEST_F(ServiceConfigTest, ErrorNoNamesWithMultipleMethodConfigs) { 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); + ASSERT_TRUE(error != GRPC_ERROR_NONE); EXPECT_TRUE(strstr(grpc_error_string(error), "No names found") != nullptr); } @@ -170,7 +194,7 @@ TEST_F(ServiceConfigTest, Parser1BasicTest1) { const char* test_json = "{\"global_param\":5}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(test_json, &error); - EXPECT_TRUE(error == GRPC_ERROR_NONE); + ASSERT_TRUE(error == GRPC_ERROR_NONE); EXPECT_TRUE((static_cast( svc_cfg->GetParsedGlobalServiceConfigObject(0))) ->value() == 5); @@ -180,7 +204,7 @@ TEST_F(ServiceConfigTest, Parser1BasicTest2) { const char* test_json = "{\"global_param\":1000}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(test_json, &error); - EXPECT_TRUE(error == GRPC_ERROR_NONE); + ASSERT_TRUE(error == GRPC_ERROR_NONE); EXPECT_TRUE((static_cast( svc_cfg->GetParsedGlobalServiceConfigObject(0))) ->value() == 1000); @@ -191,9 +215,14 @@ TEST_F(ServiceConfigTest, Parser1ErrorInvalidType) { 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), - TestParser1::InvalidTypeErrorMessage()) != nullptr); + ASSERT_TRUE(error != GRPC_ERROR_NONE); + std::regex e(std::string("(Service config parsing " + "error)(.*)(referenced_errors)(.*)(Global " + "Params)(.*)(referenced_errors)()(.*)") + + TestParser1::InvalidTypeErrorMessage()); + std::smatch match; + std::string s(grpc_error_string(error)); + EXPECT_TRUE(std::regex_search(s, match, e)); } TEST_F(ServiceConfigTest, Parser1ErrorInvalidValue) { @@ -201,9 +230,14 @@ TEST_F(ServiceConfigTest, Parser1ErrorInvalidValue) { 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), - TestParser1::InvalidValueErrorMessage()) != nullptr); + ASSERT_TRUE(error != GRPC_ERROR_NONE); + std::regex e(std::string("(Service config parsing " + "error)(.*)(referenced_errors)(.*)(Global " + "Params)(.*)(referenced_errors)()(.*)") + + TestParser1::InvalidValueErrorMessage()); + std::smatch match; + std::string s(grpc_error_string(error)); + EXPECT_TRUE(std::regex_search(s, match, e)); } TEST_F(ServiceConfigTest, Parser2BasicTest) { @@ -212,10 +246,10 @@ TEST_F(ServiceConfigTest, Parser2BasicTest) { "\"method_param\":5}]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(test_json, &error); - EXPECT_TRUE(error == GRPC_ERROR_NONE); + ASSERT_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); + EXPECT_TRUE(vector_ptr != nullptr); const auto* vector = *vector_ptr; auto parsed_object = ((*vector)[1]).get(); EXPECT_TRUE(static_cast(parsed_object)->value() == 5); @@ -227,9 +261,16 @@ TEST_F(ServiceConfigTest, Parser2ErrorInvalidType) { "\"method_param\":\"5\"}]}"; 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); + ASSERT_TRUE(error != GRPC_ERROR_NONE); + gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); + std::regex e(std::string("(Service config parsing " + "error)(.*)(referenced_errors\":\\[)(.*)(Method " + "Params)(.*)(referenced_errors)()(.*)(methodConfig)(" + ".*)(referenced_errors)(.*)") + + TestParser2::InvalidTypeErrorMessage()); + std::smatch match; + std::string s(grpc_error_string(error)); + EXPECT_TRUE(std::regex_search(s, match, e)); } TEST_F(ServiceConfigTest, Parser2ErrorInvalidValue) { @@ -238,9 +279,68 @@ TEST_F(ServiceConfigTest, Parser2ErrorInvalidValue) { "\"method_param\":-5}]}"; 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); + ASSERT_TRUE(error != GRPC_ERROR_NONE); + gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); + std::regex e(std::string("(Service config parsing " + "error)(.*)(referenced_errors\":\\[)(.*)(Method " + "Params)(.*)(referenced_errors)()(.*)(methodConfig)(" + ".*)(referenced_errors)(.*)") + + TestParser2::InvalidValueErrorMessage()); + std::smatch match; + std::string s(grpc_error_string(error)); + EXPECT_TRUE(std::regex_search(s, match, e)); +} + +// Test parsing with ErrorParsers which always add errors +class ErroredParsersScopingTest : public ::testing::Test { + protected: + void SetUp() override { + ServiceConfig::Shutdown(); + ServiceConfig::Init(); + EXPECT_TRUE(ServiceConfig::RegisterParser( + UniquePtr(New())) == 0); + EXPECT_TRUE(ServiceConfig::RegisterParser( + UniquePtr(New())) == 1); + } +}; + +TEST_F(ErroredParsersScopingTest, GlobalParams) { + const char* test_json = "{}"; + grpc_error* error = GRPC_ERROR_NONE; + auto svc_cfg = ServiceConfig::Create(test_json, &error); + ASSERT_TRUE(error != GRPC_ERROR_NONE); + gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); + std::regex e(std::string("(Service config parsing " + "error)(.*)(referenced_errors\":\\[)(.*)(Global " + "Params)(.*)(referenced_errors)()(.*)") + + ErrorParser::GlobalError() + std::string("(.*)") + + ErrorParser::GlobalError()); + std::smatch match; + std::string s(grpc_error_string(error)); + EXPECT_TRUE(std::regex_search(s, match, e)); +} + +TEST_F(ErroredParsersScopingTest, MethodParams) { + const char* test_json = "{\"methodConfig\": [{}]}"; + grpc_error* error = GRPC_ERROR_NONE; + auto svc_cfg = ServiceConfig::Create(test_json, &error); + ASSERT_TRUE(error != GRPC_ERROR_NONE); + gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); + std::regex e( + std::string("(Service config parsing " + "error)(.*)(referenced_errors\":\\[)(.*)(Global " + "Params)(.*)(referenced_errors)()(.*)") + + ErrorParser::GlobalError() + std::string("(.*)") + + ErrorParser::GlobalError() + + std::string("(.*)(Method " + "Params)(.*)(referenced_errors)(.*)(field:methodConfig " + "error:No names " + "found)(.*)(methodConfig)(.*)(referenced_errors)(.*)") + + ErrorParser::MethodError() + std::string("(.*)") + + ErrorParser::MethodError() + std::string("(.*)(No names specified)")); + std::smatch match; + std::string s(grpc_error_string(error)); + EXPECT_TRUE(std::regex_search(s, match, e)); } } // namespace testing