First version of dynamic command for Ecto.Migration #158

Eiji7 · 2019-10-29T05:48:26Z

Added Ecto.Migration.dynamic/1 macro.
Added integration test for ensuring correct order.
Added optional support for passing an anonymous function with one arity.
Added special function clausule in Migration.Runner in order to not afftect currect adapters implementations.
Documentation and specs
Ensure that {:dynamic, func} command is self-reversible in runner.

TODO:

Discuss the implementation details

Related issue: #157

Added Ecto.Migration.dynamic/1 macro Added integration test for ensuring correct order Added special function clausule in Migration.Runner in order to not afftect currect adapters implementations. Ensure that {:dynamic, macro, env} command is self-reversible in runner. TODO: Discuss the implementation Documentation and specs Related issue: #157

Eiji7 · 2019-10-29T06:04:44Z

@josevalim Looks like I did not broke anything and that's a success already. 😄

I never touched Code.eval_quoted/3 before, so from PHP experience I'm always a bit worried at this part.

I wanted to check it in "real" usage, so I have added a debug queries in migration inside integration tests just to make sure that Ecto.Adapters.SQL.query/4 works as expected. I have added a simple test which split logs into lines to ensure a correct log order.

But step wouldn't accept migration commands inside.

For sure I did not added any checks for that right now as well as tests for such usage. Do I need to add some checks or it would just raise without any extra code? Should I add more tests?

If I do not missed anything and it looks good for you I will add documentation and specs asap.

Eiji7 · 2019-10-29T09:12:17Z

When trying changes in my library I had a small problem with unquoting and I had simplified it by adding bindings support for Code.eval_quoted/3. Now it's working awesome and I'm able to finish my library with this!

Eiji7 · 2019-10-31T10:15:17Z

@josevalim: Is it a random test error (see my previous commit check)?

In previous commit it's: ecto_insert_posts:

  1) test unique constraint (Ecto.Integration.RepoTest)
     deps/ecto/integration_test/cases/repo.exs:329
     ** (Postgrex.Error) ERROR 26000 (invalid_sql_statement_name) prepared statement "ecto_insert_posts" does not exist
     code: {:ok, _}  = TestRepo.insert(changeset)
     stacktrace:
       (ecto_sql) lib/ecto/adapters/sql.ex:629: Ecto.Adapters.SQL.raise_sql_call_error/1
       (ecto) lib/ecto/repo/schema.ex:651: Ecto.Repo.Schema.apply/4
       (ecto) lib/ecto/repo/schema.ex:262: anonymous fn/15 in Ecto.Repo.Schema.do_insert/4
       deps/ecto/integration_test/cases/repo.exs:331: (test)

in latest commit it's ecto_update_posts:

  1) test insert and update with changeset (Ecto.Integration.RepoTest)
     deps/ecto/integration_test/cases/repo.exs:166
     ** (Postgrex.Error) ERROR 26000 (invalid_sql_statement_name) prepared statement "ecto_update_posts" does not exist
     code: assert %Post{text: "y", title: "world", temp: "unknown"} = TestRepo.update!(changeset)
     stacktrace:
       (ecto_sql) lib/ecto/adapters/sql.ex:629: Ecto.Adapters.SQL.raise_sql_call_error/1
       (ecto) lib/ecto/repo/schema.ex:651: Ecto.Repo.Schema.apply/4
       (ecto) lib/ecto/repo/schema.ex:349: anonymous fn/15 in Ecto.Repo.Schema.do_update/4
       (ecto) lib/ecto/repo/schema.ex:177: Ecto.Repo.Schema.update!/4
       deps/ecto/integration_test/cases/repo.exs:179: (test)

but I did not touched other tests, so … I'm a bit confused here

Also I'm about to release a release canditate for my new library something like today or tomorrow. I just need to take a look again at the whole project with "fresh eye". However the code (which uses new dynamic command macro) works without any problems. I have also added some documentation, so for me it's ready to push. However nobody even commented it yet.

josevalim · 2019-10-31T13:32:12Z

Yes, those are heisenbugs that I fail to reproduce locally.

Regarding the feature, I could not look into a lot of detail yet, but note that {:dynamic, macro, env} should actually be {:dynamic, anonymous_function}. We really don't want to eval code, we want to just execute the function.

josevalim · 2019-10-31T13:33:45Z

integration_test/sql/migration.exs

+    def change do
+      execute @migrate_first, @rollback_second
+
+      dynamic migrate_middle: @migrate_middle, rollback_middle: @rollback_middle do


If you make it an anonymous function, then you don't need to inject variables.

@josevalim Completely agree

It's always darkest before dawn

josevalim · 2019-10-31T13:34:51Z

Btw, we still need a PR that raises on non-reversible flush. :)

Eiji7 · 2019-10-31T14:50:34Z

Btw, we still need a PR that raises on non-reversible flush. :)

@josevalim Originally I was focused on making my library work. After doing this PR I'm much better following how Ecto.Migration and Ecto.Migration.Runner works, so it's different now.

I will first correct this PR as you said, after that I would quickly update my library and at end I would create a separate PR for flush().

Yes, those are heisenbugs that I fail to reproduce locally.

Not sure if I'm able to do this today, but if I would have some time tomorrow then I could take a look at this and give at least a hint.

… function with arity 0 or 1. Function with arity 1 would accept optional migration direction which would make migrations shorter. Updated documentation and tests.

Eiji7 · 2019-10-31T15:41:29Z

@josevalim How does it look like now?

I have added a small enhancement. Developer could optionally use a function with one arity and in such case we would give and migration direction. It's because I think that many people working with reversible migrations would write something like:

dynamic(fn -> my_dynamic_func(direction()) end)

With this small enhancement we would have:

dynamic(&my_dynamic_func/1)

which definitely looks much better.

Looks like all tests passed on GitHub too, so I'm going to update my library now.

josevalim · 2019-11-01T08:27:11Z

Hi @Eiji7!

I have put some thoughts into this and your questions, and I think I have arrived at a simpler API.

What if instead of introducing a new name, we simply allow anonymous functions in execute/1 and execute/2:

execute &do_on_up/0
execute &do_on_up/0, &do_on_down/0

And similar to the semantics of execute today, if you only pass up, then we should raise when reversing, because the user should be explicit if the same code (or a different one) is needed for down.

To me, this has the advantage of simplifying both our code and our API surface, plus we use semantics that are already well defined.

Once we do this, we should be ready to merge this.

…te/1` and `execute/2`. Updated `execute/1` and `execute/2` documentation. Updated tests

Eiji7 · 2019-11-01T10:07:53Z

@josevalim Done 😃

We really need to fix the problem with failing tests. Is my issue helpful? #160

If you are still unable to reproduce it then I can give you some information you which you think may be important as it's failing for me quite often.

btw.

Segmentation fault (core dumped)

hmm … ok? Did you noticed such error before on CI?

josevalim · 2019-11-01T10:23:47Z

I didn't have a time to look at it #160 yet but I have done similar steps locally and I cannot reproduce it at all. Can you reproduce it consistently? On every run? I am wondering if the issues doesn't happen on Mac and I need a Linux.

hmm … ok? Did you noticed such error before on CI?

Nope. But by definition it is not your bug (segmentation faults are always VM bugs).

josevalim · 2019-11-01T10:24:23Z

integration_test/sql/migration.exs

    end
  end

+  defmodule MigrationWithDynamicCommand do


Same comment as the other one, let's not use integration tests for these. We also need to rename Dynamic to ExecuteFun or similar.

Right, I renamed most of thing, but forgot this one.

lib/ecto/migration.ex

josevalim · 2019-11-01T10:25:44Z

lib/ecto/migration.ex


      execute "CREATE EXTENSION postgres_fdw", "DROP EXTENSION postgres_fdw"

+      alias Ecto.Adapters.SQL


We need a bit more structure around this code example. Maybe add some leading paragraph about how it works and define the whole migration file, so the defp makes sense.

I will change it to full example, but I believe that it would be nice to place such paragraph inside Flushing section which would show difference between those two functions.

josevalim · 2019-11-01T10:26:00Z

lib/ecto/migration.ex

+      execute(&execute_up/0, &execute_down/0)
+
+      defp execute_up, do: SQL.query!(repo(), "select 'Up query …';", [], [log: :info])
+      defp execute_down, do: SQL.query!(repo(), "select 'Down query …';", [], [log: :info])


repo().query!should also work. (i.e. no need to alias SQL).

EDIT: Sorry, just noticed this:

ecto_sql/test/test_repo.exs

Line 8 in a5592af

defmacro __before_compile__(_opts), do: :ok

Will replace calls with Logger.info/1 as there is no need to call any query function at all.

josevalim · 2019-11-01T10:26:30Z

The implementation looks great, we just need some final improvements on docs and move the integration test.

Eiji7 · 2019-11-01T11:37:42Z

@josevalim

I didn't have a time to look at it #160 yet but I have done similar steps locally and I cannot reproduce it at all. Can you reproduce it consistently? On every run? I am wondering if the issues doesn't happen on Mac and I need a Linux.

This happens often, but not every time I run tests. Just calling this task few times gave me this error. Unfortunately I have no Mac to test it on.

Nope. But by definition it is not your bug (segmentation faults are always VM bugs).

Yes, of course. I was just really surprised seeing that. 😄

Moved migration from integration_test/sql/migrations.exs to test/ecto/migrator_test.exs Updated "Flushing" -> "Executing and flushing" section by adding extra paragraph which mentions difference between those 2 solutions. Made documentation cleaner by wrapping code inside example module. Changed Ecto.Adapters.SQL.query!/4 calls to Logger.info/1 in order to increase readability.

# Conflicts: # test/ecto/migrator_test.exs

Eiji7 · 2019-11-01T13:07:53Z

@josevalim Sorry for that merge, I forgot to pull my own changes before 😄

Anyway it should be ready to go now.

test/ecto/migrator_test.exs

josevalim · 2019-11-01T15:12:00Z

lib/ecto/migration/runner.ex

    end
  end

+  defp reverse(func) when is_function(func, 0), do: func


Per comment below, I think this clause is wrong, we should remove it.

That's right, it was based on reverse for old dynamic proposal and now it should not work like that any more.

Added test to ensure raising FunctionClauseError. Updated test by removing unsupported `execute/1` call on rollback.

Eiji7 · 2019-11-01T15:34:47Z

Ok, hope that I did not missed anything else. 😃

josevalim · 2019-11-01T15:36:28Z

test/ecto/migrator_test.exs

  setup do
    Process.put(:migrated_versions, [1, 2, 3])
-    :ok
+    {:ok, migration_number: System.unique_integer([:positive]) + @base_migration}


Can we not do this change for now? If we are going to do so, then we should change for the whole file. Your call.

If you think so then no problem. I have added this because I was using same call for generating unique integer in more than one test/2 calls.

Right, but other tests call System.unique_integer([:positive]) too. So I would prefer to change all tests at once or none. :)

josevalim · 2019-11-01T15:58:56Z

❤️ 💚 💙 💛 💜

Eiji7 added 2 commits October 29, 2019 10:05

Add bindings support

ac4560b

Make bindings optional by adding empty list as argument default value.

8f9b507

Removed no longer needed @tag and added documentation.

6491eff

josevalim reviewed Oct 31, 2019

View reviewed changes

Changed dynamic/2 macro to dynamic/1 function which accepts anonymous…

c838a80

… function with arity 0 or 1. Function with arity 1 would accept optional migration direction which would make migrations shorter. Updated documentation and tests.

Dropped new dynamic/1 in favor of enhancing already existing `execu…

d775367

…te/1` and `execute/2`. Updated `execute/1` and `execute/2` documentation. Updated tests

Eiji7 changed the title ~~[VIP] First version of dynamic command for Ecto.Migration~~ First version of dynamic command for Ecto.Migration Nov 1, 2019

Eiji7 mentioned this pull request Nov 1, 2019

Migration change, ecto.rollback and flush problem #157

Closed