fix: resolve encryption key null issue #74
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 addresses a critical issue where encryption keys and directory paths could cause null-related problems in iOS when performing database copy and open operations. The core fix involves making these parameters nullable and implementing robust type and value validation to ensure only meaningful data is processed. Beyond this specific fix, the PR also includes a substantial update of project dependencies, particularly for Expo development tools and the 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 addresses an issue where null encryption keys were causing problems on iOS by making the encryptionKey and directory parameters nullable for database copy and open functions. The changes in ios/CblReactnative.swift correctly handle nil, NSNull, and string representations of "null" or "undefined". However, this introduces significant code duplication in the database_Copy and database_Open functions. My review focuses on refactoring this duplicated logic into a private helper function to improve code maintainability and readability. The other changes, which include dependency updates and improvements to the .gitignore file, are appropriate and well-executed.
| var config: [AnyHashable: Any] = [:] | ||
|
|
||
| // Safely convert directory - only add if not nil/NSNull and not empty | ||
| if let directory = directory, !(directory is NSNull) { | ||
| // Convert to string, handling NSString, String, or other types | ||
| let dirStr: String | ||
| if let nsStr = directory as? NSString { | ||
| dirStr = nsStr as String | ||
| } else if let str = directory as? String { | ||
| dirStr = str | ||
| } else { | ||
| dirStr = String(describing: directory) | ||
| } | ||
| let trimmedDirStr = dirStr.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| if !trimmedDirStr.isEmpty && trimmedDirStr != "undefined" && trimmedDirStr != "null" { | ||
| config["directory"] = trimmedDirStr | ||
| } | ||
| } | ||
|
|
||
| // Safely convert encryptionKey - only add if not nil/NSNull and not empty | ||
| if let encryptionKey = encryptionKey, !(encryptionKey is NSNull) { | ||
| // Convert to string, handling NSString, String, or other types | ||
| let encKeyStr: String | ||
| if let nsStr = encryptionKey as? NSString { | ||
| encKeyStr = nsStr as String | ||
| } else if let str = encryptionKey as? String { | ||
| encKeyStr = str | ||
| } else { | ||
| encKeyStr = String(describing: encryptionKey) | ||
| } | ||
| let trimmedEncKeyStr = encKeyStr.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| if !trimmedEncKeyStr.isEmpty && trimmedEncKeyStr != "undefined" && trimmedEncKeyStr != "null" { | ||
| config["encryptionKey"] = trimmedEncKeyStr | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code for safely converting and adding directory and encryptionKey to the configuration dictionary is duplicated in the database_Open function. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, this logic should be extracted into a private helper function.
You could add the following helper function to your class:
private func addStringValue(from value: Any?, toConfig config: inout [AnyHashable: Any], withKey key: String) {
guard let value = value, !(value is NSNull) else { return }
let stringValue: String
if let nsStr = value as? NSString {
stringValue = nsStr as String
} else if let str = value as? String {
stringValue = str
} else {
stringValue = String(describing: value)
}
let trimmedString = stringValue.trimmingCharacters(in: .whitespacesAndNewlines)
if !trimmedString.isEmpty && trimmedString != "undefined" && trimmedString != "null" {
config[key] = trimmedString
}
}Then, you can simplify this block as shown in the suggestion.
| var config: [AnyHashable: Any] = [:] | |
| // Safely convert directory - only add if not nil/NSNull and not empty | |
| if let directory = directory, !(directory is NSNull) { | |
| // Convert to string, handling NSString, String, or other types | |
| let dirStr: String | |
| if let nsStr = directory as? NSString { | |
| dirStr = nsStr as String | |
| } else if let str = directory as? String { | |
| dirStr = str | |
| } else { | |
| dirStr = String(describing: directory) | |
| } | |
| let trimmedDirStr = dirStr.trimmingCharacters(in: .whitespacesAndNewlines) | |
| if !trimmedDirStr.isEmpty && trimmedDirStr != "undefined" && trimmedDirStr != "null" { | |
| config["directory"] = trimmedDirStr | |
| } | |
| } | |
| // Safely convert encryptionKey - only add if not nil/NSNull and not empty | |
| if let encryptionKey = encryptionKey, !(encryptionKey is NSNull) { | |
| // Convert to string, handling NSString, String, or other types | |
| let encKeyStr: String | |
| if let nsStr = encryptionKey as? NSString { | |
| encKeyStr = nsStr as String | |
| } else if let str = encryptionKey as? String { | |
| encKeyStr = str | |
| } else { | |
| encKeyStr = String(describing: encryptionKey) | |
| } | |
| let trimmedEncKeyStr = encKeyStr.trimmingCharacters(in: .whitespacesAndNewlines) | |
| if !trimmedEncKeyStr.isEmpty && trimmedEncKeyStr != "undefined" && trimmedEncKeyStr != "null" { | |
| config["encryptionKey"] = trimmedEncKeyStr | |
| } | |
| } | |
| var config: [AnyHashable: Any] = [:]; | |
| addStringValue(from: directory, toConfig: &config, withKey: "directory") | |
| addStringValue(from: encryptionKey, toConfig: &config, withKey: "encryptionKey") |
| var config: [AnyHashable: Any] = [:] | ||
|
|
||
| // Safely convert directory - only add if not nil/NSNull and not empty | ||
| if let directory = directory, !(directory is NSNull) { | ||
| // Convert to string, handling NSString, String, or other types | ||
| let dirStr: String | ||
| if let nsStr = directory as? NSString { | ||
| dirStr = nsStr as String | ||
| } else if let str = directory as? String { | ||
| dirStr = str | ||
| } else { | ||
| dirStr = String(describing: directory) | ||
| } | ||
| let trimmedDirStr = dirStr.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| if !trimmedDirStr.isEmpty && trimmedDirStr != "undefined" && trimmedDirStr != "null" { | ||
| config["directory"] = trimmedDirStr | ||
| } | ||
| } | ||
|
|
||
| // Safely convert encryptionKey - only add if not nil/NSNull and not empty | ||
| if let encryptionKey = encryptionKey, !(encryptionKey is NSNull) { | ||
| // Convert to string, handling NSString, String, or other types | ||
| let encKeyStr: String | ||
| if let nsStr = encryptionKey as? NSString { | ||
| encKeyStr = nsStr as String | ||
| } else if let str = encryptionKey as? String { | ||
| encKeyStr = str | ||
| } else { | ||
| encKeyStr = String(describing: encryptionKey) | ||
| } | ||
| let trimmedEncKeyStr = encKeyStr.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| if !trimmedEncKeyStr.isEmpty && trimmedEncKeyStr != "undefined" && trimmedEncKeyStr != "null" { | ||
| config["encryptionKey"] = trimmedEncKeyStr | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic for parsing the directory and encryptionKey is a duplication of the code in the database_Copy function. To improve code quality and maintainability, this should be refactored into a shared private helper function as suggested in my other comment.
| var config: [AnyHashable: Any] = [:] | |
| // Safely convert directory - only add if not nil/NSNull and not empty | |
| if let directory = directory, !(directory is NSNull) { | |
| // Convert to string, handling NSString, String, or other types | |
| let dirStr: String | |
| if let nsStr = directory as? NSString { | |
| dirStr = nsStr as String | |
| } else if let str = directory as? String { | |
| dirStr = str | |
| } else { | |
| dirStr = String(describing: directory) | |
| } | |
| let trimmedDirStr = dirStr.trimmingCharacters(in: .whitespacesAndNewlines) | |
| if !trimmedDirStr.isEmpty && trimmedDirStr != "undefined" && trimmedDirStr != "null" { | |
| config["directory"] = trimmedDirStr | |
| } | |
| } | |
| // Safely convert encryptionKey - only add if not nil/NSNull and not empty | |
| if let encryptionKey = encryptionKey, !(encryptionKey is NSNull) { | |
| // Convert to string, handling NSString, String, or other types | |
| let encKeyStr: String | |
| if let nsStr = encryptionKey as? NSString { | |
| encKeyStr = nsStr as String | |
| } else if let str = encryptionKey as? String { | |
| encKeyStr = str | |
| } else { | |
| encKeyStr = String(describing: encryptionKey) | |
| } | |
| let trimmedEncKeyStr = encKeyStr.trimmingCharacters(in: .whitespacesAndNewlines) | |
| if !trimmedEncKeyStr.isEmpty && trimmedEncKeyStr != "undefined" && trimmedEncKeyStr != "null" { | |
| config["encryptionKey"] = trimmedEncKeyStr | |
| } | |
| } | |
| var config: [AnyHashable: Any] = [:]; | |
| addStringValue(from: directory, toConfig: &config, withKey: "directory") | |
| addStringValue(from: encryptionKey, toConfig: &config, withKey: "encryptionKey") |
| newName: NSString, | ||
| directory: NSString, | ||
| encryptionKey: NSString, | ||
| directory: Any?, |
There was a problem hiding this comment.
on CBL iOS side, directory won't ever be nil/NSNull. It will get created into the application workspace if is not set by user - DatabaseConfiguration.swift
This is a test we have
func testDefaultDatabaseConfiguration() {
let config = DatabaseConfiguration()
XCTAssertEqual(config.directory, DatabaseConfiguration().directory)
#if COUCHBASE_ENTERPRISE
XCTAssertNil(config.encryptionKey)
#endif
}
| var config: [AnyHashable: Any] = [:] | ||
|
|
||
| // Safely convert directory - only add if not nil/NSNull and not empty | ||
| if let directory = directory, !(directory is NSNull) { |
There was a problem hiding this comment.
this check not needed if the above holds for RN wrapper
There was a problem hiding this comment.
The directory is handled null from client side in RN.
For example, if a user is creating a React Native app and in that app if the user passed a directory as null, we are handling that here. So that a hash of directory : NSNull is not passed to IOS, IOS will either receive empty [:] or with directory,
Also if any tests are there so that we validate them
| // Safely convert encryptionKey - only add if not nil/NSNull and not empty | ||
| if let encryptionKey = encryptionKey, !(encryptionKey is NSNull) { | ||
| // Convert to string, handling NSString, String, or other types | ||
| let encKeyStr: String | ||
| if let nsStr = encryptionKey as? NSString { | ||
| encKeyStr = nsStr as String | ||
| } else if let str = encryptionKey as? String { | ||
| encKeyStr = str | ||
| } else { | ||
| encKeyStr = String(describing: encryptionKey) | ||
| } | ||
| let trimmedEncKeyStr = encKeyStr.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| if !trimmedEncKeyStr.isEmpty && trimmedEncKeyStr != "undefined" && trimmedEncKeyStr != "null" { | ||
| config["encryptionKey"] = trimmedEncKeyStr | ||
| } | ||
| } |
There was a problem hiding this comment.
in the future, we should consider moving "complex" validation logic separate so we can do it on demand everywhere
Fix for https://jira.issues.couchbase.com/browse/CBL-7495
We have made encryption key and directory nullable for database copy and database open functions, along with handling NSstring and String in IOS
Issue was not replicable on android