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

Migrate anonymous instances to database #1500

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Jul 17, 2024

Pull Request Description

This PR makes the following database changes.

  • Add index column to accounts table.
  • Migrate anonymous instances to accounts table.

There should be no impact on the UI at this point, so please let me know if anything looks or acts funky after this migration!

Related to #1494.

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

Just did a quick skim of the changes - LGTM!

I'll do some testing on my end as well to check for any possible issues with this change. Thanks again for doing this! 😄

@hjiangsu hjiangsu merged commit 3233696 into thunder-app:develop Jul 17, 2024
1 check passed
@hjiangsu
Copy link
Member

Hmm, I just tried to go from a clean install of 0.4.0 to the current changes, and the accounts are not appearing in the profile switcher anymore (both normal accounts and guest accounts)

I am seeing this error on the logs which might be the issue:

flutter: SqliteException(1): while executing, duplicate column name: custom_thumbnail, SQL logic error (code 1)
flutter:   Causing statement: ALTER TABLE drafts ADD COLUMN custom_thumbnail TEXT, parameters:

I think we might have to do some checks to see if the columns exist before adding/removing them in the migration code (or add try catch blocks to them)

@hjiangsu
Copy link
Member

I'll try to make some changes to the migration code to fix some of the issues

@micahmo
Copy link
Member Author

micahmo commented Jul 17, 2024

duplicate column name

Are you sure it was really a clean install? I'm not sure how we could get duplicate column name unless maybe you already had that column in your database from some previous testing. Let me know what you find!

@micahmo micahmo deleted the feature/migrate-instances-to-db branch July 17, 2024 17:08
@hjiangsu
Copy link
Member

Are you sure it was really a clean install? I'm not sure how we could get duplicate column name unless maybe you already had that column in your database from some previous testing.

This is what I did:

  • Uninstalled Thunder from my device (this should remove all data associated with the app)
  • Installed Thunder from the latest nightly build
  • Added some accounts and anonymous instances
  • Upgraded to latest version

I think this is what happened:

  • If I run the migration from 0.4.0 up to the current version, the Drafts database will already contain custom_thumbnail when its created in await migrator.createTable(drafts), because its present in the current table definition
  • However, since this condition from <= 3 && to > 3 is also satisfied, the migration tries to add the column when it already exists

If I go from a version where the Drafts table is created (e.g., version 2), but does not contain custom_thumbnail, then I believe no error is thrown. Hopefully this makes sense!

I'm rewriting the migration logic right now so that it does migration step-by-step instead to hopefully avoid these types of situations: https://drift.simonbinder.eu/docs/migrations/step_by_step/#generating-step-by-step-code

@micahmo
Copy link
Member Author

micahmo commented Jul 17, 2024

Ahh yes, you are totally right about what happened! Good catch. And I like the stepByStep migration strategy. That should make things much easier. Thanks!

@hjiangsu hjiangsu mentioned this pull request Jul 17, 2024
3 tasks
@micahmo micahmo mentioned this pull request Jul 17, 2024
3 tasks
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