Avoid canonical slug conflict in the same project

This adds validations that ensure that wiki pages cannot have the same
canonical slug in the same project. We cannot easily move this check to
the DB, so this is handled in logic - we check in `.find_or_create` if
either of the known slugs matches a record, and we raise errors on
conflict.

This also introduces some optimisations that eliminate unnecessary
queries.

Minor updates from review comments

Includes:

- Use Class#name instead of constant
- Wrap mutable state in an outer transaction

  The one subtle thing here is that the first query (to find the record)
  must be within the transaction, even though it does not mutate the state
  of the DB. This is because we learn important facts from that query
  (namely whether there are any conflicting canonical slugs) that could be
  invalidated outside a transaction.

- Use standard uniqueness validator

  This is more declarative than our hand-rolled implementation

- Move page construction into factory

  This requires a new trait for the wiki page factory to create pages that
  are in the wiki repo.

- Changes to some test descriptions

- Reduce DB queries in specs

we don't strictly need multiple slugs here - one will do.

- Avoid running validation if there is no project_id

- Raise error if there are no slugs at all

app/models/event.rb

@@ -81,7 +81,7 @@ class Event < ApplicationRecord
   scope :recent, -> { reorder(id: :desc) }
   scope :code_push, -> { where(action: PUSHED) }
   scope :merged, -> { where(action: MERGED) }
-  scope :for_wiki_page, -> { where(target_type: 'WikiPage::Meta') }
+  scope :for_wiki_page, -> { where(target_type: WikiPage::Meta.name) }
 
   scope :with_associations, -> do
     # We're using preload for "push_event_payload" as otherwise the association
@@ -229,7 +229,7 @@ class Event < ApplicationRecord
   end
 
   def wiki_page?
-    target_type == "WikiPage::Meta"
+    target_type == WikiPage::Meta.name
   end
 
   def milestone

app/models/wiki_page/meta.rb

@@ -4,6 +4,8 @@ class WikiPage
   class Meta < ApplicationRecord
     include Gitlab::Utils::StrongMemoize
 
+    CanonicalSlugConflictError = Class.new(ActiveRecord::RecordInvalid)
+
     self.table_name = 'wiki_page_meta'
 
     belongs_to :project
@@ -13,6 +15,11 @@ class WikiPage
 
     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
 
@@ -28,33 +35,32 @@ class WikiPage
     # validation issues.
     def self.find_or_create(last_known_slug, wiki_page)
       project = wiki_page.wiki.project
+      known_slugs = [last_known_slug, wiki_page.slug].compact.uniq
+      raise 'no slugs!' if known_slugs.empty?
 
-      meta = find_by_canonical_slug(last_known_slug, project) || create(title: wiki_page.title, project_id: project.id)
-
-      meta.update_wiki_page_attributes(wiki_page)
-      meta.insert_slugs([last_known_slug, wiki_page.slug], wiki_page.slug)
-      meta.canonical_slug = wiki_page.slug
-
-      meta
-    end
+      transaction do
+        found = find_by_canonical_slug(known_slugs, project)
+        meta = found || create(title: wiki_page.title, project_id: project.id)
 
-    def update_wiki_page_attributes(page)
-      update_column(:title, page.title) unless page.title == title
-    end
+        meta.update_state(found.nil?, known_slugs, wiki_page)
 
-    def insert_slugs(strings, canonical)
-      slug_attrs = strings.uniq.map do |slug|
-        { wiki_page_meta_id: id, slug: slug }
+        # 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
-      slugs.insert_all(slug_attrs)
     end
 
     def self.find_by_canonical_slug(canonical_slug, project)
-      meta = joins(:slugs).find_by(project_id: project.id,
-                                   wiki_page_slugs: { canonical: true, slug: canonical_slug })
+      meta, conflict = with_canonical_slug(canonical_slug)
+        .where(project_id: project.id)
+        .limit(2)
 
-      # Prevent queries for canonical_slug
-      meta.instance_variable_set(:@canonical_slug, canonical_slug) if meta
+      if conflict.present?
+        meta.errors.add(:canonical_slug, 'Duplicate value found')
+        raise CanonicalSlugConflictError.new(meta)
+      end
 
       meta
     end
@@ -67,14 +73,48 @@ class WikiPage
       return if @canonical_slug == slug
 
       if persisted?
-        slugs.update_all(canonical: false)
-        page_slug = slugs.create_with(canonical: true).find_or_create_by(slug: slug)
-        page_slug.update_column(:canonical, true) unless page_slug.canonical?
+        transaction do
+          slugs.update_all(canonical: false)
+          page_slug = slugs.create_with(canonical: true).find_or_create_by(slug: slug)
+          page_slug.update_column(: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)
+      update_wiki_page_attributes(wiki_page)
+      insert_slugs(known_slugs, created, wiki_page.slug)
+      self.canonical_slug = wiki_page.slug
+    end
+
+    private
+
+    def update_wiki_page_attributes(page)
+      update_column(:title, page.title) unless page.title == title
+    end
+
+    def insert_slugs(strings, is_new, canonical_slug)
+      slug_attrs = strings.map do |slug|
+        { wiki_page_meta_id: id, slug: slug, canonical: (is_new && slug == canonical_slug) }
+      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
+    end
   end
 end

app/models/wiki_page/slug.rb

@@ -7,22 +7,12 @@ class WikiPage
     belongs_to :wiki_page_meta, class_name: 'WikiPage::Meta', inverse_of: :slugs
 
     validates :slug, presence: true, uniqueness: { scope: :wiki_page_meta_id }
-    validate :only_one_slug_can_be_canonical_per_meta_record
+    validates :canonical, uniqueness: {
+          scope: :wiki_page_meta_id,
+          if: :canonical?,
+          message: 'Only one slug can be canonical per wiki metadata record'
+    }
 
     scope :canonical, -> { where(canonical: true) }
-
-    private
-
-    def only_one_slug_can_be_canonical_per_meta_record
-      return unless canonical?
-
-      if other_slugs.canonical.exists?
-        errors.add(:canonical, 'Only one slug can be canonical per wiki metadata record')
-      end
-    end
-
-    def other_slugs
-      self.class.unscoped.where(wiki_page_meta_id: wiki_page_meta_id)
-    end
   end
 end

spec/factories/wiki_pages.rb

@@ -30,6 +30,16 @@ FactoryBot.define do
     to_create do |page|
       page.create
     end
+
+    trait :with_real_page do
+      project { create(:project, :repository) }
+
+      page do
+        wiki.create_page(title, content)
+        page_title, page_dir = wiki.page_title_and_dir(title)
+        wiki.wiki.page(title: page_title, dir: page_dir, version: nil)
+      end
+    end
   end
 
   factory :wiki_page_meta, class: 'WikiPage::Meta' do

spec/models/event_spec.rb

@@ -112,17 +112,18 @@ describe Event do
     end
 
     context 'for an issue' do
-      let(:issue) { create(:issue, project: project) }
+      let(:title) { generate(:title) }
+      let(:issue) { create(:issue, title: title, project: project) }
       let(:target) { issue }
 
       it 'delegates to issue title' do
-        expect(event.target_title).to eq(issue.title)
+        expect(event.target_title).to eq(title)
       end
     end
 
     context 'for a wiki page' do
-      let(:wiki) { create(:project_wiki, project: project) }
-      let(:wiki_page) { create(:wiki_page, wiki: wiki) }
+      let(:title) { generate(:wiki_page_title) }
+      let(:wiki_page) { create(:wiki_page, title: title, project: project) }
       let(:event) { create(:wiki_page_event, project: project, wiki_page: wiki_page) }
 
       it 'delegates to issue title' do
@@ -484,16 +485,10 @@ describe Event do
 
   describe '#wiki_page and #wiki_page?' do
     let_it_be(:project) { create(:project, :repository) }
-    let_it_be(:title) { FFaker::Lorem.sentence }
 
     context 'for a wiki page event' do
-      let(:wiki) { create(:project_wiki, project: project) }
       let(:wiki_page) do
-        wiki.create_page(title, FFaker::Lorem.sentence)
-        page_title, page_dir = wiki.page_title_and_dir(title)
-        page = wiki.wiki.page(title: page_title, dir: page_dir, version: nil)
-
-        WikiPage.new(wiki, page)
+        create(:wiki_page, :with_real_page, project: project)
       end
 
       subject(:event) { create(:wiki_page_event, project: project, wiki_page: wiki_page) }

spec/models/wiki_page/meta_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe WikiPage::Meta do
   let_it_be(:project) { create(:project) }
+  let_it_be(:other_project) { create(:project) }
 
   describe 'Associations' do
     it { is_expected.to belong_to(:project) }
@@ -25,6 +26,21 @@ describe WikiPage::Meta do
 
     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 forbidden to have two records for the same project with the same canonical_slug' do
+      the_slug = generate(:sluggified_title)
+      create(:wiki_page_meta, canonical_slug: the_slug, project: project)
+
+      in_violation = build(:wiki_page_meta, canonical_slug: the_slug, project: project)
+
+      expect(in_violation).not_to be_valid
+    end
   end
 
   describe '#canonical_slug' do
@@ -74,9 +90,11 @@ describe WikiPage::Meta do
       end
     end
 
-    describe '= slug' do
+    describe 'canonical_slug=' do
       shared_examples 'canonical_slug setting examples' do
-        let(:lower_query_limit) { [query_limit - 1, 0].max }
+        # Constant overhead of two queries for the transaction
+        let(:upper_query_limit) { query_limit + 2 }
+        let(:lower_query_limit) { [upper_query_limit - 1, 0].max}
         let(:other_slug) { generate(:sluggified_title) }
 
         it 'changes it to the correct value' do
@@ -92,7 +110,7 @@ describe WikiPage::Meta do
         end
 
         it 'issues at most N queries' do
-          expect { subject.canonical_slug = slug }.not_to exceed_query_limit(query_limit)
+          expect { subject.canonical_slug = slug }.not_to exceed_query_limit(upper_query_limit)
         end
 
         it 'issues fewer queries if we already know the current slug' do
@@ -127,7 +145,6 @@ describe WikiPage::Meta do
 
       context 'the slug is up to date and in the DB' do
         let(:slug) { generate(:sluggified_title) }
-        let(:query_limit) { 0 }
 
         before do
           subject.canonical_slug = slug
@@ -135,14 +152,15 @@ describe WikiPage::Meta do
 
         include_examples 'canonical_slug setting examples' do
           let(:other_slug) { slug }
+          let(:upper_query_limit) { 0 }
         end
       end
     end
   end
 
   describe '.find_or_create' do
-    let(:old_title)       { FactoryBot.generate(:wiki_page_title) }
-    let(:last_known_slug) { FactoryBot.generate(:sluggified_title) }
+    let(:old_title)       { generate(:wiki_page_title) }
+    let(:last_known_slug) { generate(:sluggified_title) }
     let(:current_slug) { wiki_page.slug }
     let(:title)        { wiki_page.title }
     let(:wiki_page) { create(:wiki_page, project: project) }
@@ -152,14 +170,30 @@ describe WikiPage::Meta do
     end
 
     def create_previous_version(title = old_title, slug = last_known_slug)
-      described_class.create!(title: title, project: project, canonical_slug: slug)
+      create(:wiki_page_meta, title: title, project: project, canonical_slug: slug)
+    end
+
+    def create_context
+      # Ensure that we behave nicely with respect to other projects
+      # We have:
+      #  - page in other project with same canonical_slug
+      create(:wiki_page_meta, project: other_project, canonical_slug: wiki_page.slug)
+
+      #  - page in same project with different canonical_slug, but with
+      #    an old slug that = canonical_slug
+      different_slug = generate(:sluggified_title)
+      create(:wiki_page_meta, project: project, canonical_slug: different_slug)
+        .slugs.create(slug: wiki_page.slug)
     end
 
     shared_examples 'metadata examples' do
       it 'establishes the correct state', :aggregate_failures do
+        create_context
+
         meta = find_record
 
         expect(meta).to have_attributes(
+          valid?: true,
           canonical_slug: wiki_page.slug,
           title: wiki_page.title,
           project: wiki_page.wiki.project
@@ -183,34 +217,44 @@ describe WikiPage::Meta do
       end
     end
 
+    context 'a conflicting record exists' do
+      before do
+        create(:wiki_page_meta, project: project, canonical_slug: last_known_slug)
+        create(:wiki_page_meta, project: project, canonical_slug: current_slug)
+      end
+
+      it 'raises an error' do
+        expect { find_record }.to raise_error(ActiveRecord::RecordInvalid)
+      end
+    end
+
     context 'no existing record exists' do
       include_examples 'metadata examples' do
-        # The base case is 7 queries:
+        # The base case is 5 queries:
+        #  - 2 for the outer transaction
         #  - 1 to find the metadata object if it exists
         #  - 1 to create it if it does not
-        #  - 2 for 1 savepoint
         #  - 1 to insert last_known_slug and current_slug
-        #  - 1 to find the current slug
-        #  - 2 to set canonical status correctly
         #
         # (Log has been edited for clarity)
+        # SAVEPOINT active_record_2
+        #
         # SELECT * FROM wiki_page_meta
         #   INNER JOIN wiki_page_slugs
         #     ON wiki_page_slugs.wiki_page_meta_id = wiki_page_meta.id
         #   WHERE wiki_page_meta.project_id = ?
         #     AND wiki_page_slugs.canonical = TRUE
-        #     AND wiki_page_slugs.slug = ?
-        #   LIMIT 1
-        # SAVEPOINT active_record_2
+        #     AND wiki_page_slugs.slug IN (?,?)
+        #   LIMIT 2
+        #
         # INSERT INTO wiki_page_meta (project_id, title) VALUES (?, ?) RETURNING id
-        # RELEASE SAVEPOINT active_record_2
-        # INSERT INTO wiki_page_slugs (wiki_page_meta_id,slug)
-        #   VALUES (?, ?) (?, ?)
+        #
+        # INSERT INTO wiki_page_slugs (wiki_page_meta_id,slug,canonical)
+        #   VALUES (?, ?, ?) (?, ?, ?)
         #   ON CONFLICT  DO NOTHING RETURNING id
-        # UPDATE wiki_page_slugs SET canonical = FALSE WHERE wiki_page_meta_id = ?
-        # SELECT * FROM wiki_page_slugs WHERE wiki_page_meta_id = ? AND slug = ? LIMIT 1
-        # UPDATE wiki_page_slugs SET canonical = TRUE WHERE id = ?
-        let(:query_limit) { 8 }
+        #
+        # RELEASE SAVEPOINT active_record_2
+        let(:query_limit) { 5 }
       end
     end
 
@@ -219,7 +263,7 @@ describe WikiPage::Meta do
 
       include_examples 'metadata examples' do
         # Identical to the base case.
-        let(:query_limit) { 8 }
+        let(:query_limit) { 5 }
       end
     end
 
@@ -232,17 +276,19 @@ describe WikiPage::Meta do
       end
 
       include_examples 'metadata examples' do
+        # We just need to do the initial query, and the outer transaction
+        # SAVEPOINT active_record_2
+        #
         # SELECT * FROM wiki_page_meta
         #   INNER JOIN wiki_page_slugs
         #     ON wiki_page_slugs.wiki_page_meta_id = wiki_page_meta.id
         #   WHERE wiki_page_meta.project_id = ?
         #     AND wiki_page_slugs.canonical = TRUE
         #     AND wiki_page_slugs.slug = ?
-        #   LIMIT 1
+        #   LIMIT 2
         #
-        # INSERT INTO wiki_page_slugs (wiki_page_meta_id,slug,canonical)
-        #   VALUES (?, ?, ?) ON CONFLICT  DO NOTHING RETURNING id
-        let(:query_limit) { 2 }
+        # RELEASE SAVEPOINT active_record_2
+        let(:query_limit) { 3 }
       end
     end
 
@@ -254,8 +300,13 @@ describe WikiPage::Meta do
       end
 
       include_examples 'metadata examples' do
-        # Same as minimal case, plus the additional queries needed to update the
-        # slug.
+        # Here we need:
+        #  - 2 for the outer transaction
+        #  - 1 to find the record
+        #  - 1 to insert the new slug
+        #  - 3 to set canonical state correctly
+        #
+        # SAVEPOINT active_record_2
         #
         # SELECT * FROM wiki_page_meta
         #   INNER JOIN wiki_page_slugs
@@ -272,10 +323,11 @@ describe WikiPage::Meta do
         #   WHERE wiki_page_slugs.wiki_page_meta_id = ?
         #     AND wiki_page_slugs.slug = ?
         #     LIMIT 1
-        #
         # UPDATE wiki_page_slugs SET canonical = FALSE WHERE wiki_page_meta_id = ?
         # UPDATE wiki_page_slugs SET canonical = TRUE WHERE id = ?
-        let(:query_limit) { 5 }
+        #
+        # RELEASE SAVEPOINT active_record_2
+        let(:query_limit) { 7 }
       end
     end
 
@@ -289,6 +341,8 @@ describe WikiPage::Meta do
       include_examples 'metadata examples' do
         # Same as minimal case, plus one query to update the title.
         #
+        # SAVEPOINT active_record_2
+        #
         # SELECT * FROM wiki_page_meta
         #   INNER JOIN wiki_page_slugs
         #     ON wiki_page_slugs.wiki_page_meta_id = wiki_page_meta.id
@@ -299,9 +353,8 @@ describe WikiPage::Meta do
         #
         # UPDATE wiki_page_meta SET title = ? WHERE id = ?
         #
-        # INSERT INTO wiki_page_slugs (wiki_page_meta_id,slug,canonical)
-        #   VALUES (?, ?, ?) ON CONFLICT  DO NOTHING RETURNING id
-        let(:query_limit) { 3 }
+        # RELEASE SAVEPOINT active_record_2
+        let(:query_limit) { 4 }
       end
     end
 
@@ -318,12 +371,12 @@ describe WikiPage::Meta do
       end
 
       include_examples 'metadata examples' do
-        let(:query_limit) { 5 }
+        let(:query_limit) { 7 }
       end
     end
 
     context 'we want to change the slug a bunch of times' do
-      let(:slugs) { FactoryBot.generate_list(:sluggified_title, 3) }
+      let(:slugs) { generate_list(:sluggified_title, 3) }
 
       before do
         meta = create_previous_version
@@ -331,7 +384,7 @@ describe WikiPage::Meta do
       end
 
       include_examples 'metadata examples' do
-        let(:query_limit) { 8 }
+        let(:query_limit) { 7 }
       end
     end
 
@@ -341,18 +394,22 @@ describe WikiPage::Meta do
       end
 
       include_examples 'metadata examples' do
-        # Same as minimal case, plus one for the title, and two for the slug
+        # -- outer transaction
+        # SAVEPOINT active_record_2
         #
+        # -- to find the record
         # SELECT * FROM wiki_page_meta
         #   INNER JOIN wiki_page_slugs
         #     ON wiki_page_slugs.wiki_page_meta_id = wiki_page_meta.id
         #   WHERE wiki_page_meta.project_id = ?
         #     AND wiki_page_slugs.canonical = TRUE
-        #     AND wiki_page_slugs.slug = ?
-        #   LIMIT 1
+        #     AND wiki_page_slugs.slug IN (?,?)
+        #   LIMIT 2
         #
+        # -- to update the title
         # UPDATE wiki_page_meta SET title = ? WHERE id = ?
         #
+        # -- to update slug
         # INSERT INTO wiki_page_slugs (wiki_page_meta_id,slug,canonical)
         #   VALUES (?, ?, ?) ON CONFLICT  DO NOTHING RETURNING id
         #
@@ -364,7 +421,9 @@ describe WikiPage::Meta do
         #     LIMIT 1
         #
         # UPDATE wiki_page_slugs SET canonical = TRUE WHERE id = ?
-        let(:query_limit) { 6 }
+        #
+        # RELEASE SAVEPOINT active_record_2
+        let(:query_limit) { 8 }
       end
     end
   end

spec/models/wiki_page/slug_spec.rb

@@ -25,7 +25,7 @@ describe WikiPage::Slug do
 
       context 'there are some non-canonical slugs' do
         before do
-          create_list(:wiki_page_slug, 3)
+          create(:wiki_page_slug)
         end
 
         it { is_expected.to be_empty }
@@ -33,7 +33,7 @@ describe WikiPage::Slug do
 
       context 'there is at least one canonical slugs' do
         before do
-          create(:wiki_page_slug, canonical: true)
+          create(:wiki_page_slug, :canonical)
         end
 
         it { is_expected.not_to be_empty }
@@ -64,7 +64,7 @@ describe WikiPage::Slug do
 
       context 'there are other slugs, but they are not canonical' do
         before do
-          create_list(:wiki_page_slug, 3, wiki_page_meta: meta)
+          create(:wiki_page_slug, wiki_page_meta: meta)
         end
 
         it { is_expected.to be_valid }