FCM V1 API Migration initial commit.#8087
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
📝 PRs merging into main branchOur main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released. |
|
The public api surface has changed for the subproject firebase-messaging: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
|
The public api surface has changed for the subproject firebase-messaging: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
1 similar comment
|
The public api surface has changed for the subproject firebase-messaging: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
|
The public api surface has changed for the subproject firebase-messaging: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
|
The public api surface has changed for the subproject firebase-messaging: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
1 similar comment
|
The public api surface has changed for the subproject firebase-messaging: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
| @Deprecated | ||
| @NonNull | ||
| public Task<String> getToken() { | ||
| if (iid != null) { |
There was a problem hiding this comment.
Is there a reason for removing the FirebaseInstanceId calls? It's long deprecated, but removing the compatibility when including that SDK is probably a breaking change and would need to be documented.
There was a problem hiding this comment.
It shouldn't be breaking change since IID and FCM uses same registration path IIUC and there will not be any functional change to developers using this api. How ever I added that back in getToken and deleteToken api methods.
There was a problem hiding this comment.
If it's using a pre-FIS FirebaseInstanceID SDK it would be breaking, but I don't remember if that's actually possible. But the main problem is that there is a possible race condition here if the FirebaseInstanceID SDK tries to register at the same time, I think.
| * Returns whether the existing token is a FID based on its length. | ||
| */ | ||
| boolean isFid() { | ||
| return token.length() <= 22; |
There was a problem hiding this comment.
Just, checking whether this is actually guaranteed in both cases (FID is always <=22 and FCM token is always > 22)?
There was a problem hiding this comment.
As per : https://goto.google.com/fis-terminology#heading=h.md5r8euvz4sf the FID is always 22 or less in length. Since FCM token is the concat of FID + encoded values, the length should be greater than 22.
There was a problem hiding this comment.
I'm leaning towards not leaking this internal detail to the public. Could we simply store a boolean to differentiate tokens from FIDs?
There was a problem hiding this comment.
IMO the FCM token and FID format is already known to public as they have access to these tokens in their app.
There was a problem hiding this comment.
Could you share the public reference for the FID format, if one exists? I believe that we've been telling developers that FCM tokens are opaque strings. I'm worried that if we leak this length detail and developers start relying on it, it will be difficult for FIS to change the FID length or format in the future. Ideally, developers should treat this as an opaque string and not rely on any of its characteristics. Even though we cannot prevent them from doing this, we should at least avoid setting an example for them.
Apart from that, the new iOS and web SDKs do not expose this detail. It's better to keep this consistent across all platforms
There was a problem hiding this comment.
How about refactoring tokenNeedsRefresh like this to avoid having this isFid() method?
boolean tokenNeedsRefresh(@Nullable Store.Token token) {
String fid = fetchFid();
if (gmsRegistrationClient.isV1RegistrationEnabled()) {
if (fid != null) {
// In V1 registration, if the current FID is not equal to the existing token, then the FCM
// registration needs refresh.
return token == null
|| !fid.equalsIgnoreCase(token.token)
|| token.needsRefresh(metadata.getAppVersionCode());
} else {
// if fid is unavailable, we should avoid returning the cached token in case it's not a fid
return true;
}
}
// The only issue is that when `token` is a FID but different from `fid`,
// this will return false. I think this is a minor issue considering this edge
// case only occurs when
// 1. the app is switched from v1 registration enabled to disabled
// 2. the app's fid happens to change after the config switch
// This case unlikely occurs in prod since the app cannot switch the mode without changing the app version code.
// This case also less likely happen in dev since 2. should be rare.
return token == null || token.needsRefresh(metadata.getAppVersionCode()) || token == fid;
}
If we really want a perfect solution, isFid() is still necessary. The easiest alternative could be to store a boolean (e.g. isFid) or an enum type (e.g. FID vs V4) in this Store class and update the SDK to store the type when a registration is saved to cache. This change could be easy with AI. (I used jetski to do Web SDK changes and it seems working great)
| executorService.execute( | ||
| () -> { | ||
| try { | ||
| String installationId = Tasks.await(firebaseInstallations.getId()); |
There was a problem hiding this comment.
I think this and other similar calls might be able to use a chain of continuations of the task, something like firebaseInstallations.getId().continueWithTask(...), which might simplify this somewhat.
There was a problem hiding this comment.
IIRC the continue with task was having issues like getting callback if the parent task fails. Thats why i went this way so that we can bubble the failure back to the caller.
There was a problem hiding this comment.
Do you need to get a callback if the previous Task fails? It should end up returning a failing Task at the end, I think.
|
The public api surface has changed for the subproject firebase-messaging: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
| // app the token is for. We could add a new method onNewToken(FirebaseApp app, String token) or | ||
| // the like to handle multiple apps better. |
There was a problem hiding this comment.
| // app the token is for. We could add a new method onNewToken(FirebaseApp app, String token) or | |
| // the like to handle multiple apps better. | |
| // app the token is for. |
nit: I think we won't support multiple apps any more.
| * Returns whether the existing token is a FID based on its length. | ||
| */ | ||
| boolean isFid() { | ||
| return token.length() <= 22; |
There was a problem hiding this comment.
Could you share the public reference for the FID format, if one exists? I believe that we've been telling developers that FCM tokens are opaque strings. I'm worried that if we leak this length detail and developers start relying on it, it will be difficult for FIS to change the FID length or format in the future. Ideally, developers should treat this as an opaque string and not rely on any of its characteristics. Even though we cannot prevent them from doing this, we should at least avoid setting an example for them.
Apart from that, the new iOS and web SDKs do not expose this detail. It's better to keep this consistent across all platforms
| public void testSingleSubscribe() { | ||
| doReturn(Tasks.forResult(null)).when(mockRpc).subscribeToTopic(TEST_TOKEN, TEST_TOPIC); | ||
| // Mock client behavior for verify | ||
| // doNothing().when(mockTopicSubscriptionClient).subscribe(TEST_TOPIC); |
There was a problem hiding this comment.
Dead code? Same for code lines that are commented out in this file
4 participants
FCM V1 API Migration initial commit.