Page MenuHomePhabricator

Rollback SQLite migration to shared location
ClosedPublic

Authored by marcin on Jun 1 2022, 8:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 8:02 PM
Unknown Object (File)
Wed, Nov 13, 7:58 PM
Unknown Object (File)
Tue, Nov 12, 2:31 PM
Unknown Object (File)
Sat, Nov 2, 2:36 PM
Unknown Object (File)
Sat, Nov 2, 2:36 PM
Unknown Object (File)
Sat, Nov 2, 2:36 PM
Unknown Object (File)
Sat, Nov 2, 2:36 PM
Unknown Object (File)
Sat, Nov 2, 2:36 PM

Details

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 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.

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?

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.

marcin edited the test plan for this revision. (Show Details)

Implement re-migration from app group location in case database exists under app group path.

This revision is now accepted and ready to land.Jun 6 2022, 4:42 AM

Rebase to master before landing

native/ios/Comm/AppDelegate.mm
239

Noticed a typo here: "errores"