Add support for the new FCM registration ID.#16133
Conversation
It defaults to NO. When enabled, FCM will not generate an FCM registration token, but an FID instead.
…anagement system. Currently, the FCM SDK calls the `https://fcmtoken.googleapis.com/register` backend to register an FCM V4 registration token. Going forward, we'd like to deprecate this token and use FID instead.
Once `unregister()` is called successfully and auto-init is disabled, Sending a push notification to the FID will result in an `UNREGISTERED` error.
This method is similar to `messaging(_:didReceiveRegistrationToken:)`, but will be called instead when `isInstallationIdEnabled` is `YES`.
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. |
Doris-Ge
left a comment
There was a problem hiding this comment.
Does our existing implementation listen for a FID change via a listener and re-register on a change? If not, we may want to add that.
I'm still reviewing the PR, but I'd like to give some early feedback in case you want to address or discuss it. Since I'm not familiar with Swift, I need more time for a full review. Sorry about that!
| - (void)updateDefaultFCMToken:(NSString *)defaultFCMToken { | ||
| NSString *oldToken = self.tokenManager.defaultFCMToken; | ||
| NSString *newToken = defaultFCMToken; | ||
| if ([self.tokenManager hasTokenChangedFromOldToken:oldToken toNewToken:newToken]) { |
There was a problem hiding this comment.
Could we always invoke the callback even if the FID is not changed after a backend sync (we don't need to invoke the callback on every app start if the cached FID is not expired and it's still the same), considering that unlike a token, the FID is not going to be changed after a subsequent register() call following an unregister() call?
This change would enable developers to regularly sync the FID with their app servers, allowing for easier self-recovery during incidents. For instance, if a 404 error is mistakenly returned for a send request, leading an app server to delete a FID, invoking the callback regardless of a change would allow the server to re-acquire the FID automatically.
There was a problem hiding this comment.
I can do that. Do you want to limit this behavior to FID only or do you want the callback to be called no matter what value isInstallationIdEnabled is?
There was a problem hiding this comment.
Discussed offline and we have decided to not change the existing behavior.
There was a problem hiding this comment.
Can we update FIRMessagingTest.m to test changes in this file? Some test cases to consider:
-
When
isInstallationIdEnabledis true- A call to
register()can successfully invokedidReceiveRegistration()regardless whether the same FID has been registered or not and regardless whetherisAutoInitis enabled or not. - A call to
unregister()should invokedidUnregister()and leave the app as unregistered. - When
isAutoInit=true,didReceiveRegistration()is invoked on app start when a new FID is registered or the registered FID has expired or the FID is changed. token/deleteTokenshould throw an errorretrieveFCMTokenForSenderID/deleteFCMTokenForSenderIDshould throw an error
- A call to
-
When
isInstallationIdEnabledis falseregister()/unregister()should throw an error.
There was a problem hiding this comment.
I will add tests for the first 3 items. Regarding throwing exceptions, our style guide suggests avoiding them. go/objc-style#avoid-throwing-exceptions.
…okenFetchOperation. It's not used by other classes.
…didUnregister is called only when the deletion succeeded.
| @"apns_token" : apnsTokenString, | ||
| @"app_version" : appVersion, | ||
| @"apns_environment" : apnsEnvironment | ||
| }, |
There was a problem hiding this comment.
I've proposed adding the bundle_id field to the backend API in cl/913512427. Can we start passing it here?It should be fine, even in the worst-case scenario where the API Council disapproves the CL.
There was a problem hiding this comment.
Discussed offline. we will add that later.
| @"No Sender ID is available to register"); | ||
| return; | ||
| } | ||
| [self setupInstallationIDObserver]; |
There was a problem hiding this comment.
Shall we clear the observer during unregister()? If an app disables the auto init and calls unregister() after calling register(), not clearing the observer may automatically register the app with backend on FID changes, which may be unexpected for developers.
There was a problem hiding this comment.
Also, it seems the observer is only set up in the register() method. Calling register() is optional for apps that enable auto-init. We should make sure that those apps can also re-register on FID changes.
Consider setting up this observer regardlessly but only re-register when the app is registered as Eldhose did for Android: https://screenshot.googleplex.com/xmAGrSTKbyRMpSm.
It tests that when `FirebaseMessaging.isInstallationIdEnabled` is `YES`, calling `register()` triggers a V1 registration API HTTP request. After receiving the registered FID, the message delegate's `didReceiveRegistration` should be called.
…register`. So that it doesn't depend on the `register` being called.
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
No description provided.