FIX: `Permalink.create` didn't work as expected anymore (#29895)

This moves the logic of setting the correct permalink values back into the controller. And it replaces the validation with a simpler one, that always works, even when the model is loaded from the DB.

Follow-up to #29634 which broke import scripts and lots of documentation on Meta.

app/assets/javascripts/admin/addon/components/admin-permalink-form.gjs

@@ -45,7 +45,7 @@ export default class AdminFlagsForm extends Component {
       } else if (!isEmpty(this.args.permalink.category_id)) {
         permalinkType = "category";
         permalinkValue = this.args.permalink.category_id;
-      } else if (!isEmpty(this.args.permalink.tag_name)) {
+      } else if (!isEmpty(this.args.permalink.tag_id)) {
         permalinkType = "tag";
         permalinkValue = this.args.permalink.tag_name;
       } else if (!isEmpty(this.args.permalink.external_url)) {

app/controllers/admin/permalinks_controller.rb

@@ -20,24 +20,14 @@ class Admin::PermalinksController < Admin::AdminController
   end
 
   def create
-    permalink =
+    permalink = Permalink.create!(permalink_params)
-      Permalink.create!(
-        url: permalink_params[:url],
-        permalink_type: permalink_params[:permalink_type],
-        permalink_type_value: permalink_params[:permalink_type_value],
-      )
     render_serialized(permalink, PermalinkSerializer)
   rescue ActiveRecord::RecordInvalid => e
     render_json_error(e.record.errors.full_messages)
   end
 
   def update
-    @permalink.update!(
+    @permalink.update!(permalink_params)
-      url: permalink_params[:url],
-      permalink_type: permalink_params[:permalink_type],
-      permalink_type_value: permalink_params[:permalink_type_value],
-    )
-
     render_serialized(@permalink, PermalinkSerializer)
   rescue ActiveRecord::RecordInvalid => e
     render_json_error(e.record.errors.full_messages)
@@ -55,6 +45,24 @@ class Admin::PermalinksController < Admin::AdminController
   end
 
   def permalink_params
-    params.require(:permalink).permit(:url, :permalink_type, :permalink_type_value)
+    permitted_params =
+      params.require(:permalink).permit(:url, :permalink_type, :permalink_type_value)
+
+    {
+      url: permitted_params[:url],
+      topic_id: extract_param(permitted_params, "topic"),
+      post_id: extract_param(permitted_params, "post"),
+      category_id: extract_param(permitted_params, "category"),
+      tag_id:
+        extract_param(permitted_params, "tag").then do |tag_name|
+          (Tag.where(name: tag_name).pluck(:id).first || -1) if tag_name
+        end,
+      user_id: extract_param(permitted_params, "user"),
+      external_url: extract_param(permitted_params, "external_url"),
+    }
+  end
+
+  def extract_param(permitted_params, type)
+    permitted_params[:permalink_type] == type ? permitted_params[:permalink_type_value] : nil
   end
 end

app/models/permalink.rb

@@ -1,30 +1,16 @@
 # frozen_string_literal: true
 
 class Permalink < ActiveRecord::Base
-  attr_accessor :permalink_type, :permalink_type_value
-
   belongs_to :topic
   belongs_to :post
   belongs_to :category
   belongs_to :tag
   belongs_to :user
 
-  before_validation :clear_associations
   before_validation :normalize_url, :encode_url
-  before_validation :set_association_value
 
   validates :url, uniqueness: true
-
+  validate :exactly_one_association
-  validates :topic_id, presence: true, if: Proc.new { |permalink| permalink.topic_type? }
-  validates :post_id, presence: true, if: Proc.new { |permalink| permalink.post_type? }
-  validates :category_id, presence: true, if: Proc.new { |permalink| permalink.category_type? }
-  validates :tag_id, presence: true, if: Proc.new { |permalink| permalink.tag_type? }
-  validates :user_id, presence: true, if: Proc.new { |permalink| permalink.user_type? }
-  validates :external_url, presence: true, if: Proc.new { |permalink| permalink.external_url_type? }
-
-  %i[topic post category tag user external_url].each do |association|
-    define_method("#{association}_type?") { self.permalink_type == association.to_s }
-  end
 
   class Normalizer
     attr_reader :source
@@ -114,22 +100,14 @@ class Permalink < ActiveRecord::Base
     external_url.match?(%r{\A/[^/]}) ? "#{Discourse.base_path}#{external_url}" : external_url
   end
 
-  def clear_associations
+  def exactly_one_association
-    self.topic_id = nil
+    associations = [topic_id, post_id, category_id, tag_id, user_id, external_url]
-    self.post_id = nil
+    if associations.compact.size != 1
-    self.category_id = nil
+      errors.add(
-    self.user_id = nil
+        :base,
-    self.tag_id = nil
+        "Exactly one of topic_id, post_id, category_id, tag_id, user_id, or external_url must be set",
-    self.external_url = nil
+      )
-  end
+    end
-
-  def set_association_value
-    self.topic_id = self.permalink_type_value if self.topic_type?
-    self.post_id = self.permalink_type_value if self.post_type?
-    self.user_id = self.permalink_type_value if self.user_type?
-    self.category_id = self.permalink_type_value if self.category_type?
-    self.external_url = self.permalink_type_value if self.external_url_type?
-    self.tag_id = Tag.where(name: self.permalink_type_value).first&.id if self.tag_type?
   end
 end
 

spec/models/category_spec.rb

@@ -474,13 +474,13 @@ RSpec.describe Category do
     end
 
     it "deletes permalink when category slug is reused" do
-      Fabricate(:permalink, url: "/c/bikeshed-category")
+      Fabricate(:permalink, url: "/c/bikeshed-category", category_id: 42)
       Fabricate(:category_with_definition, slug: "bikeshed-category")
       expect(Permalink.count).to eq(0)
     end
 
     it "deletes permalink when sub category slug is reused" do
-      Fabricate(:permalink, url: "/c/main-category/sub-category")
+      Fabricate(:permalink, url: "/c/main-category/sub-category", category_id: 42)
       main_category = Fabricate(:category_with_definition, slug: "main-category")
       Fabricate(
         :category_with_definition,

spec/models/permalink_spec.rb

@@ -1,6 +1,12 @@
 # frozen_string_literal: true
 
 RSpec.describe Permalink do
+  let(:topic) { Fabricate(:topic) }
+  let(:post) { Fabricate(:post, topic: topic) }
+  let(:category) { Fabricate(:category) }
+  let(:tag) { Fabricate(:tag) }
+  let(:user) { Fabricate(:user) }
+
   describe "normalization" do
     it "correctly normalizes" do
       normalizer = Permalink::Normalizer.new("/(\\/hello.*)\\?.*/\\1|/(\\/bye.*)\\?.*/\\1")
@@ -13,92 +19,48 @@ RSpec.describe Permalink do
 
   describe "new record" do
     it "strips blanks" do
-      permalink = described_class.create!(url: " my/old/url  ")
+      permalink = described_class.create!(url: " my/old/url  ", topic_id: topic.id)
       expect(permalink.url).to eq("my/old/url")
     end
 
     it "removes leading slash" do
-      permalink = described_class.create!(url: "/my/old/url")
+      permalink = described_class.create!(url: "/my/old/url", topic_id: topic.id)
       expect(permalink.url).to eq("my/old/url")
     end
 
     it "checks for unique URL" do
-      permalink = described_class.create(url: "/my/old/url")
+      permalink = described_class.create(url: "/my/old/url", topic_id: topic.id)
       expect(permalink.errors[:url]).to be_empty
 
-      permalink = described_class.create(url: "/my/old/url")
+      permalink = described_class.create(url: "/my/old/url", topic_id: topic.id)
       expect(permalink.errors[:url]).to be_present
 
+      permalink = described_class.create(url: "my/old/url", topic_id: topic.id)
+      expect(permalink.errors[:url]).to be_present
+    end
+
+    it "ensures that exactly one associated value is set" do
       permalink = described_class.create(url: "my/old/url")
-      expect(permalink.errors[:url]).to be_present
+      expect(permalink.errors[:base]).to be_present
-    end
 
-    it "validates association" do
+      permalink = described_class.create(url: "my/old/url", topic_id: topic.id, post_id: post.id)
-      permalink = described_class.create(url: "/my/old/url", permalink_type: "topic")
+      expect(permalink.errors[:base]).to be_present
-      expect(permalink.errors[:topic_id]).to be_present
 
-      permalink = described_class.create(url: "/my/old/url", permalink_type: "post")
+      permalink = described_class.create(url: "my/old/url", post_id: post.id)
-      expect(permalink.errors[:post_id]).to be_present
+      expect(permalink.errors[:base]).to be_empty
-
-      permalink = described_class.create(url: "/my/old/url", permalink_type: "category")
-      expect(permalink.errors[:category_id]).to be_present
-
-      permalink = described_class.create(url: "/my/old/url", permalink_type: "user")
-      expect(permalink.errors[:user_id]).to be_present
-
-      permalink = described_class.create(url: "/my/old/url", permalink_type: "external_url")
-      expect(permalink.errors[:external_url]).to be_present
-
-      permalink = described_class.create(url: "/my/old/url", permalink_type: "tag")
-      expect(permalink.errors[:tag_id]).to be_present
-    end
-
-    it "clears associations when permalink_type changes" do
-      permalink = described_class.create!(url: " my/old/url  ")
-
-      permalink.update!(permalink_type_value: 1, permalink_type: "topic")
-      expect(permalink.topic_id).to eq(1)
-
-      permalink.update!(permalink_type_value: 1, permalink_type: "post")
-      expect(permalink.topic_id).to be_nil
-      expect(permalink.post_id).to eq(1)
-
-      permalink.update!(permalink_type_value: 1, permalink_type: "category")
-      expect(permalink.post_id).to be_nil
-      expect(permalink.category_id).to eq(1)
-
-      permalink.update!(permalink_type_value: 1, permalink_type: "user")
-      expect(permalink.category_id).to be_nil
-      expect(permalink.user_id).to eq(1)
-
-      permalink.update!(
-        permalink_type_value: "https://discourse.org",
-        permalink_type: "external_url",
-      )
-      expect(permalink.user_id).to be_nil
-      expect(permalink.external_url).to eq("https://discourse.org")
-
-      tag = Fabricate(:tag, name: "art")
-      permalink.update!(permalink_type_value: "art", permalink_type: "tag")
-      expect(permalink.external_url).to be_nil
-      expect(permalink.tag_id).to eq(tag.id)
-
-      permalink.update!(permalink_type_value: 1, permalink_type: "topic")
-      expect(permalink.tag_id).to be_nil
-      expect(permalink.topic_id).to eq(1)
     end
 
     context "with special characters in URL" do
       it "percent encodes any special character" do
-        permalink = described_class.create!(url: "/2022/10/03/привет-sam")
+        permalink = described_class.create!(url: "/2022/10/03/привет-sam", topic_id: topic.id)
         expect(permalink.url).to eq("2022/10/03/%D0%BF%D1%80%D0%B8%D0%B2%D0%B5%D1%82-sam")
       end
 
       it "checks for unique URL" do
-        permalink = described_class.create(url: "/2022/10/03/привет-sam")
+        permalink = described_class.create(url: "/2022/10/03/привет-sam", topic_id: topic.id)
         expect(permalink.errors[:url]).to be_empty
 
-        permalink = described_class.create(url: "/2022/10/03/привет-sam")
+        permalink = described_class.create(url: "/2022/10/03/привет-sam", topic_id: topic.id)
         expect(permalink.errors[:url]).to be_present
       end
     end
@@ -108,11 +70,6 @@ RSpec.describe Permalink do
     subject(:target_url) { permalink.target_url }
 
     let(:permalink) { Fabricate.build(:permalink) }
-    let(:topic) { Fabricate(:topic) }
-    let(:post) { Fabricate(:post, topic: topic) }
-    let(:category) { Fabricate(:category) }
-    let(:tag) { Fabricate(:tag) }
-    let(:user) { Fabricate(:user) }
 
     it "returns nil when nothing is set" do
       expect(target_url).to eq(nil)

spec/requests/admin/permalinks_controller_spec.rb

@@ -10,20 +10,10 @@ RSpec.describe Admin::PermalinksController do
       before { sign_in(admin) }
 
       it "filters url" do
-        Fabricate(:permalink, url: "/forum/23", permalink_type_value: "1", permalink_type: "topic")
+        Fabricate(:permalink, url: "/forum/23", topic_id: 1)
-        Fabricate(:permalink, url: "/forum/98", permalink_type_value: "1", permalink_type: "topic")
+        Fabricate(:permalink, url: "/forum/98", topic_id: 1)
-        Fabricate(
+        Fabricate(:permalink, url: "/discuss/topic/45", topic_id: 1)
-          :permalink,
+        Fabricate(:permalink, url: "/discuss/topic/76", topic_id: 1)
-          url: "/discuss/topic/45",
-          permalink_type_value: "1",
-          permalink_type: "topic",
-        )
-        Fabricate(
-          :permalink,
-          url: "/discuss/topic/76",
-          permalink_type_value: "1",
-          permalink_type: "topic",
-        )
 
         get "/admin/permalinks.json", params: { filter: "topic" }
 
@@ -33,26 +23,10 @@ RSpec.describe Admin::PermalinksController do
       end
 
       it "filters external url" do
-        Fabricate(
+        Fabricate(:permalink, external_url: "http://google.com")
-          :permalink,
+        Fabricate(:permalink, external_url: "http://wikipedia.org")
-          permalink_type_value: "http://google.com",
+        Fabricate(:permalink, external_url: "http://www.discourse.org")
-          permalink_type: "external_url",
+        Fabricate(:permalink, external_url: "http://try.discourse.org")
-        )
-        Fabricate(
-          :permalink,
-          permalink_type_value: "http://wikipedia.org",
-          permalink_type: "external_url",
-        )
-        Fabricate(
-          :permalink,
-          permalink_type_value: "http://www.discourse.org",
-          permalink_type: "external_url",
-        )
-        Fabricate(
-          :permalink,
-          permalink_type_value: "http://try.discourse.org",
-          permalink_type: "external_url",
-        )
 
         get "/admin/permalinks.json", params: { filter: "discourse" }
 
@@ -62,30 +36,10 @@ RSpec.describe Admin::PermalinksController do
       end
 
       it "filters url and external url both" do
-        Fabricate(
+        Fabricate(:permalink, url: "/forum/23", external_url: "http://google.com")
-          :permalink,
+        Fabricate(:permalink, url: "/discourse/98", external_url: "http://wikipedia.org")
-          url: "/forum/23",
+        Fabricate(:permalink, url: "/discuss/topic/45", external_url: "http://discourse.org")
-          permalink_type_value: "http://google.com",
+        Fabricate(:permalink, url: "/discuss/topic/76", external_url: "http://try.discourse.org")
-          permalink_type: "external_url",
-        )
-        Fabricate(
-          :permalink,
-          url: "/discourse/98",
-          permalink_type_value: "http://wikipedia.org",
-          permalink_type: "external_url",
-        )
-        Fabricate(
-          :permalink,
-          url: "/discuss/topic/45",
-          permalink_type_value: "http://discourse.org",
-          permalink_type: "external_url",
-        )
-        Fabricate(
-          :permalink,
-          url: "/discuss/topic/76",
-          permalink_type_value: "http://try.discourse.org",
-          permalink_type: "external_url",
-        )
 
         get "/admin/permalinks.json", params: { filter: "discourse" }
 

spec/requests/application_controller_spec.rb

@@ -345,11 +345,7 @@ RSpec.describe ApplicationController do
     describe "topic not found" do
       it "should not redirect to permalink if topic/category does not exist" do
         topic = create_post.topic
-        Permalink.create!(
+        Permalink.create!(url: topic.relative_url, topic_id: topic.id + 1)
-          url: topic.relative_url,
-          permalink_type_value: topic.id + 1,
-          permalink_type: "topic",
-        )
         topic.trash!
 
         SiteSetting.detailed_404 = false
@@ -364,11 +360,7 @@ RSpec.describe ApplicationController do
       it "should return permalink for deleted topics" do
         topic = create_post.topic
         external_url = "https://somewhere.over.rainbow"
-        Permalink.create!(
+        Permalink.create!(url: topic.relative_url, external_url: external_url)
-          url: topic.relative_url,
-          permalink_type_value: external_url,
-          permalink_type: "external_url",
-        )
         topic.trash!
 
         get topic.relative_url
@@ -390,12 +382,7 @@ RSpec.describe ApplicationController do
         trashed_topic = create_post.topic
         trashed_topic.trash!
         new_topic = create_post.topic
-        permalink =
+        permalink = Permalink.create!(url: trashed_topic.relative_url, topic_id: new_topic.id)
-          Permalink.create!(
-            url: trashed_topic.relative_url,
-            permalink_type_value: new_topic.id,
-            permalink_type: "topic",
-          )
 
         # no subfolder because router doesn't know about subfolder in this test
         get "/t/#{trashed_topic.slug}/#{trashed_topic.id}"
@@ -404,23 +391,14 @@ RSpec.describe ApplicationController do
 
         permalink.destroy
         category = Fabricate(:category)
-        permalink =
+        permalink = Permalink.create!(url: trashed_topic.relative_url, category_id: category.id)
-          Permalink.create!(
-            url: trashed_topic.relative_url,
-            permalink_type_value: category.id,
-            permalink_type: "category",
-          )
         get "/t/#{trashed_topic.slug}/#{trashed_topic.id}"
         expect(response.status).to eq(301)
         expect(response).to redirect_to("/forum/c/#{category.slug}/#{category.id}")
 
         permalink.destroy
         permalink =
-          Permalink.create!(
+          Permalink.create!(url: trashed_topic.relative_url, post_id: new_topic.posts.last.id)
-            url: trashed_topic.relative_url,
-            permalink_type_value: new_topic.posts.last.id,
-            permalink_type: "post",
-          )
         get "/t/#{trashed_topic.slug}/#{trashed_topic.id}"
         expect(response.status).to eq(301)
         expect(response).to redirect_to(

spec/requests/categories_controller_spec.rb

@@ -44,12 +44,7 @@ RSpec.describe CategoriesController do
     end
 
     it "respects permalinks before redirecting /category paths to /c paths" do
-      _perm =
+      _perm = Permalink.create!(url: "category/something", category_id: category.id)
-        Permalink.create!(
-          url: "category/something",
-          permalink_type_value: category.id,
-          permalink_type: "category",
-        )
 
       get "/category/something"
       expect(response).to have_http_status(:moved_permanently)

spec/requests/permalinks_controller_spec.rb

@@ -2,12 +2,10 @@
 
 RSpec.describe PermalinksController do
   fab!(:topic)
-  fab!(:permalink) { Fabricate(:permalink, url: "deadroute/topic/546") }
+  fab!(:permalink) { Fabricate(:permalink, url: "deadroute/topic/546", topic_id: topic.id) }
 
   describe "show" do
     it "should redirect to a permalink's target_url with status 301" do
-      permalink.update!(permalink_type_value: topic.id, permalink_type: "topic")
-
       get "/#{permalink.url}"
 
       expect(response).to redirect_to(topic.relative_url)
@@ -15,7 +13,6 @@ RSpec.describe PermalinksController do
     end
 
     it "should work for subfolder installs too" do
-      permalink.update!(permalink_type_value: topic.id, permalink_type: "topic")
       set_subfolder "/forum"
 
       get "/#{permalink.url}"
@@ -25,7 +22,7 @@ RSpec.describe PermalinksController do
     end
 
     it "should apply normalizations" do
-      permalink.update!(permalink_type_value: "/topic/100", permalink_type: "external_url")
+      permalink.update!(external_url: "/topic/100", topic_id: nil)
       SiteSetting.permalink_normalizations = "/(.*)\\?.*/\\1"
 
       get "/#{permalink.url}", params: { test: "hello" }
@@ -46,14 +43,9 @@ RSpec.describe PermalinksController do
     end
 
     context "when permalink's target_url is an external URL" do
-      before do
-        permalink.update!(
-          permalink_type_value: "https://github.com/discourse/discourse",
-          permalink_type: "external_url",
-        )
-      end
-
       it "redirects to it properly" do
+        permalink.update!(external_url: "https://github.com/discourse/discourse", topic_id: nil)
+
         get "/#{permalink.url}"
         expect(response).to redirect_to(permalink.external_url)
       end