Replace type of method parameter of HttpClient::SendRequest with enum This way we advertise subset of of methods we are going to use. BUG:24267885 Change-Id: I4dda539269b490d0f6a828342ee9182f539b225b Reviewed-on: https://weave-review.googlesource.com/1294 Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/libweave/examples/ubuntu/curl_http_client.cc b/libweave/examples/ubuntu/curl_http_client.cc index 5cb4395..f8172bc 100644 --- a/libweave/examples/ubuntu/curl_http_client.cc +++ b/libweave/examples/ubuntu/curl_http_client.cc
@@ -7,6 +7,7 @@ #include <base/bind.h> #include <curl/curl.h> #include <weave/provider/task_runner.h> +#include <weave/enum_to_string.h> namespace weave { namespace examples { @@ -40,7 +41,7 @@ FROM_HERE, base::Bind(error_callback, base::Owned(error.release())), {}); } -void CurlHttpClient::SendRequest(const std::string& method, +void CurlHttpClient::SendRequest(Method method, const std::string& url, const Headers& headers, const std::string& data, @@ -50,13 +51,18 @@ &curl_easy_cleanup}; CHECK(curl); - if (method == "GET") { + switch (method) { + case Method::kGet: CHECK_EQ(CURLE_OK, curl_easy_setopt(curl.get(), CURLOPT_HTTPGET, 1L)); - } else if (method == "POST") { + break; + case Method::kPost: CHECK_EQ(CURLE_OK, curl_easy_setopt(curl.get(), CURLOPT_HTTPPOST, 1L)); - } else { - CHECK_EQ(CURLE_OK, curl_easy_setopt(curl.get(), CURLOPT_CUSTOMREQUEST, - method.c_str())); + break; + case Method::kPatch: + case Method::kPut: + CHECK_EQ(CURLE_OK, curl_easy_setopt(curl.get(), CURLOPT_CUSTOMREQUEST, + weave::EnumToString(method).c_str())); + break; } CHECK_EQ(CURLE_OK, curl_easy_setopt(curl.get(), CURLOPT_URL, url.c_str())); @@ -67,7 +73,7 @@ CHECK_EQ(CURLE_OK, curl_easy_setopt(curl.get(), CURLOPT_HTTPHEADER, chunk)); - if (!data.empty() || method == "POST") { + if (!data.empty() || method == Method::kPost) { CHECK_EQ(CURLE_OK, curl_easy_setopt(curl.get(), CURLOPT_POSTFIELDS, data.c_str())); }
diff --git a/libweave/examples/ubuntu/curl_http_client.h b/libweave/examples/ubuntu/curl_http_client.h index 033102d..a5090ff 100644 --- a/libweave/examples/ubuntu/curl_http_client.h +++ b/libweave/examples/ubuntu/curl_http_client.h
@@ -24,7 +24,7 @@ public: explicit CurlHttpClient(provider::TaskRunner* task_runner); - void SendRequest(const std::string& method, + void SendRequest(Method method, const std::string& url, const Headers& headers, const std::string& data,
diff --git a/libweave/examples/ubuntu/event_http_client.cc b/libweave/examples/ubuntu/event_http_client.cc index 742eef8..b24c075 100644 --- a/libweave/examples/ubuntu/event_http_client.cc +++ b/libweave/examples/ubuntu/event_http_client.cc
@@ -93,14 +93,14 @@ EventHttpClient::EventHttpClient(EventTaskRunner* task_runner) : task_runner_{task_runner} {} -void EventHttpClient::SendRequest(const std::string& method, +void EventHttpClient::SendRequest(Method method, const std::string& url, const Headers& headers, const std::string& data, const SuccessCallback& success_callback, const ErrorCallback& error_callback) { evhttp_cmd_type method_id; - CHECK(weave::StringToEnum(method, &method_id)); + CHECK(weave::StringToEnum(weave::EnumToString(method), &method_id)); std::unique_ptr<evhttp_uri, EventDeleter> http_uri{ evhttp_uri_parse(url.c_str())}; CHECK(http_uri); @@ -146,7 +146,8 @@ return; ErrorPtr error; Error::AddToPrintf(&error, FROM_HERE, "http_client", "request_failed", - "request failed: %s %s", method.c_str(), url.c_str()); + "request failed: %s %s", EnumToString(method).c_str(), + url.c_str()); task_runner_->PostDelayedTask(FROM_HERE, base::Bind(error_callback, error.get()), {}); }
diff --git a/libweave/examples/ubuntu/event_http_client.h b/libweave/examples/ubuntu/event_http_client.h index 53a01ab..457e550 100644 --- a/libweave/examples/ubuntu/event_http_client.h +++ b/libweave/examples/ubuntu/event_http_client.h
@@ -20,7 +20,7 @@ public: explicit EventHttpClient(EventTaskRunner* task_runner); - void SendRequest(const std::string& method, + void SendRequest(Method method, const std::string& url, const Headers& headers, const std::string& data,
diff --git a/libweave/include/weave/enum_to_string.h b/libweave/include/weave/enum_to_string.h index d6c2b79..503b753 100644 --- a/libweave/include/weave/enum_to_string.h +++ b/libweave/include/weave/enum_to_string.h
@@ -52,7 +52,7 @@ return m.name; } } - NOTREACHED(); + NOTREACHED() << static_cast<int>(id); return std::string(); }
diff --git a/libweave/include/weave/provider/http_client.h b/libweave/include/weave/provider/http_client.h index 995e7d6..24b4588 100644 --- a/libweave/include/weave/provider/http_client.h +++ b/libweave/include/weave/provider/http_client.h
@@ -17,6 +17,13 @@ class HttpClient { public: + enum class Method { + kGet, + kPatch, + kPost, + kPut, + }; + class Response { public: virtual int GetStatusCode() const = 0; @@ -29,7 +36,7 @@ using Headers = std::vector<std::pair<std::string, std::string>>; using SuccessCallback = base::Callback<void(const Response&)>; - virtual void SendRequest(const std::string& method, + virtual void SendRequest(Method method, const std::string& url, const Headers& headers, const std::string& data,
diff --git a/libweave/include/weave/provider/test/mock_http_client.h b/libweave/include/weave/provider/test/mock_http_client.h index 8a9f955..72cb9b8 100644 --- a/libweave/include/weave/provider/test/mock_http_client.h +++ b/libweave/include/weave/provider/test/mock_http_client.h
@@ -28,7 +28,7 @@ ~MockHttpClient() override = default; MOCK_METHOD6(SendRequest, - void(const std::string&, + void(Method, const std::string&, const Headers&, const std::string&,
diff --git a/libweave/src/device_registration_info.cc b/libweave/src/device_registration_info.cc index 7960b77..5a5fb0d 100644 --- a/libweave/src/device_registration_info.cc +++ b/libweave/src/device_registration_info.cc
@@ -113,7 +113,7 @@ class RequestSender final { public: - RequestSender(const std::string& method, + RequestSender(HttpClient::Method method, const std::string& url, HttpClient* transport) : method_{method}, url_{url}, transport_{transport} {} @@ -122,8 +122,8 @@ const ErrorCallback& error_callback) { static int debug_id = 0; ++debug_id; - VLOG(1) << "Sending request. id:" << debug_id << " method:" << method_ - << " url:" << url_; + VLOG(1) << "Sending request. id:" << debug_id + << " method:" << EnumToString(method_) << " url:" << url_; VLOG(2) << "Request data: " << data_; auto on_success = [](int debug_id, const HttpClient::SuccessCallback& success_callback, @@ -175,7 +175,7 @@ return headers; } - std::string method_; + HttpClient::Method method_; std::string url_; std::string data_; std::string mime_type_; @@ -380,7 +380,8 @@ std::make_shared<base::Closure>(success_callback); auto shared_error_callback = std::make_shared<ErrorCallback>(error_callback); - RequestSender sender{http::kPost, GetOAuthURL("token"), http_client_}; + RequestSender sender{HttpClient::Method::kPost, GetOAuthURL("token"), + http_client_}; sender.SetFormData({ {"refresh_token", GetSettings().refresh_token}, {"client_id", GetSettings().client_id}, @@ -532,8 +533,8 @@ error_callback.Run(error.get()); return; } - DoCloudRequest(http::kGet, GetDeviceURL(), nullptr, success_callback, - error_callback); + DoCloudRequest(HttpClient::Method::kGet, GetDeviceURL(), nullptr, + success_callback, error_callback); } struct DeviceRegistrationInfo::RegisterCallbacks { @@ -574,7 +575,7 @@ auto url = GetServiceURL("registrationTickets/" + ticket_id, {{"key", GetSettings().api_key}}); - RequestSender sender{http::kPatch, url, http_client_}; + RequestSender sender{HttpClient::Method::kPatch, url, http_client_}; sender.SetJsonData(req_json); sender.Send(base::Bind(&DeviceRegistrationInfo::RegisterDeviceOnTicketSent, weak_factory_.GetWeakPtr(), ticket_id, callbacks), @@ -599,7 +600,7 @@ std::string url = GetServiceURL("registrationTickets/" + ticket_id + "/finalize", {{"key", GetSettings().api_key}}); - RequestSender{http::kPost, url, http_client_}.Send( + RequestSender{HttpClient::Method::kPost, url, http_client_}.Send( base::Bind(&DeviceRegistrationInfo::RegisterDeviceOnTicketFinalized, weak_factory_.GetWeakPtr(), callbacks), base::Bind(&DeviceRegistrationInfo::RegisterDeviceError, @@ -634,7 +635,8 @@ UpdateDeviceInfoTimestamp(*device_draft_response); // Now get access_token and refresh_token - RequestSender sender2{http::kPost, GetOAuthURL("token"), http_client_}; + RequestSender sender2{HttpClient::Method::kPost, GetOAuthURL("token"), + http_client_}; sender2.SetFormData( {{"code", auth_code}, {"client_id", GetSettings().client_id}, @@ -686,7 +688,7 @@ } void DeviceRegistrationInfo::DoCloudRequest( - const std::string& method, + HttpClient::Method method, const std::string& url, const base::DictionaryValue* body, const CloudRequestCallback& success_callback, @@ -901,8 +903,8 @@ const base::DictionaryValue& command_patch, const base::Closure& on_success, const base::Closure& on_error) { - DoCloudRequest(http::kPatch, GetServiceURL("commands/" + command_id), - &command_patch, + DoCloudRequest(HttpClient::Method::kPatch, + GetServiceURL("commands/" + command_id), &command_patch, base::Bind(&IgnoreCloudResultWithCallback, on_success), base::Bind(&IgnoreCloudErrorWithCallback, on_error)); } @@ -969,7 +971,7 @@ {}, {{"lastUpdateTimeMs", last_device_resource_updated_timestamp_}}); DoCloudRequest( - http::kPut, url, device_resource.get(), + HttpClient::Method::kPut, url, device_resource.get(), base::Bind(&DeviceRegistrationInfo::OnUpdateDeviceResourceSuccess, AsWeakPtr()), base::Bind(&DeviceRegistrationInfo::OnUpdateDeviceResourceError, @@ -1061,7 +1063,7 @@ fetch_commands_request_sent_ = true; fetch_commands_request_queued_ = false; DoCloudRequest( - http::kGet, + HttpClient::Method::kGet, GetServiceURL("commands/queue", {{"deviceId", GetSettings().cloud_id}}), nullptr, base::Bind(&DeviceRegistrationInfo::OnFetchCommandsSuccess, AsWeakPtr(), on_success), @@ -1105,8 +1107,9 @@ std::unique_ptr<base::DictionaryValue> cmd_copy{command_dict->DeepCopy()}; cmd_copy->SetString("state", "aborted"); // TODO(wiley) We could consider handling this error case more gracefully. - DoCloudRequest(http::kPut, GetServiceURL("commands/" + command_id), - cmd_copy.get(), base::Bind(&IgnoreCloudResult), + DoCloudRequest(HttpClient::Method::kPut, + GetServiceURL("commands/" + command_id), cmd_copy.get(), + base::Bind(&IgnoreCloudResult), base::Bind(&IgnoreCloudError)); } else { // Normal command, publish it to local clients. @@ -1197,7 +1200,7 @@ device_state_update_pending_ = true; DoCloudRequest( - http::kPost, GetDeviceURL("patchState"), &body, + HttpClient::Method::kPost, GetDeviceURL("patchState"), &body, base::Bind(&DeviceRegistrationInfo::OnPublishStateSuccess, AsWeakPtr(), update_id), base::Bind(&DeviceRegistrationInfo::OnPublishStateError, AsWeakPtr()));
diff --git a/libweave/src/device_registration_info.h b/libweave/src/device_registration_info.h index 8d230a9..1577cd4 100644 --- a/libweave/src/device_registration_info.h +++ b/libweave/src/device_registration_info.h
@@ -176,7 +176,7 @@ // and device removal. It is a recommended way to do cloud API // requests. // TODO(antonm): Consider moving into some other class. - void DoCloudRequest(const std::string& method, + void DoCloudRequest(provider::HttpClient::Method method, const std::string& url, const base::DictionaryValue* body, const CloudRequestCallback& success_callback, @@ -184,7 +184,7 @@ // Helper for DoCloudRequest(). struct CloudRequestData { - std::string method; + provider::HttpClient::Method method; std::string url; std::string body; CloudRequestCallback success_callback;
diff --git a/libweave/src/device_registration_info_unittest.cc b/libweave/src/device_registration_info_unittest.cc index 41b6d78..49c4ccb 100644 --- a/libweave/src/device_registration_info_unittest.cc +++ b/libweave/src/device_registration_info_unittest.cc
@@ -233,9 +233,10 @@ EXPECT_FALSE(dev_reg_->HaveRegistrationCredentials()); ReloadSettings(); - EXPECT_CALL(http_client_, - SendRequest(http::kPost, dev_reg_->GetOAuthURL("token"), - HttpClient::Headers{GetFormHeader()}, _, _, _)) + EXPECT_CALL( + http_client_, + SendRequest(HttpClient::Method::kPost, dev_reg_->GetOAuthURL("token"), + HttpClient::Headers{GetFormHeader()}, _, _, _)) .WillOnce(WithArgs<3, 4>(Invoke([]( const std::string& data, const HttpClient::SuccessCallback& callback) { @@ -261,9 +262,10 @@ ReloadSettings(); EXPECT_EQ(GcdState::kConnecting, GetGcdState()); - EXPECT_CALL(http_client_, - SendRequest(http::kPost, dev_reg_->GetOAuthURL("token"), - HttpClient::Headers{GetFormHeader()}, _, _, _)) + EXPECT_CALL( + http_client_, + SendRequest(HttpClient::Method::kPost, dev_reg_->GetOAuthURL("token"), + HttpClient::Headers{GetFormHeader()}, _, _, _)) .WillOnce(WithArgs<3, 4>(Invoke([]( const std::string& data, const HttpClient::SuccessCallback& callback) { @@ -289,9 +291,10 @@ ReloadSettings(); EXPECT_EQ(GcdState::kConnecting, GetGcdState()); - EXPECT_CALL(http_client_, - SendRequest(http::kPost, dev_reg_->GetOAuthURL("token"), - HttpClient::Headers{GetFormHeader()}, _, _, _)) + EXPECT_CALL( + http_client_, + SendRequest(HttpClient::Method::kPost, dev_reg_->GetOAuthURL("token"), + HttpClient::Headers{GetFormHeader()}, _, _, _)) .WillOnce(WithArgs<3, 4>(Invoke([]( const std::string& data, const HttpClient::SuccessCallback& callback) { @@ -318,7 +321,7 @@ SetAccessToken(); EXPECT_CALL(http_client_, - SendRequest(http::kGet, dev_reg_->GetDeviceURL(), + SendRequest(HttpClient::Method::kGet, dev_reg_->GetDeviceURL(), HttpClient::Headers{GetAuthHeader(), GetJsonHeader()}, _, _, _)) .WillOnce(WithArgs<3, 4>( @@ -382,10 +385,10 @@ std::string ticket_url = dev_reg_->GetServiceURL("registrationTickets/") + test_data::kClaimTicketId; - EXPECT_CALL( - http_client_, - SendRequest(http::kPatch, ticket_url + "?key=" + test_data::kApiKey, - HttpClient::Headers{GetJsonHeader()}, _, _, _)) + EXPECT_CALL(http_client_, + SendRequest(HttpClient::Method::kPatch, + ticket_url + "?key=" + test_data::kApiKey, + HttpClient::Headers{GetJsonHeader()}, _, _, _)) .WillOnce(WithArgs<3, 4>(Invoke([]( const std::string& data, const HttpClient::SuccessCallback& callback) { @@ -452,7 +455,7 @@ }))); EXPECT_CALL(http_client_, - SendRequest(http::kPost, + SendRequest(HttpClient::Method::kPost, ticket_url + "/finalize?key=" + test_data::kApiKey, HttpClient::Headers{}, _, _, _)) .WillOnce( @@ -471,9 +474,10 @@ callback.Run(*ReplyWithJson(200, json)); }))); - EXPECT_CALL(http_client_, - SendRequest(http::kPost, dev_reg_->GetOAuthURL("token"), - HttpClient::Headers{GetFormHeader()}, _, _, _)) + EXPECT_CALL( + http_client_, + SendRequest(HttpClient::Method::kPost, dev_reg_->GetOAuthURL("token"), + HttpClient::Headers{GetFormHeader()}, _, _, _)) .WillOnce(WithArgs<3, 4>(Invoke([]( const std::string& data, const HttpClient::SuccessCallback& callback) { @@ -572,7 +576,7 @@ TEST_F(DeviceRegistrationInfoUpdateCommandTest, SetProgress) { EXPECT_CALL(http_client_, - SendRequest(http::kPatch, command_url_, + SendRequest(HttpClient::Method::kPatch, command_url_, HttpClient::Headers{GetAuthHeader(), GetJsonHeader()}, _, _, _)) .WillOnce(WithArgs<3, 4>(Invoke([]( @@ -589,7 +593,7 @@ TEST_F(DeviceRegistrationInfoUpdateCommandTest, Complete) { EXPECT_CALL(http_client_, - SendRequest(http::kPatch, command_url_, + SendRequest(HttpClient::Method::kPatch, command_url_, HttpClient::Headers{GetAuthHeader(), GetJsonHeader()}, _, _, _)) .WillOnce(WithArgs<3, 4>(Invoke([]( @@ -606,7 +610,7 @@ TEST_F(DeviceRegistrationInfoUpdateCommandTest, Cancel) { EXPECT_CALL(http_client_, - SendRequest(http::kPatch, command_url_, + SendRequest(HttpClient::Method::kPatch, command_url_, HttpClient::Headers{GetAuthHeader(), GetJsonHeader()}, _, _, _)) .WillOnce(WithArgs<3, 4>(Invoke([](
diff --git a/libweave/src/http_constants.cc b/libweave/src/http_constants.cc index e80aae4..445ff75 100644 --- a/libweave/src/http_constants.cc +++ b/libweave/src/http_constants.cc
@@ -4,14 +4,12 @@ #include "src/http_constants.h" +#include <weave/provider/http_client.h> +#include <weave/enum_to_string.h> + namespace weave { namespace http { -const char kGet[] = "GET"; -const char kPatch[] = "PATCH"; -const char kPost[] = "POST"; -const char kPut[] = "PUT"; - const char kAuthorization[] = "Authorization"; const char kContentType[] = "Content-Type"; @@ -21,4 +19,21 @@ const char kWwwFormUrlEncoded[] = "application/x-www-form-urlencoded"; } // namespace http + +using provider::HttpClient; + +namespace { + +const weave::EnumToStringMap<HttpClient::Method>::Map kMapMethod[] = { + {HttpClient::Method::kGet, "GET"}, + {HttpClient::Method::kPost, "POST"}, + {HttpClient::Method::kPut, "PUT"}, + {HttpClient::Method::kPatch, "PATCH"}}; + +} // namespace + +template <> +LIBWEAVE_EXPORT EnumToStringMap<HttpClient::Method>::EnumToStringMap() + : EnumToStringMap(kMapMethod) {} + } // namespace weave
diff --git a/libweave/src/http_constants.h b/libweave/src/http_constants.h index f219a3c..973190e 100644 --- a/libweave/src/http_constants.h +++ b/libweave/src/http_constants.h
@@ -18,11 +18,6 @@ const int kServiceUnavailable = 503; const int kNotSupported = 501; -extern const char kGet[]; -extern const char kPatch[]; -extern const char kPost[]; -extern const char kPut[]; - extern const char kAuthorization[]; extern const char kContentType[];
diff --git a/libweave/src/weave_unittest.cc b/libweave/src/weave_unittest.cc index 8eb6a54..5e15212 100644 --- a/libweave/src/weave_unittest.cc +++ b/libweave/src/weave_unittest.cc
@@ -36,10 +36,11 @@ namespace weave { +using provider::HttpClient; +using provider::Network; +using provider::test::MockHttpClientResponse; using test::CreateDictionaryValue; using test::ValueToString; -using provider::test::MockHttpClientResponse; -using provider::Network; const char kCommandDefs[] = R"({ "base": { @@ -136,12 +137,12 @@ protected: void SetUp() override {} - void ExpectRequest(const std::string& method, + void ExpectRequest(HttpClient::Method method, const std::string& url, const std::string& json_response) { EXPECT_CALL(http_client_, SendRequest(method, url, _, _, _, _)) .WillOnce(WithArgs<4>(Invoke([json_response]( - const provider::HttpClient::SuccessCallback& callback) { + const HttpClient::SuccessCallback& callback) { std::unique_ptr<provider::test::MockHttpClientResponse> response{ new StrictMock<provider::test::MockHttpClientResponse>}; EXPECT_CALL(*response, GetStatusCode()) @@ -312,7 +313,7 @@ auto response = CreateDictionaryValue(kRegistrationResponse); response->Set("deviceDraft", draft->DeepCopy()); ExpectRequest( - "PATCH", + HttpClient::Method::kPatch, "https://www.googleapis.com/clouddevices/v1/registrationTickets/" "TICKET_ID?key=TEST_API_KEY", ValueToString(*response)); @@ -320,12 +321,13 @@ response = CreateDictionaryValue(kRegistrationFinalResponse); response->Set("deviceDraft", draft->DeepCopy()); ExpectRequest( - "POST", + HttpClient::Method::kPost, "https://www.googleapis.com/clouddevices/v1/registrationTickets/" "TICKET_ID/finalize?key=TEST_API_KEY", ValueToString(*response)); - ExpectRequest("POST", "https://accounts.google.com/o/oauth2/token", + ExpectRequest(HttpClient::Method::kPost, + "https://accounts.google.com/o/oauth2/token", kAuthTokenResponse); InitDnsSdPublishing(true, "DB");