elixir-mongo / mongodb_ecto

MongoDB adapter for Ecto
Apache License 2.0
368 stars 126 forks source link

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

Open bdtomlin opened 2 years ago

bdtomlin commented 2 years ago

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
scottmessinger commented 2 years ago

@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!

bdtomlin commented 2 years ago

@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.

scottmessinger commented 2 years ago

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!

joeapearson commented 2 years ago

@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 commented 2 years ago

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!)

bdtomlin commented 2 years ago

@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

scottmessinger commented 2 years ago

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:

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!

joeapearson commented 2 years ago

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 commented 2 years ago

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

afomi commented 8 months ago

I saw this error.

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