diff options
| -rw-r--r-- | app/controllers/projects/issues_controller.rb | 2 | ||||
| -rw-r--r-- | app/models/concerns/sortable.rb | 23 | ||||
| -rw-r--r-- | app/models/project.rb | 6 | ||||
| -rw-r--r-- | changelogs/unreleased/dm-drop-default-scope-on-sortable-finders.yml | 4 | ||||
| -rw-r--r-- | spec/models/concerns/sortable_spec.rb | 21 |
5 files changed, 52 insertions, 4 deletions
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index dfc6baa34a4..5b2de93c168 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 1176bec8873..fcc47e57b91 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -823,7 +823,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 @@ -831,7 +831,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 @@ -839,7 +839,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..931e7a1ffc0 --- /dev/null +++ b/changelogs/unreleased/dm-drop-default-scope-on-sortable-finders.yml @@ -0,0 +1,4 @@ +--- +title: Drop default ORDER scope when calling a find method on a Sortable model +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 |