Merge branch 'ajk-event-fingerprints' into 'master'

Add fingerprints for events

See merge request gitlab-org/gitlab!31021

app/models/event.rb

@@ -8,6 +8,7 @@ class Event < ApplicationRecord
   include CreatedAtFilterable
   include Gitlab::Utils::StrongMemoize
   include UsageStatistics
+  include ShaAttribute
 
   default_scope { reorder(nil) } # rubocop:disable Cop/DefaultScope
 
@@ -48,6 +49,8 @@ class Event < ApplicationRecord
   RESET_PROJECT_ACTIVITY_INTERVAL = 1.hour
   REPOSITORY_UPDATED_AT_INTERVAL = 5.minutes
 
+  sha_attribute :fingerprint
+
   enum action: ACTIONS, _suffix: true
 
   delegate :name, :email, :public_email, :username, to: :author, prefix: true, allow_nil: true
@@ -82,6 +85,10 @@ class Event < ApplicationRecord
   scope :recent, -> { reorder(id: :desc) }
   scope :for_wiki_page, -> { where(target_type: 'WikiPage::Meta') }
   scope :for_design, -> { where(target_type: 'DesignManagement::Design') }
+  scope :for_fingerprint, ->(fingerprint) do
+    fingerprint.present? ? where(fingerprint: fingerprint) : none
+  end
+  scope :for_action, ->(action) { where(action: action) }
 
   scope :with_associations, -> do
     # We're using preload for "push_event_payload" as otherwise the association

app/models/wiki_page.rb

@@ -98,6 +98,7 @@ class WikiPage
   def slug
     attributes[:slug].presence || wiki.wiki.preview_slug(title, format)
   end
+  alias_method :id, :slug # required to use build_stubbed
 
   alias_method :to_param, :slug
 
@@ -265,8 +266,8 @@ class WikiPage
     '../shared/wikis/wiki_page'
   end
 
-  def id
-    page.version.to_s
+  def sha
+    page.version&.sha
   end
 
   def title_changed?

app/services/event_create_service.rb

@@ -100,25 +100,21 @@ class EventCreateService
   # @param [WikiPage::Meta] wiki_page_meta The event target
   # @param [User] author The event author
   # @param [Symbol] action One of the Event::WIKI_ACTIONS
+  # @param [String] fingerprint The de-duplication fingerprint
   #
-  # @return a tuple of event and either :found or :created
-  def wiki_event(wiki_page_meta, author, action)
+  # The fingerprint, if provided, should be sufficient to find duplicate events.
+  # Suitable values would be, for example, the current page SHA.
+  #
+  # @return [Event] the event
+  def wiki_event(wiki_page_meta, author, action, fingerprint)
     raise IllegalActionError, action unless Event::WIKI_ACTIONS.include?(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 event.destroyed_action?
-      time_stamp = wiki_page_meta.updated_at
-      event.update_columns(updated_at: time_stamp, created_at: time_stamp)
-    end
-
     Gitlab::UsageDataCounters::TrackUniqueActions.track_action(event_action: action, event_target: wiki_page_meta.class, author_id: author.id)
 
-    event
+    duplicate = Event.for_wiki_meta(wiki_page_meta).for_fingerprint(fingerprint).first
+    return duplicate if duplicate.present?
+
+    create_record_event(wiki_page_meta, author, action, fingerprint.presence)
   end
 
   def approve_mr(merge_request, current_user)
@@ -127,44 +123,37 @@ class EventCreateService
 
   private
 
-  def existing_wiki_event(wiki_page_meta, action)
-    if Event.actions.fetch(action) == Event.actions[:destroyed]
-      most_recent = Event.for_wiki_meta(wiki_page_meta).recent.first
-      return most_recent if most_recent.present? && Event.actions[most_recent.action] == Event.actions[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)
+  def create_record_event(record, current_user, status, fingerprint = nil)
     create_event(record.resource_parent, current_user, status,
-                 target_id: record.id, target_type: record.class.name)
+                 fingerprint: fingerprint,
+                 target_id: record.id,
+                 target_type: record.class.name)
   end
 
   # If creating several events, this method will insert them all in a single
   # statement
   #
-  # @param [[Eventable, Symbol]] a list of pairs of records and a valid status
+  # @param [[Eventable, Symbol, String]] a list of tuples of records, a valid status, and fingerprint
   # @param [User] the author of the event
-  def create_record_events(pairs, current_user)
+  def create_record_events(tuples, current_user)
     base_attrs = {
       created_at: Time.now.utc,
       updated_at: Time.now.utc,
       author_id: current_user.id
     }
 
-    attribute_sets = pairs.map do |record, status|
+    attribute_sets = tuples.map do |record, status, fingerprint|
       action = Event.actions[status]
       raise IllegalActionError, "#{status} is not a valid status" if action.nil?
 
       parent_attrs(record.resource_parent)
         .merge(base_attrs)
-        .merge(action: action, target_id: record.id, target_type: record.class.name)
+        .merge(action: action, fingerprint: fingerprint, target_id: record.id, target_type: record.class.name)
     end
 
     result = Event.insert_all(attribute_sets, returning: %w[id])
 
-    pairs.each do |record, status|
+    tuples.each do |record, status, _|
       Gitlab::UsageDataCounters::TrackUniqueActions.track_action(event_action: status, event_target: record.class, author_id: current_user.id)
     end
 
@@ -198,8 +187,12 @@ class EventCreateService
     )
     attributes.merge!(parent_attrs(resource_parent))
 
+    if attributes[:fingerprint].present?
+      Event.safe_find_or_create_by!(attributes)
+    else
       Event.create!(attributes)
     end
+  end
 
   def parent_attrs(resource_parent)
     resource_parent_attr = case resource_parent

app/services/git/wiki_push_service.rb

@@ -41,7 +41,12 @@ module Git
     end
 
     def create_event_for(change)
-      event_service.execute(change.last_known_slug, change.page, change.event_action)
+      event_service.execute(
+        change.last_known_slug,
+        change.page,
+        change.event_action,
+        change.sha
+      )
     end
 
     def event_service

app/services/git/wiki_push_service/change.rb

@@ -33,6 +33,10 @@ module Git
         strip_extension(raw_change.old_path || raw_change.new_path)
       end
 
+      def sha
+        change[:newrev]
+      end
+
       private
 
       attr_reader :raw_change, :change, :wiki

app/services/wiki_pages/base_service.rb

@@ -44,7 +44,9 @@ module WikiPages
     end
 
     def create_wiki_event(page)
-      response = WikiPages::EventCreateService.new(current_user).execute(slug_for_page(page), page, event_action)
+      response = WikiPages::EventCreateService
+        .new(current_user)
+        .execute(slug_for_page(page), page, event_action, fingerprint(page))
 
       log_error(response.message) if response.error?
     end
@@ -52,6 +54,10 @@ module WikiPages
     def slug_for_page(page)
       page.slug
     end
+
+    def fingerprint(page)
+      page.sha
+    end
   end
 end
 

app/services/wiki_pages/destroy_service.rb

@@ -21,5 +21,9 @@ module WikiPages
     def event_action
       :destroyed
     end
+
+    def fingerprint(page)
+      page.wiki.repository.head_commit.sha
+    end
   end
 end

app/services/wiki_pages/event_create_service.rb

@@ -9,11 +9,11 @@ module WikiPages
       @author = author
     end
 
-    def execute(slug, page, action)
+    def execute(slug, page, action, event_fingerprint)
       event = Event.transaction do
         wiki_page_meta = WikiPage::Meta.find_or_create(slug, page)
 
-        ::EventCreateService.new.wiki_event(wiki_page_meta, author, action)
+        ::EventCreateService.new.wiki_event(wiki_page_meta, author, action, event_fingerprint)
       end
 
       ServiceResponse.success(payload: { event: event })

changelogs/unreleased/ajk-event-fingerprints.yml

+---
+title: Use fingerprint column on events to ensure event uniqueness
+merge_request: 31021
+author:
+type: changed

db/migrate/20200504191813_add_fingerprint_to_events.rb

+# frozen_string_literal: true
+
+class AddFingerprintToEvents < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    unless column_exists?(:events, :fingerprint)
+      with_lock_retries { add_column :events, :fingerprint, :binary }
+    end
+
+    unless check_constraint_exists?(:events, constraint_name)
+      add_check_constraint(
+        :events,
+        "octet_length(fingerprint) <= 128",
+        constraint_name,
+        validate: true
+      )
+    end
+  end
+
+  def down
+    remove_check_constraint(:events, constraint_name)
+
+    if column_exists?(:events, :fingerprint)
+      with_lock_retries { remove_column :events, :fingerprint }
+    end
+  end
+
+  def constraint_name
+    check_constraint_name(:events, :fingerprint, 'max_length')
+  end
+end

db/migrate/20200504200709_add_index_on_fingerprint_and_target_type_to_events.rb

+# frozen_string_literal: true
+
+class AddIndexOnFingerprintAndTargetTypeToEvents < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  KEYS = [:target_type, :target_id, :fingerprint]
+
+  def up
+    add_concurrent_index :events, KEYS, using: :btree, unique: true
+  end
+
+  def down
+    remove_concurrent_index :events, KEYS
+  end
+end

db/structure.sql

@@ -11405,7 +11405,9 @@ CREATE TABLE public.events (
     updated_at timestamp with time zone NOT NULL,
     action smallint NOT NULL,
     target_type character varying,
-    group_id bigint
+    group_id bigint,
+    fingerprint bytea,
+    CONSTRAINT check_97e06e05ad CHECK ((octet_length(fingerprint) <= 128))
 );
 
 CREATE SEQUENCE public.events_id_seq
@@ -19327,6 +19329,8 @@ CREATE INDEX index_events_on_project_id_and_id ON public.events USING btree (pro
 
 CREATE INDEX index_events_on_target_type_and_target_id ON public.events USING btree (target_type, target_id);
 
+CREATE UNIQUE INDEX index_events_on_target_type_and_target_id_and_fingerprint ON public.events USING btree (target_type, target_id, fingerprint);
+
 CREATE INDEX index_evidences_on_release_id ON public.evidences USING btree (release_id);
 
 CREATE INDEX index_expired_and_not_notified_personal_access_tokens ON public.personal_access_tokens USING btree (id, expires_at) WHERE ((impersonation = false) AND (revoked = false) AND (expire_notification_delivered = false));
@@ -23704,6 +23708,8 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200430123614
 20200430130048
 20200430174637
+20200504191813
+20200504200709
 20200505164958
 20200505171834
 20200505172405

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -41,6 +41,7 @@ Event:
 - updated_at
 - action
 - author_id
+- fingerprint
 WikiPage::Meta:
 - id
 - title

spec/models/event_spec.rb

@@ -111,6 +111,45 @@ RSpec.describe Event do
         expect(found).not_to include(false_positive)
       end
     end
+
+    describe '.for_fingerprint' do
+      let_it_be(:with_fingerprint) { create(:event, fingerprint: 'aaa') }
+
+      before_all do
+        create(:event)
+        create(:event, fingerprint: 'bbb')
+      end
+
+      it 'returns none if there is no fingerprint' do
+        expect(described_class.for_fingerprint(nil)).to be_empty
+        expect(described_class.for_fingerprint('')).to be_empty
+      end
+
+      it 'returns none if there is no match' do
+        expect(described_class.for_fingerprint('not-found')).to be_empty
+      end
+
+      it 'can find a given event' do
+        expect(described_class.for_fingerprint(with_fingerprint.fingerprint))
+          .to contain_exactly(with_fingerprint)
+      end
+    end
+  end
+
+  describe '#fingerprint' do
+    it 'is unique scoped to target' do
+      issue = create(:issue)
+      mr = create(:merge_request)
+
+      expect { create_list(:event, 2, target: issue, fingerprint: '1234') }
+        .to raise_error(include('fingerprint'))
+
+      expect do
+        create(:event, target: mr, fingerprint: 'abcd')
+        create(:event, target: issue, fingerprint: 'abcd')
+        create(:event, target: issue, fingerprint: 'efgh')
+      end.not_to raise_error
+    end
   end
 
   describe "Push event" do

spec/requests/api/project_milestones_spec.rb

@@ -45,10 +45,11 @@ RSpec.describe API::ProjectMilestones do
 
   describe 'PUT /projects/:id/milestones/:milestone_id to test observer on close' do
     it 'creates an activity event when a milestone is closed' do
-      expect(Event).to receive(:create!)
+      path = "/projects/#{project.id}/milestones/#{milestone.id}"
 
-      put api("/projects/#{project.id}/milestones/#{milestone.id}", user),
-          params: { state_event: 'close' }
+      expect do
+        put api(path, user), params: { state_event: 'close' }
+      end.to change(Event, :count).by(1)
     end
   end
 

spec/services/event_create_service_spec.rb

@@ -171,45 +171,53 @@ RSpec.describe EventCreateService do
     let_it_be(:wiki_page) { create(:wiki_page) }
     let_it_be(:meta) { create(:wiki_page_meta, :for_wiki_page, wiki_page: wiki_page) }
 
-    Event::WIKI_ACTIONS.each do |action|
-      context "The action is #{action}" do
-        let(:event) { service.wiki_event(meta, user, action) }
+    let(:fingerprint) { generate(:sha) }
 
-        it 'creates the event', :aggregate_failures do
-          expect(event).to have_attributes(
+    def create_event
+      service.wiki_event(meta, user, action, fingerprint)
+    end
+
+    where(:action) { Event::WIKI_ACTIONS.map { |action| [action] } }
+
+    with_them do
+      it 'creates the event' do
+        expect(create_event).to have_attributes(
           wiki_page?: true,
           valid?: true,
           persisted?: true,
           action: action.to_s,
           wiki_page: wiki_page,
-            author: user
+          author: user,
+          fingerprint: fingerprint
         )
       end
 
+      it 'is idempotent', :aggregate_failures do
+        event = nil
+        expect { event = create_event }.to change(Event, :count).by(1)
+        duplicate = nil
+        expect { duplicate = create_event }.not_to change(Event, :count)
+
+        expect(duplicate).to eq(event)
+      end
+
       it 'records the event in the event counter' do
         stub_feature_flags(Gitlab::UsageDataCounters::TrackUniqueActions::FEATURE_FLAG => true)
         counter_class = Gitlab::UsageDataCounters::TrackUniqueActions
         tracking_params = { event_action: counter_class::WIKI_ACTION, date_from: Date.yesterday, date_to: Date.today }
 
-          expect { event }
+        expect { create_event }
           .to change { counter_class.count_unique_events(tracking_params) }
-            .from(0).to(1)
-        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
+          .by(1)
       end
     end
 
     (Event.actions.keys - Event::WIKI_ACTIONS).each do |bad_action|
       context "The action is #{bad_action}" do
+        let(:action) { bad_action }
+
         it 'raises an error' do
-          expect { service.wiki_event(meta, user, bad_action) }.to raise_error(described_class::IllegalActionError)
+          expect { create_event }.to raise_error(described_class::IllegalActionError)
         end
       end
     end

spec/services/git/wiki_push_service_spec.rb

@@ -218,7 +218,7 @@ RSpec.describe Git::WikiPushService, services: true do
       message = 'something went very very wrong'
       allow_next_instance_of(WikiPages::EventCreateService, current_user) do |service|
         allow(service).to receive(:execute)
-          .with(String, WikiPage, Symbol)
+          .with(String, WikiPage, Symbol, String)
           .and_return(ServiceResponse.error(message: message))
       end
 

spec/services/wiki_pages/event_create_service_spec.rb

@@ -12,7 +12,8 @@ RSpec.describe WikiPages::EventCreateService do
     let_it_be(:page) { create(:wiki_page, project: project) }
     let(:slug) { generate(:sluggified_title) }
     let(:action) { :created }
-    let(:response) { subject.execute(slug, page, action) }
+    let(:fingerprint) { page.sha }
+    let(:response) { subject.execute(slug, page, action, fingerprint) }
 
     context 'the user is nil' do
       subject { described_class.new(nil) }

spec/support/shared_examples/requests/api/notes_shared_examples.rb

@@ -158,9 +158,11 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
     end
 
     it "creates an activity event when a note is created", :sidekiq_might_not_need_inline do
-      expect(Event).to receive(:create!)
+      uri = "/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes"
 
-      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params: { body: 'hi!' }
+      expect do
+        post api(uri, user), params: { body: 'hi!' }
+      end.to change(Event, :count).by(1)
     end
 
     context 'setting created_at' do