Merge branch 'revert-2c879643' into 'master'

Revert "Merge branch 'revert-12499' into 'master'"

See merge request !12633

app/controllers/projects/issues_controller.rb

@@ -227,7 +227,7 @@ def create_merge_request
   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)
 

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

app/models/project.rb

@@ -815,7 +815,7 @@ def ci_services
   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 @@ def deployment_services
   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 @@ def monitoring_services
   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?

changelogs/unreleased/dm-drop-default-scope-on-sortable-finders.yml

+---
+title: Improve performance of lookups of issues, merge requests etc by dropping unnecessary ORDER BY clause
+merge_request:
+author:

spec/models/concerns/sortable_spec.rb

+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