Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #195 Added Migration code for sqlite files from Document to Application Support folder #1026

Merged
merged 5 commits into from
Aug 5, 2019
Merged

Fix #195 Added Migration code for sqlite files from Document to Application Support folder #1026

merged 5 commits into from
Aug 5, 2019

Conversation

danishjafri88
Copy link
Contributor

@danishjafri88 danishjafri88 commented Apr 2, 2019 •
edited by iccub
Loading

Sqlite file is now migrated to the new folder. There two questions that need to be answered/invetigated before merge.

  1. How will this function when there is data model version change(ie DB migration).
  2. Before we allow iTunesFileSharing, we need to make sure all contents of Documents Directory are allowed for user visibility.
    Fore Example: https-everywhere-data folder and Locally saved cookies(can be moved to Application support).

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • My patch or PR title has a standard commit message that looks like Fix #123: This fixes the shattered coffee cup! (or No Bug: <message> if no relevant ticket)
  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()
  • New files have MPL-2.0 license header.

Test Plan:

We have to test if the app upgrades correctly so:

  • install previous app version, add few bookmarks, visit some websites so history is filled
  • upgrade to version with this PR merged
  • verify it doesn't crash, and Bookmarks/History are staying

Screenshots:

Before:
Screenshot 2019-07-25 at 12 22 59 PM
After:
Screenshot 2019-07-25 at 12 25 03 PM

Reviewer Checklist:

  • PR is linked to an issue via Zenhub.
  • Issues are assigned to at least one epic.
  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable)

@danishjafri88 danishjafri88 changed the title Fix #195 Added Migration code for sqlite files from Document to Application Support folder [WIP] Fix #195 Added Migration code for sqlite files from Document to Application Support folder Apr 3, 2019
@danishjafri88 danishjafri88 force-pushed the enhancement/fix.issue.195 branch from 9aa1aae to 1c170e7 Compare April 10, 2019 22:41
@danishjafri88 danishjafri88 changed the title [WIP] Fix #195 Added Migration code for sqlite files from Document to Application Support folder Fix #195 Added Migration code for sqlite files from Document to Application Support folder Apr 10, 2019
@iccub iccub mentioned this pull request Jul 24, 2019
Merged
14 tasks
@iccub iccub requested review from jhreis and iccub July 24, 2019 20:48
jhreis
jhreis previously requested changes Jul 30, 2019
View reviewed changes
Client/Application/AppDelegate.swift Outdated
@@ -64,6 +64,8 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UIViewControllerRestorati

HttpsEverywhereStats.shared.startLoading()

//Moves Coredata sqlite file from Documents dir to application support dir.
DataController.shared.migrateToNewPathIfNeeded()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably move this into the migration, and just let the migration call below handle this if possible. Is there a timing issue or something?

Best not to require different migrations to happen at different places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I need the db migration to run before any db access. If I can enforce that no one will ever write anything DB before migration code then we can move it there. Any ideas we can do that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there are issues right now, I think this is relatively safe to assume. Migrations should generally always happen before DB access, and hasn't been an issue this far (famous last words, right?).

Also, it would only matter if we accessed the DB before someone had this migration take place, so after a couple months, this migration will likely not be very useful/important.

If we have genuine concerns about this, we should consider moving the entire migration process up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in DM, The placement of the db migration is curcial because of !DataController.shared.storeExists() check. Create a facade for the DB migration in Migration Code though.

iccub
iccub reviewed Jul 30, 2019
View reviewed changes
Data/models/DataController.swift Outdated
fileprivate extension URL {

func sqliteFiles(dbName: String) throws -> [URL] {
return try FileManager.default.contentsOfDirectory(at: self, includingPropertiesForKeys: nil, options: []).filter({$0.lastPathComponent.hasPrefix(dbName)})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't that retrieve all files, not only with .sqlite extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes thats intended. I want to retrieve all files that have a name prefix Brave.sqlite like Brave.sqlite-wal etc. I think keeping this function in an extension may not be the best way, so moving it into the migration method itself.

@danishjafri88 danishjafri88 force-pushed the enhancement/fix.issue.195 branch from 8e8323a to fd7b1ba Compare July 30, 2019 19:36
@danishjafri88 danishjafri88 requested review from jhreis and iccub July 30, 2019 19:36
iccub
iccub suggested changes Jul 31, 2019
View reviewed changes
Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good

Client/Application/AppDelegate.swift Outdated
@@ -64,8 +64,10 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UIViewControllerRestorati

HttpsEverywhereStats.shared.startLoading()


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

@iccub iccub added the blocked If a ticket is blocked for some reason, if not using a sub-block label, please provide info in issue label Jul 31, 2019
@danishjafri88 danishjafri88 force-pushed the enhancement/fix.issue.195 branch from fd7b1ba to ea25db6 Compare July 31, 2019 15:29
@iccub iccub removed the blocked If a ticket is blocked for some reason, if not using a sub-block label, please provide info in issue label Aug 1, 2019
@iccub iccub dismissed jhreis’s stale review August 5, 2019 17:22

Joel Discussed the issue he had with Danish and DM and agreed to leave it as it is

@iccub iccub merged commit c0aa10c into development Aug 5, 2019
@iccub iccub deleted the enhancement/fix.issue.195 branch August 5, 2019 17:25
jhreis added a commit that referenced this pull request Sep 19, 2019
@jhreis
Revert "Fix #195 Migrate database from Documents to Application Suppo…
592298f 
…rt folder (#1026)"

This reverts commit c0aa10c.
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@iccub iccub iccub approved these changes

@jhreis jhreis Awaiting requested review from jhreis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danishjafri88 @jhreis @iccub