Merge branch 'split-events-into-push-events' into 'master'

Use a separate table for storing push events

See merge request !12463

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)
     &middot;
-    = 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