diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2017-08-11 17:40:03 +0300 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2017-08-11 17:40:03 +0300 |
commit | e80a893ff0ea8466099f6478183631af55933db2 (patch) | |
tree | 58b07a7faf58a768711a975715f91c1e6334d9d8 /app | |
parent | 9c11894a656603c59c03d8f35fbe8cdf7a1ad0c4 (diff) | |
parent | aac1de46c9be659b74da12f704412f38292974db (diff) |
Merge branch 'split-events-into-push-events' into 'master'
Use a separate table for storing push events
See merge request !12463
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/dashboard/projects_controller.rb | 8 | ||||
-rw-r--r-- | app/controllers/dashboard_controller.rb | 6 | ||||
-rw-r--r-- | app/controllers/groups_controller.rb | 6 | ||||
-rw-r--r-- | app/controllers/projects_controller.rb | 9 | ||||
-rw-r--r-- | app/models/event.rb | 59 | ||||
-rw-r--r-- | app/models/event_collection.rb | 98 | ||||
-rw-r--r-- | app/models/event_for_migration.rb | 5 | ||||
-rw-r--r-- | app/models/push_event.rb | 126 | ||||
-rw-r--r-- | app/models/push_event_payload.rb | 22 | ||||
-rw-r--r-- | app/services/event_create_service.rb | 9 | ||||
-rw-r--r-- | app/services/push_event_payload_service.rb | 120 | ||||
-rw-r--r-- | app/views/events/_commit.html.haml | 4 | ||||
-rw-r--r-- | app/views/events/_event_push.atom.haml | 19 | ||||
-rw-r--r-- | app/views/events/event/_push.html.haml | 13 |
14 files changed, 466 insertions, 38 deletions
diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index 74fe45e1ff6..f71ab702e71 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -52,8 +52,10 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController end def load_events - @events = Event.in_projects(load_projects(params.merge(non_public: true))) - @events = event_filter.apply_filter(@events).with_associations - @events = @events.limit(20).offset(params[:offset] || 0) + projects = load_projects(params.merge(non_public: true)) + + @events = EventCollection + .new(projects, offset: params[:offset].to_i, filter: event_filter) + .to_a end end diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index f9c31920302..19a5db6fd17 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -29,9 +29,9 @@ class DashboardController < Dashboard::ApplicationController current_user.authorized_projects end - @events = Event.in_projects(projects) - @events = @event_filter.apply_filter(@events).with_associations - @events = @events.limit(20).offset(params[:offset] || 0) + @events = EventCollection + .new(projects, offset: params[:offset].to_i, filter: @event_filter) + .to_a end def set_show_full_reference diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 27137ffde54..f76b3f69e9e 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -160,9 +160,9 @@ class GroupsController < Groups::ApplicationController end def load_events - @events = Event.in_projects(@projects) - @events = event_filter.apply_filter(@events).with_associations - @events = @events.limit(20).offset(params[:offset] || 0) + @events = EventCollection + .new(@projects, offset: params[:offset].to_i, filter: event_filter) + .to_a end def user_actions diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 8dfe0f51709..e93f34498d6 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -301,10 +301,11 @@ class ProjectsController < Projects::ApplicationController end def load_events - @events = @project.events.recent - @events = event_filter.apply_filter(@events).with_associations - limit = (params[:limit] || 20).to_i - @events = @events.limit(limit).offset(params[:offset] || 0) + projects = Project.where(id: @project.id) + + @events = EventCollection + .new(projects, offset: params[:offset].to_i, filter: event_filter) + .to_a end def project_params diff --git a/app/models/event.rb b/app/models/event.rb index 8d93a228494..f2a560a6b56 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -48,6 +48,7 @@ class Event < ActiveRecord::Base belongs_to :author, class_name: "User" belongs_to :project belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations + has_one :push_event_payload, foreign_key: :event_id # For Hash only serialize :data # rubocop:disable Cop/ActiveRecordSerialize @@ -55,19 +56,51 @@ class Event < ActiveRecord::Base # Callbacks after_create :reset_project_activity after_create :set_last_repository_updated_at, if: :push? + after_create :replicate_event_for_push_events_migration # Scopes scope :recent, -> { reorder(id: :desc) } scope :code_push, -> { where(action: PUSHED) } - scope :in_projects, ->(projects) do - where(project_id: projects.pluck(:id)).recent + scope :in_projects, -> (projects) do + sub_query = projects + .except(:order) + .select(1) + .where('projects.id = events.project_id') + + where('EXISTS (?)', sub_query).recent + end + + scope :with_associations, -> do + # We're using preload for "push_event_payload" as otherwise the association + # is not always available (depending on the query being built). + includes(:author, :project, project: :namespace) + .preload(:target, :push_event_payload) end - scope :with_associations, -> { includes(:author, :project, project: :namespace).preload(:target) } scope :for_milestone_id, ->(milestone_id) { where(target_type: "Milestone", target_id: milestone_id) } + self.inheritance_column = 'action' + class << self + def find_sti_class(action) + if action.to_i == PUSHED + PushEvent + else + Event + end + end + + def subclass_from_attributes(attrs) + # Without this Rails will keep calling this method on the returned class, + # resulting in an infinite loop. + return unless self == Event + + action = attrs.with_indifferent_access[inheritance_column].to_i + + PushEvent if action == PUSHED + end + # Update Gitlab::ContributionsCalendar#activity_dates if this changes def contributions where("action = ? OR (target_type IN (?) AND action IN (?)) OR (target_type = ? AND action = ?)", @@ -290,6 +323,16 @@ class Event < ActiveRecord::Base @commits ||= (data[:commits] || []).reverse end + def commit_title + commit = commits.last + + commit[:message] if commit + end + + def commit_id + commit_to || commit_from + end + def commits_count data[:total_commits_count] || commits.count || 0 end @@ -385,6 +428,16 @@ class Event < ActiveRecord::Base user ? author_id == user.id : false end + # We're manually replicating data into the new table since database triggers + # are not dumped to db/schema.rb. This could mean that a new installation + # would not have the triggers in place, thus losing events data in GitLab + # 10.0. + def replicate_event_for_push_events_migration + new_attributes = attributes.with_indifferent_access.except(:title, :data) + + EventForMigration.create!(new_attributes) + end + private def recent_update? diff --git a/app/models/event_collection.rb b/app/models/event_collection.rb new file mode 100644 index 00000000000..8b8244314af --- /dev/null +++ b/app/models/event_collection.rb @@ -0,0 +1,98 @@ +# A collection of events to display in an event list. +# +# An EventCollection is meant to be used for displaying events to a user (e.g. +# in a controller), it's not suitable for building queries that are used for +# building other queries. +class EventCollection + # To prevent users from putting too much pressure on the database by cycling + # through thousands of events we put a limit on the number of pages. + MAX_PAGE = 10 + + # projects - An ActiveRecord::Relation object that returns the projects for + # which to retrieve events. + # filter - An EventFilter instance to use for filtering events. + def initialize(projects, limit: 20, offset: 0, filter: nil) + @projects = projects + @limit = limit + @offset = offset + @filter = filter + end + + # Returns an Array containing the events. + def to_a + return [] if current_page > MAX_PAGE + + relation = if Gitlab::Database.join_lateral_supported? + relation_with_join_lateral + else + relation_without_join_lateral + end + + relation.with_associations.to_a + end + + private + + # Returns the events relation to use when JOIN LATERAL is not supported. + # + # This relation simply gets all the events for all authorized projects, then + # limits that set. + def relation_without_join_lateral + events = filtered_events.in_projects(projects) + + paginate_events(events) + end + + # Returns the events relation to use when JOIN LATERAL is supported. + # + # This relation is built using JOIN LATERAL, producing faster queries than a + # regular LIMIT + OFFSET approach. + def relation_with_join_lateral + projects_for_lateral = projects.select(:id).to_sql + + lateral = filtered_events + .limit(limit_for_join_lateral) + .where('events.project_id = projects_for_lateral.id') + .to_sql + + # The outer query does not need to re-apply the filters since the JOIN + # LATERAL body already takes care of this. + outer = base_relation + .from("(#{projects_for_lateral}) projects_for_lateral") + .joins("JOIN LATERAL (#{lateral}) AS #{Event.table_name} ON true") + + paginate_events(outer) + end + + def filtered_events + @filter ? @filter.apply_filter(base_relation) : base_relation + end + + def paginate_events(events) + events.limit(@limit).offset(@offset) + end + + def base_relation + # We want to have absolute control over the event queries being built, thus + # we're explicitly opting out of any default scopes that may be set. + Event.unscoped.recent + end + + def limit_for_join_lateral + # Applying the OFFSET on the inside of a JOIN LATERAL leads to incorrect + # results. To work around this we need to increase the inner limit for every + # page. + # + # This means that on page 1 we use LIMIT 20, and an outer OFFSET of 0. On + # page 2 we use LIMIT 40 and an outer OFFSET of 20. + @limit + @offset + end + + def current_page + (@offset / @limit) + 1 + end + + def projects + @projects.except(:order) + end +end diff --git a/app/models/event_for_migration.rb b/app/models/event_for_migration.rb new file mode 100644 index 00000000000..a1672da5eec --- /dev/null +++ b/app/models/event_for_migration.rb @@ -0,0 +1,5 @@ +# This model is used to replicate events between the old "events" table and the +# new "events_for_migration" table that will replace "events" in GitLab 10.0. +class EventForMigration < ActiveRecord::Base + self.table_name = 'events_for_migration' +end diff --git a/app/models/push_event.rb b/app/models/push_event.rb new file mode 100644 index 00000000000..3f1ff979de6 --- /dev/null +++ b/app/models/push_event.rb @@ -0,0 +1,126 @@ +class PushEvent < Event + # This validation exists so we can't accidentally use PushEvent with a + # different "action" value. + validate :validate_push_action + + # Authors are required as they're used to display who pushed data. + # + # We're just validating the presence of the ID here as foreign key constraints + # should ensure the ID points to a valid user. + validates :author_id, presence: true + + # The project is required to build links to commits, commit ranges, etc. + # + # We're just validating the presence of the ID here as foreign key constraints + # should ensure the ID points to a valid project. + validates :project_id, presence: true + + # The "data" field must not be set for push events since it's not used and a + # waste of space. + validates :data, absence: true + + # These fields are also not used for push events, thus storing them would be a + # waste. + validates :target_id, absence: true + validates :target_type, absence: true + + def self.sti_name + PUSHED + end + + def push? + true + end + + def push_with_commits? + !!(commit_from && commit_to) + end + + def tag? + return super unless push_event_payload + + push_event_payload.tag? + end + + def branch? + return super unless push_event_payload + + push_event_payload.branch? + end + + def valid_push? + return super unless push_event_payload + + push_event_payload.ref.present? + end + + def new_ref? + return super unless push_event_payload + + push_event_payload.created? + end + + def rm_ref? + return super unless push_event_payload + + push_event_payload.removed? + end + + def commit_from + return super unless push_event_payload + + push_event_payload.commit_from + end + + def commit_to + return super unless push_event_payload + + push_event_payload.commit_to + end + + def ref_name + return super unless push_event_payload + + push_event_payload.ref + end + + def ref_type + return super unless push_event_payload + + push_event_payload.ref_type + end + + def branch_name + return super unless push_event_payload + + ref_name + end + + def tag_name + return super unless push_event_payload + + ref_name + end + + def commit_title + return super unless push_event_payload + + push_event_payload.commit_title + end + + def commit_id + commit_to || commit_from + end + + def commits_count + return super unless push_event_payload + + push_event_payload.commit_count + end + + def validate_push_action + return if action == PUSHED + + errors.add(:action, "the action #{action.inspect} is not valid") + end +end diff --git a/app/models/push_event_payload.rb b/app/models/push_event_payload.rb new file mode 100644 index 00000000000..6cdb1cd4fe9 --- /dev/null +++ b/app/models/push_event_payload.rb @@ -0,0 +1,22 @@ +class PushEventPayload < ActiveRecord::Base + include ShaAttribute + + belongs_to :event, inverse_of: :push_event_payload + + validates :event_id, :commit_count, :action, :ref_type, presence: true + validates :commit_title, length: { maximum: 70 } + + sha_attribute :commit_from + sha_attribute :commit_to + + enum action: { + created: 0, + removed: 1, + pushed: 2 + } + + enum ref_type: { + branch: 0, + tag: 1 + } +end diff --git a/app/services/event_create_service.rb b/app/services/event_create_service.rb index 0f3a485a3fd..0b7e4f187f7 100644 --- a/app/services/event_create_service.rb +++ b/app/services/event_create_service.rb @@ -71,7 +71,14 @@ class EventCreateService end def push(project, current_user, push_data) - create_event(project, current_user, Event::PUSHED, data: push_data) + # We're using an explicit transaction here so that any errors that may occur + # when creating push payload data will result in the event creation being + # rolled back as well. + Event.transaction do + event = create_event(project, current_user, Event::PUSHED) + + PushEventPayloadService.new(event, push_data).execute + end Users::ActivityService.new(current_user, 'push').execute end diff --git a/app/services/push_event_payload_service.rb b/app/services/push_event_payload_service.rb new file mode 100644 index 00000000000..b0a389c85f9 --- /dev/null +++ b/app/services/push_event_payload_service.rb @@ -0,0 +1,120 @@ +# Service class for creating push event payloads as stored in the +# "push_event_payloads" table. +# +# Example: +# +# data = Gitlab::DataBuilder::Push.build(...) +# event = Event.create(...) +# +# PushEventPayloadService.new(event, data).execute +class PushEventPayloadService + # event - The event this push payload belongs to. + # push_data - A Hash produced by `Gitlab::DataBuilder::Push.build` to use for + # building the push payload. + def initialize(event, push_data) + @event = event + @push_data = push_data + end + + # Creates and returns a new PushEventPayload row. + # + # This method will raise upon encountering validation errors. + # + # Returns an instance of PushEventPayload. + def execute + @event.build_push_event_payload( + commit_count: commit_count, + action: action, + ref_type: ref_type, + commit_from: commit_from_id, + commit_to: commit_to_id, + ref: trimmed_ref, + commit_title: commit_title, + event_id: @event.id + ) + + @event.push_event_payload.save! + @event.push_event_payload + end + + # Returns the commit title to use. + # + # The commit title is limited to the first line and a maximum of 70 + # characters. + def commit_title + commit = @push_data.fetch(:commits).last + + return nil unless commit && commit[:message] + + raw_msg = commit[:message] + + # Find where the first line ends, without turning the entire message into an + # Array of lines (this is a waste of memory for large commit messages). + index = raw_msg.index("\n") + message = index ? raw_msg[0..index] : raw_msg + + message.strip.truncate(70) + end + + def commit_from_id + if create? + nil + else + revision_before + end + end + + def commit_to_id + if remove? + nil + else + revision_after + end + end + + def commit_count + @push_data.fetch(:total_commits_count) + end + + def ref + @push_data.fetch(:ref) + end + + def revision_before + @push_data.fetch(:before) + end + + def revision_after + @push_data.fetch(:after) + end + + def trimmed_ref + Gitlab::Git.ref_name(ref) + end + + def create? + Gitlab::Git.blank_ref?(revision_before) + end + + def remove? + Gitlab::Git.blank_ref?(revision_after) + end + + def action + if create? + :created + elsif remove? + :removed + else + :pushed + end + end + + def ref_type + if Gitlab::Git.tag_ref?(ref) + :tag + else + :branch + end + end +end diff --git a/app/views/events/_commit.html.haml b/app/views/events/_commit.html.haml index ad434a64556..98cdcca3ecc 100644 --- a/app/views/events/_commit.html.haml +++ b/app/views/events/_commit.html.haml @@ -1,5 +1,5 @@ %li.commit .commit-row-title - = link_to truncate_sha(commit[:id]), project_commit_path(project, commit[:id]), class: "commit-sha", alt: '', title: truncate_sha(commit[:id]) + = link_to truncate_sha(event.commit_id), project_commit_path(project, event.commit_id), class: "commit-sha", alt: '', title: truncate_sha(event.commit_id) · - = markdown event_commit_title(commit[:message]), project: project, pipeline: :single_line, author: event.author + = markdown event_commit_title(event.commit_title), project: project, pipeline: :single_line, author: event.author diff --git a/app/views/events/_event_push.atom.haml b/app/views/events/_event_push.atom.haml index 9fcacfbbf36..bf655f9d21a 100644 --- a/app/views/events/_event_push.atom.haml +++ b/app/views/events/_event_push.atom.haml @@ -1,14 +1,13 @@ %div{ xmlns: "http://www.w3.org/1999/xhtml" } - - event.commits.first(15).each do |commit| - %p - %strong= commit[:author][:name] - = link_to "(##{truncate_sha(commit[:id])})", project_commit_path(event.project, id: commit[:id]) - %i - at - = commit[:timestamp].to_time.to_s(:short) - %blockquote= markdown(escape_once(commit[:message]), pipeline: :atom, project: event.project, author: event.author) - - if event.commits_count > 15 + %p + %strong= event.author_name + = link_to "(#{truncate_sha(event.commit_id)})", project_commit_path(event.project, event.commit_id) + %i + at + = event.created_at.to_s(:short) + %blockquote= markdown(escape_once(event.commit_title), pipeline: :atom, project: event.project, author: event.author) + - if event.commits_count > 1 %p %i \... and - = pluralize(event.commits_count - 15, "more commit") + = pluralize(event.commits_count - 1, "more commit") diff --git a/app/views/events/event/_push.html.haml b/app/views/events/event/_push.html.haml index 54b414cc62a..973c652ad88 100644 --- a/app/views/events/event/_push.html.haml +++ b/app/views/events/event/_push.html.haml @@ -14,9 +14,7 @@ - if event.push_with_commits? .event-body %ul.well-list.event_commits - - few_commits = event.commits[0...2] - - few_commits.each do |commit| - = render "events/commit", commit: commit, project: project, event: event + = render "events/commit", project: project, event: event - create_mr = event.new_ref? && create_mr_button?(project.default_branch, event.ref_name, project) && event.authored_by?(current_user) - if event.commits_count > 1 @@ -44,9 +42,6 @@ = link_to create_mr_path(project.default_branch, event.ref_name, project) do Create Merge Request - elsif event.rm_ref? - - repository = project.repository - - last_commit = repository.commit(event.commit_from) - - if last_commit - .event-body - %ul.well-list.event_commits - = render "events/commit", commit: last_commit, project: project, event: event + .event-body + %ul.well-list.event_commits + = render "events/commit", project: project, event: event |