From b33c0e0332d7737e2805d9ae84acdfc6f683fa50 Mon Sep 17 00:00:00 2001 From: Janusz Sobczak Date: Fri, 15 Sep 2023 12:10:07 -0700 Subject: [PATCH] Migrate to AnyInvocable in auth module Merged success/failure callbacks into a single callback in SignInCallback and AccessTokenCallback to simplify resource management and to avoid making copies. PiperOrigin-RevId: 565744310 --- .../test/fast_pair_fake_http_client.h | 24 ++++++++------ fastpair/server_access/BUILD | 11 +++++++ .../server_access/fast_pair_client_impl.cc | 32 +++++++++++-------- .../fast_pair_client_impl_test.cc | 27 +++++++++++++--- internal/network/BUILD | 9 +++--- internal/network/http_client.h | 11 +++++-- internal/network/http_client_impl.cc | 16 +++++++--- internal/network/http_client_impl.h | 9 +++--- internal/test/fake_http_client.h | 21 ++++++------ 9 files changed, 106 insertions(+), 54 deletions(-) diff --git a/fastpair/internal/test/fast_pair_fake_http_client.h b/fastpair/internal/test/fast_pair_fake_http_client.h index 1bd12bb5..4723edf5 100644 --- a/fastpair/internal/test/fast_pair_fake_http_client.h +++ b/fastpair/internal/test/fast_pair_fake_http_client.h @@ -15,15 +15,20 @@ #ifndef THIRD_PARTY_NEARBY_FASTPAIR_INTERNAL_TEST_FAST_PAIR_FAKE_HTTP_CLIENT_H_ #define THIRD_PARTY_NEARBY_FASTPAIR_INTERNAL_TEST_FAST_PAIR_FAKE_HTTP_CLIENT_H_ -#include +#include #include #include #include #include #include +#include "absl/functional/any_invocable.h" +#include "absl/status/status.h" #include "absl/status/statusor.h" #include "internal/network/http_client.h" +#include "internal/network/http_request.h" +#include "internal/network/http_response.h" +#include "internal/network/http_status_code.h" namespace nearby { namespace network { @@ -32,7 +37,7 @@ class FastPairFakeHttpClient : public HttpClient { public: struct RequestInfo { HttpRequest request; - std::function&)> callback; + absl::AnyInvocable&)> callback; }; FastPairFakeHttpClient() = default; @@ -44,18 +49,19 @@ class FastPairFakeHttpClient : public HttpClient { FastPairFakeHttpClient(FastPairFakeHttpClient&&) = default; FastPairFakeHttpClient& operator=(FastPairFakeHttpClient&&) = default; - void StartRequest(const HttpRequest& request, - std::function&)> - callback) override { + void StartRequest( + const HttpRequest& request, + absl::AnyInvocable&)> callback) + override { RequestInfo request_info; request_info.request = request; - request_info.callback = callback; + request_info.callback = std::move(callback); request_infos_.push_back(std::move(request_info)); } void StartCancellableRequest( std::unique_ptr request, - std::function&)> callback) + absl::AnyInvocable&)> callback) override {} absl::StatusOr GetResponse( @@ -70,8 +76,8 @@ class FastPairFakeHttpClient : public HttpClient { return; } - auto request_info = request_infos_.at(pos); - if (request_info.callback != nullptr) { + auto& request_info = request_infos_.at(pos); + if (request_info.callback) { request_info.callback(response); } diff --git a/fastpair/server_access/BUILD b/fastpair/server_access/BUILD index 89bfffa7..6ca7f1dc 100644 --- a/fastpair/server_access/BUILD +++ b/fastpair/server_access/BUILD @@ -33,10 +33,13 @@ cc_library( "//fastpair/proto:fastpair_cc_proto", "//fastpair/proto:proto_to_json", "//internal/account", + "//internal/auth:credential", "//internal/auth:types", "//internal/base", "//internal/network:types", "//internal/platform:types", + "//internal/platform/implementation:types", + "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:str_format", @@ -92,12 +95,20 @@ cc_test( "//internal/account", "//internal/account:test_support", "//internal/auth:credential", + "//internal/auth:types", "//internal/network:types", "//internal/platform:types", "//internal/platform/implementation/g3", # build_cleaner: keep + "//internal/preferences", "//internal/test", "//internal/test/google3_only:test", "@com_github_protobuf_matchers//protobuf-matchers", + "@com_google_absl//absl/functional:any_invocable", + "@com_google_absl//absl/log:check", + "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", + "@com_google_absl//absl/strings", + "@com_google_absl//absl/strings:string_view", "@com_google_googletest//:gtest_main", ], ) diff --git a/fastpair/server_access/fast_pair_client_impl.cc b/fastpair/server_access/fast_pair_client_impl.cc index 8a5960e8..21590686 100644 --- a/fastpair/server_access/fast_pair_client_impl.cc +++ b/fastpair/server_access/fast_pair_client_impl.cc @@ -14,20 +14,28 @@ #include "fastpair/server_access/fast_pair_client_impl.h" -#include #include #include #include #include +#include "absl/status/status.h" +#include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" #include "absl/strings/string_view.h" #include "absl/synchronization/notification.h" #include "fastpair/common/fast_pair_switches.h" +#include "fastpair/server_access/fast_pair_http_notifier.h" #include "internal/account/account_manager.h" +#include "internal/auth/auth_status_util.h" +#include "internal/auth/authentication_manager.h" #include "internal/network/http_client.h" +#include "internal/network/http_request.h" +#include "internal/network/http_response.h" #include "internal/network/url.h" +#include "internal/platform/device_info.h" +#include "internal/platform/implementation/device_info.h" #include "internal/platform/logging.h" namespace nearby { @@ -274,19 +282,15 @@ absl::StatusOr FastPairClientImpl::GetAccessToken() { absl::StatusOr result; absl::Notification notification; authentication_manager_->FetchAccessToken( - account->id, { - .success_cb = - [&](absl::string_view access_token) { - result = std::string(access_token); - notification.Notify(); - }, - .failure_cb = - [&](auth::AuthStatus status) { - result = absl::UnknownError( - absl::StrCat(static_cast(status))); - notification.Notify(); - }, - }); + account->id, + [&](auth::AuthStatus status, absl::string_view access_token) { + if (status == auth::AuthStatus::SUCCESS) { + result = std::string(access_token); + } else { + result = absl::UnknownError(absl::StrCat(static_cast(status))); + } + notification.Notify(); + }); notification.WaitForNotification(); return result; } diff --git a/fastpair/server_access/fast_pair_client_impl_test.cc b/fastpair/server_access/fast_pair_client_impl_test.cc index f2c2c0b3..255cfa18 100644 --- a/fastpair/server_access/fast_pair_client_impl_test.cc +++ b/fastpair/server_access/fast_pair_client_impl_test.cc @@ -25,23 +25,39 @@ #include "gmock/gmock.h" #include "protobuf-matchers/protocol-buffer-matchers.h" #include "gtest/gtest.h" +#include "absl/functional/any_invocable.h" +#include "absl/log/check.h" +#include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "absl/strings/ascii.h" +#include "absl/strings/escaping.h" +#include "absl/strings/numbers.h" +#include "absl/strings/string_view.h" +#include "fastpair/common/account_key.h" +#include "fastpair/common/device_metadata.h" #include "fastpair/common/fast_pair_device.h" #include "fastpair/common/fast_pair_prefs.h" #include "fastpair/common/fast_pair_switches.h" +#include "fastpair/common/protocol.h" #include "fastpair/proto/data.proto.h" #include "fastpair/proto/enum.proto.h" #include "fastpair/proto/fast_pair_string.proto.h" #include "fastpair/proto/proto_builder.h" +#include "fastpair/server_access/fast_pair_client.h" #include "fastpair/server_access/fast_pair_http_notifier.h" #include "internal/account/account_manager.h" #include "internal/account/fake_account_manager.h" #include "internal/auth/auth_status_util.h" +#include "internal/auth/authentication_manager.h" #include "internal/network/http_client.h" #include "internal/network/http_request.h" #include "internal/network/http_response.h" #include "internal/network/http_status_code.h" #include "internal/network/url.h" +#include "internal/platform/device_info.h" +#include "internal/platform/task_runner.h" #include "internal/platform/task_runner_impl.h" +#include "internal/preferences/preferences_manager.h" #include "internal/test/fake_device_info.h" #include "internal/test/google3_only/fake_authentication_manager.h" @@ -89,11 +105,11 @@ class MockHttpClient : public HttpClient { public: MOCK_METHOD(void, StartRequest, (const HttpRequest& request, - std::function&)>), + absl::AnyInvocable&)>), (override)); MOCK_METHOD(void, StartCancellableRequest, (std::unique_ptr request, - std::function&)>), + absl::AnyInvocable&)>), (override)); MOCK_METHOD(absl::StatusOr, GetResponse, (const HttpRequest&), (override)); @@ -119,9 +135,10 @@ std::vector ExpectQueryStringValues( // A gMock matcher to match proto values. Use this matcher like: // request/response proto, expected_proto; // EXPECT_THAT(proto, MatchesProto(expected_proto)); -MATCHER_P(MatchesProto, expected_proto, - absl::StrCat(negation ? "does not match" : "matches", - testing::PrintToString(expected_proto.SerializeAsString()))) { +MATCHER_P( + MatchesProto, expected_proto, + absl::StrCat(negation ? "does not match" : "matches", + testing::PrintToString(expected_proto.SerializeAsString()))) { return arg.has_value() && arg->SerializeAsString() == expected_proto.SerializeAsString(); } diff --git a/internal/network/BUILD b/internal/network/BUILD index 9a1b24f5..52f6f3de 100644 --- a/internal/network/BUILD +++ b/internal/network/BUILD @@ -28,7 +28,9 @@ cc_library( ], deps = [ "//internal/platform:types", + "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/container:flat_hash_map", + "@com_google_absl//absl/functional:any_invocable", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", @@ -56,13 +58,12 @@ cc_library( deps = [ ":types", "//internal/platform:types", - "//internal/platform/implementation:platform", + "//internal/platform/implementation:comm", "@com_google_absl//absl/base:core_headers", - "@com_google_absl//absl/container:flat_hash_map", + "@com_google_absl//absl/functional:any_invocable", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", - "@com_google_absl//absl/strings:str_format", - "@com_google_absl//absl/synchronization", + "@com_google_absl//absl/strings", ], ) diff --git a/internal/network/http_client.h b/internal/network/http_client.h index 156ca091..4e2bbedc 100644 --- a/internal/network/http_client.h +++ b/internal/network/http_client.h @@ -15,12 +15,15 @@ #ifndef THIRD_PARTY_NEARBY_INTERNAL_NETWORK_HTTP_CLIENT_H_ #define THIRD_PARTY_NEARBY_INTERNAL_NETWORK_HTTP_CLIENT_H_ -#include #include +#include "absl/base/thread_annotations.h" +#include "absl/functional/any_invocable.h" +#include "absl/status/status.h" #include "absl/status/statusor.h" #include "internal/network/http_request.h" #include "internal/network/http_response.h" +#include "internal/platform/mutex.h" #include "internal/platform/mutex_lock.h" namespace nearby { @@ -60,12 +63,14 @@ class HttpClient { // Starts HTTP request in asynchronization mode. virtual void StartRequest( const HttpRequest& request, - std::function&)> callback) = 0; + absl::AnyInvocable&)> + callback) = 0; // Starts cancellable request in asynchronization mode. virtual void StartCancellableRequest( std::unique_ptr request, - std::function&)> callback) = 0; + absl::AnyInvocable&)> + callback) = 0; // Gets HTTP response in synchronization mode. virtual absl::StatusOr GetResponse( diff --git a/internal/network/http_client_impl.cc b/internal/network/http_client_impl.cc index 9a59c769..53174e5e 100644 --- a/internal/network/http_client_impl.cc +++ b/internal/network/http_client_impl.cc @@ -14,14 +14,20 @@ #include "internal/network/http_client_impl.h" -#include #include #include #include #include +#include "absl/functional/any_invocable.h" #include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "absl/strings/str_cat.h" #include "internal/network/debug.h" +#include "internal/network/http_request.h" +#include "internal/network/http_response.h" +#include "internal/network/http_status_code.h" +#include "internal/platform/implementation/http_loader.h" #include "internal/platform/logging.h" #include "internal/platform/mutex_lock.h" #include "internal/platform/single_thread_executor.h" @@ -31,10 +37,10 @@ namespace network { void NearbyHttpClient::StartRequest( const HttpRequest& request, - std::function&)> callback) { + absl::AnyInvocable&)> callback) { MutexLock lock(&mutex_); executor_.Execute( - [request = std::move(request), callback = std::move(callback)]() { + [request = std::move(request), callback = std::move(callback)]() mutable { NEARBY_LOGS(INFO) << __func__ << ": Start async request to url=" << request.GetUrl().GetUrlPath(); absl::StatusOr response = InternalGetResponse(request); @@ -58,7 +64,7 @@ void NearbyHttpClient::StartRequest( void NearbyHttpClient::StartCancellableRequest( std::unique_ptr cancellable_request, - std::function&)> callback) { + absl::AnyInvocable&)> callback) { MutexLock lock(&mutex_); if (cancellable_request == nullptr) { NEARBY_LOGS(ERROR) << __func__ << ": invalid cancellable request."; @@ -68,7 +74,7 @@ void NearbyHttpClient::StartCancellableRequest( executor_ .Execute( [cancellable_request = std::move(cancellable_request), - callback = std::move(callback)]() { + callback = std::move(callback)]() mutable { NEARBY_LOGS(INFO) << __func__ << ": Start async request to url=" << cancellable_request->http_request().GetUrl().GetUrlPath(); diff --git a/internal/network/http_client_impl.h b/internal/network/http_client_impl.h index 904c6ced..f10f245b 100644 --- a/internal/network/http_client_impl.h +++ b/internal/network/http_client_impl.h @@ -37,13 +37,14 @@ class NearbyHttpClient : public HttpClient { NearbyHttpClient(NearbyHttpClient&&) = default; NearbyHttpClient& operator=(NearbyHttpClient&&) = default; - void StartRequest(const HttpRequest& request, - std::function&)> - callback) override ABSL_LOCKS_EXCLUDED(mutex_); + void StartRequest( + const HttpRequest& request, + absl::AnyInvocable&)> callback) + override ABSL_LOCKS_EXCLUDED(mutex_); void StartCancellableRequest( std::unique_ptr request, - std::function&)> callback) + absl::AnyInvocable&)> callback) override ABSL_LOCKS_EXCLUDED(mutex_); // Gets HTTP response in synchronization mode. diff --git a/internal/test/fake_http_client.h b/internal/test/fake_http_client.h index 14e04a80..000143af 100644 --- a/internal/test/fake_http_client.h +++ b/internal/test/fake_http_client.h @@ -17,13 +17,13 @@ #include -#include #include #include #include #include #include +#include "absl/functional/any_invocable.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" @@ -39,7 +39,7 @@ class FakeHttpClient : public HttpClient { public: struct RequestInfo { HttpRequest request; - std::function&)> callback; + absl::AnyInvocable&)> callback; }; FakeHttpClient() = default; @@ -51,22 +51,23 @@ class FakeHttpClient : public HttpClient { FakeHttpClient(FakeHttpClient&&) = default; FakeHttpClient& operator=(FakeHttpClient&&) = default; - void StartRequest(const HttpRequest& request, - std::function&)> - callback) override { + void StartRequest( + const HttpRequest& request, + absl::AnyInvocable&)> callback) + override { RequestInfo request_info; request_info.request = request; - request_info.callback = callback; + request_info.callback = std::move(callback); request_infos_.push_back(std::move(request_info)); } void StartCancellableRequest( std::unique_ptr request, - std::function&)> callback) + absl::AnyInvocable&)> callback) override { RequestInfo request_info; request_info.request = request->http_request(); - request_info.callback = callback; + request_info.callback = std::move(callback); request_infos_.push_back(std::move(request_info)); } @@ -90,8 +91,8 @@ class FakeHttpClient : public HttpClient { if (pos >= request_infos_.size()) { return; } - auto request_info = request_infos_.at(pos); - if (request_info.callback != nullptr) { + auto& request_info = request_infos_.at(pos); + if (request_info.callback) { request_info.callback(response); }