Update replicator implementation for new config #19
Conversation
Summary of ChangesHello @jayant-dhingra-cb, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant refactoring of the replicator configuration handling to support a new, more granular API that allows individual collections to have distinct replication settings, aligning with the native iOS SDK. It also ensures seamless backward compatibility for existing implementations using the older configuration format. Furthermore, extensive documentation has been added to key public functions across Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed update to the replicator implementation, adding support for a new, more flexible configuration API while maintaining backward compatibility with the old one. The introduction of new DTOs and a dispatcher for handling both API formats is a solid approach. The extensive documentation added to public methods is excellent and will greatly improve the library's usability and maintainability. I've identified a few issues, including a critical bug in the legacy API path, and have provided suggestions for improvement.
| if let pinnedCert = pinnedServerCertificate.data(using: .utf8) { | ||
| replConfig.pinnedServerCertificate = SecCertificateCreateWithData(nil, pinnedCert as CFData) | ||
| print("[OLD API Step 6] ✅ Pinned certificate set") | ||
| } else { | ||
| print("[OLD API Step 6] ⚠️ Pinned certificate data conversion failed") | ||
| } |
There was a problem hiding this comment.
There's a critical bug here in handling the pinned certificate for the old API path. The pinnedServerCertificate string is expected to be Base64-encoded, but it's being converted to Data using .utf8 encoding. This will result in an invalid certificate. You should use Data(base64Encoded: pinnedServerCertificate) and also verify that the data can be used to create a SecCertificate, throwing an error if either step fails, as you've correctly done in the new API path implementation.
guard let certData = Data(base64Encoded: pinnedServerCertificate) else {
print("[OLD API Step 6] ⚠️ Pinned certificate data is not valid base64")
throw ReplicatorError.fatalError(message: "Invalid pinned server certificate: not valid base64")
}
guard let pinnedCert = SecCertificateCreateWithData(nil, certData as CFData) else {
print("[OLD API Step 6] ⚠️ Pinned certificate data is not a valid certificate")
throw ReplicatorError.fatalError(message: "Invalid pinned server certificate: not a valid certificate")
}
replConfig.pinnedServerCertificate = pinnedCert
print("[OLD API Step 6] ✅ Pinned certificate set")
There was a problem hiding this comment.
There are a couple of settings which were already part of the ReplicatorConfiguration class, such as this pinnedServerCertificate. Is fine if we can't separate the logic enough to have them set in the same place but should be doing exactly the same for both new and old methods. This should use base64
| replConfig.heartbeat = TimeInterval(heartbeat.intValue) | ||
| replConfig.maxAttempts = UInt(maxAttempts.intValue) | ||
| replConfig.maxAttemptWaitTime = TimeInterval(maxAttemptWaitTime.intValue) |
There was a problem hiding this comment.
The conversion from NSNumber to TimeInterval and UInt using .intValue is unsafe and can lead to data truncation if the numbers are large. The new API path handles this more safely using .int64Value for TimeInterval and .uintValue for UInt. Please update this to match the safer implementation from the new API path to prevent potential issues.
| replConfig.heartbeat = TimeInterval(heartbeat.intValue) | |
| replConfig.maxAttempts = UInt(maxAttempts.intValue) | |
| replConfig.maxAttemptWaitTime = TimeInterval(maxAttemptWaitTime.intValue) | |
| replConfig.heartbeat = TimeInterval(exactly: heartbeat.int64Value) ?? 300 | |
| replConfig.maxAttempts = maxAttempts.uintValue | |
| replConfig.maxAttemptWaitTime = TimeInterval(exactly: maxAttemptWaitTime.int64Value) ?? 0 |
| print("\n╔════════════════════════════════════════════════════════════════╗") | ||
| print("║ [ReplicatorHelper] STARTING REPLICATOR CREATION ║") | ||
| print("╚════════════════════════════════════════════════════════════════╝") | ||
|
|
||
| // Parse JSON string to data | ||
| print("[Step 1] Converting JSON string to data...") | ||
| guard let jsonData = collectionConfigJson.data(using: .utf8) else { | ||
| print("[Step 1] ❌ FAILED: Unable to convert JSON string to data") | ||
| throw ReplicatorError.fatalError(message: "Unable to convert JSON string to data") | ||
| } | ||
| print("[Step 1] ✅ JSON string converted to data") | ||
|
|
||
| // Try to parse as generic JSON to detect format | ||
| print("[Step 2] Parsing JSON to detect format...") | ||
| guard let jsonArray = try? JSONSerialization.jsonObject(with: jsonData) as? [[String: Any]], | ||
| let firstItem = jsonArray.first else { | ||
| print("[Step 2] ❌ FAILED: Invalid JSON format") | ||
| throw ReplicatorError.fatalError(message: "Invalid JSON format: expected array of objects") | ||
| } | ||
| print("[Step 2] ✅ JSON array parsed, count:", jsonArray.count) | ||
|
|
||
| // Detect format by checking for "collection" (NEW) vs "collections" (OLD) | ||
| print("[Step 3] Detecting API format...") | ||
| let isNewApi = firstItem["collection"] != nil | ||
| let isOldApi = firstItem["collections"] != nil | ||
| print("[Step 3] Detection result - isNewApi:", isNewApi, "isOldApi:", isOldApi) | ||
| print("[Step 3] firstItem keys:", Array(firstItem.keys)) | ||
|
|
||
| if isNewApi { | ||
| print("[Step 4] 🔷 Using NEW API path") | ||
| let decoder = JSONDecoder() | ||
| let collectionConfig = try decoder.decode([CollectionConfigurationDto].self, from: jsonData) | ||
| print("[Step 4] ✅ Decoded NEW API config, count:", collectionConfig.count) | ||
| return try replicatorConfigFromJson(data, collectionConfiguration: collectionConfig) | ||
| } else if isOldApi { | ||
| print("[Step 4] 🔶 Using OLD API path") | ||
| let decoder = JSONDecoder() | ||
| let collectionConfig = try decoder.decode([CollectionConfigItem].self, from: jsonData) | ||
| print("[Step 4] ✅ Decoded OLD API config, count:", collectionConfig.count) | ||
| return try replicatorConfigFromJsonOldApi(data, collectionConfiguration: collectionConfig) | ||
| } else { | ||
| print("[Step 4] ❌ FAILED: Unrecognized format (no 'collection' or 'collections' key)") | ||
| throw ReplicatorError.fatalError(message: "Unrecognized collection configuration format") | ||
| } | ||
| } |
There was a problem hiding this comment.
While the extensive print statements are useful for debugging during development, they add a lot of noise to the console logs in a production environment. It would be better to replace them with a proper logging framework (like os.log or a third-party library) that allows controlling log levels. This would allow enabling detailed logs in debug builds while keeping production logs clean.
There was a problem hiding this comment.
my understanding is that these logs won't show to the end user of RN, is that correct?
| if let authenticator = data["authenticator"] as? [String: Any], | ||
| let type = authenticator["type"] as? String, | ||
| let authData = authenticator["data"] as? [String: Any] { | ||
| print("[OLD API Step 7] Authenticator type:", type) | ||
| switch type { | ||
| case "basic": | ||
| if let username = authData["username"] as? String, | ||
| let password = authData["password"] as? String { | ||
| replConfig.authenticator = BasicAuthenticator(username: username, password: password) | ||
| print("[OLD API Step 7] ✅ Basic authenticator set") | ||
| } | ||
| case "session": | ||
| if let sessionID = authData["sessionId"] as? String { | ||
| let cookieName = authData["cookieName"] as? String ?? "SyncGatewaySession" | ||
| replConfig.authenticator = SessionAuthenticator(sessionID: sessionID, cookieName: cookieName) | ||
| print("[OLD API Step 7] ✅ Session authenticator set") | ||
| } | ||
| default: | ||
| print("[OLD API Step 7] ⚠️ Unknown authenticator type") | ||
| break | ||
| } | ||
| } else { | ||
| print("[OLD API Step 7] ℹ️ No authenticator") | ||
| } |
There was a problem hiding this comment.
This block duplicates the logic for creating an authenticator, which is already implemented in the replicatorAuthenticatorFromConfig helper function. To improve maintainability and reduce code duplication, you should refactor this to call the existing helper function, similar to how it's done in the new API path.
if let authenticatorConfig = data["authenticator"] as? [String: Any] {
if let authenticator = ReplicatorHelper.replicatorAuthenticatorFromConfig(authenticatorConfig) {
replConfig.authenticator = authenticator
print("[OLD API Step 7] ✅ Authenticator set")
} else {
print("[OLD API Step 7] ⚠️ Unknown or invalid authenticator config")
}
} else {
print("[OLD API Step 7] ℹ️ No authenticator")
}There was a problem hiding this comment.
For the new API section above is different. It also has not changed for CBL in between 3.2-4.0 so either we have the same implementation either we set this into a helper method alongside pinnedCert and the others.
For the new API, the only order imposed is that CollectionConfiguration exists before ReplicatorConfiguration is instantiated. Saying that, we can still set specific ReplicatorConfiguration params at the end, before the init of the Replicator itself, for both the new and old API. It might be possible that all this logic that has not changed to be only in one place (in a helper function).
| if config.channels.count > 0 { | ||
| colConfig.channels = config.channels | ||
| } | ||
|
|
||
| // Process push and pull filters | ||
| if let pushFilterStr = item.config.pushFilter, !pushFilterStr.isEmpty { | ||
| collectionConfig.pushFilter = createFilter(from: pushFilterStr) | ||
| // Set document IDs filter | ||
| if config.documentIds.count > 0 { | ||
| colConfig.documentIDs = config.documentIds | ||
| } |
There was a problem hiding this comment.
These if checks for channels.count > 0 and documentIds.count > 0 are redundant. Assigning an empty array to colConfig.channels or colConfig.documentIDs is a valid operation and has the same effect as the property retaining its default empty array value. You can simplify the code by removing these checks and assigning the properties directly.
// Set channels (for pull replication)
colConfig.channels = config.channels
// Set document IDs filter
colConfig.documentIDs = config.documentIds| print("\n╔═══════════════════════════════════════════════════════════════╗") | ||
| print("║ [ReplicatorManager] CREATING REPLICATOR INSTANCE ║") | ||
| print("╚═══════════════════════════════════════════════════════════════╝") | ||
|
|
||
| print("[ReplicatorManager Step 1] Generating unique ID...") | ||
| let id = UUID().uuidString | ||
| print("[ReplicatorManager Step 1] ✅ ID generated: \(id)") | ||
|
|
||
| print("[ReplicatorManager Step 2] Creating ReplicatorConfiguration...") | ||
| let config = try ReplicatorHelper.replicatorConfigFromJson( | ||
| replicatorConfig, | ||
| collectionConfigJson: collectionConfigJson | ||
| ) | ||
| print("[ReplicatorManager Step 2] ✅ ReplicatorConfiguration created") | ||
|
|
||
| print("[ReplicatorManager Step 3] Creating Replicator instance...") | ||
| do { | ||
| let replicator = Replicator(config: config) | ||
| print("[ReplicatorManager Step 3] ✅ Replicator instance created") | ||
|
|
||
| print("[ReplicatorManager Step 4] Storing replicator in registry...") | ||
| replicators[id] = replicator | ||
| print("[ReplicatorManager Step 4] ✅ Replicator stored, total count: \(replicators.count)") | ||
|
|
||
| print("\n╔═══════════════════════════════════════════════════════════════╗") | ||
| print("║ [ReplicatorManager] ✅ REPLICATOR INSTANCE CREATED ║") | ||
| print("╚═══════════════════════════════════════════════════════════════╝\n") | ||
|
|
||
| return id | ||
| } catch { | ||
| print("[ReplicatorManager Step 3] ❌ FAILED: \(error)") | ||
| print("[ReplicatorManager Step 3] Error type: \(type(of: error))") | ||
| throw error | ||
| } |
There was a problem hiding this comment.
Similar to ReplicatorHelper.swift, these print statements are great for debugging but should be replaced with a proper logging framework for production code. This allows for configurable log levels and avoids cluttering console output in release builds.
| print("\n╔════════════════════════════════════════════════════════════════╗") | ||
| print("║ [ReplicatorHelper] STARTING REPLICATOR CREATION ║") | ||
| print("╚════════════════════════════════════════════════════════════════╝") | ||
|
|
||
| // Parse JSON string to data | ||
| print("[Step 1] Converting JSON string to data...") | ||
| guard let jsonData = collectionConfigJson.data(using: .utf8) else { | ||
| print("[Step 1] ❌ FAILED: Unable to convert JSON string to data") | ||
| throw ReplicatorError.fatalError(message: "Unable to convert JSON string to data") | ||
| } | ||
| print("[Step 1] ✅ JSON string converted to data") | ||
|
|
||
| // Try to parse as generic JSON to detect format | ||
| print("[Step 2] Parsing JSON to detect format...") | ||
| guard let jsonArray = try? JSONSerialization.jsonObject(with: jsonData) as? [[String: Any]], | ||
| let firstItem = jsonArray.first else { | ||
| print("[Step 2] ❌ FAILED: Invalid JSON format") | ||
| throw ReplicatorError.fatalError(message: "Invalid JSON format: expected array of objects") | ||
| } | ||
| print("[Step 2] ✅ JSON array parsed, count:", jsonArray.count) | ||
|
|
||
| // Detect format by checking for "collection" (NEW) vs "collections" (OLD) | ||
| print("[Step 3] Detecting API format...") | ||
| let isNewApi = firstItem["collection"] != nil | ||
| let isOldApi = firstItem["collections"] != nil | ||
| print("[Step 3] Detection result - isNewApi:", isNewApi, "isOldApi:", isOldApi) | ||
| print("[Step 3] firstItem keys:", Array(firstItem.keys)) | ||
|
|
||
| if isNewApi { | ||
| print("[Step 4] 🔷 Using NEW API path") | ||
| let decoder = JSONDecoder() | ||
| let collectionConfig = try decoder.decode([CollectionConfigurationDto].self, from: jsonData) | ||
| print("[Step 4] ✅ Decoded NEW API config, count:", collectionConfig.count) | ||
| return try replicatorConfigFromJson(data, collectionConfiguration: collectionConfig) | ||
| } else if isOldApi { | ||
| print("[Step 4] 🔶 Using OLD API path") | ||
| let decoder = JSONDecoder() | ||
| let collectionConfig = try decoder.decode([CollectionConfigItem].self, from: jsonData) | ||
| print("[Step 4] ✅ Decoded OLD API config, count:", collectionConfig.count) | ||
| return try replicatorConfigFromJsonOldApi(data, collectionConfiguration: collectionConfig) | ||
| } else { | ||
| print("[Step 4] ❌ FAILED: Unrecognized format (no 'collection' or 'collections' key)") | ||
| throw ReplicatorError.fatalError(message: "Unrecognized collection configuration format") | ||
| } | ||
| } |
There was a problem hiding this comment.
my understanding is that these logs won't show to the end user of RN, is that correct?
| let endpoint = URLEndpoint(url: URL(string: url)!) | ||
| // Validate collections array is not empty | ||
| guard !collectionConfiguration.isEmpty else { | ||
| throw ReplicatorError.fatalError(message: "At least one collection configuration is required") |
There was a problem hiding this comment.
We do handle this in CBL side, as we don't allow for [CollectionConfiguration] to be nil at init of ReplicatorConfiguration
public init(collections: [CollectionConfiguration], target: Endpoint) {
self.target = target
for config in collections {
guard let collection = config.collection else {
Precondition.assert(false, message: "Each collection configuration must have a non-null collection.")
return
}
if self.db == nil {
self.db = collection.database
} else {
Precondition.assert(
self.db!.impl == collection.database.impl,
message: "Collection \(collection.fullName) belongs to a different database instance.")
}
self.collectionConfigMap[collection] = config
}
}
So I don't think we need to check for this again at the integration level.
| replConfig.allowReplicatingInBackground = allowReplicationInBackground | ||
| replConfig.enableAutoPurge = autoPurgeEnabled | ||
| replConfig.heartbeat = TimeInterval(heartbeat.intValue) | ||
| replConfig.maxAttempts = UInt(maxAttempts.intValue) |
There was a problem hiding this comment.
there's direct conversion available here: maxAttempts.uintValue
| replConfig.acceptOnlySelfSignedServerCertificate = acceptSelfSignedCerts | ||
| replConfig.allowReplicatingInBackground = allowReplicationInBackground | ||
| replConfig.enableAutoPurge = autoPurgeEnabled | ||
| replConfig.heartbeat = TimeInterval(heartbeat.intValue) |
There was a problem hiding this comment.
If entry is NSNumber then it should be TimeInterval(heartbeat.doubleValue)
| replConfig.enableAutoPurge = autoPurgeEnabled | ||
| replConfig.heartbeat = TimeInterval(heartbeat.intValue) | ||
| replConfig.maxAttempts = UInt(maxAttempts.intValue) | ||
| replConfig.maxAttemptWaitTime = TimeInterval(maxAttemptWaitTime.intValue) |
There was a problem hiding this comment.
Same here TimeInterval(maxAttemptWaitTime.doubleValue)
| let heartbeat = data["heartbeat"] as? NSNumber, | ||
| let maxAttempts = data["maxAttempts"] as? NSNumber, | ||
| let maxAttemptWaitTime = data["maxAttemptWaitTime"] as? NSNumber, |
There was a problem hiding this comment.
I am not sure about corresponding types on JS for TimeInterval, but if you are looking for ObjC types here, these are the native CBL's:
NSTimeInterval heartbeat;
NSUInteger maxAttempts;
NSTimeInterval maxAttemptWaitTime;
While for Swift - already reflected down the line when calling setting up the CBL args:
heartbeat: TimeInterval
maxAttempts: UInt
maxAttemptWaitTime: TimeInterval
I am wondering if we can avoid converting number values as much as possible, what is the expected value from the user end on RN end? Can we expect something else than a generic NSNumber here?
CC @pasin for thoughts on this and the conversion below
| if let pinnedCert = pinnedServerCertificate.data(using: .utf8) { | ||
| replConfig.pinnedServerCertificate = SecCertificateCreateWithData(nil, pinnedCert as CFData) | ||
| print("[OLD API Step 6] ✅ Pinned certificate set") | ||
| } else { | ||
| print("[OLD API Step 6] ⚠️ Pinned certificate data conversion failed") | ||
| } |
There was a problem hiding this comment.
There are a couple of settings which were already part of the ReplicatorConfiguration class, such as this pinnedServerCertificate. Is fine if we can't separate the logic enough to have them set in the same place but should be doing exactly the same for both new and old methods. This should use base64
|
|
||
| /// **[NEW API]** Creates ReplicatorConfiguration with typed DTOs | ||
| /// | ||
| /// **Internal function** - Called after JSON parsing and format detection |
| if let authenticator = data["authenticator"] as? [String: Any], | ||
| let type = authenticator["type"] as? String, | ||
| let authData = authenticator["data"] as? [String: Any] { | ||
| print("[OLD API Step 7] Authenticator type:", type) | ||
| switch type { | ||
| case "basic": | ||
| if let username = authData["username"] as? String, | ||
| let password = authData["password"] as? String { | ||
| replConfig.authenticator = BasicAuthenticator(username: username, password: password) | ||
| print("[OLD API Step 7] ✅ Basic authenticator set") | ||
| } | ||
| case "session": | ||
| if let sessionID = authData["sessionId"] as? String { | ||
| let cookieName = authData["cookieName"] as? String ?? "SyncGatewaySession" | ||
| replConfig.authenticator = SessionAuthenticator(sessionID: sessionID, cookieName: cookieName) | ||
| print("[OLD API Step 7] ✅ Session authenticator set") | ||
| } | ||
| default: | ||
| print("[OLD API Step 7] ⚠️ Unknown authenticator type") | ||
| break | ||
| } | ||
| } else { | ||
| print("[OLD API Step 7] ℹ️ No authenticator") | ||
| } |
There was a problem hiding this comment.
For the new API section above is different. It also has not changed for CBL in between 3.2-4.0 so either we have the same implementation either we set this into a helper method alongside pinnedCert and the others.
For the new API, the only order imposed is that CollectionConfiguration exists before ReplicatorConfiguration is instantiated. Saying that, we can still set specific ReplicatorConfiguration params at the end, before the init of the Replicator itself, for both the new and old API. It might be possible that all this logic that has not changed to be only in one place (in a helper function).
No description provided.