Fixing the varchar size migration bug (Issue #150)

lib/migration_generator/operation.ex

@@ -565,10 +565,10 @@ defmodule AshPostgres.MigrationGenerator.Operation do
         end
 
       size =
-        if Map.get(attribute, :size) != Map.get(old_attribute, :size) do
-          if attribute.size do
-            ", size: #{attribute.size}"
-          end
+        if attribute[:size] &&
+             (Map.get(attribute, :size) != Map.get(old_attribute, :size) ||
+                attribute.type != old_attribute.type) do
+          ", size: #{attribute.size}"
         end
 
       precision =

test/migration_generator_test.exs

@@ -3489,6 +3489,275 @@ defmodule AshPostgres.MigrationGeneratorTest do
     end
   end
 
+  describe "varchar migration_types on modify" do
+    setup do
+      :ok
+    end
+
+    test "modify includes varchar size when adding migration_types to existing string column", %{
+      snapshot_path: snapshot_path,
+      migration_path: migration_path
+    } do
+      defresource MyResource do
+        postgres do
+          table "my_resources"
+          repo(AshPostgres.TestRepo)
+        end
+
+        actions do
+          defaults([:create, :read, :update, :destroy])
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:blibs, :string, public?: true)
+        end
+      end
+
+      defdomain([MyResource])
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: snapshot_path,
+        migration_path: migration_path,
+        quiet: true,
+        format: false,
+        auto_name: true
+      )
+
+      defresource MyResource do
+        postgres do
+          table "my_resources"
+          migration_types(blibs: {:varchar, 255})
+          repo(AshPostgres.TestRepo)
+        end
+
+        actions do
+          defaults([:create, :read, :update, :destroy])
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:blibs, :string, public?: true)
+        end
+      end
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: snapshot_path,
+        migration_path: migration_path,
+        quiet: true,
+        format: false,
+        auto_name: true
+      )
+
+      assert [_file1, file2] =
+               Enum.sort(Path.wildcard("#{migration_path}/**/*_migrate_resources*.exs"))
+               |> Enum.reject(&String.contains?(&1, "extensions"))
+
+      second_migration = File.read!(file2)
+
+      assert second_migration =~ ~S[modify :blibs, :varchar, size: 255]
+      assert second_migration =~ ~S[modify :blibs, :text]
+    end
+
+    test "modify includes new size when changing from one varchar size to another", %{
+      snapshot_path: snapshot_path,
+      migration_path: migration_path
+    } do
+      defresource MyResource do
+        postgres do
+          table "my_resources_varchar_change"
+          migration_types(blibs: {:varchar, 100})
+          repo(AshPostgres.TestRepo)
+        end
+
+        actions do
+          defaults([:create, :read, :update, :destroy])
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:blibs, :string, public?: true)
+        end
+      end
+
+      defdomain([MyResource])
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: snapshot_path,
+        migration_path: migration_path,
+        quiet: true,
+        format: false,
+        auto_name: true
+      )
+
+      defresource MyResource do
+        postgres do
+          table "my_resources_varchar_change"
+          migration_types(blibs: {:varchar, 255})
+          repo(AshPostgres.TestRepo)
+        end
+
+        actions do
+          defaults([:create, :read, :update, :destroy])
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:blibs, :string, public?: true)
+        end
+      end
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: snapshot_path,
+        migration_path: migration_path,
+        quiet: true,
+        format: false,
+        auto_name: true
+      )
+
+      assert [_file1, file2] =
+               Enum.sort(Path.wildcard("#{migration_path}/**/*_migrate_resources*.exs"))
+               |> Enum.reject(&String.contains?(&1, "extensions"))
+
+      second_migration = File.read!(file2)
+
+      assert second_migration =~ ~S[modify :blibs, :varchar, size: 255]
+      assert second_migration =~ ~S[modify :blibs, :varchar, size: 100]
+    end
+
+    test "modify includes size when changing text to binary with migration_types", %{
+      snapshot_path: snapshot_path,
+      migration_path: migration_path
+    } do
+      defresource MyResource do
+        postgres do
+          table "my_resources_binary"
+          repo(AshPostgres.TestRepo)
+        end
+
+        actions do
+          defaults([:create, :read, :update, :destroy])
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:blobs, :string, public?: true)
+        end
+      end
+
+      defdomain([MyResource])
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: snapshot_path,
+        migration_path: migration_path,
+        quiet: true,
+        format: false,
+        auto_name: true
+      )
+
+      defresource MyResource do
+        postgres do
+          table "my_resources_binary"
+          migration_types(blobs: {:binary, 500})
+          repo(AshPostgres.TestRepo)
+        end
+
+        actions do
+          defaults([:create, :read, :update, :destroy])
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:blobs, :string, public?: true)
+        end
+      end
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: snapshot_path,
+        migration_path: migration_path,
+        quiet: true,
+        format: false,
+        auto_name: true
+      )
+
+      assert [_file1, file2] =
+               Enum.sort(Path.wildcard("#{migration_path}/**/*_migrate_resources*.exs"))
+               |> Enum.reject(&String.contains?(&1, "extensions"))
+
+      second_migration = File.read!(file2)
+
+      assert second_migration =~ ~S[modify :blobs, :binary, size: 500]
+      assert second_migration =~ ~S[modify :blobs, :text]
+    end
+
+    test "modify only affects attribute with migration_types when multiple string attributes exist", %{
+      snapshot_path: snapshot_path,
+      migration_path: migration_path
+    } do
+      defresource MyResource do
+        postgres do
+          table "my_resources_multi"
+          repo(AshPostgres.TestRepo)
+        end
+
+        actions do
+          defaults([:create, :read, :update, :destroy])
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:blibs, :string, public?: true)
+          attribute(:blobs, :string, public?: true)
+        end
+      end
+
+      defdomain([MyResource])
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: snapshot_path,
+        migration_path: migration_path,
+        quiet: true,
+        format: false,
+        auto_name: true
+      )
+
+      defresource MyResource do
+        postgres do
+          table "my_resources_multi"
+          migration_types(blibs: {:varchar, 255})
+          repo(AshPostgres.TestRepo)
+        end
+
+        actions do
+          defaults([:create, :read, :update, :destroy])
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:blibs, :string, public?: true)
+          attribute(:blobs, :string, public?: true)
+        end
+      end
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: snapshot_path,
+        migration_path: migration_path,
+        quiet: true,
+        format: false,
+        auto_name: true
+      )
+
+      assert [_file1, file2] =
+               Enum.sort(Path.wildcard("#{migration_path}/**/*_migrate_resources*.exs"))
+               |> Enum.reject(&String.contains?(&1, "extensions"))
+
+      second_migration = File.read!(file2)
+
+      assert second_migration =~ ~S[modify :blibs, :varchar, size: 255]
+      refute second_migration =~ ~S[modify :blobs]
+    end
+  end
+
   describe "create_table_options" do
     setup do
       :ok