Ensure wiki event creation is idempotent

This ensures that we don't create duplicate events due to event handlers
being run twice, once in the services and once in the post-receive
handlers.

To achieve this wiki page metadata objects have their update-at
attribute set to the most recent commit time, so that we can know if an
event for a given page state has already been applied. In the case of
deletes, where there is no most-recent-commit, we look to see if the
most recent event for a page is a deletion.

In future work, we would probably want to use commit shas here, but this
allows this feature to function without DB changes.

app/models/event.rb

@@ -96,6 +96,8 @@ class Event < ApplicationRecord
   end
 
   scope :for_milestone_id, ->(milestone_id) { where(target_type: "Milestone", target_id: milestone_id) }
+  scope :for_wiki_meta, ->(meta) { where(target_type: 'WikiPage::Meta', target_id: meta.id) }
+  scope :created_at, ->(time) { where(created_at: time) }
 
   # Authors are required as they're used to display who pushed data.
   #

app/models/wiki_page/meta.rb

@@ -23,46 +23,60 @@ class WikiPage
 
     alias_method :resource_parent, :project
 
-    # 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
-    #
-    # As with all `find_or_create` methods, this one raises errors on
-    # 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?
-
-      transaction do
-        found = find_by_canonical_slug(known_slugs, project)
-        meta = found || create(title: wiki_page.title, project_id: project.id)
-
-        meta.update_state(found.nil?, known_slugs, wiki_page)
-
-        # 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
+    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
+      #
+      # As with all `find_or_create` methods, this one raises errors on
+      # validation issues.
+      def 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?
+
+        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
-    end
 
-    def self.find_by_canonical_slug(canonical_slug, project)
-      meta, conflict = with_canonical_slug(canonical_slug)
-        .where(project_id: project.id)
-        .limit(2)
+      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)
+        if conflict.present?
+          meta.errors.add(:canonical_slug, 'Duplicate value found')
+          raise CanonicalSlugConflictError.new(meta)
+        end
+
+        meta
       end
 
-      meta
+      private
+
+      def wiki_page_updates(wiki_page)
+        last_commit_date = wiki_page.version.commit.committed_date
+        {
+          title: wiki_page.title,
+          created_at: last_commit_date,
+          updated_at: last_commit_date
+        }
+      end
     end
 
     def canonical_slug
@@ -85,24 +99,21 @@ class WikiPage
       @canonical_slug = slug
     end
 
-    def update_state(created, known_slugs, wiki_page)
-      update_wiki_page_attributes(wiki_page)
+    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
 
-    def update_columns(attrs = {})
-      super(attrs.reverse_merge(updated_at: Time.now.utc))
-    end
-
-    def self.update_all(attrs = {})
-      super(attrs.reverse_merge(updated_at: Time.now.utc))
-    end
-
     private
 
-    def update_wiki_page_attributes(page)
-      update_columns(title: page.title) unless page.title == title
+    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)

app/services/event_create_service.rb

@@ -87,16 +87,38 @@ class EventCreateService
   # @param [WikiPage::Meta] wiki_page_meta The event target
   # @param [User] author The event author
   # @param [Integer] action One of the Event::WIKI_ACTIONS
+  #
+  # @return a tuple of event and either :found or :created
   def wiki_event(wiki_page_meta, author, action)
     return unless Feature.enabled?(:wiki_events)
 
     raise IllegalActionError, action unless Event::WIKI_ACTIONS.include?(action)
 
-    create_record_event(wiki_page_meta, author, action)
+    if duplicate = existing_wiki_event(wiki_page_meta, action)
+      return duplicate
+    end
+
+    event = create_record_event(wiki_page_meta, author, action)
+    # Ensure that the event is linked in time to the metadata, for non-deletes
+    unless action == Event::DESTROYED
+      time_stamp = wiki_page_meta.updated_at
+      event.update_columns(updated_at: time_stamp, created_at: time_stamp)
+    end
+
+    event
   end
 
   private
 
+  def existing_wiki_event(wiki_page_meta, action)
+    if action == Event::DESTROYED
+      most_recent = Event.for_wiki_meta(wiki_page_meta).recent.first
+      return most_recent if most_recent.present? && most_recent.action == action
+    else
+      Event.for_wiki_meta(wiki_page_meta).created_at(wiki_page_meta.updated_at).first
+    end
+  end
+
   def create_record_event(record, current_user, status)
     create_event(record.resource_parent, current_user, status, target_id: record.id, target_type: record.class.name)
   end

app/services/wiki_pages/event_create_service.rb

@@ -12,10 +12,11 @@ module WikiPages
 
       event = Event.transaction do
         wiki_page_meta = WikiPage::Meta.find_or_create(slug, page)
+
         ::EventCreateService.new.wiki_event(wiki_page_meta, author, action)
       end
 
-      ServiceResponse.success(message: 'Event created', payload: { event: event })
+      ServiceResponse.success(payload: { event: event })
     rescue ::EventCreateService::IllegalActionError, ::ActiveRecord::ActiveRecordError => e
       ServiceResponse.error(message: e.message, payload: { error: e })
     end

spec/models/event_spec.rb

@@ -84,6 +84,21 @@ describe Event do
     end
   end
 
+  describe 'scopes' do
+    describe 'created_at' do
+      it 'can find the right event' do
+        time = 1.day.ago
+        event = create(:event, created_at: time)
+        false_positive = create(:event, created_at: 2.days.ago)
+
+        found = described_class.created_at(time)
+
+        expect(found).to include(event)
+        expect(found).not_to include(false_positive)
+      end
+    end
+  end
+
   describe "Push event" do
     let(:project) { create(:project, :private) }
     let(:user) { project.owner }
@@ -511,6 +526,14 @@ describe Event do
         expect(described_class.not_wiki_page).to match_array(non_wiki_events)
       end
     end
+
+    describe '.for_wiki_meta' do
+      it 'finds events for a given wiki page metadata object' do
+        event = events.select(&:wiki_page?).first
+
+        expect(described_class.for_wiki_meta(event.target)).to contain_exactly(event)
+      end
+    end
   end
 
   describe '#wiki_page and #wiki_page?' do

spec/models/wiki_page/meta_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe WikiPage::Meta do
-  let_it_be(:project) { create(:project) }
+  let_it_be(:project) { create(:project, :wiki_repo) }
   let_it_be(:other_project) { create(:project) }
 
   describe 'Associations' do
@@ -169,8 +169,11 @@ describe WikiPage::Meta do
       described_class.find_or_create(last_known_slug, wiki_page)
     end
 
-    def create_previous_version(title = old_title, slug = last_known_slug)
-      create(:wiki_page_meta, title: title, project: project, canonical_slug: slug)
+    def create_previous_version(title: old_title, slug: last_known_slug, date: wiki_page.version.commit.committed_date)
+      create(:wiki_page_meta,
+             title: title, project: project,
+             created_at: date, updated_at: date,
+             canonical_slug: slug)
     end
 
     def create_context
@@ -198,6 +201,8 @@ describe WikiPage::Meta do
           title: wiki_page.title,
           project: wiki_page.wiki.project
         )
+        expect(meta.updated_at).to eq(wiki_page.version.commit.committed_date)
+        expect(meta.created_at).not_to be_after(meta.updated_at)
         expect(meta.slugs.where(slug: last_known_slug)).to exist
         expect(meta.slugs.canonical.where(slug: wiki_page.slug)).to exist
       end
@@ -209,22 +214,24 @@ describe WikiPage::Meta do
       end
     end
 
-    context 'the slug is too long' do
-      let(:last_known_slug) { FFaker::Lorem.characters(2050) }
+    context 'there are problems' do
+      context 'the slug is too long' do
+        let(:last_known_slug) { FFaker::Lorem.characters(2050) }
 
-      it 'raises an error' do
-        expect { find_record }.to raise_error ActiveRecord::ValueTooLong
+        it 'raises an error' do
+          expect { find_record }.to raise_error ActiveRecord::ValueTooLong
+        end
       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
+      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)
+        it 'raises an error' do
+          expect { find_record }.to raise_error(ActiveRecord::RecordInvalid)
+        end
       end
     end
 
@@ -258,6 +265,17 @@ describe WikiPage::Meta do
       end
     end
 
+    context 'the commit happened a day ago' do
+      before do
+        allow(wiki_page.version.commit).to receive(:committed_date).and_return(1.day.ago)
+      end
+
+      include_examples 'metadata examples' do
+        # Identical to the base case.
+        let(:query_limit) { 5 }
+      end
+    end
+
     context 'the last_known_slug is the same as the current slug, as on creation' do
       let(:last_known_slug) { current_slug }
 
@@ -292,6 +310,33 @@ describe WikiPage::Meta do
       end
     end
 
+    context 'a record exists in the DB, but we need to update timestamps' do
+      let(:last_known_slug) { current_slug }
+      let(:old_title) { title }
+
+      before do
+        create_previous_version(date: 1.week.ago)
+      end
+
+      include_examples 'metadata examples' do
+        # We need the query, and the update
+        # 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 2
+        #
+        # UPDATE wiki_page_meta SET updated_at = ?date WHERE id = ?id
+        #
+        # RELEASE SAVEPOINT active_record_2
+        let(:query_limit) { 4 }
+      end
+    end
+
     context 'we need to update the slug, but not the title' do
       let(:old_title) { title }
 
@@ -359,14 +404,14 @@ describe WikiPage::Meta do
     end
 
     context 'we want to change the slug back to a previous version' do
-      let(:slug_1) { 'foo' }
-      let(:slug_2) { 'bar' }
+      let(:slug_1) { generate(:sluggified_title) }
+      let(:slug_2) { generate(:sluggified_title) }
 
       let(:wiki_page) { create(:wiki_page, title: slug_1, project: project) }
       let(:last_known_slug) { slug_2 }
 
       before do
-        meta = create_previous_version(title, slug_1)
+        meta = create_previous_version(title: title, slug: slug_1)
         meta.canonical_slug = slug_2
       end
 

spec/services/event_create_service_spec.rb

@@ -162,7 +162,7 @@ describe EventCreateService do
       context "The action is #{action}" do
         let(:event) { service.wiki_event(meta, user, action) }
 
-        it 'creates the event' do
+        it 'creates the event', :aggregate_failures do
           expect(event).to have_attributes(
             wiki_page?: true,
             valid?: true,
@@ -173,6 +173,14 @@ describe EventCreateService do
           )
         end
 
+        it 'is idempotent', :aggregate_failures do
+          expect { event }.to change(Event, :count).by(1)
+          duplicate = nil
+          expect { duplicate = service.wiki_event(meta, user, action) }.not_to change(Event, :count)
+
+          expect(duplicate).to eq(event)
+        end
+
         context 'the feature is disabled' do
           before do
             stub_feature_flags(wiki_events: false)

spec/services/wiki_pages/event_create_service_spec.rb

@@ -41,6 +41,18 @@ describe WikiPages::EventCreateService do
       expect(response).to be_success
     end
 
+    context 'the action is a deletion' do
+      let(:action) { Event::DESTROYED }
+
+      it 'does not synchronize the wiki metadata timestamps with the git commit' do
+        expect_next_instance_of(WikiPage::Meta) do |instance|
+          expect(instance).not_to receive(:synch_times_with_page)
+        end
+
+        response
+      end
+    end
+
     it 'creates a wiki page event' do
       expect { response }.to change(Event, :count).by(1)
     end