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

Migrations on SQLite: altering table to add i16 column fails due to no default value #14

Open
CrsiX opened this issue Nov 2, 2022 · 3 comments
Labels
discussion needed There is something to discuss before going on

Comments

@CrsiX
Copy link
Contributor

CrsiX commented Nov 2, 2022

Assuming you have a plain User model with something like ID and username.
If you want to add a new column age of type i16 without any further annotations, applying the migration fails.

Migration file:

[Migration]
Hash = '7006828879424352989'
Initial = false
Dependency = 2
Replaces = []

[[Migration.Operations]]
Type = 'CreateField'
Model = 'user'

[Migration.Operations.Field]
Name = 'age'
Type = 'int16'

[[Migration.Operations.Field.Annotations]]
Type = 'not_null'

Error output of the command rorm-cli migrate --database-config sqlite.toml --log-sql:

CREATE TABLE IF NOT EXISTS _rorm__last_migration (id INTEGER PRIMARY KEY AUTOINCREMENT, updated_at TEXT , migration_id INTEGER NOT NULL) STRICT; 
CREATE TRIGGER IF NOT EXISTS _rorm__last_migration_updated_at_auto_update_time AFTER UPDATE ON _rorm__last_migration FOR EACH ROW BEGIN UPDATE _rorm__last_migration SET updated_at = CURRENT_TIMESTAMP WHERE ROWID = NEW.ROWID; END;
SELECT migration_id FROM _rorm__last_migration ORDER BY id DESC LIMIT 1;
ALTER TABLE user ADD COLUMN age INTEGER NOT NULL;
Error: error returned from database: (code: 1) Cannot add a NOT NULL column with default value NULL

Caused by:
    (code: 1) Cannot add a NOT NULL column with default value NULL

In the Rust source, the model's field age doesn't have any annotations and therefore no default value as well:

#[derive(Clone, Model, Debug)]
pub struct User {
    #[rorm(id)]
    pub(crate) id: i64,
    #[rorm(max_length = 255)]
    pub(crate) username: String,

    pub(crate) age: i16,
}

This is probably caused by the migration annotation not_null, which is not present in the Rust source -- that's why no default value is set as well.

@myOmikron
Copy link
Member

myOmikron commented Nov 5, 2022

Yep that's correct.

I kind of expected that to happen, but I'm unsure what's the cleanest solution for this.

The underlying problem is, that for a column without a default set, the db assumes a DEFAULT NULL.
This clashes with the NOT NULL constraint, so the alter operation fails.

There are two solutions for this (see Django as reference):

  • Provide a default value via migrator that gets written in the migration. There must also be a flag for the migrator to recognize this default as a default that doesn't originated from the internal models.
  • Provide a default value in code and rerun the make-migrations tools.

@WebFreak001
@gammelalf

@myOmikron myOmikron added the discussion needed There is something to discuss before going on label Nov 5, 2022
@gammelalf
Copy link
Member

I think I'd like to have both.

In theory it's cleaner to provide a default in your model declaration. But from django experience, I'd say that this doesn't always fit the model's meaning. Therefore, we should eventually support both.

In django, you'd sometimes have to write some custom code in the migration file to transform the existing rows in the way you'd like them to be. Since we're not python, this is not possible. But we support custom SQL migrations anyway so it should be fine.

Maybe a flag for the migrator stating, that a custom SQL query didn't change a table definition but just some of its rows?
This way the migrator could safely continue modifying tables down the chain.

@WebFreak001
Copy link
Member

Thinking how this is probably going to be used in production, I think it would be nice to only provide a kind-of-dummy default for the old rows in the table, which only get applied once when migrating. Then afterwards the default is removed again, so it's not possible to create new rows without the column set.

Implementation wise this could be done by adding the column with the default value, then altering the column to remove the default value again. In sqlite this would require a full table copy because it doesn't support altering columns iirc, but I think we need to implement that workaround for other migration types (altering columns) anyway?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed There is something to discuss before going on
Projects
None yet
Development

No branches or pull requests

4 participants