-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Reset RC per-instance userdefaults if config database was not found. #4896
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
Conversation
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.
Just to confirm, the issue happens when users uninstall the app and reinstall again but we thought config is fetched because user default is not reset?
LGTM on presubmit green |
@@ -31,6 +31,7 @@ | |||
#define RCNTableNameInternalMetadata "internal_metadata" | |||
#define RCNTableNameExperiment "experiment" | |||
|
|||
static BOOL gIsNewDatabase; |
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.
Does this need to be made thread-safe?
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 is. The variable is used on the database serial queue.
@@ -102,6 +102,14 @@ - (instancetype)initWithDatabaseManager:(RCNConfigDBManager *)manager | |||
_userDefaultsManager = [[RCNUserDefaultsManager alloc] initWithAppName:appName | |||
bundleID:_bundleIdentifier | |||
namespace:_FIRNamespace]; | |||
|
|||
// Check if the config database is new. If so, clear the configs saved in userDefaults. | |||
if ([_DBManager isNewDatabase]) { |
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.
How can this test ever fail? I don't see gIsNewDatabase ever set to NO.
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.
The test case setup method above creates a new test database. We just want to verify that this flag is set to true whenever an existing database is not found.
@ryanwilson nvm, I was adding you and wondering if NSUserDefault can opt out for backup, looks like it's not available. |
It happens on a restore of a previous backup of the device. The config database is gone, but the last eTag is still remembered (since it is in the user defaults). Hence, a new fetch call succeeds but only returns NO_CHANGE since the eTag was sent up. |
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.
Addresses b/148386739, #4677 , #4734. Backgroubd: We explicitly disable backup of the sqlite database. Hence upon restore, we do not have any data. This causes a fetch to be made, but we send the last eTag (which is saved in userdefaults and cannot be opted out of backup) along with the fetch request which causes the RC backend to send a NO_CHANGE back. Fix is to identify if the database is gone, and if so, reset the userdefaults.