Commit 843f4b94 authored by Sean McGivern's avatar Sean McGivern

Ignore ordering when calling find_by on finders

We shouldn't care about the ordering here; if we did, it would be more
appropriate to use `take` or `first`. Having the ordering can result in
the database picking a bad query plan, as it might think sorting the
whole table first is the best option.
parent d83e5740
...@@ -3,13 +3,13 @@ ...@@ -3,13 +3,13 @@
module FinderMethods module FinderMethods
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def find_by!(*args) def find_by!(*args)
raise_not_found_unless_authorized execute.find_by!(*args) raise_not_found_unless_authorized execute.reorder(nil).find_by!(*args)
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def find_by(*args) def find_by(*args)
if_authorized execute.find_by(*args) if_authorized execute.reorder(nil).find_by(*args)
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
......
...@@ -12,7 +12,7 @@ describe FinderMethods do ...@@ -12,7 +12,7 @@ describe FinderMethods do
end end
def execute def execute
Project.all Project.all.order(id: :desc)
end end
end end
end end
...@@ -38,6 +38,16 @@ describe FinderMethods do ...@@ -38,6 +38,16 @@ describe FinderMethods do
it 'raises not found the user does not have access' do it 'raises not found the user does not have access' do
expect { finder.find_by!(id: unauthorized_project.id) }.to raise_error(ActiveRecord::RecordNotFound) expect { finder.find_by!(id: unauthorized_project.id) }.to raise_error(ActiveRecord::RecordNotFound)
end end
it 'ignores ordering' do
# Memoise the finder result so we can add message expectations to it
relation = finder.execute
allow(finder).to receive(:execute).and_return(relation)
expect(relation).to receive(:reorder).with(nil).and_call_original
finder.find_by!(id: authorized_project.id)
end
end end
describe '#find' do describe '#find' do
...@@ -66,5 +76,15 @@ describe FinderMethods do ...@@ -66,5 +76,15 @@ describe FinderMethods do
it 'returns nil when the user does not have access' do it 'returns nil when the user does not have access' do
expect(finder.find_by(id: unauthorized_project.id)).to be_nil expect(finder.find_by(id: unauthorized_project.id)).to be_nil
end end
it 'ignores ordering' do
# Memoise the finder result so we can add message expectations to it
relation = finder.execute
allow(finder).to receive(:execute).and_return(relation)
expect(relation).to receive(:reorder).with(nil).and_call_original
finder.find_by(id: authorized_project.id)
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment