Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bdtomlin opened this issue Dec 4, 2021 · 10 comments
Open

Comments

@bdtomlin
Copy link

bdtomlin commented Dec 4, 2021

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
Copy link
Collaborator

@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
Copy link
Author

bdtomlin commented Dec 8, 2021

@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
Copy link
Collaborator

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: commoncurriculum#2 (comment) and would also like to address this in the future. I'd welcome any PRs!

@joeapearson
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Collaborator

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!

@joeapearson
Copy link
Contributor

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
Copy link
Contributor

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

@afomi
Copy link

afomi commented Dec 23, 2023

I saw this error.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants