Add possibility to define bidirectional migrations #62

psliwa · 2016-11-24T14:45:54Z

This PR is for #57.

DSL hasn't changed, api is simple. If you want to define backward migration, you can use backTo function on Migrator. I've assumed not for every forward migration has to exist backward migration (so backTo invocation is optional), because when forward migration eg. adds new field, backward migration is not needed. The second reason for optional backTo migration is just not using backward migrations in the project.

Usage from tests:

val migrator = from[String, V1]
   .to[V2](_ + "V2")
   .backTo[V1](_.replace("V2", ""))
   .to[V3](_ + "V3")
   .backTo[V2](_.replace("V3", ""))
   .to[V4](_ + "V4")
   .backTo[V3](_.replace("V4", ""))
   .to[V5](_ + "V5")

psliwa · 2016-12-14T15:51:55Z

@agemooij Any chance to take a look at this PR soon?

agemooij · 2016-12-22T10:30:43Z

@raboof what do you think of this PR?

agemooij

This code seems to always apply all forward and backward migrations instead of allowing them to cancel each other out. Have you considered removing migrations if they would be cancelled out by a backward one?

agemooij · 2016-12-22T10:31:50Z

stamina-core/src/main/scala/stamina/migrations.scala

-  implicit class MigrationWithComposition[T](val firstMigration: Migration[T]) extends AnyVal {
-    def &&(secondMigration: Migration[T]): Migration[T] = {
-      (value: T) ⇒ secondMigration(firstMigration(value))
-    }


Why was this removed?

Because it is not used anymore.

agemooij · 2016-12-22T10:37:03Z

stamina-core/src/main/scala/stamina/migrations.scala

   * creating Migrator[T, V2], etc. Its migration will be the identity
   * function so calling its migrate function will not have any effect.
   */
-  def from[T, V <: V1: VersionInfo]: Migrator[T, V] = new Migrator[T, V](Map(Version.numberFor[V] → identityMigration[T]))


Why not make the empty map a default argument of the Migrator class?

Because backward migrations are empty when we start defining migration - so empty map is natural initial value.

psliwa · 2016-12-22T13:30:06Z

This code seems to always apply all forward and backward migrations instead of allowing them to cancel each other out. Have you considered removing migrations if they would be cancelled out by a backward one?

I am not sure what you mean under always apply all forward and backward migrations instead of allowing them to cancel each other out. The idea is to have migrations decoupled from persisters, so I can use migrations alone. When I want to migrate something from version X to Y, Migrator should apply transformations. If those transformations are forward or backward - it depends on given versions.

Have you considered removing migrations if they would be cancelled out by a backward one?
I don't fully understand, there is no sense in removing migrations when backward one is added. They coexists. Which one is used (backward or forward) depends on if fromVersion is greater or lower than toVersion. In Persisters only forward migrations are used, backward migrations we would like to use in our rollback tool.

psliwa · 2017-03-01T19:00:15Z

@agemooij ping

psliwa · 2017-03-29T17:28:35Z

Any progress? Maybe something is unclear?

psliwa · 2017-04-15T15:53:32Z

@agemooij @raboof ? 🙏

agemooij · 2017-04-18T12:54:41Z

@psliwa Sorry man. Stamina has been so completely out of scope for me lately that it's hard to spend time on it .

I had some minor review comments before that you haven't answered yet but overall I'm ok with merging this.

@raboof? What do you think?

raboof

@agemooij @psliwa sorry for taking so long to respond too.

This looks reasonable, might be good to address the couple of small comments given on the PR, and perhaps add some documentation? But otherwise LGTM.

raboof · 2017-05-05T15:18:52Z

stamina-core/src/main/scala/stamina/migrations.scala

   */
-  class Migrator[R, V <: Version: VersionInfo] private[stamina] (migrations: Map[Int, Migration[R]] = Map.empty) {
-    def canMigrate(fromVersion: Int): Boolean = migrations.contains(fromVersion)
+  class Migrator[R, V <: Version: VersionInfo] private[stamina] (forwardMigrations: Map[Int, Migration[R]] = Map.empty, backwardMigrations: Map[Int, Migration[R]] = Map.empty) {


Is this correct? It's not obvious to me that we now also need forwardMigrations.contains(toVersion), and shouldn't this also take into account that we can support backwards migrations?

There is a simple code from tests:

val migrator = from[String, V1] .to[V2](_ + "V2") .backTo[V1](_.replace("V2", "")) .to[V3](_ + "V3") .backTo[V2](_.replace("V3", "")) .to[V4](_ + "V4") .backTo[V3](_.replace("V4", "")) .to[V5](_ + "V5")

The assumption is to define continuous forward migrations, so you cannot define migrations only for version 1 and version 3. You have to define migration also for version 2. So in case when we are going to migrate from version 2 to version 5, we need migrations from 2 to 3, from 3 to 4 and from 4 to 5. Migration from 4 to 5 is under 5 key, it is toVersion, so it is needed as well. canMigrate function checks in this case existence of 2 -> 3 and 4 -> 5 migrations, because forward migrations are defined continuously. Backward migrations are optional, so they doesn't affect canMigrate function at all. If backward migration is not defined for particular version, identity function is used.

psliwa · 2017-06-08T18:58:07Z

@raboof ping

agemooij · 2017-06-12T07:34:32Z

@psliwa we are currently waiting on your replies to our comments.

psliwa · 2016-11-24T14:47:30Z

stamina-core/src/test/scala/stamina/migrations/MigratorSpec.scala

+
+import stamina._
+
+class MigratorSpec extends StaminaSpec {


This test suits to scalacheck, do you think I can add scalacheck as dependency and rewrite this test?

psliwa · 2016-12-22T13:20:45Z

stamina-core/src/main/scala/stamina/migrations.scala

-  implicit class MigrationWithComposition[T](val firstMigration: Migration[T]) extends AnyVal {
-    def &&(secondMigration: Migration[T]): Migration[T] = {
-      (value: T) ⇒ secondMigration(firstMigration(value))
-    }


Because it is not used anymore.

psliwa · 2016-12-22T13:21:53Z

stamina-core/src/main/scala/stamina/migrations.scala

   * creating Migrator[T, V2], etc. Its migration will be the identity
   * function so calling its migrate function will not have any effect.
   */
-  def from[T, V <: V1: VersionInfo]: Migrator[T, V] = new Migrator[T, V](Map(Version.numberFor[V] → identityMigration[T]))


Because backward migrations are empty when we start defining migration - so empty map is natural initial value.

psliwa · 2017-05-15T07:38:25Z

stamina-core/src/main/scala/stamina/migrations.scala

   */
-  class Migrator[R, V <: Version: VersionInfo] private[stamina] (migrations: Map[Int, Migration[R]] = Map.empty) {
-    def canMigrate(fromVersion: Int): Boolean = migrations.contains(fromVersion)
+  class Migrator[R, V <: Version: VersionInfo] private[stamina] (forwardMigrations: Map[Int, Migration[R]] = Map.empty, backwardMigrations: Map[Int, Migration[R]] = Map.empty) {


There is a simple code from tests:

val migrator = from[String, V1] .to[V2](_ + "V2") .backTo[V1](_.replace("V2", "")) .to[V3](_ + "V3") .backTo[V2](_.replace("V3", "")) .to[V4](_ + "V4") .backTo[V3](_.replace("V4", "")) .to[V5](_ + "V5")

The assumption is to define continuous forward migrations, so you cannot define migrations only for version 1 and version 3. You have to define migration also for version 2. So in case when we are going to migrate from version 2 to version 5, we need migrations from 2 to 3, from 3 to 4 and from 4 to 5. Migration from 4 to 5 is under 5 key, it is toVersion, so it is needed as well. canMigrate function checks in this case existence of 2 -> 3 and 4 -> 5 migrations, because forward migrations are defined continuously. Backward migrations are optional, so they doesn't affect canMigrate function at all. If backward migration is not defined for particular version, identity function is used.

psliwa · 2017-06-12T09:00:28Z

@agemooij Sorry, I am not sure how github conversations work. I added comments long ago, but they were marked as Pending and probably you weren't able to see them. Right now you should be able to see them, if you have any more questions - go ahead.

agemooij · 2017-06-12T09:27:57Z

Looks like you started a review but did not finish it. This is a common mistake made by quite a few people I know since Github introduced this feature.

@raboof could you also have a look at the review comments?

psliwa · 2017-08-03T15:05:08Z

@agemooij @raboof ping

knirski · 2017-12-18T12:45:27Z

We will close this PR (@psliwa ping) in favor of #71, which is already proven to be working on production :)

add possibility to define bidirectional migrations

e3f0ed4

agemooij reviewed Dec 22, 2016

View reviewed changes

raboof reviewed May 5, 2017

View reviewed changes

psliwa commented Jun 12, 2017

View reviewed changes

psliwa mentioned this pull request Aug 7, 2017

Add possibility to unpersist data one generation younger #71

Open

psliwa closed this Dec 20, 2017

Add possibility to define bidirectional migrations #62

Add possibility to define bidirectional migrations #62

Uh oh!

Conversation

psliwa commented Nov 24, 2016

Uh oh!

psliwa commented Dec 14, 2016

Uh oh!

agemooij commented Dec 22, 2016

Uh oh!

agemooij left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psliwa commented Dec 22, 2016

Uh oh!

psliwa commented Mar 1, 2017

Uh oh!

psliwa commented Mar 29, 2017

Uh oh!

psliwa commented Apr 15, 2017

Uh oh!

agemooij commented Apr 18, 2017

Uh oh!

raboof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psliwa commented Jun 8, 2017

Uh oh!

agemooij commented Jun 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psliwa commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agemooij commented Jun 12, 2017

Uh oh!

psliwa commented Aug 3, 2017

Uh oh!

knirski commented Dec 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

psliwa commented Jun 12, 2017 •

edited

Loading