Commit b5831814 authored by Alex Kalderimis's avatar Alex Kalderimis Committed by Adam Hegyi

Add fingerprint to events

Adds a new columns on events (fingerprint) and an index
on fingerprint and target_type
parent b600de60
......@@ -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
......
......@@ -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?
......
......@@ -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,7 +187,11 @@ class EventCreateService
)
attributes.merge!(parent_attrs(resource_parent))
Event.create!(attributes)
if attributes[:fingerprint].present?
Event.safe_find_or_create_by!(attributes)
else
Event.create!(attributes)
end
end
def parent_attrs(resource_parent)
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -21,5 +21,9 @@ module WikiPages
def event_action
:destroyed
end
def fingerprint(page)
page.wiki.repository.head_commit.sha
end
end
end
......@@ -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 })
......
---
title: Use fingerprint column on events to ensure event uniqueness
merge_request: 31021
author:
type: changed
# 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
# 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
......@@ -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
......
......@@ -41,6 +41,7 @@ Event:
- updated_at
- action
- author_id
- fingerprint
WikiPage::Meta:
- id
- title
......
......@@ -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
......
......@@ -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
......
......@@ -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) }
it 'creates the event', :aggregate_failures do
expect(event).to have_attributes(
wiki_page?: true,
valid?: true,
persisted?: true,
action: action.to_s,
wiki_page: wiki_page,
author: user
)
end
let(:fingerprint) { generate(:sha) }
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 }
def create_event
service.wiki_event(meta, user, action, fingerprint)
end
expect { event }
.to change { counter_class.count_unique_events(tracking_params) }
.from(0).to(1)
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,
fingerprint: fingerprint
)
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)
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
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 { create_event }
.to change { counter_class.count_unique_events(tracking_params) }
.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
......
......@@ -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
......
......@@ -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) }
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment