Test15#162
Conversation
![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 WebDAV backup and restore functionality, secured with biometric authentication for configuration. It overhauls the Export and Import UIs using Material 3 components like Sliders and Chips, and adds an info banner for user guidance. Key stability improvements include preventing crashes from zero-value grid sizes, fixing a potential infinite loop in the RateLimiter, and separating filter preferences for movies and TV shows. Review feedback highlights critical issues in the WebDAV implementation, specifically the need to restrict imports to .db files to prevent database corruption and ensuring shared database instances are not prematurely closed in background workers. Improvements were also suggested for the thread-safety of preference initialization and the accuracy of the WebDAV connection test.
| val backupFileType = androidx.preference.PreferenceManager.getDefaultSharedPreferences(this).getString("backup_file_type", "DB") | ||
| val fileName = when (backupFileType) { | ||
| "JSON" -> "movies.json" | ||
| "CSV (Movies and Shows)" -> "movies.csv" | ||
| "CSV (All data)" -> "movies_with_episodes.csv" | ||
| else -> "movies.db" | ||
| } |
There was a problem hiding this comment.
Critical bug: The current implementation allows downloading JSON or CSV files from WebDAV and then copies their content directly into the SQLite database file (movies.db). This will result in database corruption as SQLite cannot parse JSON/CSV as a database format. Since the app explicitly states that only .db format is supported for imports, you should restrict the WebDAV restore process to .db files only.
val backupFileType = androidx.preference.PreferenceManager.getDefaultSharedPreferences(this).getString("backup_file_type", "DB")
if (backupFileType != "DB") {
Toast.makeText(this, getString(R.string.json_import_not_supported), Toast.LENGTH_SHORT).show()
return
}
val fileName = "movies.db"| val backupFileType = androidx.preference.PreferenceManager.getDefaultSharedPreferences(this).getString("backup_file_type", "DB") | ||
| val fileName = when (backupFileType) { | ||
| "JSON" -> "movies.json" | ||
| "CSV (Movies and Shows)" -> "movies.csv" | ||
| "CSV (All data)" -> "movies_with_episodes.csv" | ||
| else -> "movies.db" | ||
| } |
There was a problem hiding this comment.
Critical bug: The current implementation allows downloading JSON or CSV files from WebDAV and then copies their content directly into the SQLite database file (movies.db). This will result in database corruption as SQLite cannot parse JSON/CSV as a database format. Since the app explicitly states that only .db format is supported for imports, you should restrict the WebDAV restore process to .db files only.
val backupFileType = androidx.preference.PreferenceManager.getDefaultSharedPreferences(this).getString("backup_file_type", "DB")
if (backupFileType != "DB") {
Toast.makeText(this, getString(R.string.json_import_not_supported), Toast.LENGTH_SHORT).show()
return
}
val fileName = "movies.db"|
|
||
| val response = getClient().newCall(requestBuilder.build()).execute() | ||
| response.close() | ||
| return response.isSuccessful || response.code == 405 || response.code == 404 |
There was a problem hiding this comment.
Including HTTP 404 (Not Found) as a successful connection test is misleading. If a user provides a URL to a specific file that does not exist, the connection test should ideally fail or indicate that the resource was not found, rather than showing a "Connection successful!" message. This could lead to confusion when subsequent backup or restore operations fail.
| return response.isSuccessful || response.code == 405 || response.code == 404 | |
| return response.isSuccessful || response.code == 405 |
| fun getEncryptedSharedPreferences(context: Context): SharedPreferences { | ||
| if (sharedPreferences == null) { | ||
| val masterKey = MasterKey.Builder(context) | ||
| .setKeyScheme(MasterKey.KeyScheme.AES256_GCM) | ||
| .build() | ||
|
|
||
| sharedPreferences = EncryptedSharedPreferences.create( | ||
| context, | ||
| PREFS_FILENAME, | ||
| masterKey, | ||
| EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, | ||
| EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM | ||
| ) | ||
| } | ||
| return sharedPreferences!! | ||
| } |
There was a problem hiding this comment.
The initialization of sharedPreferences is not thread-safe. Multiple threads calling getEncryptedSharedPreferences simultaneously could result in multiple instances being created or race conditions. Consider using a synchronized block or a thread-safe initialization pattern.
| databaseHelper.getJSONExportString(db) | ||
| } | ||
| output.write(json.toByteArray()) |
There was a problem hiding this comment.
Using .use on databaseHelper.readableDatabase will close the database instance. If MovieDatabaseHelper provides a shared database connection (which is common for performance and consistency), closing it here might cause crashes or errors in other parts of the app that are still using the database. It is generally safer to let the OpenHelper manage the database lifecycle and only close cursors. This also applies to lines 89 and 97.
| databaseHelper.getJSONExportString(db) | |
| } | |
| output.write(json.toByteArray()) | |
| val json = databaseHelper.getJSONExportString(databaseHelper.readableDatabase) |
1 participant
No description provided.