Commit 13a7ea6f authored by Tiger Watson's avatar Tiger Watson

Merge branch 'pl-finder-methods-find' into 'master'

Respect finder's filter in FinderMethods#find

See merge request gitlab-org/gitlab!83657
parents 62f0e99c c4282032
...@@ -13,16 +13,23 @@ module FinderMethods ...@@ -13,16 +13,23 @@ module FinderMethods
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def find(*args) def find(*args)
raise_not_found_unless_authorized model.find(*args) raise_not_found_unless_authorized execute.reorder(nil).find(*args)
end end
# rubocop: enable CodeReuse/ActiveRecord
private private
def raise_not_found_unless_authorized(result) def raise_not_found_unless_authorized(result)
result = if_authorized(result) result = if_authorized(result)
raise(ActiveRecord::RecordNotFound, "Couldn't find #{model}") unless result unless result
# This fetches the model from the `ActiveRecord::Relation` but does not
# actually execute the query.
model = execute.model
raise ActiveRecord::RecordNotFound, "Couldn't find #{model}"
end
result result
end end
...@@ -32,11 +39,7 @@ module FinderMethods ...@@ -32,11 +39,7 @@ module FinderMethods
# this is currently the case in the `MilestoneFinder` # this is currently the case in the `MilestoneFinder`
return result unless respond_to?(:current_user, true) return result unless respond_to?(:current_user, true)
if can_read_object?(result) result if can_read_object?(result)
result
else
nil
end
end end
def can_read_object?(object) def can_read_object?(object)
...@@ -53,10 +56,4 @@ module FinderMethods ...@@ -53,10 +56,4 @@ module FinderMethods
# Not all objects define `#to_ability_name`, so attempt to derive it: # Not all objects define `#to_ability_name`, so attempt to derive it:
object.model_name.singular object.model_name.singular
end end
# This fetches the model from the `ActiveRecord::Relation` but does not
# actually execute the query.
def model
execute.model
end
end end
...@@ -183,7 +183,7 @@ RSpec.describe Mutations::Issues::Create do ...@@ -183,7 +183,7 @@ RSpec.describe Mutations::Issues::Create do
context 'epics are unavailable' do context 'epics are unavailable' do
it 'is unsuccessful' do it 'is unsuccessful' do
expect(resolved_mutation[:errors]).to contain_exactly("Couldn't find Epic") expect(resolved_mutation[:errors]).to contain_exactly(/^Couldn't find Epic/)
end end
it 'does not create an issue' do it 'does not create an issue' do
......
...@@ -8,7 +8,7 @@ RSpec.describe Projects::TodosController do ...@@ -8,7 +8,7 @@ RSpec.describe Projects::TodosController do
let(:issue) { create(:issue, project: project) } let(:issue) { create(:issue, project: project) }
let(:merge_request) { create(:merge_request, source_project: project) } let(:merge_request) { create(:merge_request, source_project: project) }
let(:design) { create(:design, project: project, issue: issue) } let(:design) { create(:design, :with_versions, project: project, issue: issue) }
let(:parent) { project } let(:parent) { project }
shared_examples 'issuable todo actions' do shared_examples 'issuable todo actions' do
......
...@@ -12,7 +12,7 @@ RSpec.describe FinderMethods do ...@@ -12,7 +12,7 @@ RSpec.describe FinderMethods do
end end
def execute def execute
Project.all.order(id: :desc) Project.where.not(name: 'foo').order(id: :desc)
end end
private private
...@@ -21,13 +21,16 @@ RSpec.describe FinderMethods do ...@@ -21,13 +21,16 @@ RSpec.describe FinderMethods do
end end
end end
let(:user) { create(:user) } let_it_be(:user) { create(:user) }
let(:finder) { finder_class.new(user) } let_it_be(:authorized_project) { create(:project) }
let(:authorized_project) { create(:project) } let_it_be(:unmatched_project) { create(:project, name: 'foo') }
let(:unauthorized_project) { create(:project) } let_it_be(:unauthorized_project) { create(:project) }
before do subject(:finder) { finder_class.new(user) }
before_all do
authorized_project.add_developer(user) authorized_project.add_developer(user)
unmatched_project.add_developer(user)
end end
# rubocop:disable Rails/FindById # rubocop:disable Rails/FindById
...@@ -36,8 +39,12 @@ RSpec.describe FinderMethods do ...@@ -36,8 +39,12 @@ RSpec.describe FinderMethods do
expect(finder.find_by!(id: authorized_project.id)).to eq(authorized_project) expect(finder.find_by!(id: authorized_project.id)).to eq(authorized_project)
end end
it 'raises not found when the project is not found' do it 'raises not found when the project is not found by id' do
expect { finder.find_by!(id: 0) }.to raise_error(ActiveRecord::RecordNotFound) expect { finder.find_by!(id: non_existing_record_id) }.to raise_error(ActiveRecord::RecordNotFound)
end
it 'raises not found when the project is not found by filter' do
expect { finder.find_by!(id: unmatched_project.id) }.to raise_error(ActiveRecord::RecordNotFound)
end end
it 'raises not found the user does not have access' do it 'raises not found the user does not have access' do
...@@ -61,13 +68,27 @@ RSpec.describe FinderMethods do ...@@ -61,13 +68,27 @@ RSpec.describe FinderMethods do
expect(finder.find(authorized_project.id)).to eq(authorized_project) expect(finder.find(authorized_project.id)).to eq(authorized_project)
end end
it 'raises not found when the project is not found' do it 'raises not found when the project is not found by id' do
expect { finder.find(0) }.to raise_error(ActiveRecord::RecordNotFound) expect { finder.find(non_existing_record_id) }.to raise_error(ActiveRecord::RecordNotFound)
end
it 'raises not found when the project is not found by filter' do
expect { finder.find(unmatched_project.id) }.to raise_error(ActiveRecord::RecordNotFound)
end end
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(unauthorized_project.id) }.to raise_error(ActiveRecord::RecordNotFound) expect { finder.find(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(authorized_project.id)
end
end end
describe '#find_by' do describe '#find_by' do
...@@ -75,8 +96,12 @@ RSpec.describe FinderMethods do ...@@ -75,8 +96,12 @@ RSpec.describe FinderMethods do
expect(finder.find_by(id: authorized_project.id)).to eq(authorized_project) expect(finder.find_by(id: authorized_project.id)).to eq(authorized_project)
end end
it 'returns nil when the project is not found' do it 'returns nil when the project is not found by id' do
expect(finder.find_by(id: 0)).to be_nil expect(finder.find_by(id: non_existing_record_id)).to be_nil
end
it 'returns nil when the project is not found by filter' do
expect(finder.find_by(id: unmatched_project.id)).to be_nil
end end
it 'returns nil when the user does not have access' do it 'returns nil when the user does not have access' do
......
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