Automatically migrating models does not work well with new installs

[emilsjolander]

When doing migration.createTable() and then adding a column with migration.addColumn() it fails on new installs.

There are two solutions. We could automatically track changes of models and migrate when necessary, this would probably be hard to integrate with custom migrations in a predictable way which would be the downside.

The other solution would be to drop auto migrating of models and the user would need to manually migrate all columns. This would however cause the need for bigger changes as annotations currently made on columns would be unnecessary as those constraints should be added on the migration (which might be better than having to annotate properties?).

[emilsjolander]

Some suggestions

https://gist.github.com/emilsjolander/9991512

[jayohms]

I can see the dilemma here. I think that initial database model creation should be as simple as possible, without having to redefine a model that's already present in the provided Model annotations.

Additionally, trying to do too much "magic" on migrations always seems a bit dangerous. Is there a fundamental reason why database versioning is not exposed to manage database upgrades?

Here's a model that I can see solving these issues:

int DB_VERSION_NUMBER = 3;

Sprinkles s = Sprinkles.init(this, DB_VERSION_NUMBER);
s.checkMigration(new Migrator() {
    void onCreate() {
        // Happenened at version 1
        s.registerModel(Note.class)
    }
    void onMigrate(Migration m, int oldVersion, int newVersion) {
        if (oldVersion < 2) {
            m.addColumn(Note.class, "first_new_column_annotation_name");
        }
        if (oldVersion < 3) {
            m.addColumn(Note.class, "second_new_column_annotation_name");
        }
    }
});

I'm not intimately familiar with the code internals yet, so maybe I'm missing something. But this approach allows for flexibility, leveraging the Models we create and minimizing the need for raw sql.

[karllindmark]

Just my two cents (a bit unrelated) - the new Migration system from the snapshot works great Emil. :-) On Apr 10, 2014 7:39 PM, "Jay Ohms" notifications@github.com wrote:

I can see the dilemma here. I think that initial database model creation should be as simple as possible, without having to redefine a model that's already present in the provided Model annotations.

Additionally, trying to do too much "magic" on migrations always seems a bit dangerous. Is there a fundamental reason why database versioning is not exposed to manage database upgrades?

Here's a model that I can see solving these issues:

int DB_VERSION_NUMBER = 3;

Sprinkles s = Sprinkles.init(this, DB_VERSION_NUMBER); s.checkMigration(new Migrator() { void onCreate() { // Happenened at version 1 s.registerModel(Note.class) } void onMigrate(Migration m, int oldVersion, int newVersion) { if (oldVersion < 2) { m.addColumn(Note.class, "first_new_column_annotation_name"); } if (oldVersion < 3) { m.addColumn(Note.class, "second_new_column_annotation_name"); } } });

I'm not intimately familiar with the code internals yet, so maybe I'm missing something. But this approach allows for flexibility, leveraging the Models we create and minimizing the need for raw sql.

Reply to this email directly or view it on GitHubhttps://github.com/emilsjolander/sprinkles/issues/67#issuecomment-40114693 .

[jayohms]

Additionally, one of the main reason I really love Sprinkles is that annotations on the Model define everything that's necessary. No database logic code is necessary anywhere else.

Straying away from that Model/annotation simplicity would seem like the wrong direction. Sprinkles is no joke on of my favorite libraries in all my Android dev, and I'd like to see it maintain that simplicity that 1.2.4 currently offers :)

[emilsjolander]

@jayohms thanks for the input :+1:, regarding not exposing the database version, i just thought it would make things easier without causing any harm. Checking the database version on upgrade manually can easily lead to typos and bad bugs.

What i like with the 1.3.0 system:

• Impossible to do illegal migrations (removing a column constraint after a column has been created for example).
• You can look at the SQLite docs for help if you wan to do any non standard things.

What i don't like with the proposed 1.3.0 system:

• Repetition of column names and primary key properties

What we could do:

• Begin with registering models with s.registerModel().
• Add migrations for each version upgrade with s.registerMigration()
• New installs will never run any migrations registered with s.registerMigration(). As all the current database properties are defined in the model registered with s.registerModel() there is no need to run them to "get up to speed".

What do you think of this?

[jayohms]

@emilsjolander I like your proposal, specifically the idea of only running migrations for upgrades. This allows Models to be easily registered (and all tables created on install). Would s.registerMigration() do an automatic migration based on the updated Model, or ask for the specific incremental column/table updates?

And by all means, if you want to also expose raw SQL execution for more power/flexibility for advanced SQLite use, that's great. My major concern was requiring the Model be redefined in raw SQL for something standard and simple.

[jayohms]

@emilsjolander Also, how will databases be upgraded that are older than 1 version out of date, or have applied some migrations but not others yet? Not all migrations may need to run in certain instances.

I know version numbers make things less "simple", but it does remove all ambiguity and offers precision. If they can be avoided - great, but introducing too many workarounds to avoid a versioning model (which is a proven approach) may create too many drawbacks.

[vinc3m1]

I think hiding version numbers would be confusing. Exposing version numbers would make it much more explicit and clear which migrations are running.

From experience with web development I really like django's south migration pattern where migrations are generated for you based on your models, and then allow you to customize after. You're still repeating code from your model definition, but the code gen prevents typos. Idk if there's an easy way to do that with Android, might have to be a gradle extension.

[emilsjolander]

Sorry for the delay! School and stuff :+1:

First of all, regarding the database version, i would gladly add it if i see a valid reason to have it. But as long as each migration is a separate object i see no reason to just handle the logic och which migration to run internally in sprinkles.

I've been using the 1.3.0-SNAPSHOT migration system for an app i'm building and i think it works great. I do however also like the simplicity of just migrating a model class and specifying the constraints on the model. I am a bit torn between the two solutions because when i set out to make sprinkles one of the main goals was to not recreate SQL in a java API, i think these solutions always end up bad even though they might look pretty at first. This is however exactly what the model annotations have become. Everything from autoincrement to check and index are being specified in the model which not only clutters up the model class but also makes for an api that is hard to use as it requires mastering both sqlite and sprinkles.

As you might have guessed i am leaning towards sticking with a slightly modified version of the current 1.3.0-SNAPSHOT build. Switching away from specifying everything in the model class does come with downsides and i am aware of them. One of them is needing to start a project by writing raw SQL for each model which might not be very fun but it's also much less of a hassle than one might think. Because auto migrations would only really work for the initial migration i don't see this as a very big loss.

An upside of starting out with a most low level of migration API's (while still providing some convenience) it could be expanded with helper functions in the future without losing any backwards compatibility.

I am open to any convincing arguments to why this is a horrible move :+1: