-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
replacing FSTGetTokenResult by C++ Token implementation #805
Changes from all commits
dc4f02c
b502b8e
bfbb0e2
a64ca8c
9fb7911
369f812
6e4fdef
5cb11ca
6775d9b
7d10621
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,11 +35,13 @@ | |
|
||
#import "Firestore/Protos/objc/google/firestore/v1beta1/Firestore.pbrpc.h" | ||
|
||
#include "Firestore/core/src/firebase/firestore/auth/token.h" | ||
#include "Firestore/core/src/firebase/firestore/core/database_info.h" | ||
#include "Firestore/core/src/firebase/firestore/model/database_id.h" | ||
#include "Firestore/core/src/firebase/firestore/util/string_apple.h" | ||
|
||
namespace util = firebase::firestore::util; | ||
using firebase::firestore::auth::Token; | ||
using firebase::firestore::core::DatabaseInfo; | ||
using firebase::firestore::model::DatabaseId; | ||
|
||
|
@@ -301,16 +303,18 @@ - (void)invokeRPCWithFactory:(GRPCProtoCall * (^)(void))rpcFactory | |
// but I'm not sure how to detect that right now. http://b/32762461 | ||
[self.credentials | ||
getTokenForcingRefresh:NO | ||
completion:^(FSTGetTokenResult *_Nullable result, NSError *_Nullable error) { | ||
completion:^(Token result, NSError *_Nullable error) { | ||
error = [FSTDatastore firestoreErrorForError:error]; | ||
[self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{ | ||
if (error) { | ||
errorHandler(error); | ||
} else { | ||
GRPCProtoCall *rpc = rpcFactory(); | ||
[FSTDatastore prepareHeadersForRPC:rpc | ||
databaseID:&self.databaseInfo->database_id() | ||
token:result.token]; | ||
[FSTDatastore | ||
prepareHeadersForRPC:rpc | ||
databaseID:&self.databaseInfo->database_id() | ||
token:(result.is_valid() ? result.token() | ||
: absl::string_view())]; | ||
[rpc start]; | ||
} | ||
}]; | ||
|
@@ -334,8 +338,8 @@ - (FSTWriteStream *)createWriteStream { | |
/** Adds headers to the RPC including any OAuth access token if provided .*/ | ||
+ (void)prepareHeadersForRPC:(GRPCCall *)rpc | ||
databaseID:(const DatabaseId *)databaseID | ||
token:(nullable NSString *)token { | ||
rpc.oauth2AccessToken = token; | ||
token:(const absl::string_view)token { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (see comment in FSTDatastore.h:87) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
rpc.oauth2AccessToken = token.data() == nullptr ? nil : util::WrapNSString(token); | ||
rpc.requestHeaders[kXGoogAPIClientHeader] = [FSTDatastore googAPIClientHeaderValue]; | ||
// This header is used to improve routing and project isolation by the backend. | ||
rpc.requestHeaders[kGoogleCloudResourcePrefix] = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
#include <string> | ||
|
||
#include "Firestore/core/src/firebase/firestore/auth/user.h" | ||
#include "Firestore/core/src/firebase/firestore/util/firebase_assert.h" | ||
#include "absl/strings/string_view.h" | ||
|
||
namespace firebase { | ||
|
@@ -45,6 +46,7 @@ class Token { | |
|
||
/** The actual raw token. */ | ||
const std::string& token() const { | ||
FIREBASE_ASSERT(is_valid_); | ||
return token_; | ||
} | ||
|
||
|
@@ -56,9 +58,26 @@ 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. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a porting note since this concept of an invalid token doesn't exist elsewhere. Something to indicate why we need it, etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
static const Token& Invalid(); | ||
|
||
private: | ||
Token(); | ||
|
||
const std::string token_; | ||
const User user_; | ||
const bool is_valid_; | ||
}; | ||
|
||
} // namespace auth | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using
const*
if you want to allow nulls. Otherwise, adjust the docstring to state how to not provide a token. (An empty string_view might be a legit way to do this.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it accepts nullptr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note:
absl::string_view{nullptr}
is not valid (if I read what is meant here correctly), similar to howstd::string{nullptr}
is undefined behavior. It should be eitherabsl::string_view{nullptr, 0}
orabsl::NullSafeStringView(possibly_null_argument)
.