sihl icon indicating copy to clipboard operation
sihl copied to clipboard

use mutable list to register migrations

Open timohuber opened this issue 1 year ago • 6 comments

timohuber avatar May 29 '24 13:05 timohuber

@aronerben Updated PR using a mutable list to store the registered migration.

For some reason, the migrations are registered in the reversed order, we register the services.

For example:

let services =
  [ Service.Migration.register (Database.Migration.migrations ())
  ; Service.Token.register ()
  ; Service.User.register ()
  ; Service.Email.register ()
  ]
;;

Will create migrations in the following order:

email
user
token
....

Do you know why this behaves like this?

timohuber avatar May 29 '24 13:05 timohuber

@timohuber Hmm, the implementation seems correct. Could you figure out where this reversal happens by logging? For example, is it reversed in let execute ?ctx migrations = ... already?

aronerben avatar May 29 '24 14:05 aronerben

@aronerben Sorry, I was unclear. This behavior is not directly related to this PR.

The services are started in a different order than passed to the with_services function and, therefore, the migrations. They are sorted in top_sort_lifecycles. Do you know what the reason is to sort the lifecycles?

timohuber avatar May 30 '24 06:05 timohuber

@timohuber Ah, I see. top_sort_lifecycles is necessary to sort the services based on their dependencies. So user depends on email (as each user should have an email address), which implies that email should be loaded before user. I guess the migrations are also run before.

aronerben avatar May 30 '24 08:05 aronerben

@aronerben Yes, you are right. migrations are run first. Do you have an idea how we can ensure that all migrations from sihl are executed before the one registered by the app? Store them into two different variables, as the first draft did?

The current workaround is to prefix them to make sure they are executed last in alphabetical order.

timohuber avatar May 31 '24 06:05 timohuber

@timohuber Technically, if you register your own services and make them dependent on the corresponding Sihl service it depends on, the migration should be run after the Sihl migration...

Would that work? Otherwise we just separate Sihl migrations and custom migrations via the way you suggested.

(Of course, the cleanest solution would be to rework the migration system, as was planned in Sihl 4.0...)

aronerben avatar Jun 04 '24 13:06 aronerben