Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Migrate from sqflite to drift packages #1266

Merged
merged 6 commits into from
Apr 3, 2024

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented Apr 2, 2024

Pull Request Description

This PR migrates our use of sqflite to drift. In short, Drift is a library that builds on top of SQLite to provide some really useful features, including:

  • Convenience methods for writing database queries using dart and raw SQL
  • Code gen which generates classes from tables
  • and more as mentioned in the pub description

This is part of a bigger picture which will allow us to provide an offline experience (#322) and other improvements. While using the existing sqflite would work for these purposes, it's much more difficult to accomplish due to the fact that we have to write raw SQL queries, and create classes for each of the associated tables. Having to do this hinders the DX and makes it more tedious to accomplish.

I'm hoping that this solution will help mitigate those issues.

This migration will likely happen in multiple stages:

  1. Remove usages of sqflite, and migrate all existing information to sqlite using drift. I'm not entirely sure if sqflite and sqlite are compatible with each other, so I opted to create a whole new database, and manually perform data migration. This is what this PR accomplishes.
  2. Reduce code complexity. This means removing Account, Favorite, and LocalCommunitymodels in favour of the autogen classes provided by drift, and refactoring parts of the code to use the new autogenerated classes.
  3. Once the databases have been migrated to sqlite, remove usages of sqflite from our codebase. This includes removing the sqflite dependencies. This step will likely occur in the future after a couple of versions
  4. Work on additional improvements once we have this framework up. This could include things such as caching API calls into the database for offline use, improve the anonymous account experience, etc.

Update: I've created some tests to check proper migration

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@hjiangsu hjiangsu marked this pull request as ready for review April 3, 2024 00:19
@hjiangsu
Copy link
Member Author

hjiangsu commented Apr 3, 2024

Alright, this should be ready now! I believe I've tested as many cases as I could (both on emulator and on my physical device).

Everything seems to look okay from what I can see. For now, I've opted to not remove the original database just in case it needs to be restored. The "Delete Local Database" action in Settings only deletes the new database (for now) as a way to test migration.

@micahmo If you are able to, could you try this out and let me know if you encounter any issues on your end? Thanks!

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

The code LGTM! I did a very brief test (basically just running the app) and things seem to be working ok!

@hjiangsu
Copy link
Member Author

hjiangsu commented Apr 3, 2024

I'll go ahead and merge this in. I'll generate a nightly build with the current changes, and give a week for any last minute fixes before I finalize 0.3.0 for general release!

@hjiangsu hjiangsu merged commit b22951c into develop Apr 3, 2024
1 check passed
@hjiangsu hjiangsu deleted the feature/database-migration branch April 3, 2024 14:31
@micahmo
Copy link
Member

micahmo commented Apr 3, 2024

@hjiangsu I noticed one issue so far. If you use the temporary account switcher, it seems to lose the actual account afterwards on the account page. The drawer and switcher are still correct.

qemu-system-x86_64_5h5YEyvBbp.mp4

Let me know if you want me to look at this (since I worked on the temp account switcher) or you (since you added the new db).

@hjiangsu
Copy link
Member Author

hjiangsu commented Apr 3, 2024

Hmm, I can take a quick look at this!

@hjiangsu
Copy link
Member Author

hjiangsu commented Apr 3, 2024

Update: I don't think this is directly related to the DB changes. It seems like it's an issue with the reload parameter whenever we switch accounts temporarily.

When we switch accounts, this line triggers in on<SwitchAccount>:

emit(state.copyWith(status: AuthStatus.loading, isLoggedIn: false));

Since reload isnt set here, reload = true by default and the account page changes to the non-logged in status (because of isLoggedIn: false)

When we restore the original user, reload is still set to false so the account page doesn't update. Note that the account page updates properly if we open the drawer before navigating to the account page

@hjiangsu
Copy link
Member Author

hjiangsu commented Apr 3, 2024

I believe the easiest solution for now would be to remove the if (!state.reload) return; checks on account_page.dart. This will cause some loading screens if we switch to the account page fast enough, but would ensure that the account page always reflects the correct user.

I did try some other alternatives but they caused issues with reloading the proper user when temporarily switching accounts, etc. I think a longer term solution for this would likely have to do with merging AccountBloc and AuthBloc together into one to make it easier to manage changes in account states.

@micahmo
Copy link
Member

micahmo commented Apr 3, 2024

Thanks so much for taking a look at this! If it's not related to the db upgrade then I can look further as well.

I think I had added if (!state.reload) return; to the account page because you can temporarily switch accounts from the account settings page, which is directly above the account page, meaning that navigating back would always show some weird switching.

Here's what it looks like now.

qemu-system-x86_64_7TSFpuN5oL.mp4

And here's what it looks like with those lines. Not the end of the world, but not super clean either.

qemu-system-x86_64_XSIYNpo7tM.mp4

Do you think it's worth my time to do any further investigation, or should we just remove those lines?

@hjiangsu
Copy link
Member Author

hjiangsu commented Apr 3, 2024

You can give it a shot if you'd like to find an alternate solution! I don't think its a particularly large issue since this use-case won't be triggered often, but having smoother transitions is always good.

@micahmo
Copy link
Member

micahmo commented Apr 4, 2024

Quick update! First, you are right that this doesn't seem to be related to the new db. I was able to repro in a commit before that change. It's just a very specific sequence of steps that I might have never tried before.

  1. Launch app fresh.
  2. Navigate to account page.
  3. Navigate back to main feed.
  4. Create post.
  5. Change user posting as.
  6. Close post page.
  7. Navigate to account page.

Lastly, I think you were right that the initial emit for refreshing is probably what's causing problems. It seems like adding the reload parameter helps that case. I'll open a PR for this change!

This was referenced Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants