From 2c78c7f4cdb1fae2650563f80feb294d2b7e5d80 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Tue, 4 Jul 2017 17:08:41 +0000
Subject: [PATCH] Revert "Merge branch 'revert-12499' into 'master'"
This reverts merge request !12557
---
app/controllers/projects/issues_controller.rb | 2 +-
app/models/concerns/sortable.rb | 23 +++++++++++++++++++
app/models/project.rb | 6 ++---
...drop-default-scope-on-sortable-finders.yml | 4 ++++
spec/models/concerns/sortable_spec.rb | 21 +++++++++++++++++
5 files changed, 52 insertions(+), 4 deletions(-)
create mode 100644 changelogs/unreleased/dm-drop-default-scope-on-sortable-finders.yml
create mode 100644 spec/models/concerns/sortable_spec.rb
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index ca483c105b6..54f108353cd 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -227,7 +227,7 @@ class Projects::IssuesController < Projects::ApplicationController
def issue
return @issue if defined?(@issue)
# The Sortable default scope causes performance issues when used with find_by
- @noteable = @issue ||= @project.issues.where(iid: params[:id]).reorder(nil).take!
+ @noteable = @issue ||= @project.issues.find_by!(iid: params[:id])
return render_404 unless can?(current_user, :read_issue, @issue)
diff --git a/app/models/concerns/sortable.rb b/app/models/concerns/sortable.rb
index a155a064032..fdacfa5a194 100644
--- a/app/models/concerns/sortable.rb
+++ b/app/models/concerns/sortable.rb
@@ -5,6 +5,25 @@
module Sortable
extend ActiveSupport::Concern
+ module DropDefaultScopeOnFinders
+ # Override these methods to drop the `ORDER BY id DESC` default scope.
+ # See http://dba.stackexchange.com/a/110919 for why we do this.
+ %i[find find_by find_by!].each do |meth|
+ define_method meth do |*args, &block|
+ return super(*args, &block) if block
+
+ unordered_relation = unscope(:order)
+
+ # We cannot simply call `meth` on `unscope(:order)`, since that is also
+ # an instance of the same relation class this module is included into,
+ # which means we'd get infinite recursion.
+ # We explicitly use the original implementation to prevent this.
+ original_impl = method(__method__).super_method.unbind
+ original_impl.bind(unordered_relation).call(*args)
+ end
+ end
+ end
+
included do
# By default all models should be ordered
# by created_at field starting from newest
@@ -18,6 +37,10 @@ module Sortable
scope :order_updated_asc, -> { reorder(updated_at: :asc) }
scope :order_name_asc, -> { reorder(name: :asc) }
scope :order_name_desc, -> { reorder(name: :desc) }
+
+ # All queries (relations) on this model are instances of this `relation_klass`.
+ relation_klass = relation_delegate_class(ActiveRecord::Relation)
+ relation_klass.prepend DropDefaultScopeOnFinders
end
module ClassMethods
diff --git a/app/models/project.rb b/app/models/project.rb
index 241e7e60dd2..8e9e42d28ab 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -815,7 +815,7 @@ class Project < ActiveRecord::Base
end
def ci_service
- @ci_service ||= ci_services.reorder(nil).find_by(active: true)
+ @ci_service ||= ci_services.find_by(active: true)
end
def deployment_services
@@ -823,7 +823,7 @@ class Project < ActiveRecord::Base
end
def deployment_service
- @deployment_service ||= deployment_services.reorder(nil).find_by(active: true)
+ @deployment_service ||= deployment_services.find_by(active: true)
end
def monitoring_services
@@ -831,7 +831,7 @@ class Project < ActiveRecord::Base
end
def monitoring_service
- @monitoring_service ||= monitoring_services.reorder(nil).find_by(active: true)
+ @monitoring_service ||= monitoring_services.find_by(active: true)
end
def jira_tracker?
diff --git a/changelogs/unreleased/dm-drop-default-scope-on-sortable-finders.yml b/changelogs/unreleased/dm-drop-default-scope-on-sortable-finders.yml
new file mode 100644
index 00000000000..b359a25053a
--- /dev/null
+++ b/changelogs/unreleased/dm-drop-default-scope-on-sortable-finders.yml
@@ -0,0 +1,4 @@
+---
+title: Improve performance of lookups of issues, merge requests etc by dropping unnecessary ORDER BY clause
+merge_request:
+author:
diff --git a/spec/models/concerns/sortable_spec.rb b/spec/models/concerns/sortable_spec.rb
new file mode 100644
index 00000000000..d1e17c4f684
--- /dev/null
+++ b/spec/models/concerns/sortable_spec.rb
@@ -0,0 +1,21 @@
+require 'spec_helper'
+
+describe Sortable do
+ let(:relation) { Issue.all }
+
+ describe '#where' do
+ it 'orders by id, descending' do
+ order_node = relation.where(iid: 1).order_values.first
+ expect(order_node).to be_a(Arel::Nodes::Descending)
+ expect(order_node.expr.name).to eq(:id)
+ end
+ end
+
+ describe '#find_by' do
+ it 'does not order' do
+ expect(relation).to receive(:unscope).with(:order).and_call_original
+
+ relation.find_by(iid: 1)
+ end
+ end
+end
--
GitLab