foundation of simple db migration

[debris]

No description provided.

[tomusdrw]

What's the reason of having from and to version?
Is it possible to go 3->2?
What is the use case of skipping a number? (like going 1->3 instead of 1->2->3 - what happens if there is this kind of amibguity?)

Wouldn't a to_version property for migration be sufficient? (you always go from to_version-1 and only one way)

[debris]

Wouldn't a to_version property for migration be sufficient? (you always go from to_version-1 and only one way)

Not entirely. to_version and from_version guarantees data integrity. There won't be a single database version without migration rules. Also with from_version and to_version it's gonna be easier to squash changes after some time.

[gavofyork]

it's gonna be easier to squash changes after some time.

POIROAE (premature optimisation is the root of all evil)

having separate rules for 1->2, 2->3 and 1->3 in order to (prematurely) optimise the 1->3 upgrade process just makes it twice as easy for a subtle upgrade-inconsistency bug to creep in.

[tomusdrw]

I don't see how it guarantees data integritiy.
Having just one migration per version seems much simpler for me.

Also migrations vector is not sorted so you need to rely on the order of adding migrations anyway if you have multiple migrations with the same from_version (which should not happen anyway, but current design doesn't enforce that)

[debris]

I agree with POIROAE. Let's forget squashing argument.

To me

trait Migration {
  fn from_version(&self) -> &'static str;
  fn to_version(&self) -> &'static str;
};

Describes migration better than:

trait Migration {
  fn version(&self) -> &'static str;
}

Because we can clearly see what version of database this migration expects to find and it's possible to check migration integrity.

let mut result = BTreeMap::new();
manager.add_migration(Migration0).unwrap();
manager.add_migration(Migration1).unwrap();
manager.add_migration(Migration2).unwrap();

and it cannot be achieved without from param:

let mut result = BTreeMap::new();
manager.add_migration(Migration0);
// there is missing migration from 0 to 1, the code still compiles fine
// manager.add_migration(Migration1);
manager.add_migration(Migration2);

[tomusdrw]

Can't really see the point here. You can validate integrity with single field too (and when you have from+to the code above still compiles fine when the migration is missing) - you have actually encoded single version name in types Migration{} :)

[tomusdrw]

Might be worth to change version from string to enum.
Benefits:

• All possible versions in single place (might include additional comments / meta information)
• Pattern matching all possible versions is possible.
• Can be ordered.
• You can enforce that migrations exists for all versions.
• Less prone to merge conflicts.

[debris]

👍, but let's keep it simple for now. let's begin with strings and later switch to enums

[tomusdrw]

imho it's not keeping it simple - it keeps important assumptions outside of the code. It was ok with versioning the DB but if you are starting to build migration framework it would be worthwile to use enums from the start.

How will ensure that migrations exists for all possible versions with strings?

[debris]

We discussed the issue with @tomusdrw and decided to keep the pr unchanged.

[debris]

Replaced from_version(), to_version() with a single version() function returning integer.