Skip to content
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

Add support for other Firebase products to integrate with Remote Config. #6692

Merged
merged 11 commits into from
Oct 23, 2020
1 change: 1 addition & 0 deletions FirebaseRemoteConfig.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ app update.
# 'FirebaseRemoteConfig/Tests/Unit/RCNConfigTest.m',
'FirebaseRemoteConfig/Tests/Unit/RCNConfigExperimentTest.m',
'FirebaseRemoteConfig/Tests/Unit/RCNConfigValueTest.m',
'FirebaseRemoteConfig/Tests/Unit/RCNPersonalizationTest.m',
# 'FirebaseRemoteConfig/Tests/Unit/RCNRemoteConfig+FIRAppTest.m',
'FirebaseRemoteConfig/Tests/Unit/RCNRemoteConfigTest.m',
# 'FirebaseRemoteConfig/Tests/Unit/RCNThrottlingTests.m',
Expand Down
1 change: 1 addition & 0 deletions FirebaseRemoteConfig/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- [fixed] Fixed database creation on tvOS. (#6612)
- [changed] Updated public API documentation to no longer reference removed APIs. (#6641)
- [fixed] Updated `activateWithCompletion:` to use completion handler for experiment updates. (#3687)
- [changed] Log metadata for Remote Config parameter get calls. (#6692)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this more descriptive? As written this might raise concerns for developers. What exactly is the impact of the change for users of the RemoteConfig libraries?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the RC team wants to keep this intentionally vague, since most developers won't be able to use this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, something like "Added preliminary infrastructure to support future features" or nothing at all might be better. If I read the code right, additional logging is only happening when Personalization is being used. Saying unspecified logging is happening can be concerning to developers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to your suggestion.


# v4.9.1
- [fixed] Fix an `attempt to insert nil object` crash in `fetchWithExpirationDuration:`. (#6522)
Expand Down
34 changes: 34 additions & 0 deletions FirebaseRemoteConfig/Sources/FIRRemoteConfig.m
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#import "FirebaseRemoteConfig/Sources/RCNConfigExperiment.h"
#import "FirebaseRemoteConfig/Sources/RCNConfigValue_Internal.h"
#import "FirebaseRemoteConfig/Sources/RCNDevice.h"
#import "FirebaseRemoteConfig/Sources/RCNPersonalization.h"

/// Remote Config Error Domain.
/// TODO: Rename according to obj-c style for constants.
Expand All @@ -41,6 +42,10 @@
/// Timeout value for waiting on a fetch response.
static NSString *const kRemoteConfigFetchTimeoutKey = @"_rcn_fetch_timeout";

/// Listener for the get methods.
typedef void (^FIRRemoteConfigListener)(NSString *_Nonnull, NSDictionary *_Nonnull)
NS_SWIFT_NAME(FIRRemoteConfigListener);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove NS_SWIFT_NAME. APIs exposed to Swift should not use typedefs and this looks like its only for internal APIs anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


@interface FIRRemoteConfigSettings () {
BOOL _developerModeEnabled;
}
Expand Down Expand Up @@ -68,6 +73,7 @@ @implementation FIRRemoteConfig {
RCNConfigExperiment *_configExperiment;
dispatch_queue_t _queue;
NSString *_appName;
NSMutableArray *_listeners;
}

static NSMutableDictionary<NSString *, NSMutableDictionary<NSString *, FIRRemoteConfig *> *>
Expand Down Expand Up @@ -161,6 +167,14 @@ - (instancetype)initWithAppName:(NSString *)appName
options:options];

[_settings loadConfigFromMetadataTable];

if (analytics) {
_listeners = [[NSMutableArray alloc] init];
[RCNPersonalization setAnalytics:analytics];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add tests for FIRRemoteConfig integration with personalization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

[self addListener:^(NSString *key, NSDictionary *config) {
[RCNPersonalization logArmActive:key config:config];
}];
}
}
return self;
}
Expand Down Expand Up @@ -192,6 +206,24 @@ - (void)ensureInitializedWithCompletionHandler:
});
}

/// Adds a listener that will be called whenever one of the get methods is called.
/// @param listener Function that takes in the parameter key and the config.
- (void)addListener:(nonnull FIRRemoteConfigListener)listener {
@synchronized(_listeners) {
[_listeners addObject:listener];
}
}

- (void)callListeners:(NSString *)key config:(NSDictionary *)config {
@synchronized(_listeners) {
for (FIRRemoteConfigListener listener in _listeners) {
dispatch_async(_queue, ^{
listener(key, config);
});
}
}
}

#pragma mark - fetch

- (void)fetchWithCompletionHandler:(FIRRemoteConfigFetchCompletion)completionHandler {
Expand Down Expand Up @@ -286,6 +318,7 @@ - (void)activateWithCompletion:(FIRRemoteConfigActivateChangeCompletion)completi
forNamespace:self->_FIRNamespace];
strongSelf->_settings.lastApplyTimeInterval = [[NSDate date] timeIntervalSince1970];
FIRLogDebug(kFIRLoggerRemoteConfig, @"I-RCN000069", @"Config activated.");
[strongSelf->_configContent activatePersonalization];
[strongSelf->_configExperiment updateExperimentsWithHandler:^(NSError *_Nullable error) {
if (completion) {
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
Expand Down Expand Up @@ -328,6 +361,7 @@ - (FIRRemoteConfigValue *)configValueForKey:(NSString *)key {
@"Key %@ should come from source:%zd instead coming from source: %zd.", key,
(long)FIRRemoteConfigSourceRemote, (long)value.source);
}
[self callListeners:key config:[self->_configContent getMetadataForNamespace:FQNamespace]];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: getConfigAndMetadataForNamespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return;
}
value = self->_configContent.defaultConfig[FQNamespace][key];
Expand Down
2 changes: 2 additions & 0 deletions FirebaseRemoteConfig/Sources/RCNConfigConstants.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ static const char *RCNRemoteConfigQueueLabel = "com.google.GoogleConfigService.F
static NSString *const RCNFetchResponseKeyEntries = @"entries";
/// Key that includes data for experiment descriptions in ABT.
static NSString *const RCNFetchResponseKeyExperimentDescriptions = @"experimentDescriptions";
/// Key that includes data for Personalization metadata.
static NSString *const RCNFetchResponseKeyPersonalizationMetadata = @"personalizationMetadata";
/// Error key.
static NSString *const RCNFetchResponseKeyError = @"error";
/// Error code.
Expand Down
6 changes: 6 additions & 0 deletions FirebaseRemoteConfig/Sources/RCNConfigContent.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,10 @@ typedef NS_ENUM(NSInteger, RCNDBSource) {
toSource:(RCNDBSource)source
forNamespace:(NSString *)FIRNamespace;

/// Sets the fetched Personalization metadata to active.
- (void)activatePersonalization;

/// Gets the active config and Personalization metadata.
- (NSDictionary *)getMetadataForNamespace:(NSString *)FIRNamespace;

@end
42 changes: 41 additions & 1 deletion FirebaseRemoteConfig/Sources/RCNConfigContent.m
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ @implementation RCNConfigContent {
NSMutableDictionary *_fetchedConfig;
/// Default config provided by user.
NSMutableDictionary *_defaultConfig;
/// Active Personalization metadata that is currently used.
NSDictionary *_activePersonalization;
/// Pending Personalization metadata that is latest data from server that might or might not be
/// applied.
NSDictionary *_fetchedPersonalization;
/// DBManager
RCNConfigDBManager *_DBManager;
/// Current bundle identifier;
Expand Down Expand Up @@ -72,14 +77,16 @@ - (instancetype)initWithDBManager:(RCNConfigDBManager *)DBManager {
_activeConfig = [[NSMutableDictionary alloc] init];
_fetchedConfig = [[NSMutableDictionary alloc] init];
_defaultConfig = [[NSMutableDictionary alloc] init];
_activePersonalization = [[NSDictionary alloc] init];
_fetchedPersonalization = [[NSDictionary alloc] init];
_bundleIdentifier = [[NSBundle mainBundle] bundleIdentifier];
if (!_bundleIdentifier) {
FIRLogNotice(kFIRLoggerRemoteConfig, @"I-RCN000038",
@"Main bundle identifier is missing. Remote Config might not work properly.");
_bundleIdentifier = @"";
}
_DBManager = DBManager;
_configLoadFromDBSemaphore = dispatch_semaphore_create(0);
_configLoadFromDBSemaphore = dispatch_semaphore_create(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a comment why the particular semaphore value is set. Also, maybe it will be a bit more readable if we call dispatch_semaphore_signal closer to the operation that requires it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the database methods closer.

[self loadConfigFromMainTable];
}
return self;
Expand Down Expand Up @@ -204,10 +211,17 @@ - (void)updateConfigContentWithResponse:(NSDictionary *)response
if ([state isEqualToString:RCNFetchResponseKeyStateUpdate]) {
[self handleUpdateStateForConfigNamespace:currentNamespace
withEntries:response[RCNFetchResponseKeyEntries]];
[self handleUpdatePersonalization:response[RCNFetchResponseKeyPersonalizationMetadata]];
return;
}
}

- (void)activatePersonalization {
_activePersonalization = _fetchedPersonalization;
[_DBManager insertOrUpdatePersonalizationConfig:_activePersonalization
fromSource:RCNDBSourceActive];
}

#pragma mark State handling
- (void)handleNoChangeStateForConfigNamespace:(NSString *)currentNamespace {
if (!_fetchedConfig[currentNamespace]) {
Expand Down Expand Up @@ -263,6 +277,14 @@ - (void)handleUpdateStateForConfigNamespace:(NSString *)currentNamespace
}
}

- (void)handleUpdatePersonalization:(NSDictionary *)metadata {
if (!metadata) {
return;
}
_fetchedPersonalization = metadata;
[_DBManager insertOrUpdatePersonalizationConfig:metadata fromSource:RCNDBSourceFetched];
}

#pragma mark - database

/// This method is only meant to be called at init time. The underlying logic will need to be
Expand All @@ -284,6 +306,14 @@ - (void)loadConfigFromMainTable {
self->_defaultConfig = [defaultConfig mutableCopy];
dispatch_semaphore_signal(self->_configLoadFromDBSemaphore);
}];

[_DBManager loadPersonalizationWithCompletionHandler:^(
BOOL success, NSDictionary *fetchedPersonalization,
NSDictionary *activePersonalization, NSDictionary *defaultConfig) {
self->_fetchedPersonalization = [fetchedPersonalization copy];
self->_activePersonalization = [activePersonalization copy];
dispatch_semaphore_signal(self->_configLoadFromDBSemaphore);
}];
}

/// Update the current config result to main table.
Expand Down Expand Up @@ -314,6 +344,16 @@ - (NSDictionary *)defaultConfig {
return _defaultConfig;
}

- (NSDictionary *)getMetadataForNamespace:(NSString *)FIRNamespace {
/// If this is the first time reading the active metadata, we might still be reading it from the
/// database.
[self checkAndWaitForInitialDatabaseLoad];
return @{
RCNFetchResponseKeyEntries : _activeConfig[FIRNamespace],
RCNFetchResponseKeyPersonalizationMetadata : _activePersonalization
};
}

/// We load the database async at init time. Block all further calls to active/fetched/default
/// configs until load is done.
/// @return Database load completion status.
Expand Down
7 changes: 7 additions & 0 deletions FirebaseRemoteConfig/Sources/RCNConfigDBManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ typedef void (^RCNDBLoadCompletion)(BOOL success,
/// Load experiment from experiment table.
/// @param handler The callback when reading from DB is complete.
- (void)loadExperimentWithCompletionHandler:(RCNDBCompletion)handler;
/// Load Personalization from table.
/// @param handler The callback when reading from DB is complete.
- (void)loadPersonalizationWithCompletionHandler:(RCNDBLoadCompletion)handler;

/// Insert a record in metadata table.
/// @param columnNameToValue The column name and its value to be inserted in metadata table.
Expand Down Expand Up @@ -100,6 +103,10 @@ typedef void (^RCNDBLoadCompletion)(BOOL success,
- (void)updateMetadataWithOption:(RCNUpdateOption)option
values:(NSArray *)values
completionHandler:(RCNDBCompletion)handler;

/// Insert or update the data in Personalizatoin config.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: typo Personalization

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

- (BOOL)insertOrUpdatePersonalizationConfig:(NSDictionary *)metadata fromSource:(RCNDBSource)source;

/// Clear the record of given namespace and package name
/// before updating the table.
- (void)deleteRecordFromMainTableWithNamespace:(NSString *)namespace_p
Expand Down
122 changes: 121 additions & 1 deletion FirebaseRemoteConfig/Sources/RCNConfigDBManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#define RCNTableNameMetadata "fetch_metadata"
#define RCNTableNameInternalMetadata "internal_metadata"
#define RCNTableNameExperiment "experiment"
#define RCNTableNamePersonalization "personalization"

static BOOL gIsNewDatabase;
/// SQLite file name in versions 0, 1 and 2.
Expand Down Expand Up @@ -277,11 +278,15 @@ - (BOOL)createTableSchema {

static const char *createTableExperiment = "create TABLE IF NOT EXISTS " RCNTableNameExperiment
" (_id INTEGER PRIMARY KEY, key TEXT, value BLOB)";
static const char *createTablePersonalization =
"create TABLE IF NOT EXISTS " RCNTableNamePersonalization
" (_id INTEGER PRIMARY KEY, key INTEGER, value BLOB)";

return [self executeQuery:createTableMain] && [self executeQuery:createTableMainActive] &&
[self executeQuery:createTableMainDefault] && [self executeQuery:createTableMetadata] &&
[self executeQuery:createTableInternalMetadata] &&
[self executeQuery:createTableExperiment];
[self executeQuery:createTableExperiment] &&
[self executeQuery:createTablePersonalization];
}

- (void)removeDatabaseOnDatabaseQueueAtPath:(NSString *)path {
Expand Down Expand Up @@ -568,6 +573,48 @@ - (BOOL)updateExperimentMetadata:(NSData *)dataValue {
return YES;
}

- (BOOL)insertOrUpdatePersonalizationConfig:(NSDictionary *)dataValue
fromSource:(RCNDBSource)source {
RCN_MUST_NOT_BE_MAIN_THREAD();

NSError *error;
NSData *JSONPayload = [NSJSONSerialization dataWithJSONObject:dataValue
options:NSJSONWritingPrettyPrinted
error:&error];

if (!JSONPayload || error) {
FIRLogError(kFIRLoggerRemoteConfig, @"I-RCN000075",
@"Invalid Personalization payload to be serialized.");
}

const char *SQL = "INSERT OR REPLACE INTO " RCNTableNamePersonalization
" (_id, key, value) values ((SELECT _id from " RCNTableNamePersonalization
" WHERE key = ?), ?, ?)";

sqlite3_stmt *statement = [self prepareSQL:SQL];
if (!statement) {
return NO;
}

if (sqlite3_bind_int(statement, 1, (int)source) != SQLITE_OK) {
return [self logErrorWithSQL:SQL finalizeStatement:statement returnValue:NO];
}

if (sqlite3_bind_int(statement, 2, (int)source) != SQLITE_OK) {
return [self logErrorWithSQL:SQL finalizeStatement:statement returnValue:NO];
}
if (sqlite3_bind_blob(statement, 3, JSONPayload.bytes, (int)JSONPayload.length, NULL) !=
SQLITE_OK) {
return [self logErrorWithSQL:SQL finalizeStatement:statement returnValue:NO];
}

if (sqlite3_step(statement) != SQLITE_DONE) {
return [self logErrorWithSQL:SQL finalizeStatement:statement returnValue:NO];
}
sqlite3_finalize(statement);
return YES;
}

#pragma mark - update

- (void)updateMetadataWithOption:(RCNUpdateOption)option
Expand Down Expand Up @@ -801,6 +848,79 @@ - (void)loadExperimentWithCompletionHandler:(RCNDBCompletion)handler {
return results;
}

- (void)loadPersonalizationWithCompletionHandler:(RCNDBLoadCompletion)handler {
__weak RCNConfigDBManager *weakSelf = self;
dispatch_async(_databaseOperationQueue, ^{
RCNConfigDBManager *strongSelf = weakSelf;
if (!strongSelf) {
return;
}

NSDictionary *activePersonalization;
NSData *personalizationResult = [strongSelf loadPersonalizationTableFromKey:RCNDBSourceActive];
// There should be only one entry for Personalization metadata.
if (personalizationResult) {
NSError *error;
activePersonalization = [NSJSONSerialization JSONObjectWithData:personalizationResult
options:0
error:&error];
}
if (!activePersonalization) {
activePersonalization = [[NSMutableDictionary alloc] init];
}

NSDictionary *fetchedPersonalization;
personalizationResult = [strongSelf loadPersonalizationTableFromKey:RCNDBSourceFetched];
// There should be only one entry for Personalization metadata.
if (personalizationResult) {
NSError *error;
fetchedPersonalization = [NSJSONSerialization JSONObjectWithData:personalizationResult
options:0
error:&error];
}
if (!fetchedPersonalization) {
fetchedPersonalization = [[NSMutableDictionary alloc] init];
}

if (handler) {
dispatch_async(dispatch_get_main_queue(), ^{
handler(YES, fetchedPersonalization, activePersonalization, nil);
});
}
});
}

- (NSData *)loadPersonalizationTableFromKey:(int)key {
RCN_MUST_NOT_BE_MAIN_THREAD();

NSMutableArray *results = [[NSMutableArray alloc] init];
const char *SQL = "SELECT value FROM " RCNTableNamePersonalization " WHERE key = ?";
sqlite3_stmt *statement = [self prepareSQL:SQL];
if (!statement) {
return nil;
}

if (sqlite3_bind_int(statement, 1, key) != SQLITE_OK) {
[self logErrorWithSQL:SQL finalizeStatement:statement returnValue:NO];
return nil;
}
NSData *personalizationData;
while (sqlite3_step(statement) == SQLITE_ROW) {
personalizationData = [NSData dataWithBytes:(char *)sqlite3_column_blob(statement, 0)
length:sqlite3_column_bytes(statement, 0)];
if (personalizationData) {
[results addObject:personalizationData];
}
}

sqlite3_finalize(statement);
// There should be only one entry in this table.
if (results.count != 1) {
return nil;
}
return results[0];
}

- (NSDictionary *)loadInternalMetadataTable {
__block NSMutableDictionary *internalMetadataTableResult;
__weak RCNConfigDBManager *weakSelf = self;
Expand Down
Loading