elixir-ecto / ecto_sql

SQL-based adapters for Ecto and database migrations
https://hexdocs.pm/ecto_sql
Apache License 2.0
578 stars 312 forks source link

Migrator error: ** (MatchError) no match of right hand side value: :ok #628

Closed yastanotheruser closed 3 months ago

yastanotheruser commented 4 months ago

Elixir version

Elixir 1.16.3 (compiled with Erlang/OTP 26)

Database and Version

SQLite3

Ecto Versions

3.11.3

Database Adapter and Versions (postgrex, myxql, etc)

0.16.0

Current behavior

When applying or rolling back migrations a MatchError occurs in the run_maybe_in_transaction function of the Ecto.Migrator module. I believe this happens because the attempt function always returns :ok, which doesn't match the {:ok, result} pattern.

https://github.com/elixir-ecto/ecto_sql/blob/48fc2ad6e8afb022f8454350e23122c3304451d1/lib/ecto/migrator.ex#L363-L368

Expected behavior

Migration functions shouldn't produce this error.

warmwaffles commented 4 months ago

I don't know if that's the bug though. The return signature looks to be :ok | nil which attempt/8 is used in || chains. Can you share what the migration is that you are trying to rollback?

josevalim commented 4 months ago

Can you please provide the full error message with stacktrace?

yastanotheruser commented 4 months ago

The migration does get executed/reverted, but the match still fails. I need to execute the migrations before running my tests with the Ecto.Migrator.run function, and I have to wrap this call with try because of this error.

defmodule MyApp.Repo.Migrations.AddRuleTable do
  use Ecto.Migration

  @table "rule"

  def change do
    create table(@table, primary_key: false) do
      add :id, :identity, primary_key: true
      add :name, :string, size: 200
      # other fields
    end

    create unique_index(@table, :name)
  end
end

Reproduce with:

$ mix ecto.migrate -r MyApp.Repo
14:35:10.877 [info] == Running 20240729172243 MyApp.Repo.Migrations.AddRuleTable.change/0 forward

14:35:10.882 [info] create table rule

14:35:10.892 [info] create index rule_name_index

14:35:10.893 [info] == Migrated 20240729172243 in 0.0s

** (MatchError) no match of right hand side value: :ok
    (ecto_sql 3.11.3) lib/ecto/migrator.ex:354: Ecto.Migrator.run_maybe_in_transaction/5
    (elixir 1.16.3) lib/task/supervised.ex:101: Task.Supervised.invoke_mfa/2
    (elixir 1.16.3) lib/task/supervised.ex:36: Task.Supervised.reply/4

I also noticed the result of the called transaction function is never used.

josevalim commented 4 months ago

@yastanotheruser Repo.transaction should wrap the value returned by the transaction in an {:ok, _} tuple, so you get {:ok, :ok} in this case. So if there is an issue, it is elsewhere, but not at the pointed line.

yastanotheruser commented 4 months ago

When I change the return value of attempt to {:ok, nil} and compile with mix deps.compile ecto_sql I'm no longer getting the error.

greg-rychlewski commented 4 months ago

@yastanotheruser Would you be able to set up a small script like this with your dependencies, migrations and run command: https://github.com/wojtekmach/mix_install_examples/blob/main/ecto_sql.exs

Sorry to ask but the behaviour you're seeing is very unusual and I cannot pinpoint what might be causing it.

yastanotheruser commented 3 months ago

It turns out this happened because I was incorrectly overriding the repo transaction function. Closing this issue. Thanks to @greg-rychlewski for the script idea.