Merge branch '216904-fj-refactor-wiki-page-meta-slug' into 'master'

Refactor WikiPage::Meta and WikiPage::Slug

See merge request gitlab-org/gitlab!48755

app/models/concerns/has_wiki_page_meta_attributes.rb

+# frozen_string_literal: true
+
+module HasWikiPageMetaAttributes
+  extend ActiveSupport::Concern
+  include Gitlab::Utils::StrongMemoize
+
+  CanonicalSlugConflictError = Class.new(ActiveRecord::RecordInvalid)
+  WikiPageInvalid = Class.new(ArgumentError)
+
+  included do
+    has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+
+    validates :title, length: { maximum: 255 }, allow_nil: false
+    validate :no_two_metarecords_in_same_container_can_have_same_canonical_slug
+
+    scope :with_canonical_slug, ->(slug) do
+      slug_table_name = klass.reflect_on_association(:slugs).table_name
+
+      joins(:slugs).where(slug_table_name => { canonical: true, slug: slug })
+    end
+  end
+
+  class_methods do
+    # Return the (updated) WikiPage::Meta record for a given wiki page
+    #
+    # If none is found, then a new record is created, and its fields are set
+    # to reflect the wiki_page passed.
+    #
+    # @param [String] last_known_slug
+    # @param [WikiPage] wiki_page
+    #
+    # This method raises errors on validation issues.
+    def find_or_create(last_known_slug, wiki_page)
+      raise WikiPageInvalid unless wiki_page.valid?
+
+      container = wiki_page.wiki.container
+      known_slugs = [last_known_slug, wiki_page.slug].compact.uniq
+      raise 'No slugs found! This should not be possible.' if known_slugs.empty?
+
+      transaction do
+        updates = wiki_page_updates(wiki_page)
+        found = find_by_canonical_slug(known_slugs, container)
+        meta = found || create!(updates.merge(container_attrs(container)))
+
+        meta.update_state(found.nil?, known_slugs, wiki_page, updates)
+
+        # We don't need to run validations here, since find_by_canonical_slug
+        # guarantees that there is no conflict in canonical_slug, and DB
+        # constraints on title and project_id/group_id enforce our other invariants
+        # This saves us a query.
+        meta
+      end
+    end
+
+    def find_by_canonical_slug(canonical_slug, container)
+      meta, conflict = with_canonical_slug(canonical_slug)
+        .where(container_attrs(container))
+        .limit(2)
+
+      if conflict.present?
+        meta.errors.add(:canonical_slug, 'Duplicate value found')
+        raise CanonicalSlugConflictError.new(meta)
+      end
+
+      meta
+    end
+
+    private
+
+    def wiki_page_updates(wiki_page)
+      last_commit_date = wiki_page.version_commit_timestamp || Time.now.utc
+
+      {
+        title: wiki_page.title,
+        created_at: last_commit_date,
+        updated_at: last_commit_date
+      }
+    end
+
+    def container_key
+      raise NotImplementedError
+    end
+
+    def container_attrs(container)
+      { container_key => container.id }
+    end
+  end
+
+  def canonical_slug
+    strong_memoize(:canonical_slug) { slugs.canonical.take&.slug }
+  end
+
+  # rubocop:disable Gitlab/ModuleWithInstanceVariables
+  def canonical_slug=(slug)
+    return if @canonical_slug == slug
+
+    if persisted?
+      transaction do
+        slugs.canonical.update_all(canonical: false)
+        page_slug = slugs.create_with(canonical: true).find_or_create_by(slug: slug)
+        page_slug.update_columns(canonical: true) unless page_slug.canonical?
+      end
+    else
+      slugs.new(slug: slug, canonical: true)
+    end
+
+    @canonical_slug = slug
+  end
+  # rubocop:enable Gitlab/ModuleWithInstanceVariables
+
+  def update_state(created, known_slugs, wiki_page, updates)
+    update_wiki_page_attributes(updates)
+    insert_slugs(known_slugs, created, wiki_page.slug)
+    self.canonical_slug = wiki_page.slug
+  end
+
+  private
+
+  def update_wiki_page_attributes(updates)
+    # Remove all unnecessary updates:
+    updates.delete(:updated_at) if updated_at == updates[:updated_at]
+    updates.delete(:created_at) if created_at <= updates[:created_at]
+    updates.delete(:title) if title == updates[:title]
+
+    update_columns(updates) unless updates.empty?
+  end
+
+  def insert_slugs(strings, is_new, canonical_slug)
+    creation = Time.current.utc
+
+    slug_attrs = strings.map do |slug|
+      slug_attributes(slug, canonical_slug, is_new, creation)
+    end
+    slugs.insert_all(slug_attrs) unless !is_new && slug_attrs.size == 1
+
+    @canonical_slug = canonical_slug if is_new || strings.size == 1 # rubocop:disable Gitlab/ModuleWithInstanceVariables
+  end
+
+  def slug_attributes(slug, canonical_slug, is_new, creation)
+    {
+      slug: slug,
+      canonical: (is_new && slug == canonical_slug),
+      created_at: creation,
+      updated_at: creation
+    }.merge(slug_meta_attributes)
+  end
+
+  def slug_meta_attributes
+    { self.association(:slugs).reflection.foreign_key => id }
+  end
+
+  def no_two_metarecords_in_same_container_can_have_same_canonical_slug
+    container_id = attributes[self.class.container_key.to_s]
+
+    return unless container_id.present? && canonical_slug.present?
+
+    offending = self.class.with_canonical_slug(canonical_slug).where(self.class.container_key => container_id)
+    offending = offending.where.not(id: id) if persisted?
+
+    if offending.exists?
+      errors.add(:canonical_slug, 'each page in a wiki must have a distinct canonical slug')
+    end
+  end
+end

app/models/concerns/has_wiki_page_slug_attributes.rb

+# frozen_string_literal: true
+
+module HasWikiPageSlugAttributes
+  extend ActiveSupport::Concern
+
+  included do
+    validates :slug, uniqueness: { scope: meta_foreign_key }
+    validates :slug, length: { maximum: 2048 }, allow_nil: false
+    validates :canonical, uniqueness: {
+          scope: meta_foreign_key,
+          if: :canonical?,
+          message: 'Only one slug can be canonical per wiki metadata record'
+    }
+
+    scope :canonical, -> { where(canonical: true) }
+
+    def update_columns(attrs = {})
+      super(attrs.reverse_merge(updated_at: Time.current.utc))
+    end
+  end
+
+  def self.update_all(attrs = {})
+    super(attrs.reverse_merge(updated_at: Time.current.utc))
+  end
+end

app/models/wiki_page/meta.rb

@@ -2,149 +2,20 @@
 
 class WikiPage
   class Meta < ApplicationRecord
-    include Gitlab::Utils::StrongMemoize
-
-    CanonicalSlugConflictError = Class.new(ActiveRecord::RecordInvalid)
-    WikiPageInvalid = Class.new(ArgumentError)
+    include HasWikiPageMetaAttributes
 
     self.table_name = 'wiki_page_meta'
 
     belongs_to :project
 
     has_many :slugs, class_name: 'WikiPage::Slug', foreign_key: 'wiki_page_meta_id', inverse_of: :wiki_page_meta
-    has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
-    validates :title, presence: true
     validates :project_id, presence: true
-    validate :no_two_metarecords_in_same_project_can_have_same_canonical_slug
-
-    scope :with_canonical_slug, ->(slug) do
-      joins(:slugs).where(wiki_page_slugs: { canonical: true, slug: slug })
-    end
 
     alias_method :resource_parent, :project
 
-    class << self
-      # Return the (updated) WikiPage::Meta record for a given wiki page
-      #
-      # If none is found, then a new record is created, and its fields are set
-      # to reflect the wiki_page passed.
-      #
-      # @param [String] last_known_slug
-      # @param [WikiPage] wiki_page
-      #
-      # This method raises errors on validation issues.
-      def find_or_create(last_known_slug, wiki_page)
-        raise WikiPageInvalid unless wiki_page.valid?
-
-        project = wiki_page.wiki.project
-        known_slugs = [last_known_slug, wiki_page.slug].compact.uniq
-        raise 'No slugs found! This should not be possible.' if known_slugs.empty?
-
-        transaction do
-          updates = wiki_page_updates(wiki_page)
-          found = find_by_canonical_slug(known_slugs, project)
-          meta = found || create!(updates.merge(project_id: project.id))
-
-          meta.update_state(found.nil?, known_slugs, wiki_page, updates)
-
-          # We don't need to run validations here, since find_by_canonical_slug
-          # guarantees that there is no conflict in canonical_slug, and DB
-          # constraints on title and project_id enforce our other invariants
-          # This saves us a query.
-          meta
-        end
-      end
-
-      def find_by_canonical_slug(canonical_slug, project)
-        meta, conflict = with_canonical_slug(canonical_slug)
-          .where(project_id: project.id)
-          .limit(2)
-
-        if conflict.present?
-          meta.errors.add(:canonical_slug, 'Duplicate value found')
-          raise CanonicalSlugConflictError.new(meta)
-        end
-
-        meta
-      end
-
-      private
-
-      def wiki_page_updates(wiki_page)
-        last_commit_date = wiki_page.version_commit_timestamp || Time.now.utc
-
-        {
-          title: wiki_page.title,
-          created_at: last_commit_date,
-          updated_at: last_commit_date
-        }
-      end
-    end
-
-    def canonical_slug
-      strong_memoize(:canonical_slug) { slugs.canonical.first&.slug }
-    end
-
-    def canonical_slug=(slug)
-      return if @canonical_slug == slug
-
-      if persisted?
-        transaction do
-          slugs.canonical.update_all(canonical: false)
-          page_slug = slugs.create_with(canonical: true).find_or_create_by(slug: slug)
-          page_slug.update_columns(canonical: true) unless page_slug.canonical?
-        end
-      else
-        slugs.new(slug: slug, canonical: true)
-      end
-
-      @canonical_slug = slug
-    end
-
-    def update_state(created, known_slugs, wiki_page, updates)
-      update_wiki_page_attributes(updates)
-      insert_slugs(known_slugs, created, wiki_page.slug)
-      self.canonical_slug = wiki_page.slug
-    end
-
-    private
-
-    def update_wiki_page_attributes(updates)
-      # Remove all unnecessary updates:
-      updates.delete(:updated_at) if updated_at == updates[:updated_at]
-      updates.delete(:created_at) if created_at <= updates[:created_at]
-      updates.delete(:title) if title == updates[:title]
-
-      update_columns(updates) unless updates.empty?
-    end
-
-    def insert_slugs(strings, is_new, canonical_slug)
-      creation = Time.current.utc
-
-      slug_attrs = strings.map do |slug|
-        {
-          wiki_page_meta_id: id,
-          slug: slug,
-          canonical: (is_new && slug == canonical_slug),
-          created_at: creation,
-          updated_at: creation
-        }
-      end
-      slugs.insert_all(slug_attrs) unless !is_new && slug_attrs.size == 1
-
-      @canonical_slug = canonical_slug if is_new || strings.size == 1
-    end
-
-    def no_two_metarecords_in_same_project_can_have_same_canonical_slug
-      return unless project_id.present? && canonical_slug.present?
-
-      offending = self.class.with_canonical_slug(canonical_slug).where(project_id: project_id)
-      offending = offending.where.not(id: id) if persisted?
-
-      if offending.exists?
-        errors.add(:canonical_slug, 'each page in a wiki must have a distinct canonical slug')
-      end
+    def self.container_key
+      :project_id
     end
   end
 end

app/models/wiki_page/slug.rb

@@ -2,25 +2,14 @@
 
 class WikiPage
   class Slug < ApplicationRecord
-    self.table_name = 'wiki_page_slugs'
-
-    belongs_to :wiki_page_meta, class_name: 'WikiPage::Meta', inverse_of: :slugs
-
-    validates :slug, presence: true, uniqueness: { scope: :wiki_page_meta_id }
-    validates :canonical, uniqueness: {
-          scope: :wiki_page_meta_id,
-          if: :canonical?,
-          message: 'Only one slug can be canonical per wiki metadata record'
-    }
+    def self.meta_foreign_key
+      :wiki_page_meta_id
+    end
 
-    scope :canonical, -> { where(canonical: true) }
+    include HasWikiPageSlugAttributes
 
-    def update_columns(attrs = {})
-      super(attrs.reverse_merge(updated_at: Time.current.utc))
-    end
+    self.table_name = 'wiki_page_slugs'
 
-    def self.update_all(attrs = {})
-      super(attrs.reverse_merge(updated_at: Time.current.utc))
-    end
+    belongs_to :wiki_page_meta, class_name: 'WikiPage::Meta', inverse_of: :slugs
   end
 end

spec/models/wiki_page/meta_spec.rb

@@ -25,13 +25,8 @@ RSpec.describe WikiPage::Meta do
     end
 
     it { is_expected.to validate_presence_of(:project_id) }
-    it { is_expected.to validate_presence_of(:title) }
-
-    it 'is forbidden to add extremely long titles' do
-      expect do
-        create(:wiki_page_meta, project: project, title: FFaker::Lorem.characters(300))
-      end.to raise_error(ActiveRecord::ValueTooLong)
-    end
+    it { is_expected.to validate_length_of(:title).is_at_most(255) }
+    it { is_expected.not_to allow_value(nil).for(:title) }
 
     it 'is forbidden to have two records for the same project with the same canonical_slug' do
       the_slug = generate(:sluggified_title)

spec/models/wiki_page/slug_spec.rb

@@ -48,8 +48,9 @@ RSpec.describe WikiPage::Slug do
       build(:wiki_page_slug, canonical: canonical, wiki_page_meta: meta)
     end
 
-    it { is_expected.to validate_presence_of(:slug) }
     it { is_expected.to validate_uniqueness_of(:slug).scoped_to(:wiki_page_meta_id) }
+    it { is_expected.to validate_length_of(:slug).is_at_most(2048) }
+    it { is_expected.not_to allow_value(nil).for(:slug) }
 
     describe 'only_one_slug_can_be_canonical_per_meta_record' do
       context 'there are no other slugs' do