fix: don't rewrite identities when only global? is changed

fix: don't modify an attribute when it only needs to be renamed

Author: zachdaniel

lib/migration_generator/migration_generator.ex

@@ -2305,21 +2305,22 @@ defmodule AshPostgres.MigrationGenerator do
          Enum.find(
            old_snapshot.attributes,
            fn old_attribute ->
-             source_match =
-               Enum.find_value(attributes_to_rename, old_attribute.source, fn {new, old} ->
+             renaming_to_source =
+               Enum.find_value(attributes_to_rename, fn {new, old} ->
                  if old.source == old_attribute.source do
                    new.source
                  end
                end)
 
-             source_match ==
+             (renaming_to_source || old_attribute.source) ==
                attribute.source &&
                attributes_unequal?(
                  old_attribute,
                  attribute,
                  snapshot.repo,
                  old_snapshot,
-                 snapshot
+                 snapshot,
+                 not is_nil(renaming_to_source)
                )
            end
          )}
@@ -2522,11 +2523,25 @@ defmodule AshPostgres.MigrationGenerator do
 
   # This exists to handle the fact that the remapping of the key name -> source caused attributes
   # to be considered unequal. We ignore things that only differ in that way using this function.
-  defp attributes_unequal?(left, right, repo, _old_snapshot, _new_snapshot) do
+  defp attributes_unequal?(left, right, repo, _old_snapshot, _new_snapshot, ignore_names?) do
     left = clean_for_equality(left, repo)
 
     right = clean_for_equality(right, repo)
 
+    left =
+      if ignore_names? do
+        Map.drop(left, [:source, :name])
+      else
+        left
+      end
+
+    right =
+      if ignore_names? do
+        Map.drop(left, [:source, :name])
+      else
+        right
+      end
+
     left != right
   end
 
@@ -2583,7 +2598,7 @@ defmodule AshPostgres.MigrationGenerator do
   end
 
   def changing_multitenancy_affects_identities?(snapshot, old_snapshot) do
-    snapshot.multitenancy != old_snapshot.multitenancy ||
+    Map.delete(snapshot.multitenancy, :global) != Map.delete(old_snapshot.multitenancy, :global) ||
       snapshot.base_filter != old_snapshot.base_filter
   end
 

test/migration_generator_test.exs

@@ -584,6 +584,77 @@ defmodule AshPostgres.MigrationGeneratorTest do
     end
   end
 
+  describe "changing global multitenancy" do
+    setup do
+      on_exit(fn ->
+        File.rm_rf!("test_snapshots_path")
+        File.rm_rf!("test_migration_path")
+      end)
+
+      defposts do
+        identities do
+          identity(:title, [:title])
+        end
+
+        multitenancy do
+          strategy(:attribute)
+          attribute(:organization_id)
+          global?(false)
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:title, :string, public?: true)
+          attribute(:organization_id, :string)
+        end
+      end
+
+      defdomain([Post])
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: "test_snapshots_path",
+        migration_path: "test_migration_path",
+        quiet: true,
+        format: false
+      )
+
+      :ok
+    end
+
+    test "when changing multitenancy to global, identities aren't rewritten" do
+      defposts do
+        identities do
+          identity(:title, [:title])
+        end
+
+        multitenancy do
+          strategy(:attribute)
+          attribute(:organization_id)
+          global?(true)
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:title, :string, public?: true)
+          attribute(:organization_id, :string)
+        end
+      end
+
+      defdomain([Post])
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: "test_snapshots_path",
+        migration_path: "test_migration_path",
+        quiet: true,
+        format: false
+      )
+
+      assert [_file1] =
+               Enum.sort(Path.wildcard("test_migration_path/**/*_migrate_resources*.exs"))
+               |> Enum.reject(&String.contains?(&1, "extensions"))
+    end
+  end
+
   describe "creating follow up migrations" do
     setup do
       on_exit(fn ->
@@ -614,6 +685,37 @@ defmodule AshPostgres.MigrationGeneratorTest do
       :ok
     end
 
+    test "when renaming an attribute of an index, it is properly renamed without modifying the attribute" do
+      defposts do
+        identities do
+          identity(:title, [:foobar])
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:foobar, :string, public?: true)
+        end
+      end
+
+      defdomain([Post])
+
+      send(self(), {:mix_shell_input, :yes?, true})
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: "test_snapshots_path",
+        migration_path: "test_migration_path",
+        quiet: true,
+        format: false
+      )
+
+      assert [_file1, file2] =
+               Enum.sort(Path.wildcard("test_migration_path/**/*_migrate_resources*.exs"))
+               |> Enum.reject(&String.contains?(&1, "extensions"))
+
+      contents = File.read!(file2)
+      refute contents =~ "modify"
+    end
+
     test "when renaming an index, it is properly renamed" do
       defposts do
         postgres do