Rollback SQLite migration to shared location
ClosedPublic
Actions

Authored by marcin on Jun 1 2022, 8:24 AM.

Details

Reviewers

tomek
atul

Commits

rCOMMa45ef9d83e06: Rollback SQLite migration to shared location

Summary

This differential rollbacks SQLite file migration from app-specific to app-groups location

Test Plan

Comment re-migration code and use getAppGroupSQLiteFilePath instead. Build the app, check that the path is an app group path. Then uncomment re-migration code, use getSQLiteFilePath and build the app. Check that it works and sqlite file path is app-specific and no database migration is executed. It is essential that app is not uninstalled between those two steps.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

marcin created this revision.Jun 1 2022, 8:24 AM

Herald added subscribers: • abosh, • adrian, ashoat. · View Herald TranscriptJun 1 2022, 8:24 AM

Harbormaster completed remote builds in B9575: Diff 13273.Jun 1 2022, 8:31 AM

marcin requested review of this revision.Jun 1 2022, 8:31 AM

tomek requested changes to this revision.Jun 1 2022, 9:49 AM

tomek added inline comments.

native/ios/Comm/AppDelegate.mm
247–249 ↗	(On Diff #13273)	Shouldn't we do something opposite: move from `sqliteFilePath` to `[appSpecificSQLiteFilePath UTF8String]`?

This revision now requires changes to proceed.Jun 1 2022, 9:49 AM

marcin requested review of this revision.Jun 3 2022, 5:03 AM

marcin added inline comments.

native/ios/Comm/AppDelegate.mm
247–249 ↗	(On Diff #13273)	This differential reverts changes that introduced `getAppSpecificSQLiteFilePath` method in the first place. Previously we had just one function in `Tools.h` called `getSQLiteFilePath` which returned database path that was app-specific. Changes that this differential introduces bring back former functionality and `[Tools getSQLiteFilePath]` returns app-specific path for SQLite database.

tomek added inline comments.Jun 3 2022, 5:57 AM

native/ios/Comm/AppDelegate.mm
247–249 ↗	(On Diff #13273)	Yes, but as a part of the solution we were moving the database from one location to another. Shouldn't we move it back to the app-specific location if `sqliteFilePath` exists?

marcin added inline comments.Jun 6 2022, 1:50 AM

native/ios/Comm/AppDelegate.mm
247–249 ↗	(On Diff #13273)	Are you asking because the code that migrates database was already released to users? If yes, then you are right. But I suggest that instead of `sqliteFilePath` and `appSpecificSQLiteFilePath` we use `sqliteFilePath` for the app specific (default) and appGroupSQLiteFilePath`. I think it is reasonable that the default sqlite path is accessed with the simplest method name. Otherwise we could be more straightforward and use `appSpecificSQLiteFilePath` and `appGroupSQLiteFilePath`. But we definitely should not use `sqliteFilePath` for the path that is not a default one and we cannot even tell (at this moment) when we are going to use it.