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

[regression] migration changing column started to fail #647

Closed dkuku closed 1 week ago

dkuku commented 3 weeks ago

Elixir version

Elixir 1.16.2 (compiled with Erlang/OTP 26)

Database and Version

any

Ecto Versions

master

Database Adapter and Versions (postgrex, myxql, etc)

postgrex 0.19.2

Current behavior

this migrations started to fail recently:

  def change do
    alter table(:employees) do
      modify(:email, :string, size: 65_535, from: {:string, size: 255})
    end
  end

with error

21:00:26.999 [info] alter table employees
** (Postgrex.Error) ERROR 42601 (syntax_error) syntax error at end of input

    query: ALTER TABLE "employees" 
    (ecto_sql 3.12.1) lib/ecto/adapters/sql.ex:1096: Ecto.Adapters.SQL.raise_sql_call_error/1
    (elixir 1.16.2) lib/enum.ex:1700: Enum."-map/2-lists^map/1-1-"/2
    (ecto_sql 3.12.1) lib/ecto/adapters/sql.ex:1203: Ecto.Adapters.SQL.execute_ddl/4
    (ecto_sql 3.12.1) lib/ecto/migration/runner.ex:348: Ecto.Migration.Runner.log_and_execute_ddl/3
    (elixir 1.16.2) lib/enum.ex:1700: Enum."-map/2-lists^map/1-1-"/2
    (ecto_sql 3.12.1) lib/ecto/migration/runner.ex:311: Ecto.Migration.Runner.perform_operation/3
    (stdlib 5.2.3) timer.erl:270: :timer.tc/2
    (ecto_sql 3.12.1) lib/ecto/migration/runner.ex:25: Ecto.Migration.Runner.run/8

It fails into [this branch] of the if (https://github.com/elixir-ecto/ecto_sql/blame/75ddf591fbe56977b041836079c01be298384024/lib/ecto/adapters/postgres/connection.ex#L1588) but returns [[], [], []] I think it suppose to be be executed the else branch. I'm assuming it was introduced in this pr because my master version from before 2 weeks was working fine

Expected behavior

the migration works on 3.12.1

josevalim commented 3 weeks ago

Hrm, we may need to revert the PR after all and introduce a command explicitly to change everything but the type.

dkuku commented 3 weeks ago

the type returned from column_type(type, opts) is identical on 167 and 173 Thats the opts to this function: [size: 65535, from: {:string, [size: 255]}]

  167 +      column_type = column_type(type, opts)                                                                                            
  168 +      from_column_type = extract_column_type(opts[:from])                                                                              
  169 +                                                                                                                                       
  170 +      drop_reference_expr = drop_reference_expr(opts[:from], table, name)                                                              
  171 +      any_drop_ref? = drop_reference_expr != []                                                                                        
  172 +                                                                                                                                       
  173 +      if column_type == column_type(from_column_type, opts) do   
josevalim commented 3 weeks ago

Oh, so it would work if we extracted it properly? 🤔 Can you please send a PR?