Fix reversible migration modify/3 (#384)

integration_test/sql/migration.exs

@@ -76,15 +76,17 @@ defmodule Ecto.Integration.MigrationTest do
     def change do
       create table(:modify_from_products) do
         add :value, :integer
+        add :nullable, :integer, null: false
       end
 
       if direction() == :up do
         flush()
-        PoolRepo.insert_all "modify_from_products", [[value: 1]]
+        PoolRepo.insert_all "modify_from_products", [[value: 1, nullable: 1]]
       end
 
       alter table(:modify_from_products) do
         modify :value, :bigint, from: :integer
+        modify :nullable, :bigint, null: true, from: {:integer, null: false}
       end
     end
   end

lib/ecto/migration.ex

@@ -1035,10 +1035,12 @@ defmodule Ecto.Migration do
   Modifies the type of a column when altering a table.
 
   This command is not reversible unless the `:from` option is provided.
-  If the `:from` value is a `%Reference{}`, the adapter will try to drop
+  When the `:from` option is set, the adapter will try to drop
   the corresponding foreign key constraints before modifying the type.
-  Note `:from` cannot be used to modify primary keys, as those are
-  generally trickier to make reversible.
+  Generally speaking, you want to pass the type and each option
+  you are modifying to `:from`, so the column can be rolled back properly.
+  However, note that `:from` cannot be be used to modify primary keys,
+  as those are generally trickier to revert.
 
   See `add/3` for more information on supported types.
 
@@ -1054,11 +1056,21 @@ defmodule Ecto.Migration do
         modify :title, :text
       end
 
+      # Self rollback when using the :from option
+      alter table("posts") do
+        modify :title, :text, from: :string
+      end
+
+      # Modify column with rollback options
+      alter table("posts") do
+        modify :title, :text, null: false, from: {:string, null: true}
+      end
+
   ## Options
 
     * `:null` - determines whether the column accepts null values.
     * `:default` - changes the default value of the column.
-    * `:from` - specifies the current type of the column.
+    * `:from` - specifies the current type and options of the column.
     * `:size` - specifies the size of the type (for example, the number of characters).
       The default is no size.
     * `:precision` - the precision for a numeric type. Required when `:scale` is

lib/ecto/migration/runner.ex

@@ -253,8 +253,17 @@ defmodule Ecto.Migration.Runner do
   end
   defp table_reverse([{:modify, name, type, opts} | t], acc) do
     case opts[:from] do
-      nil -> false
-      from -> table_reverse(t, [{:modify, name, from, Keyword.put(opts, :from, type)} | acc])
+      nil ->
+        false
+
+      {reverse_type, from_opts} when is_list(from_opts) ->
+        reverse_from = {type, Keyword.delete(opts, :from)}
+        reverse_opts = Keyword.put(from_opts, :from, reverse_from)
+        table_reverse(t, [{:modify, name, reverse_type, reverse_opts} | acc])
+
+      reverse_type ->
+        reverse_opts = Keyword.put(opts, :from, type)
+        table_reverse(t, [{:modify, name, reverse_type, reverse_opts} | acc])
     end
   end
   defp table_reverse([{:add, name, _type, _opts} | t], acc) do

test/ecto/adapters/myxql_test.exs

@@ -1335,6 +1335,7 @@ defmodule Ecto.Adapters.MyXQLTest do
                 {:modify, :status, :string, from: :integer},
                 {:modify, :user_id, :integer, from: %Reference{table: :users}},
                 {:modify, :group_id, %Reference{table: :groups, column: :gid}, from: %Reference{table: :groups}},
+                {:modify, :status, :string, [null: false, size: 100, from: {:integer, null: true, size: 50}]},
                 {:remove, :summary},
                 {:remove, :body, :text, []},
                 {:remove, :space_id, %Reference{table: :author}, []},
@@ -1357,6 +1358,7 @@ defmodule Ecto.Adapters.MyXQLTest do
     DROP FOREIGN KEY `posts_group_id_fkey`,
     MODIFY `group_id` BIGINT UNSIGNED,
     ADD CONSTRAINT `posts_group_id_fkey` FOREIGN KEY (`group_id`) REFERENCES `groups`(`gid`),
+    MODIFY `status` varchar(100) NOT NULL,
     DROP `summary`,
     DROP `body`,
     DROP FOREIGN KEY `posts_space_id_fkey`,

test/ecto/adapters/postgres_test.exs

@@ -1595,6 +1595,7 @@ defmodule Ecto.Adapters.PostgresTest do
              {:modify, :status, :string, from: :integer},
              {:modify, :user_id, :integer, from: %Reference{table: :users}},
              {:modify, :group_id, %Reference{table: :groups, column: :gid}, from: %Reference{table: :groups}},
+             {:modify, :status, :string, [null: false, size: 100, from: {:integer, null: true, size: 50}]},
              {:remove, :summary},
              {:remove, :body, :text, []},
              {:remove, :space_id, %Reference{table: :author}, []},
@@ -1625,6 +1626,8 @@ defmodule Ecto.Adapters.PostgresTest do
     DROP CONSTRAINT "posts_group_id_fkey",
     ALTER COLUMN "group_id" TYPE bigint,
     ADD CONSTRAINT "posts_group_id_fkey" FOREIGN KEY ("group_id") REFERENCES "groups"("gid"),
+    ALTER COLUMN "status" TYPE varchar(100),
+    ALTER COLUMN "status" SET NOT NULL,
     DROP COLUMN "summary",
     DROP COLUMN "body",
     DROP CONSTRAINT "posts_space_id_fkey",

test/ecto/migration_test.exs

@@ -718,12 +718,18 @@ defmodule Ecto.MigrationTest do
     alter table(:posts) do
       add :summary, :text
       modify :extension, :text, from: :string
+      modify :author, :string, null: false, from: :text
+      modify :title, :string, null: false, size: 100, from: {:text, null: true, size: 255}
     end
     flush()
 
     assert last_command() ==
-           {:alter, %Table{name: "posts"},
-              [{:modify, :extension, :string, from: :text}, {:remove, :summary}]}
+           {:alter, %Table{name: "posts"}, [
+              {:modify, :title, :text, [from: {:string, null: false, size: 100}, null: true, size: 255]},
+              {:modify, :author, :text, [from: :string, null: false]},
+              {:modify, :extension, :string, from: :text},
+              {:remove, :summary}
+            ]}
 
     assert_raise Ecto.MigrationError, ~r/cannot reverse migration command/, fn ->
       alter table(:posts) do