mongodb_ecto icon indicating copy to clipboard operation
mongodb_ecto copied to clipboard

Migrations aren't working because `Mongo.Ecto.execute_ddl` isn't implemented

Open bdtomlin opened this issue 2 years ago • 10 comments

It looks like Ecto expects drivers to implement a function called execute_ddl that doesn't exist in mongodb_ecto.

When running mix ecto.migrate that method it called in order to create the migrations table, and then it fails. Also note that the migrate task is not available unless you leave ecto_sql in place. So it seems like mongodb_ecto will need to implement that task as well.

I expect there is some other stuff that is probably missing also like managing the schema migrations after the collection has been created.

Here's what happens when trying to migrate so to demonstrate the expected function signature:

14:12:17.615 [error] Could not create schema migrations table. This error usually happens due to the following:

  * The database does not exist
  * The "schema_migrations" table, which Ecto uses for managing
    migrations, was defined by another library
  * There is a deadlock while migrating (such as using concurrent
    indexes with a migration_lock)

To fix the first issue, run "mix ecto.create".

To address the second, you can run "mix ecto.drop" followed by
"mix ecto.create". Alternatively you may configure Ecto to use
another table and/or repository for managing migrations:

    config :demo, Demo.Repo,
      migration_source: "some_other_table_for_schema_migrations",
      migration_repo: AnotherRepoForSchemaMigrations

The full error report is shown below.

** (UndefinedFunctionError) function Mongo.Ecto.execute_ddl/3 is undefined or private. Did you mean one of:

      * execute/5

    (mongodb_ecto 1.0.0-beta.2) Mongo.Ecto.execute_ddl(%{cache: #Reference<0.2208074840.2087321610.212502>, opts: [timeout: 15000, pool_size: 2], pid: #PID<0.289.0>, pool: {Demo.Repo.Pool, [pool_timeout: 5000, repo: Demo.Repo, telemetry_prefix: [:demo, :repo], otp_app: :demo, timeout: 15000, adapter: Mongo.Ecto, database: "demo", username: nil, password: nil, hostname: "localhost", show_sensitive_data_on_connection_error: true, pool_size: 2]}, repo: Demo.Repo, telemetry: {Demo.Repo, :debug, [:demo, :repo, :query]}}, {:create_if_not_exists, %Ecto.Migration.Table{comment: nil, engine: nil, name: :schema_migrations, options: nil, prefix: nil, primary_key: true}, [{:add, :version, :bigint, [primary_key: true]}, {:add, :inserted_at, :naive_datetime, []}]}, [timeout: :infinity, log: false, schema_migration: true])
    (ecto_sql 3.7.1) lib/ecto/migrator.ex:678: Ecto.Migrator.verbose_schema_migration/3
    (ecto_sql 3.7.1) lib/ecto/migrator.ex:504: Ecto.Migrator.lock_for_migrations/4
    (ecto_sql 3.7.1) lib/ecto/migrator.ex:419: Ecto.Migrator.run/4
    (ecto_sql 3.7.1) lib/ecto/migrator.ex:146: Ecto.Migrator.with_repo/3
    (ecto_sql 3.7.1) lib/mix/tasks/ecto.migrate.ex:138: anonymous fn/5 in Mix.Tasks.Ecto.Migrate.run/2
    (elixir 1.12.3) lib/enum.ex:2385: Enum."-reduce/3-lists^foldl/2-0-"/3
    (ecto_sql 3.7.1) lib/mix/tasks/ecto.migrate.ex:126: Mix.Tasks.Ecto.Migrate.run/2

bdtomlin avatar Dec 04 '21 20:12 bdtomlin

@bdtomlin Sorry about that! Migrations aren't supported with this adapter. We should add that to the readme and throw a more helpful error message!

scottmessinger avatar Dec 08 '21 03:12 scottmessinger

@scottmessinger I saw issue #12, and #25 and thought the intent was to support a limited migration set (for changing data, adding indexes, and things like that). It would be good to make it clear if that is not the intent.

It would be nice to support those features, but if not to give direction on other options for handling indexes and similar situations.

bdtomlin avatar Dec 08 '21 20:12 bdtomlin

It would be nice to support those features, but if not to give direction on other options for handling indexes and similar situations.

Totally agree! @joeapearson ran into this here: https://github.com/commoncurriculum/mongodb_ecto_forked/pull/2#issuecomment-894351130 and would also like to address this in the future. I'd welcome any PRs!

scottmessinger avatar Dec 08 '21 22:12 scottmessinger

@bdtomlin it doesn't really make sense to support migrations in the same way that they are in SQL land. IIRC there are some places in the Ecto code that make that assumption; this may be one of them.

My idea (to be vetted by everyone else) would be to add some kind of separate index management feature, possibly augmented with the information we can get out of an Ecto.Schema.

Can you say more about what you're specifically trying to accomplish? What does your DDL do?

joeapearson avatar Dec 11 '21 16:12 joeapearson

In cases like this (ones where Ecto makes an assumption that doesn't make total sense in Mongo land), I think we need an explicit raise with a helpful error message. This will be a good starting point while we discuss what the best solution is - at least then @bdtomlin would get some helpful guidance along the way (as much as I appreciate this wouldn't be a proper fix!)

joeapearson avatar Dec 11 '21 17:12 joeapearson

@joeapearson,

It was not really a surprise when migrations failed. I understand how migrations in mongodb are a different animal that migrations in SQL. The surprising thing was that there was no intent to support migrations based on the issues I mentioned.

I think migrations would be helpful for things such as fields being renamed, modified, or transformed. It is also useful for cleaning up data that is no longer in use, restructuring collections, etc.

I understand if there is no intent to support migrations at all, and I agree that a better message would help, also maybe adding a comment to those issues I referenced saying it is not supported.

FWIW, Mongoid does not support migrations either, but there is another gem that does: https://github.com/adacosta/mongoid_rails_migrations

bdtomlin avatar Dec 11 '21 17:12 bdtomlin

The surprising thing was that there was no intent to support migrations based on the issues I mentioned.

So, I guess the "official" policy (in that there is an official policy) is that @joeapearson and I would love to have some support for migrations in the adapter (especially or mainly index management) but have no current plans to implement them. We'd welcome any PRs to add support!

In order of priorities, I would think:

  • creating/deleting indexes (I don't think you can modify an existing index, but if so, modifying an index)
  • creating/removing collections
  • renaming fields
  • [other things]

Let me know if you get a chance to tackle this (even if in a different order than listed above) -- I'd be happy to review the PR!

scottmessinger avatar Dec 13 '21 16:12 scottmessinger

Yep sounds good. @bdtomlin it wasn't a "never", more of a "yes that sounds nice but no one has much time at the moment", so for now we're looking for a quick fix (like a nice error message, I know this isn't really a "fix" for you but at least you wouldn't have been left completely unaided), and then we're looking for consensus on what we'd like to build.

@scottmessinger agree with your prioritisations.

joeapearson avatar Dec 13 '21 18:12 joeapearson

@bdtomlin do you have a simple repo that we can look at to work with on this?

joeapearson avatar Dec 23 '21 16:12 joeapearson

I saw this error.

In my config.exs, My .Repo was defined in ecto_repos: [...],. Removing the line resolved this error.

afomi avatar Dec 23 '23 01:12 afomi