elixir-ecto / ecto

A toolkit for data mapping and language integrated query.
https://hexdocs.pm/ecto
Apache License 2.0
6.09k stars 1.42k forks source link

Attempting to use Ecto.Migrator in my app code causes runtime errors #1894

Closed brandonparsons closed 7 years ago

brandonparsons commented 7 years ago

Environment


I am using Postgres schemas and the prefix functionality from Ecto. There is one place in my app code (tenant registration) where I create a new schema, and then want to run a suite of migrations on that schema.

I am getting runtime errors in my test suite when attempting to run this code, as per below:

** (CompileError) /myapp/_build/test/lib/shared_repo/priv/repo/tenant_migrations/20161226051041_a_migration.exs:1: cannot define module SharedRepo.Repo.TenantMigrations.RebaseMigrations because it is currently being defined in /myapp/_build/test/lib/shared_repo/priv/repo/tenant_migrations/20161226051041_a_migration.exs:1
stacktrace:
  (elixir) src/elixir_module.erl:138: :elixir_module.build/5
  (elixir) src/elixir_module.erl:67: :elixir_module.do_compile/5
  (stdlib) erl_eval.erl:670: :erl_eval.do_apply/6
  (elixir) src/elixir.erl:224: :elixir.erl_eval/3
  (elixir) src/elixir.erl:212: :elixir.eval_forms/4
  (elixir) src/elixir_compiler.erl:66: :elixir_compiler.eval_compilation/3
  (elixir) src/elixir_lexical.erl:17: :elixir_lexical.run/3
  (elixir) src/elixir_compiler.erl:30: :elixir_compiler.quoted/3
  (elixir) lib/code.ex:328: Code.load_file/2
  (ecto) lib/ecto/migrator.ex:239: anonymous fn/4 in Ecto.Migrator.migrate/4
  (elixir) lib/enum.ex:1229: Enum."-map/2-lists^map/1-0-"/2
  (myapp) lib/myapp/resource_actions/operators.ex:86: MyApp.ResourceActions.Operators.handle_schema/1
  (ecto) lib/ecto/multi.ex:406: Ecto.Multi.apply_operation/5
  (elixir) lib/enum.ex:1755: Enum."-reduce/3-lists^foldl/2-0-"/3
  (ecto) lib/ecto/multi.ex:396: anonymous fn/5 in Ecto.Multi.apply_operations/5
  (ecto) lib/ecto/adapters/sql.ex:615: anonymous fn/3 in Ecto.Adapters.SQL.do_transaction/3
  (db_connection) lib/db_connection.ex:1274: DBConnection.transaction_run/4
  (db_connection) lib/db_connection.ex:1198: DBConnection.run_begin/3
  (db_connection) lib/db_connection.ex:789: DBConnection.transaction/3
  (elixir) src/elixir.erl:224: :elixir.erl_eval/3

The offending code is as follows (note that I am using a non-standard migrations folder path for migrations I intend to run inside tenant schemas):

    migrations_path = Application.app_dir(:shared_repo, "priv/repo/tenant_migrations")
    opts = [prefix: schema_name, all: true, log: false]
    result = Ecto.Migrator.run(Repo, migrations_path, :up, opts)
    case result do
      [_version] -> {:ok, nil}
      otherwise -> {:error, otherwise}
    end

It is possible that this has something to do with the test setup for Ecto DB connections also. I only seem to get failures when attempting to run these tests async: true.

Let me know if any further information is required.

josevalim commented 7 years ago

Running migrations loads all migrations in the given directory. Therefore if your tests are async, it means you may have two tests running at the same time, and both will attempt to load and compile the migrations modules, leading exactly to the error you are seeing now. The error message says exactly that:

cannot define module SharedRepo.Repo.TenantMigrations.RebaseMigrations because it is currently being defined in /myapp/_build/test/lib/shared_repo/priv/repo/tenant_migrations/20161226051041_a_migration.exs:1

I think the best would be to make sure those modules are compiled and loaded when your app starts. Unfortunately there is no migration API that allows you to give a list of modules to migrate but it should not be complicated to provide a pull request that allows so.

josevalim commented 7 years ago

We could, for example, support:

Ecto.Migrator.run(Repo, [{0, MyApp.Migration1}, {1, MyApp.Migration2}, ...], :up, opts)
brandonparsons commented 7 years ago

Thanks for the reply Jose. I'm glad it was a result of the async tests and not something harder to understand :)

  1. Would throwing require statements for the modules in test_helper.exs address the first part of your suggestion (ensuring all the migration modules are compiled and loaded when app starts)?
  2. I'll have a look at the code in Ecto.Migrator and see what I can figure out. I'm new to Elixir so will have to determine if this is inside my skill level.
josevalim commented 7 years ago
  1. @brandonparsons that would not solve it because the migration would always try to load the files. Plus you would still have the same error in production.

  2. Feel free to ask any questions or ping me for help. I am also on IRC in case you want a faster way to communicate. :)

brandonparsons commented 7 years ago

@josevalim IRC would certainly be more efficient! Only problem is that I only program as a "hobby" (not full-time) and we have a small child so my time is pretty limited :) I just hop onto the computer when I get a chance so asynchronous communication usually works better.

I had a look at this and think it might be just at, or slightly above my skill level. I'll poke around a bit.

I think I figured out what you meant by loaded and compiled as the app boots. Do you mean that instead of placing these migration files in priv/repo (and loaded when migrations are run) you'd put these new migrations within lib so that the modules area available?

With regards to running tests for the ecto library - how do I have to have my local postgres install set up so that I can run MIX_ENV=pg mix test ?

josevalim commented 7 years ago

you'd put these new migrations within lib so that the modules area available?

Yes!

With regards to running tests for the ecto library - how do I have to have my local postgres install set up so that I can run MIX_ENV=pg mix test ?

We support the PG_URL environment variable. By default we use "postgres:postgres@localhost" so if you hvae a postgres username with postgres password you are good. Otherwise you need to set it to your user:pass with credentials to also create databases.

However, I believe you don't need to run PG tests for this feature, mix test should be enough as nothing of what you are implementing is actually database specific. :)

brandonparsons commented 7 years ago

@josevalim I took at stab at this in #1897. Please go easy on me :)

brandonparsons commented 7 years ago

@josevalim I'm trying to write my app code to figure out exactly how to take advantage of this. I don't want to hard-code the [{version, module_name}, {}, ....] list anywhere in my code, as it would be easy to forget a migration module.

Is there a way to list all modules under a given namespace? I couldn't find it by googling around. For example, if I store all my tenant migrations under Myapp.TenantMigrations (so a given migration would be Myapp.TenantMigrations.AMigrationModule), is there a way to list these out at runtime?

That way, I could programmatically build up the list by sticking the version as a module attribute, or intimately tying the filenames to the modules.

brandonparsons commented 7 years ago

I have tried things along the lines of:

migration_source = Application.app_dir(:myapp, "tenant_migrations")
migration_source |> Path.join("*") |> Path.wildcard

But nothing is showing up when I play with this in development / iex.

brandonparsons commented 7 years ago

So the issue appears to be that because these modules are never called anywhere in the app, Elixir isn't loading them on boot, and therefore they don't show up in the _build directory. This means the filenames can't be found through Application.app_dir(:myapp, "tenant_migrations").

After some code spelunking, I've found that this can create the {version, module} tuples:

  defp module_list do
    Application.app_dir(:myapp, "ebin")
    |> Path.join("*")
    |> Path.wildcard()
    |> Enum.map(&handle_beam_file_path/1)
    |> Enum.reject(&is_nil/1)
    |> Enum.map(&to_module_name/1)
    |> Enum.map(&create_tuple/1)
  end

  defp handle_beam_file_path(path) do
    capture_module_name = ~r/.+Myapp\.TenantMigrations\.(.+)\.beam/
    case Regex.run(capture_module_name, path, capture: :all_but_first) do
      nil -> nil
      [captured] -> captured
    end
  end

  defp to_module_name(mod_str), do: Module.concat(["Myapp.TenantMigrations", mod_str])

  defp create_tuple(mod), do: {mod.version, mod}

If this is doing something horribly broken, please let me know! The only thing that seems like it may be an issue is looking in the ebin folder for .beam files and transforming those strings.

As you can see, because I can't grab the filename of these migrations, I have to create a version public function in the migration module instead.

josevalim commented 7 years ago

Suggestions:

  1. Instead of Path.join+Path.wildcard, you can use File.ls!
  2. Instead of a regex, you can do: case filename do "Elixir.Myapp.TenantMigrations." <> suffix -> ... end. Then you can use the functions in the Path module to remove the .beam extension.
  3. Don't forget to sort at the end!!!
brandonparsons commented 7 years ago

Thank you sir. Appreciate your help.

🎉

OvermindDL1 commented 7 years ago

Thank you sir. Appreciate your help.

A document or even better a library to do this when you finish would be awesome. :-)