[rc-swift] ConfigDBManager.swift #14126
Conversation
paulb777
commented
Nov 14, 2024
•
paulb777
commented
- Implements RC's underlying ConfigDBManager in Swift
- Migrates database queue to an actor
- Refactors some of the mocks out of the the unit tests
- Pod testing works on Xcode 16. Other CI failures look to be transition-related
Generated by 🚫 Danger |
Apple API Diff ReportCommit: 0095daf FirebaseRemoteConfigClassesFIRRemoteConfigValue[ADDED] -initWithData:source:Swift:
+ init ( data : Data ?, source : RemoteConfigSource )
Objective-C:
+ - ( instancetype _Nonnull ) initWithData :( nullable NSData * ) data source :( FIRRemoteConfigSource ) source ; |
paulb777
force-pushed
the
pb-config-db-manager
branch
from
524ab5e
to
c1eefe4
Compare
| @@ -84,83 +84,83 @@ jobs: | |||
| scripts/third_party/travis/retry.sh scripts/pod_lib_lint.rb ${{ matrix.podspec }} --platforms=${{ matrix.target }} \ | |||
| ${{ matrix.build-env.tests }} | |||
|
|
|||
| spm-package-resolved: | |||
There was a problem hiding this comment.
No SPM while using ObjC unit tests in transition
| @@ -26,7 +26,7 @@ @implementation FIRRemoteConfigValue { | |||
| } | |||
|
|
|||
| /// Designated initializer | |||
| - (instancetype)initWithData:(NSData *)data source:(FIRRemoteConfigSource)source { | |||
| - (instancetype)initWithData:(nullable NSData *)data source:(FIRRemoteConfigSource)source { | |||
There was a problem hiding this comment.
fix for existing api usage
| @@ -193,6 +194,21 @@ - (instancetype)initWithAppName:(NSString *)appName | |||
| return self; | |||
| } | |||
|
|
|||
| - (instancetype)initWithAppName:(NSString *)appName | |||
There was a problem hiding this comment.
Inject userDefaults for testing. This will be a single init with an optional when it goes to Swift
| @@ -21,7 +21,7 @@ | |||
| @class RCNConfigDBManager; | |||
|
|
|||
| /// This internal class contains a set of variables that are unique among all the config instances. | |||
| /// It also handles all metadata and internal metadata. This class is not thread safe and does not | |||
There was a problem hiding this comment.
Internal metadata handling was dead code. See #14153
| @@ -66,8 +79,6 @@ @interface RCNConfigSettings () { | |||
| NSString *_googleAppID; | |||
| /// The user defaults manager scoped to this RC instance of FIRApp and namespace. | |||
| RCNUserDefaultsManager *_userDefaultsManager; | |||
| /// The timestamp of last eTag update. | |||
| NSTimeInterval _lastETagUpdateTime; | |||
| } | ||
| return metadata; | ||
| - (void)loadConfigFromMetadataTable { | ||
| [_DBManager |
There was a problem hiding this comment.
Fixes synchronous db reads from ObjC
There was a problem hiding this comment.
Do clients of this API need to change now that it's async?
There was a problem hiding this comment.
Yes. That should be done in this PR
| @@ -0,0 +1,376 @@ | |||
| // Copyright 2024 Google LLC | |||
There was a problem hiding this comment.
This file is the internal API for the DB. The guts of the implementation is in DatabaseActor which is the code that was previously in a dedicated queue.
There was a problem hiding this comment.
This looks reasonable to me. There are potentially some DB optimizations we could do (e.g., creating the prepared statements once and resetting them to bind new values and then finalizing them on deinit) but I think that's beyond the scope of this Swift re-write and would be better to leave it closer to the original implementation (especially since you weren't sure if we wanted to keep the sqlite implementation).
| NSLog(@"%f %f", strongSelf->_settings.lastETagUpdateTime, | ||
| strongSelf->_settings.lastApplyTimeInterval); |
There was a problem hiding this comment.
Is checking this diff in intentional? Maybe should be deleted?
There was a problem hiding this comment.
Yep - good catch - removing
| } | ||
| return metadata; | ||
| - (void)loadConfigFromMetadataTable { | ||
| [_DBManager |
There was a problem hiding this comment.
Do clients of this API need to change now that it's async?
| return internalMetadata | ||
| } | ||
|
|
||
| private func migrateV1Namespace(toV2Namespace: Void) { |
There was a problem hiding this comment.
I think the toV2Namespace is useless.
| private func migrateV1Namespace(toV2Namespace: Void) { | |
| private func migrateV1NamespaceToV2Namespace() { |
|
|
||
| func insertMetadataTable(withValues columnNameToValue: [String: Any]) -> Bool { | ||
| let sql = """ | ||
| INSERT into fetch_metadata (\ |
There was a problem hiding this comment.
I think this is inserting into the wrong table. It should be fetch_metadata_v2.
The objc impl:
#define RCNTableNameMetadata "fetch_metadata_v2"
/* snip */
static const char *SQL =
"INSERT INTO " RCNTableNameMetadata
" (bundle_identifier, namespace, fetch_time, digest_per_ns, device_context, "
"app_context, success_fetch_time, failure_fetch_time, last_fetch_status, "
"last_fetch_error, last_apply_time, last_set_defaults_time) values (?, ?, ?, ?, ?, ?, "
"?, ?, ?, ?, ?, ?)";| INSERT into fetch_metadata (\ | |
| INSERT into fetch_metadata_v2 (\ |
There was a problem hiding this comment.
Yes - good catch - It's sometimes surprising what the AI misses.
| sql = "UPDATE fetch_metadata SET last_apply_time = ? WHERE namespace = ?" | ||
| case .defaultTime: | ||
| sql = "UPDATE fetch_metadata SET last_set_defaults_time = ? WHERE namespace = ?" | ||
| case .fetchStatus: | ||
| sql = | ||
| "UPDATE fetch_metadata SET last_fetch_status = ?, last_fetch_error = ? WHERE namespace = ?" |
There was a problem hiding this comment.
| sql = "UPDATE fetch_metadata SET last_apply_time = ? WHERE namespace = ?" | |
| case .defaultTime: | |
| sql = "UPDATE fetch_metadata SET last_set_defaults_time = ? WHERE namespace = ?" | |
| case .fetchStatus: | |
| sql = | |
| "UPDATE fetch_metadata SET last_fetch_status = ?, last_fetch_error = ? WHERE namespace = ?" | |
| sql = "UPDATE fetch_metadata_v2 SET last_apply_time = ? WHERE namespace = ?" | |
| case .defaultTime: | |
| sql = "UPDATE fetch_metadata_v2 SET last_set_defaults_time = ? WHERE namespace = ?" | |
| case .fetchStatus: | |
| sql = | |
| "UPDATE fetch_metadata_v2 SET last_fetch_status = ?, last_fetch_error = ? WHERE namespace = ?" |
| last_fetch_error, \ | ||
| last_apply_time, \ | ||
| last_set_defaults_time \ | ||
| FROM fetch_metadata \ |
There was a problem hiding this comment.
| FROM fetch_metadata \ | |
| FROM fetch_metadata_v2 \ |
| """ | ||
|
|
||
| let createMetadata = """ | ||
| CREATE TABLE IF NOT EXISTS fetch_metadata ( |
There was a problem hiding this comment.
| CREATE TABLE IF NOT EXISTS fetch_metadata ( | |
| CREATE TABLE IF NOT EXISTS fetch_metadata_v2 ( |
|
I commented or resolved all comments except for most of the ones in DatabaseActor which I'll look at in a few hours |
|
Going to merge it here. Thanks for the reviews @ncooke3 and @andrewheard. One note is that callbacks from the Database APIs are not wrapped in main thread because they're already in a Task, can cause deadlocks and should be replaced by async APIs when the callers are in Swift. |
|
After merging, I rebased |
