Prevent redirect loops and chains when renaming pages

app/models/spina/page.rb

@@ -115,7 +115,7 @@ def touch_navigations
     end
 
     def rewrite_rule
-      RewriteRule.where(old_path: old_path).first_or_create.update(new_path: materialized_path) if old_path != materialized_path
+      RewriteRule.record(old_path, materialized_path)
     end
 
     def localized_materialized_path

app/models/spina/rewrite_rule.rb

@@ -1,5 +1,15 @@
 module Spina
   class RewriteRule < ApplicationRecord
     validates :old_path, uniqueness: true
+
+    # Records a redirect from old_path to new_path without leaving
+    # loops (A -> B and B -> A) or chains (A -> B -> C) behind.
+    def self.record(old_path, new_path)
+      return if old_path.blank? || old_path == new_path
+
+      where(old_path: new_path).delete_all
+      where(new_path: old_path).update_all(new_path: new_path)
+      where(old_path: old_path).first_or_create.update(new_path: new_path)
+    end
   end
 end

test/models/spina/page_test.rb

@@ -40,6 +40,16 @@ def setup
       assert_equal "/custom-slug", @demo.materialized_path
     end
 
+    test "renaming a page back and forth does not leave redirect loops or chains" do
+      page = FactoryBot.create(:page, title: "Looping page")
+      page.update(url_title: "somewhere-else")
+      page.update(url_title: "looping-page")
+
+      rules = RewriteRule.pluck(:old_path, :new_path)
+      assert_not rules.any? { |old_path, new_path| rules.include?([new_path, old_path]) }
+      assert_equal [page.materialized_path], RewriteRule.distinct.pluck(:new_path)
+    end
+
     test "build slug from ancestors" do
       about = FactoryBot.create :about_page
       page = FactoryBot.create :services_page

test/models/spina/rewrite_rule_test.rb

@@ -0,0 +1,40 @@
+require "test_helper"
+
+module Spina
+  class RewriteRuleTest < ActiveSupport::TestCase
+    test "record creates a rewrite rule" do
+      RewriteRule.record("/a", "/b")
+
+      assert_equal [["/a", "/b"]], RewriteRule.pluck(:old_path, :new_path)
+    end
+
+    test "record skips blank or unchanged paths" do
+      RewriteRule.record(nil, "/a")
+      RewriteRule.record("", "/a")
+      RewriteRule.record("/a", "/a")
+
+      assert_empty RewriteRule.all
+    end
+
+    test "record updates an existing rule for the same old_path" do
+      RewriteRule.record("/a", "/b")
+      RewriteRule.record("/a", "/c")
+
+      assert_equal [["/a", "/c"]], RewriteRule.pluck(:old_path, :new_path)
+    end
+
+    test "renaming back does not leave a redirect loop" do
+      RewriteRule.record("/a", "/b")
+      RewriteRule.record("/b", "/a")
+
+      assert_equal [["/b", "/a"]], RewriteRule.pluck(:old_path, :new_path)
+    end
+
+    test "renaming again does not leave a redirect chain" do
+      RewriteRule.record("/a", "/b")
+      RewriteRule.record("/b", "/c")
+
+      assert_equal [["/a", "/c"], ["/b", "/c"]], RewriteRule.pluck(:old_path, :new_path).sort
+    end
+  end
+end