Details

Reviewers

tomek
• karol
ashoat

Commits

rCOMMa9c364854643: Make App Groups SQLite path a default one. Implement migration from previous…

Summary

This diff changes implementation of getSQliteFilePath method to return new sqlite path defined by App Groups. Previous version of the method is also kept under different name. It also implements migrationfrom old to new path. Migration uses std::rename instead of NSFileManager API since, std::rename documentation explicitly states that file content shall not be altered in the case of failure. NSFileManager probably works similar but I could not find confirmation in its documentation.

Test Plan

Unistall Comm, build app with new code and install it again. Record new path defined by App Groups. Unistall comm, build with previous version of getSQliteFilePath. Check that the path looks like previous versions. Finally build the new code again, install it, check that database is again under app groups path and the app works correctly which means migration was succesfull.

Diff Detail

Repository

rCOMM Comm

Branch

marcin/migrate-sqlite

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.May 10 2022, 4:54 AM

Herald added subscribers: • abosh, atul, • adrian, ashoat. · View Herald TranscriptMay 10 2022, 4:54 AM

marcin added a parent revision: D3992: Enable App Groups in Comm and Notification Service.May 10 2022, 4:54 AM

Harbormaster completed remote builds in B8985: Diff 12495.May 10 2022, 5:01 AM

marcin requested review of this revision.May 10 2022, 5:01 AM

ashoat added inline comments.May 10 2022, 6:21 AM

native/ios/Comm/AppDelegate.mm
237 ↗	(On Diff #12495)	Nit: would it be best to stick with a consistent `//` comment syntax?
246–248 ↗	(On Diff #12495)	This pattern should always be avoided: if (cond1) { if (cond2) { ... } } It can always be simplified to: if (cond1 && cond2) { ... } As a general rule, you should try to keep indentation to a minimum level.

tomek requested changes to this revision.May 11 2022, 5:22 AM

tomek added inline comments.

native/ios/Comm/Tools.mm
8–33 ↗	(On Diff #12495)	The logic for determining the url / path is different. Could you share a doc that describes how to access group url / create db path?

This revision now requires changes to proceed.May 11 2022, 5:22 AM

marcin added inline comments.May 11 2022, 5:34 AM

native/ios/Comm/Tools.mm
8–33 ↗	(On Diff #12495)	I am not sure what do you mean? I did not change the logic for obtaining db path. This diff just changes the NSFileManager method to obtain directory in which it should be located to get directory for the App Group.

Be consistent about comment style. Minimize indenation.

Harbormaster completed remote builds in B9010: Diff 12548.May 11 2022, 8:12 AM

tomek accepted this revision.May 12 2022, 2:02 AM

tomek added inline comments.

native/ios/Comm/Tools.mm
8–33 ↗	(On Diff #12495)	I was asking if you are sure that this is the proper way to get a path in the app group. It seems fine, but wanted to make sure that what you did is a good practice.