From 32f500ec7f6901b7a58b71ab515722e37b95ec15 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 16 Mar 2018 12:15:43 -0400 Subject: [PATCH 1/4] Change CredentialsProvider::TokenListener to use StatusOr Rather than a token plus error code/msg. Note that this is a direct conversion. In particular, it is still possible to have a token that isn't valid, i.e. this is still possible: ``` void foo(StatusOr token) { assert(token.ok()); Token& t = token.ValueOrDie(); assert(!t.is_valid()); } ``` --- Firestore/Source/Remote/FSTDatastore.mm | 11 +++++----- Firestore/Source/Remote/FSTStream.mm | 13 +++++------ .../firestore/auth/credentials_provider.h | 8 ++----- .../auth/empty_credentials_provider.cc | 2 +- .../firebase_credentials_provider_apple.mm | 22 ++++++++++++++----- .../src/firebase/firestore/util/error_apple.h | 5 +++++ .../auth/credentials_provider_test.cc | 6 ++--- .../auth/empty_credentials_provider_test.cc | 17 +++++++++----- .../firebase_credentials_provider_test.mm | 15 ++++++------- 9 files changed, 55 insertions(+), 44 deletions(-) diff --git a/Firestore/Source/Remote/FSTDatastore.mm b/Firestore/Source/Remote/FSTDatastore.mm index e63017ae77b..b5ef00f4d69 100644 --- a/Firestore/Source/Remote/FSTDatastore.mm +++ b/Firestore/Source/Remote/FSTDatastore.mm @@ -305,20 +305,19 @@ - (void)invokeRPCWithFactory:(GRPCProtoCall * (^)(void))rpcFactory // TODO(mikelehen): We should force a refresh if the previous RPC failed due to an expired token, // but I'm not sure how to detect that right now. http://b/32762461 _credentials->GetToken( - /*force_refresh=*/false, - [self, rpcFactory, errorHandler](Token result, const int64_t error_code, - const absl::string_view error_msg) { - NSError *error = util::WrapNSError(error_code, error_msg); + /*force_refresh=*/false, [self, rpcFactory, errorHandler](util::StatusOr result) { + NSError *error = util::WrapNSError(result.status()); [self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{ if (error) { errorHandler(error); } else { GRPCProtoCall *rpc = rpcFactory(); + const Token &token = result.ValueOrDie(); [FSTDatastore prepareHeadersForRPC:rpc databaseID:&self.databaseInfo->database_id() - token:(result.user().is_authenticated() ? result.token() - : absl::string_view())]; + token:(token.user().is_authenticated() ? token.token() + : absl::string_view())]; [rpc start]; } }]; diff --git a/Firestore/Source/Remote/FSTStream.mm b/Firestore/Source/Remote/FSTStream.mm index 019b0bcc339..87da0c62450 100644 --- a/Firestore/Source/Remote/FSTStream.mm +++ b/Firestore/Source/Remote/FSTStream.mm @@ -265,17 +265,15 @@ - (void)startWithDelegate:(id)delegate { _delegate = delegate; _credentials->GetToken( - /*force_refresh=*/false, - [self](Token result, const int64_t error_code, const absl::string_view error_msg) { - NSError *error = util::WrapNSError(error_code, error_msg); + /*force_refresh=*/false, [self](util::StatusOr result) { [self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{ - [self resumeStartWithToken:result error:error]; + [self resumeStartWithToken:result]; }]; }); } /** Add an access token to our RPC, after obtaining one from the credentials provider. */ -- (void)resumeStartWithToken:(const Token &)token error:(NSError *)error { +- (void)resumeStartWithToken:(const util::StatusOr &)result { [self.workerDispatchQueue verifyIsCurrentQueue]; if (self.state == FSTStreamStateStopped) { @@ -287,9 +285,9 @@ - (void)resumeStartWithToken:(const Token &)token error:(NSError *)error { // TODO(mikelehen): We should force a refresh if the previous RPC failed due to an expired token, // but I'm not sure how to detect that right now. http://b/32762461 - if (error) { + if (!result.ok()) { // RPC has not been started yet, so just invoke higher-level close handler. - [self handleStreamClose:error]; + [self handleStreamClose:util::WrapNSError(result.status())]; return; } @@ -297,6 +295,7 @@ - (void)resumeStartWithToken:(const Token &)token error:(NSError *)error { _rpc = [self createRPCWithRequestsWriter:self.requestsWriter]; [_rpc setResponseDispatchQueue:self.workerDispatchQueue.queue]; + const Token &token = result.ValueOrDie(); [FSTDatastore prepareHeadersForRPC:_rpc databaseID:&self.databaseInfo->database_id() diff --git a/Firestore/core/src/firebase/firestore/auth/credentials_provider.h b/Firestore/core/src/firebase/firestore/auth/credentials_provider.h index b9a8a24a86a..1aa76df70ad 100644 --- a/Firestore/core/src/firebase/firestore/auth/credentials_provider.h +++ b/Firestore/core/src/firebase/firestore/auth/credentials_provider.h @@ -23,6 +23,7 @@ #include "Firestore/core/include/firebase/firestore/firestore_errors.h" #include "Firestore/core/src/firebase/firestore/auth/token.h" #include "Firestore/core/src/firebase/firestore/auth/user.h" +#include "Firestore/core/src/firebase/firestore/util/statusor.h" #include "absl/strings/string_view.h" namespace firebase { @@ -30,12 +31,7 @@ namespace firestore { namespace auth { // `TokenErrorListener` is a listener that gets a token or an error. -// token: An auth token as a string, or nullptr if error occurred. -// error_code: The error code if one occurred, or else FirestoreErrorCode::Ok. -// error_msg: The error if one occurred, or else nullptr. -typedef std::function - TokenListener; +typedef std::function)> TokenListener; // Listener notified with a User change. typedef std::function UserChangeListener; diff --git a/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc b/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc index 0fa70c0540f..4c7aacba990 100644 --- a/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc +++ b/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc @@ -27,7 +27,7 @@ void EmptyCredentialsProvider::GetToken(bool force_refresh, UNUSED(force_refresh); if (completion) { // Invalid token will force the GRPC fallback to use default settings. - completion(Token::Invalid(), FirestoreErrorCode::Ok, ""); + completion(Token::Invalid()); } } diff --git a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm b/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm index 1babe82d206..7f7aa08d277 100644 --- a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm +++ b/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm @@ -23,6 +23,9 @@ #include "Firestore/core/src/firebase/firestore/util/firebase_assert.h" #include "Firestore/core/src/firebase/firestore/util/string_apple.h" +// NB: This is also defined in Firestore/Source/Public/FIRFirestoreErrors.h +NSString* const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain"; + namespace firebase { namespace firestore { namespace auth { @@ -96,13 +99,20 @@ // Cancel the request since the user changed while the request was // outstanding so the response is likely for a previous user (which // user, we can't be sure). - completion(Token::Invalid(), FirestoreErrorCode::Aborted, - "getToken aborted due to user change."); + completion(util::Status(FirestoreErrorCode::Aborted, + "getToken aborted due to user change.")); } else { - completion( - Token{util::MakeStringView(token), contents->current_user}, - error == nil ? FirestoreErrorCode::Ok : error.code, - error == nil ? "" : util::MakeStringView(error.localizedDescription)); + if (error == nil) { + completion(Token{util::MakeStringView(token), contents->current_user}); + } else { + FirestoreErrorCode error_code = FirestoreErrorCode::Unknown; + if (error.domain == FIRFirestoreErrorDomain) { + error_code = static_cast(error.code); + } + completion( + util::Status(FirestoreErrorCode::Unknown, + util::MakeStringView(error.localizedDescription))); + } } }; diff --git a/Firestore/core/src/firebase/firestore/util/error_apple.h b/Firestore/core/src/firebase/firestore/util/error_apple.h index e31cfd6df9c..c1a191c4417 100644 --- a/Firestore/core/src/firebase/firestore/util/error_apple.h +++ b/Firestore/core/src/firebase/firestore/util/error_apple.h @@ -24,6 +24,7 @@ #include "Firestore/Source/Public/FIRFirestoreErrors.h" // for FIRFirestoreErrorDomain #include "Firestore/core/include/firebase/firestore/firestore_errors.h" +#include "Firestore/core/src/firebase/firestore/util/status.h" #include "Firestore/core/src/firebase/firestore/util/string_apple.h" #include "absl/strings/string_view.h" @@ -43,6 +44,10 @@ inline NSError* WrapNSError(const int64_t error_code, userInfo:@{NSLocalizedDescriptionKey : WrapNSString(error_msg)}]; } +inline NSError* WrapNSError(const util::Status& status) { + return WrapNSError(status.code(), status.error_message()); +} + } // namespace util } // namespace firestore } // namespace firebase diff --git a/Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc b/Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc index 6895061b16d..69c3deff4cb 100644 --- a/Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc +++ b/Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc @@ -16,6 +16,7 @@ #include "Firestore/core/src/firebase/firestore/auth/credentials_provider.h" +#include "Firestore/core/src/firebase/firestore/util/statusor.h" #include "gtest/gtest.h" namespace firebase { @@ -25,11 +26,8 @@ namespace auth { #define UNUSED(x) (void)(x) TEST(CredentialsProvider, Typedef) { - TokenListener token_listener = [](Token token, const int64_t error_code, - const absl::string_view error_msg) { + TokenListener token_listener = [](util::StatusOr token) { UNUSED(token); - UNUSED(error_code); - UNUSED(error_msg); }; EXPECT_NE(nullptr, token_listener); EXPECT_TRUE(token_listener); diff --git a/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc b/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc index 3b487f38afd..f0ee23d2cf2 100644 --- a/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc +++ b/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc @@ -16,6 +16,7 @@ #include "Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.h" +#include "Firestore/core/src/firebase/firestore/util/statusor.h" #include "gtest/gtest.h" namespace firebase { @@ -25,14 +26,18 @@ namespace auth { TEST(EmptyCredentialsProvider, GetToken) { EmptyCredentialsProvider credentials_provider; credentials_provider.GetToken( - /*force_refresh=*/true, [](Token token, const int64_t error_code, - const absl::string_view error_msg) { - EXPECT_FALSE(token.is_valid()); - const User& user = token.user(); + /*force_refresh=*/true, [](util::StatusOr token) { + EXPECT_TRUE(token.ok()); + EXPECT_FALSE(token.ValueOrDie().is_valid()); + const User& user = token.ValueOrDie().user(); EXPECT_EQ("", user.uid()); EXPECT_FALSE(user.is_authenticated()); - EXPECT_EQ(FirestoreErrorCode::Ok, error_code); - EXPECT_EQ("", error_msg); + // TODO(rsgowman): the next two statements just test the implementation + // of StatusOr, so aren't useful here anymore. However, I'm keeping them + // for one more commit, since it will shortly not be possible for a + // StatusOr to be ok() while the token itself is not is_valid() + EXPECT_EQ(FirestoreErrorCode::Ok, token.status().code()); + EXPECT_EQ("", token.status().error_message()); }); } diff --git a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm b/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm index 3660d53f9f3..c2fd7a36753 100644 --- a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm +++ b/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm @@ -20,6 +20,7 @@ #import #import +#include "Firestore/core/src/firebase/firestore/util/statusor.h" #include "Firestore/core/src/firebase/firestore/util/string_apple.h" #include "Firestore/core/test/firebase/firestore/testutil/app_testing.h" @@ -42,14 +43,13 @@ FirebaseCredentialsProvider credentials_provider(app); credentials_provider.GetToken( - /*force_refresh=*/true, [](Token token, const int64_t error_code, - const absl::string_view error_msg) { + /*force_refresh=*/true, [](util::StatusOr result) { + EXPECT_TRUE(result.ok()); + const Token& token = result.ValueOrDie(); EXPECT_EQ("", token.token()); const User& user = token.user(); EXPECT_EQ("", user.uid()); EXPECT_FALSE(user.is_authenticated()); - EXPECT_EQ(FirestoreErrorCode::Ok, error_code) << error_code; - EXPECT_EQ("", error_msg) << error_msg; }); } @@ -58,14 +58,13 @@ FirebaseCredentialsProvider credentials_provider(app); credentials_provider.GetToken( - /*force_refresh=*/true, [](Token token, const int64_t error_code, - const absl::string_view error_msg) { + /*force_refresh=*/true, [](util::StatusOr result) { + EXPECT_TRUE(result.ok()); + const Token& token = result.ValueOrDie(); EXPECT_EQ("", token.token()); const User& user = token.user(); EXPECT_EQ("fake uid", user.uid()); EXPECT_TRUE(user.is_authenticated()); - EXPECT_EQ(FirestoreErrorCode::Ok, error_code) << error_code; - EXPECT_EQ("", error_msg) << error_msg; }); } From 9e01b414477924d788f208d0978d0d0954faca61 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Mon, 19 Mar 2018 17:24:47 -0400 Subject: [PATCH 2/4] Eliminate the concept of an invalid Token Instead, we'll just use StatusOr. Note that unauthenticated tokens are handled as a special case; they're created via: Token::Unauthenticated() and are otherwise "valid", though attempting to retrieve the raw token on one of these tokens will cause an assertion failure. --- .../auth/empty_credentials_provider.cc | 4 ++-- .../core/src/firebase/firestore/auth/token.cc | 10 +++++----- .../core/src/firebase/firestore/auth/token.h | 18 +++--------------- .../auth/empty_credentials_provider_test.cc | 15 +++++---------- .../auth/firebase_credentials_provider_test.mm | 2 +- .../test/firebase/firestore/auth/token_test.cc | 6 ++---- 6 files changed, 18 insertions(+), 37 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc b/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc index 4c7aacba990..9c826c52dbc 100644 --- a/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc +++ b/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc @@ -26,8 +26,8 @@ void EmptyCredentialsProvider::GetToken(bool force_refresh, TokenListener completion) { UNUSED(force_refresh); if (completion) { - // Invalid token will force the GRPC fallback to use default settings. - completion(Token::Invalid()); + // Unauthenticated token will force the GRPC fallback to use default settings. + completion(Token::Unauthenticated()); } } diff --git a/Firestore/core/src/firebase/firestore/auth/token.cc b/Firestore/core/src/firebase/firestore/auth/token.cc index 4ee1b698e85..27e63ad50a1 100644 --- a/Firestore/core/src/firebase/firestore/auth/token.cc +++ b/Firestore/core/src/firebase/firestore/auth/token.cc @@ -21,15 +21,15 @@ namespace firestore { namespace auth { Token::Token(const absl::string_view token, const User& user) - : token_(token), user_(user), is_valid_(true) { + : token_(token), user_(user) { } -Token::Token() : token_(), user_(User::Unauthenticated()), is_valid_(false) { +Token::Token() : token_(), user_(User::Unauthenticated()) { } -const Token& Token::Invalid() { - static const Token kInvalidToken; - return kInvalidToken; +const Token& Token::Unauthenticated() { + static const Token kUnauthenticatedToken; + return kUnauthenticatedToken; } } // namespace auth diff --git a/Firestore/core/src/firebase/firestore/auth/token.h b/Firestore/core/src/firebase/firestore/auth/token.h index ff8d2f02f6e..00252b9e994 100644 --- a/Firestore/core/src/firebase/firestore/auth/token.h +++ b/Firestore/core/src/firebase/firestore/auth/token.h @@ -46,7 +46,7 @@ class Token { /** The actual raw token. */ const std::string& token() const { - FIREBASE_ASSERT(is_valid_); + FIREBASE_ASSERT(user_.is_authenticated()); return token_; } @@ -58,26 +58,14 @@ class Token { return user_; } - /** - * Whether the token is a valid one. - * - * ## Portability notes: Invalid token is the equivalent of nil in the iOS - * token implementation. We use value instead of pointer for Token instance in - * the C++ migration. - */ - bool is_valid() const { - return is_valid_; - } - - /** Returns an invalid token. */ - static const Token& Invalid(); + /** Returns a token for an unauthenticated user. */ + static const Token& Unauthenticated(); private: Token(); const std::string token_; const User user_; - const bool is_valid_; }; } // namespace auth diff --git a/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc b/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc index f0ee23d2cf2..97df6ad0c35 100644 --- a/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc +++ b/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc @@ -26,18 +26,13 @@ namespace auth { TEST(EmptyCredentialsProvider, GetToken) { EmptyCredentialsProvider credentials_provider; credentials_provider.GetToken( - /*force_refresh=*/true, [](util::StatusOr token) { - EXPECT_TRUE(token.ok()); - EXPECT_FALSE(token.ValueOrDie().is_valid()); - const User& user = token.ValueOrDie().user(); + /*force_refresh=*/true, [](util::StatusOr result) { + EXPECT_TRUE(result.ok()); + const Token& token = result.ValueOrDie(); + EXPECT_ANY_THROW(token.token()); + const User& user = result.ValueOrDie().user(); EXPECT_EQ("", user.uid()); EXPECT_FALSE(user.is_authenticated()); - // TODO(rsgowman): the next two statements just test the implementation - // of StatusOr, so aren't useful here anymore. However, I'm keeping them - // for one more commit, since it will shortly not be possible for a - // StatusOr to be ok() while the token itself is not is_valid() - EXPECT_EQ(FirestoreErrorCode::Ok, token.status().code()); - EXPECT_EQ("", token.status().error_message()); }); } diff --git a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm b/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm index c2fd7a36753..de2e419e246 100644 --- a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm +++ b/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm @@ -46,7 +46,7 @@ /*force_refresh=*/true, [](util::StatusOr result) { EXPECT_TRUE(result.ok()); const Token& token = result.ValueOrDie(); - EXPECT_EQ("", token.token()); + EXPECT_ANY_THROW(token.token()); const User& user = token.user(); EXPECT_EQ("", user.uid()); EXPECT_FALSE(user.is_authenticated()); diff --git a/Firestore/core/test/firebase/firestore/auth/token_test.cc b/Firestore/core/test/firebase/firestore/auth/token_test.cc index 8f784d65197..e34053e5ccb 100644 --- a/Firestore/core/test/firebase/firestore/auth/token_test.cc +++ b/Firestore/core/test/firebase/firestore/auth/token_test.cc @@ -26,14 +26,12 @@ TEST(Token, Getter) { Token token("token", User("abc")); EXPECT_EQ("token", token.token()); EXPECT_EQ(User("abc"), token.user()); - EXPECT_TRUE(token.is_valid()); } -TEST(Token, InvalidToken) { - const Token& token = Token::Invalid(); +TEST(Token, UnauthenticatedToken) { + const Token& token = Token::Unauthenticated(); EXPECT_ANY_THROW(token.token()); EXPECT_EQ(User::Unauthenticated(), token.user()); - EXPECT_FALSE(token.is_valid()); } } // namespace auth From c8faf265950f5e2bdf97faa5e2654dc3bc1463f4 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 20 Mar 2018 15:42:32 -0400 Subject: [PATCH 3/4] review feedback --- Firestore/Source/Remote/FSTDatastore.mm | 5 ++--- Firestore/Source/Remote/FSTStream.mm | 2 +- .../auth/firebase_credentials_provider_apple.mm | 8 ++++++-- .../core/src/firebase/firestore/auth/token.cc | 5 +---- .../core/src/firebase/firestore/auth/token.h | 10 +++++++--- .../src/firebase/firestore/util/error_apple.h | 6 +++--- .../auth/empty_credentials_provider_test.cc | 2 +- .../auth/firebase_credentials_provider_test.mm | 15 ++++++++++++--- 8 files changed, 33 insertions(+), 20 deletions(-) diff --git a/Firestore/Source/Remote/FSTDatastore.mm b/Firestore/Source/Remote/FSTDatastore.mm index b5ef00f4d69..63499dfa13e 100644 --- a/Firestore/Source/Remote/FSTDatastore.mm +++ b/Firestore/Source/Remote/FSTDatastore.mm @@ -306,10 +306,9 @@ - (void)invokeRPCWithFactory:(GRPCProtoCall * (^)(void))rpcFactory // but I'm not sure how to detect that right now. http://b/32762461 _credentials->GetToken( /*force_refresh=*/false, [self, rpcFactory, errorHandler](util::StatusOr result) { - NSError *error = util::WrapNSError(result.status()); [self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{ - if (error) { - errorHandler(error); + if (!result.ok()) { + errorHandler(util::MakeNSError(result.status())); } else { GRPCProtoCall *rpc = rpcFactory(); const Token &token = result.ValueOrDie(); diff --git a/Firestore/Source/Remote/FSTStream.mm b/Firestore/Source/Remote/FSTStream.mm index 87da0c62450..a5c36c8fd89 100644 --- a/Firestore/Source/Remote/FSTStream.mm +++ b/Firestore/Source/Remote/FSTStream.mm @@ -287,7 +287,7 @@ - (void)resumeStartWithToken:(const util::StatusOr &)result { // but I'm not sure how to detect that right now. http://b/32762461 if (!result.ok()) { // RPC has not been started yet, so just invoke higher-level close handler. - [self handleStreamClose:util::WrapNSError(result.status())]; + [self handleStreamClose:util::MakeNSError(result.status())]; return; } diff --git a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm b/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm index 7f7aa08d277..d7edf32d9ce 100644 --- a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm +++ b/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm @@ -103,14 +103,18 @@ "getToken aborted due to user change.")); } else { if (error == nil) { - completion(Token{util::MakeStringView(token), contents->current_user}); + if (token != nil) { + completion(Token{util::MakeStringView(token), contents->current_user}); + } else { + completion(Token::Unauthenticated()); + } } else { FirestoreErrorCode error_code = FirestoreErrorCode::Unknown; if (error.domain == FIRFirestoreErrorDomain) { error_code = static_cast(error.code); } completion( - util::Status(FirestoreErrorCode::Unknown, + util::Status(error_code, util::MakeStringView(error.localizedDescription))); } } diff --git a/Firestore/core/src/firebase/firestore/auth/token.cc b/Firestore/core/src/firebase/firestore/auth/token.cc index 27e63ad50a1..0e64909b944 100644 --- a/Firestore/core/src/firebase/firestore/auth/token.cc +++ b/Firestore/core/src/firebase/firestore/auth/token.cc @@ -24,11 +24,8 @@ Token::Token(const absl::string_view token, const User& user) : token_(token), user_(user) { } -Token::Token() : token_(), user_(User::Unauthenticated()) { -} - const Token& Token::Unauthenticated() { - static const Token kUnauthenticatedToken; + static const Token kUnauthenticatedToken(absl::string_view(), User::Unauthenticated()); return kUnauthenticatedToken; } diff --git a/Firestore/core/src/firebase/firestore/auth/token.h b/Firestore/core/src/firebase/firestore/auth/token.h index 00252b9e994..4b2f3aa78f6 100644 --- a/Firestore/core/src/firebase/firestore/auth/token.h +++ b/Firestore/core/src/firebase/firestore/auth/token.h @@ -58,12 +58,16 @@ class Token { return user_; } - /** Returns a token for an unauthenticated user. */ + /** + * Returns a token for an unauthenticated user. + * + * ## Portability notes: An unauthenticated token is the equivalent of + * nil/null in the iOS/TypeScript token implementation. We use a reference + * instead of a pointer for Token instances in the C++ migration. + */ static const Token& Unauthenticated(); private: - Token(); - const std::string token_; const User user_; }; diff --git a/Firestore/core/src/firebase/firestore/util/error_apple.h b/Firestore/core/src/firebase/firestore/util/error_apple.h index c1a191c4417..e7c77c90cf3 100644 --- a/Firestore/core/src/firebase/firestore/util/error_apple.h +++ b/Firestore/core/src/firebase/firestore/util/error_apple.h @@ -33,7 +33,7 @@ namespace firestore { namespace util { // Translates a set of error_code and error_msg to an NSError. -inline NSError* WrapNSError(const int64_t error_code, +inline NSError* MakeNSError(const int64_t error_code, const absl::string_view error_msg) { if (error_code == FirestoreErrorCode::Ok) { return nil; @@ -44,8 +44,8 @@ inline NSError* WrapNSError(const int64_t error_code, userInfo:@{NSLocalizedDescriptionKey : WrapNSString(error_msg)}]; } -inline NSError* WrapNSError(const util::Status& status) { - return WrapNSError(status.code(), status.error_message()); +inline NSError* MakeNSError(const util::Status& status) { + return MakeNSError(status.code(), status.error_message()); } } // namespace util diff --git a/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc b/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc index 97df6ad0c35..60845e5c5b7 100644 --- a/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc +++ b/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc @@ -30,7 +30,7 @@ TEST(EmptyCredentialsProvider, GetToken) { EXPECT_TRUE(result.ok()); const Token& token = result.ValueOrDie(); EXPECT_ANY_THROW(token.token()); - const User& user = result.ValueOrDie().user(); + const User& user = token.user(); EXPECT_EQ("", user.uid()); EXPECT_FALSE(user.is_authenticated()); }); diff --git a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm b/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm index de2e419e246..5825ade0f01 100644 --- a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm +++ b/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm @@ -17,6 +17,7 @@ #include "Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.h" #import + #import #import @@ -30,14 +31,22 @@ namespace firestore { namespace auth { -FIRApp* AppWithFakeUid(NSString* _Nullable uid) { +FIRApp* AppWithFakeUidAndToken(NSString* _Nullable uid, NSString* _Nullable token) { FIRApp* app = testutil::AppForUnitTesting(); app.getUIDImplementation = ^NSString* { return uid; }; + app.getTokenImplementation = ^(BOOL, FIRTokenCallback callback) { + callback(token, nil); + }; return app; } +FIRApp* AppWithFakeUid(NSString* _Nullable uid) { + return AppWithFakeUidAndToken(uid, uid == nil ? nil : @"default token"); +} + + TEST(FirebaseCredentialsProviderTest, GetTokenUnauthenticated) { FIRApp* app = AppWithFakeUid(nil); @@ -54,14 +63,14 @@ } TEST(FirebaseCredentialsProviderTest, GetToken) { - FIRApp* app = AppWithFakeUid(@"fake uid"); + FIRApp* app = AppWithFakeUidAndToken(@"fake uid", @"token for fake uid"); FirebaseCredentialsProvider credentials_provider(app); credentials_provider.GetToken( /*force_refresh=*/true, [](util::StatusOr result) { EXPECT_TRUE(result.ok()); const Token& token = result.ValueOrDie(); - EXPECT_EQ("", token.token()); + EXPECT_EQ("token for fake uid", token.token()); const User& user = token.user(); EXPECT_EQ("fake uid", user.uid()); EXPECT_TRUE(user.is_authenticated()); From dc61654a46b3d089c0831c7ae90ba0dce1de3de4 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 20 Mar 2018 15:44:43 -0400 Subject: [PATCH 4/4] style.sh --- .../auth/empty_credentials_provider.cc | 3 +- .../firebase_credentials_provider_apple.mm | 60 +++++++++---------- .../core/src/firebase/firestore/auth/token.cc | 3 +- .../firebase_credentials_provider_test.mm | 4 +- 4 files changed, 36 insertions(+), 34 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc b/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc index 9c826c52dbc..da4198d19ab 100644 --- a/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc +++ b/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc @@ -26,7 +26,8 @@ void EmptyCredentialsProvider::GetToken(bool force_refresh, TokenListener completion) { UNUSED(force_refresh); if (completion) { - // Unauthenticated token will force the GRPC fallback to use default settings. + // Unauthenticated token will force the GRPC fallback to use default + // settings. completion(Token::Unauthenticated()); } } diff --git a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm b/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm index d7edf32d9ce..2bd3accbc91 100644 --- a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm +++ b/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm @@ -87,38 +87,38 @@ int initial_user_counter = contents_->user_counter; std::weak_ptr weak_contents = contents_; - void (^get_token_callback)(NSString*, NSError*) = ^( - NSString* _Nullable token, NSError* _Nullable error) { - std::shared_ptr contents = weak_contents.lock(); - if (!contents) { - return; - } - - std::unique_lock lock(contents->mutex); - if (initial_user_counter != contents->user_counter) { - // Cancel the request since the user changed while the request was - // outstanding so the response is likely for a previous user (which - // user, we can't be sure). - completion(util::Status(FirestoreErrorCode::Aborted, - "getToken aborted due to user change.")); - } else { - if (error == nil) { - if (token != nil) { - completion(Token{util::MakeStringView(token), contents->current_user}); - } else { - completion(Token::Unauthenticated()); + void (^get_token_callback)(NSString*, NSError*) = + ^(NSString* _Nullable token, NSError* _Nullable error) { + std::shared_ptr contents = weak_contents.lock(); + if (!contents) { + return; } - } else { - FirestoreErrorCode error_code = FirestoreErrorCode::Unknown; - if (error.domain == FIRFirestoreErrorDomain) { - error_code = static_cast(error.code); + + std::unique_lock lock(contents->mutex); + if (initial_user_counter != contents->user_counter) { + // Cancel the request since the user changed while the request was + // outstanding so the response is likely for a previous user (which + // user, we can't be sure). + completion(util::Status(FirestoreErrorCode::Aborted, + "getToken aborted due to user change.")); + } else { + if (error == nil) { + if (token != nil) { + completion( + Token{util::MakeStringView(token), contents->current_user}); + } else { + completion(Token::Unauthenticated()); + } + } else { + FirestoreErrorCode error_code = FirestoreErrorCode::Unknown; + if (error.domain == FIRFirestoreErrorDomain) { + error_code = static_cast(error.code); + } + completion(util::Status( + error_code, util::MakeStringView(error.localizedDescription))); + } } - completion( - util::Status(error_code, - util::MakeStringView(error.localizedDescription))); - } - } - }; + }; [contents_->app getTokenForcingRefresh:force_refresh withCallback:get_token_callback]; diff --git a/Firestore/core/src/firebase/firestore/auth/token.cc b/Firestore/core/src/firebase/firestore/auth/token.cc index 0e64909b944..6fe5fc48ea2 100644 --- a/Firestore/core/src/firebase/firestore/auth/token.cc +++ b/Firestore/core/src/firebase/firestore/auth/token.cc @@ -25,7 +25,8 @@ Token::Token(const absl::string_view token, const User& user) } const Token& Token::Unauthenticated() { - static const Token kUnauthenticatedToken(absl::string_view(), User::Unauthenticated()); + static const Token kUnauthenticatedToken(absl::string_view(), + User::Unauthenticated()); return kUnauthenticatedToken; } diff --git a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm b/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm index 5825ade0f01..9d358b56577 100644 --- a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm +++ b/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm @@ -31,7 +31,8 @@ namespace firestore { namespace auth { -FIRApp* AppWithFakeUidAndToken(NSString* _Nullable uid, NSString* _Nullable token) { +FIRApp* AppWithFakeUidAndToken(NSString* _Nullable uid, + NSString* _Nullable token) { FIRApp* app = testutil::AppForUnitTesting(); app.getUIDImplementation = ^NSString* { return uid; @@ -46,7 +47,6 @@ return AppWithFakeUidAndToken(uid, uid == nil ? nil : @"default token"); } - TEST(FirebaseCredentialsProviderTest, GetTokenUnauthenticated) { FIRApp* app = AppWithFakeUid(nil);