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

[Documentation] Add warnings about common migration problems #3140

Closed
HasanAlqaisi opened this issue Aug 11, 2024 · 4 comments
Closed

[Documentation] Add warnings about common migration problems #3140

HasanAlqaisi opened this issue Aug 11, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@HasanAlqaisi
Copy link

HasanAlqaisi commented Aug 11, 2024

Hi,

I had an issue with the manual migration that was hard to trace and it can happen when writing the migration code without enough knowledge or paying enough attention.

The issue is that I followed the instructions in the docs and wrote code like this:

    onUpgrade: (Migrator m, int from, int to) async {
    if (from < 2) {
      // We've changed title type to be nullable
      await m.alterTable(TableMigration(todoItems));
    }

    if (from < 3) {
      await m.addColumn(todoItems, todoItems.updatedAt);
    }
    },

When user updates directly from version 1 to 3 the following error will occur:

Error log

[ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: SqliteException(1): while executing, no such column: "updated_at" - should this be a string literal in single-quotes?, SQL logic error (code 1) Causing statement: INSERT INTO tmp_for_copy_todo_items ("id", "title", "body", "created_at", "updated_at") SELECT "id", "title", "body", "created_at", "updated_at" FROM "todo_items";, parameters:

The issue was happening to the users and it was hard to trace, but I found your comment here.
Although my code is not calling alter table twice on the same table explicitly but I understand that addColumn function uses it under the hood.

I think that using step-by-step migration will help to avoid this issue, but it would be useful for projects that are still using the old way & new package users to be warned about this kind of migration issues.

I suggest the following:

  • Add some lints to apply the best practices and warn against possible issues (I am not sure if this is applicable).
  • Add an obvious warning in the docs about the above issue (and similar issues if there are) that can lead to migration errors when using the manual way.
@HasanAlqaisi HasanAlqaisi added the enhancement New feature or request label Aug 11, 2024
@dickermoshe
Copy link
Collaborator

We're in middle of a complete docs refresh.
The new docs will do a better job of explaining how migrations should be done.

However we already warn how step-by-step migrations are a requirement to write a working migrations.

Drift makes is very easy to test and verify migrations.
It would be easier to build an automatic migration system than to build a linter that could figure out if the migrations are being done incorrectly.

Moving to step-by-step migrations is quite simple.

If you feel this is a reasonable position, close the issue. I won't close it myself, I don't like telling people their issues aren't important.

@simolus3
Copy link
Owner

A downside of step by step migrations is that users may still run into this problem with their existing migrations not covered by tests/step-by-step migrations.

I'll add a warning box to the main migration page strongly suggesting the use of these tools. With the rewrite we should probably just highlight the tools as the default way to do things and mention manual setups as a fallback.

@HasanAlqaisi
Copy link
Author

I agree with @dickermoshe about the linter, and moving to step-by-step migration is easy.

For now, my point is that when we see the onUpgrade function here, we tend to use the same way to write the migration code. So I suggest making it obvious that this way it discouraged.

Also it would be nice if we state in General tips something like 'Be careful when using the manual migration, don't let the code calls alterTable twice on the same table or  alterTable then addColumn

If you see that this will be taken care of and there is no need to let this issue open, feel free to close it :)

@simolus3
Copy link
Owner

I've updated the docs in f74c5fa to position the tools more prominently and to point out this footgun in particular.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants