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

-    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

   * Creates a Migrator[T, V1] that can function as a builder for
   * 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]))
+  def from[T, V <: V1: VersionInfo]: Migrator[T, V] = new Migrator[T, V](Map(Version.numberFor[V] → identityMigration[T]), Map.empty)


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) {
+    def canMigrate(fromVersion: Int, toVersion: Int): Boolean = forwardMigrations.contains(fromVersion) && forwardMigrations.contains(toVersion)


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

-    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

   * Creates a Migrator[T, V1] that can function as a builder for
   * 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]))
+  def from[T, V <: V1: VersionInfo]: Migrator[T, V] = new Migrator[T, V](Map(Version.numberFor[V] → identityMigration[T]), Map.empty)


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) {
+    def canMigrate(fromVersion: Int, toVersion: Int): Boolean = forwardMigrations.contains(fromVersion) && forwardMigrations.contains(toVersion)


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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to define bidirectional migrations #62

Add possibility to define bidirectional migrations #62

psliwa commented Nov 24, 2016

psliwa commented Dec 14, 2016

agemooij commented Dec 22, 2016

agemooij left a comment

agemooij Dec 22, 2016

psliwa Dec 22, 2016

agemooij Dec 22, 2016

psliwa Dec 22, 2016

psliwa commented Dec 22, 2016

psliwa commented Mar 1, 2017

psliwa commented Mar 29, 2017

psliwa commented Apr 15, 2017

agemooij commented Apr 18, 2017

raboof left a comment

raboof May 5, 2017

psliwa May 15, 2017

psliwa commented Jun 8, 2017

agemooij commented Jun 12, 2017

psliwa Nov 24, 2016

psliwa Dec 22, 2016

psliwa Dec 22, 2016

psliwa May 15, 2017

psliwa commented Jun 12, 2017 •

edited

Loading

agemooij commented Jun 12, 2017

psliwa commented Aug 3, 2017

knirski commented Dec 18, 2017

Add possibility to define bidirectional migrations #62

Add possibility to define bidirectional migrations #62

Conversation

psliwa commented Nov 24, 2016

psliwa commented Dec 14, 2016

agemooij commented Dec 22, 2016

agemooij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psliwa commented Dec 22, 2016

psliwa commented Mar 1, 2017

psliwa commented Mar 29, 2017

psliwa commented Apr 15, 2017

agemooij commented Apr 18, 2017

raboof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psliwa commented Jun 8, 2017

agemooij commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psliwa commented Jun 12, 2017 • edited Loading

agemooij commented Jun 12, 2017

psliwa commented Aug 3, 2017

knirski commented Dec 18, 2017

psliwa commented Jun 12, 2017 •

edited

Loading