diff --git a/src/api_manager/config.cc b/src/api_manager/config.cc index 1fb658202..6cc427c36 100644 --- a/src/api_manager/config.cc +++ b/src/api_manager/config.cc @@ -134,80 +134,92 @@ bool Config::LoadQuotaRule(ApiManagerEnvInterface *env) { bool Config::LoadHttpMethods(ApiManagerEnvInterface *env, PathMatcherBuilder *pmb) { - std::set all_urls, urls_with_options; + std::set all_urls; + + // A HttpRule may have additional bindings, but only at the top level. + // If the top level rule fails to register, its additional bindings + // won't be registered. + for (const auto &rule : service_.http().rules()) { + // additional bindings use the same top level selector. + MethodInfoImpl *mi = GetOrCreateMethodInfoImpl(rule.selector(), "", ""); + + if (!RegisterHttpRule(env, pmb, mi, all_urls, rule)) { + env->LogError("Failed to add http rule: " + rule.DebugString()); + } else { + for (const auto &additional_rule : rule.additional_bindings()) { + if (!RegisterHttpRule(env, pmb, mi, all_urls, additional_rule)) { + env->LogError("Failed to add additional_binding rule: " + + additional_rule.DebugString()); + } + } + } + } + // By default, allow_cors is false. This means that the default behavior // of ESP is to reject all "OPTIONS" requests. If customers want to enable // CORS, they need to set "allow_cors" to true in swagger config. - bool allow_cors = false; for (const auto &endpoint : service_.endpoints()) { if (endpoint.name() == service_.name() && endpoint.allow_cors()) { - allow_cors = true; - env->LogDebug("CORS is allowed."); - break; + env->LogInfo("CORS is allowed."); + return AddOptionsMethodForAllUrls(env, pmb, all_urls); } } - for (const auto &rule : service_.http().rules()) { - const string &selector = rule.selector(); - const string *url = nullptr; - const char *http_method = nullptr; - - switch (rule.pattern_case()) { - case ::google::api::HttpRule::kGet: - url = &rule.get(); - http_method = http_get; - break; - case ::google::api::HttpRule::kPut: - url = &rule.put(); - http_method = http_put; - break; - case ::google::api::HttpRule::kPost: - url = &rule.post(); - http_method = http_post; - break; - case ::google::api::HttpRule::kDelete: - url = &rule.delete_(); - http_method = http_delete; - break; - case ::google::api::HttpRule::kPatch: - url = &rule.patch(); - http_method = http_patch; - break; - case ::google::api::HttpRule::kCustom: - url = &rule.custom().path(); - http_method = rule.custom().kind().c_str(); - break; - default: - break; - } - - if (http_method == nullptr || url == nullptr || url->empty()) { - env->LogError("Invalid HTTP binding encountered."); - continue; - } + return true; +} - MethodInfoImpl *mi = GetOrCreateMethodInfoImpl(selector, "", ""); +bool Config::RegisterHttpRule(ApiManagerEnvInterface *env, + PathMatcherBuilder *pmb, + MethodInfoImpl *mi, + std::set &all_urls, + const ::google::api::HttpRule &rule) { + const string *url = nullptr; + const char *http_method = nullptr; + + switch (rule.pattern_case()) { + case ::google::api::HttpRule::kGet: + url = &rule.get(); + http_method = http_get; + break; + case ::google::api::HttpRule::kPut: + url = &rule.put(); + http_method = http_put; + break; + case ::google::api::HttpRule::kPost: + url = &rule.post(); + http_method = http_post; + break; + case ::google::api::HttpRule::kDelete: + url = &rule.delete_(); + http_method = http_delete; + break; + case ::google::api::HttpRule::kPatch: + url = &rule.patch(); + http_method = http_patch; + break; + case ::google::api::HttpRule::kCustom: + url = &rule.custom().path(); + http_method = rule.custom().kind().c_str(); + break; + default: + break; + } - auto status = pmb->Register(http_method, *url, rule.body(), mi); - if (!status.ok()) { - env->LogError(status.message()); - } else if (allow_cors) { - all_urls.insert(*url); - if (strcmp(http_method, http_options) == 0) { - urls_with_options.insert(*url); - } - } + if (http_method == nullptr || url == nullptr || url->empty()) { + env->LogError("Invalid HTTP binding encountered."); + return false; } - if (!allow_cors) { - return true; + auto status = pmb->Register(http_method, *url, rule.body(), mi); + if (!status.ok()) { + env->LogError(status.message()); + return false; } - // Remove urls with options. - for (auto url : urls_with_options) { - all_urls.erase(url); + if (strcmp(http_method, http_options) != 0) { + all_urls.insert(*url); } - return AddOptionsMethodForAllUrls(env, pmb, all_urls); + return true; } bool Config::AddOptionsMethodForAllUrls(ApiManagerEnvInterface *env, diff --git a/src/api_manager/config.h b/src/api_manager/config.h index 60342276e..476b29a47 100644 --- a/src/api_manager/config.h +++ b/src/api_manager/config.h @@ -106,10 +106,16 @@ class Config { bool LoadService(ApiManagerEnvInterface *env, const std::string &service_config); - // Create MethodInfo for HTTP methods, register them to PathMatcher. + // Loads the Http rules, registers and adds CORS support if required. bool LoadHttpMethods(ApiManagerEnvInterface *env, PathMatcherBuilder *pmb); + // Register a Http rule to the path matcher builder. + bool RegisterHttpRule(ApiManagerEnvInterface *env, + PathMatcherBuilder *pmb, + MethodInfoImpl *mi, std::set &all_urls, + const ::google::api::HttpRule &rule); + // Add a special option method info for all URLs to support CORS. bool AddOptionsMethodForAllUrls(ApiManagerEnvInterface *env, PathMatcherBuilder *pmb, diff --git a/src/api_manager/config_test.cc b/src/api_manager/config_test.cc index 9053193da..04f9dd7df 100644 --- a/src/api_manager/config_test.cc +++ b/src/api_manager/config_test.cc @@ -900,6 +900,112 @@ TEST(Config, TestHttpOptions) { ASSERT_EQ(nullptr, method2); } +TEST(Config, TestAdditionalBindings) { + MockApiManagerEnvironmentWithLog env; + + static const char config_text[] = R"( + name: "Service.Name" + endpoints { + name: "Service.Name" + allow_cors: true + } + http { + rules { + selector: "GetMessages" + get: "/v1/messages/{message_id}" + additional_bindings { + get: "/v1/users/{user_id}/messages/{message_id}" + } + additional_bindings { + get: "/v2/messages/{user_id}/{message_id}" + } + } + } +)"; + + std::unique_ptr config = Config::Create(&env, config_text, ""); + ASSERT_TRUE(config); + + auto method = config->GetMethodInfo("GET", "/v1/messages/1234"); + ASSERT_NE(nullptr, method); + + ASSERT_EQ(method, + config->GetMethodInfo("GET", "/v1/users/123/messages/1234")); + ASSERT_EQ(method, config->GetMethodInfo("GET", "/v2/messages/123/1234")); + + // OPTIONS added + for (auto path : {"/v1/messages/1234", "/v1/users/123/messages/1234", + "/v2/messages/123/1234"}) { + auto method = config->GetMethodInfo("OPTIONS", path); + ASSERT_NE(nullptr, method); + ASSERT_EQ("CORS", method->name()); + ASSERT_FALSE(method->auth()); + // For all added OPTIONS methods, allow_unregistered_calls is true. + ASSERT_TRUE(method->allow_unregistered_calls()); + } +} + +TEST(Config, TestAdditionalBindingsFailedTopLevel) { + MockApiManagerEnvironmentWithLog env; + + // Top level rule has empty url, so it will fail to add + static const char config_text[] = R"( + name: "Service.Name" + http { + rules { + selector: "GetMessages" + get: "" + additional_bindings { + get: "/v1/users" + } + } + } +)"; + + std::unique_ptr config = Config::Create(&env, config_text, ""); + ASSERT_TRUE(config); + + // The additional bindings is not added since top level rule fails + auto method = config->GetMethodInfo("GET", "/v1/users"); + ASSERT_EQ(nullptr, method); +} + +TEST(Config, TestAdditionalBindingsFailOne) { + MockApiManagerEnvironmentWithLog env; + + // one of additional_bindings fails with empty url + // but other additional_bindings is added + static const char config_text[] = R"( + name: "Service.Name" + http { + rules { + selector: "GetMessages" + get: "/v1/messages/{message_id}" + additional_bindings { + get: "" + } + additional_bindings { + get: "/v1/users/{user_id}/messages/{message_id}" + } + } + } +)"; + + std::unique_ptr config = Config::Create(&env, config_text, ""); + ASSERT_TRUE(config); + + auto method = config->GetMethodInfo("GET", "/v1/messages/1234"); + ASSERT_NE(nullptr, method); + + ASSERT_EQ(method, + config->GetMethodInfo("GET", "/v1/users/123/messages/1234")); + + // OPTIONS are not added + ASSERT_EQ(nullptr, config->GetMethodInfo("OPTIONS", "/v1/messages/1234")); + ASSERT_EQ(nullptr, + config->GetMethodInfo("OPTIONS", "/v1/users/123/messages/1234")); +} + TEST(Config, TestHttpOptionsSelector) { MockApiManagerEnvironmentWithLog env; diff --git a/test/transcoding/README.md b/test/transcoding/README.md index 34077670b..b6beb09d7 100644 --- a/test/transcoding/README.md +++ b/test/transcoding/README.md @@ -49,12 +49,16 @@ producer_project_id: "" name: ".appspot.com" id: "2016-08-25r1" ``` - * add following rules to http rule: + * add following additional_bindings to CreateBook http rule: ``` rules { selector: "endpoints.examples.bookstore.Bookstore.CreateBook" - post: "/shelves/{shelf}/books/{book.id}/{book.author}" - body: "book.title" + post: "/shelves/{shelf}/books" + body: "book" ++ additional_bindings { ++ post: "/shelves/{shelf}/books/{book.id}/{book.author}" ++ body: "book.title" ++ } } ``` - Replace /tmp/out.txt with test/transcoding/service.pb.txt diff --git a/test/transcoding/service.pb.txt b/test/transcoding/service.pb.txt index 313fc97f9..9a0055767 100644 --- a/test/transcoding/service.pb.txt +++ b/test/transcoding/service.pb.txt @@ -837,11 +837,10 @@ http { selector: "endpoints.examples.bookstore.Bookstore.CreateBook" post: "/shelves/{shelf}/books" body: "book" - } - rules { - selector: "endpoints.examples.bookstore.Bookstore.CreateBook" - post: "/shelves/{shelf}/books/{book.id}/{book.author}" - body: "book.title" + additional_bindings { + post: "/shelves/{shelf}/books/{book.id}/{book.author}" + body: "book.title" + } } rules { selector: "endpoints.examples.bookstore.Bookstore.GetBook"