From 932a9a0c77a02e1d948f45cffe5e936e915ae0bc Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Sat, 15 Jun 2019 07:10:58 +0200 Subject: Use NotesFinder to fetch notes on API and Controllers Fix missing iid query on NotesFinder Changed parameters of find_noteable, so changes across a few files were needed. MergeRequest also requires iid instead of id query Make NotesFinder fail with RecordNotFound again Add specs for target_iid Using RSpec tablesyntax for target_iid specs Revert "Using RSpec tablesyntax for target_iid specs" This reverts commit ba45c7f569a. Allow find_by! here Fix variable name Add readable check Revert "Add readable check" This reverts commit 9e3a1a7aa39. Remove unnecessary assignment Add required changes for EE Fix parameter count Reduce code duplication by extracting a noteable module method The call to find_noteable was redundant so multiple files and lines have changed in that commit to use the newly introduced module method `noteable`. Replace casecmp with include check Add parent_type parameter Revert "Reduce code duplication by extracting a noteable module method" This reverts commit 8c0923babff16. Method is no longer needed Check whether noteable can be read by user --- app/finders/notes_finder.rb | 15 +++++++++++++-- lib/api/discussions.rb | 18 +++++++++--------- lib/api/helpers.rb | 5 ----- lib/api/helpers/notes_helpers.rb | 21 ++++++++++++++++----- lib/api/notes.rb | 12 +++++------- lib/api/resource_label_events.rb | 4 ++-- spec/finders/notes_finder_spec.rb | 26 ++++++++++++++++++++++++++ 7 files changed, 71 insertions(+), 30 deletions(-) diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 817aac8b5d5..83bf015488f 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -34,8 +34,9 @@ class NotesFinder target_type = @params[:target_type] target_id = @params[:target_id] + target_iid = @params[:target_iid] - return @target = nil unless target_type && target_id + return @target = nil unless target_type && (target_id || target_iid) @target = if target_type == "commit" @@ -43,12 +44,22 @@ class NotesFinder @project.commit(target_id) end else - noteables_for_type(target_type).find(target_id) + noteables_for_type_by_id(target_type, target_id, target_iid) end end private + def noteables_for_type_by_id(type, id, iid) + query = if id + { id: id } + else + { iid: iid } + end + + noteables_for_type(type).find_by!(query) # rubocop: disable CodeReuse/ActiveRecord + end + def init_collection if target notes_on_target diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb index 693172b7d08..cc62ce22a1b 100644 --- a/lib/api/discussions.rb +++ b/lib/api/discussions.rb @@ -25,7 +25,7 @@ module API end # rubocop: disable CodeReuse/ActiveRecord get ":id/#{noteables_path}/:noteable_id/discussions" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = noteable.notes .inc_relations_for_view @@ -47,7 +47,7 @@ module API requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable' end get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = readable_discussion_notes(noteable, params[:discussion_id]) if notes.empty? @@ -82,7 +82,7 @@ module API end end post ":id/#{noteables_path}/:noteable_id/discussions" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) type = params[:position] ? 'DiffNote' : 'DiscussionNote' id_key = noteable.is_a?(Commit) ? :commit_id : :noteable_id @@ -112,7 +112,7 @@ module API requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable' end get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = readable_discussion_notes(noteable, params[:discussion_id]) if notes.empty? @@ -132,7 +132,7 @@ module API optional :created_at, type: String, desc: 'The creation date of the note' end post ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = readable_discussion_notes(noteable, params[:discussion_id]) first_note = notes.first @@ -166,7 +166,7 @@ module API requires :note_id, type: Integer, desc: 'The ID of a note' end get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) get_note(noteable, params[:note_id]) end @@ -183,7 +183,7 @@ module API exactly_one_of :body, :resolved end put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) if params[:resolved].nil? update_note(noteable, params[:note_id]) @@ -201,7 +201,7 @@ module API requires :note_id, type: Integer, desc: 'The ID of a note' end delete ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) delete_note(noteable, params[:note_id]) end @@ -216,7 +216,7 @@ module API requires :resolved, type: Boolean, desc: 'Mark discussion resolved/unresolved' end put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) resolve_discussion(noteable, params[:discussion_id], params[:resolved]) end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 00bcf6b055b..70b5064c72a 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -183,11 +183,6 @@ module API user_project.commit_by(oid: id) end - def find_project_snippet(id) - finder_params = { project: user_project } - SnippetsFinder.new(current_user, finder_params).find(id) - end - # rubocop: disable CodeReuse/ActiveRecord def find_merge_request_with_access(iid, access_level = :read_merge_request) merge_request = user_project.merge_requests.find_by!(iid: iid) diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index a068de4361c..c444766249d 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -73,16 +73,27 @@ module API "read_#{noteable.class.to_s.underscore}".to_sym end - def find_noteable(parent, noteables_str, noteable_id) - noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend + def find_noteable(parent_type, parent_id, noteable_type, noteable_id) + params = params_by_noteable_type_and_id(noteable_type, noteable_id) - readable = can?(current_user, noteable_read_ability_name(noteable), noteable) - - return not_found!(noteables_str) unless readable + noteable = NotesFinder.new(user_project, current_user, params).target + noteable = nil unless can?(current_user, noteable_read_ability_name(noteable), noteable) + return not_found!(noteable_type) unless noteable noteable end + def params_by_noteable_type_and_id(type, id) + target_type = type.name.underscore + { target_type: target_type }.tap do |h| + if %w(issue merge_request).include?(target_type) + h[:target_iid] = id + else + h[:target_id] = id + end + end + end + def noteable_parent(noteable) public_send("user_#{noteable.class.parent_class.to_s.underscore}") # rubocop:disable GitlabSecurity/PublicSend end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 416cf39d3ec..9381f045144 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -15,8 +15,6 @@ module API requires :id, type: String, desc: "The ID of a #{parent_type}" end resource parent_type.pluralize.to_sym, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do - noteables_str = noteable_type.to_s.underscore.pluralize - desc "Get a list of #{noteable_type.to_s.downcase} notes" do success Entities::Note end @@ -30,7 +28,7 @@ module API end # rubocop: disable CodeReuse/ActiveRecord get ":id/#{noteables_str}/:noteable_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) # We exclude notes that are cross-references and that cannot be viewed # by the current user. By doing this exclusion at this level and not @@ -56,7 +54,7 @@ module API requires :noteable_id, type: Integer, desc: 'The ID of the noteable' end get ":id/#{noteables_str}/:noteable_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) get_note(noteable, params[:note_id]) end @@ -69,7 +67,7 @@ module API optional :created_at, type: String, desc: 'The creation date of the note' end post ":id/#{noteables_str}/:noteable_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) opts = { note: params[:body], @@ -96,7 +94,7 @@ module API requires :body, type: String, desc: 'The content of a note' end put ":id/#{noteables_str}/:noteable_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) update_note(noteable, params[:note_id]) end @@ -109,7 +107,7 @@ module API requires :note_id, type: Integer, desc: 'The ID of a note' end delete ":id/#{noteables_str}/:noteable_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) delete_note(noteable, params[:note_id]) end diff --git a/lib/api/resource_label_events.rb b/lib/api/resource_label_events.rb index 448bef12cec..505a6c68c9c 100644 --- a/lib/api/resource_label_events.rb +++ b/lib/api/resource_label_events.rb @@ -26,7 +26,7 @@ module API # rubocop: disable CodeReuse/ActiveRecord get ":id/#{eventables_str}/:eventable_id/resource_label_events" do - eventable = find_noteable(parent_type, eventables_str, params[:eventable_id]) + eventable = find_noteable(parent_type, params[:id], eventable_type, params[:eventable_id]) events = eventable.resource_label_events.includes(:label, :user) present paginate(events), with: Entities::ResourceLabelEvent @@ -42,7 +42,7 @@ module API requires :eventable_id, types: [Integer, String], desc: 'The ID of the eventable' end get ":id/#{eventables_str}/:eventable_id/resource_label_events/:event_id" do - eventable = find_noteable(parent_type, eventables_str, params[:eventable_id]) + eventable = find_noteable(parent_type, params[:id], eventable_type, params[:eventable_id]) event = eventable.resource_label_events.find(params[:event_id]) present event, with: Entities::ResourceLabelEvent diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 0a685152cf9..87bde4ca2f6 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -292,5 +292,31 @@ describe NotesFinder do expect(subject.target).to eq(commit) end end + + context 'target_iid' do + let(:issue) { create(:issue, project: project) } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + it 'finds issues by iid' do + iid_params = { target_type: 'issue', target_iid: issue.iid } + expect(described_class.new(project, user, iid_params).target).to eq(issue) + end + + it 'finds merge requests by iid' do + iid_params = { target_type: 'merge_request', target_iid: merge_request.iid } + expect(described_class.new(project, user, iid_params).target).to eq(merge_request) + end + + it 'returns nil if both target_id and target_iid are not given' do + params_without_any_id = { target_type: 'issue' } + expect(described_class.new(project, user, params_without_any_id).target).to be_nil + end + + it 'prioritizes target_id over target_iid' do + issue2 = create(:issue, project: project) + iid_params = { target_type: 'issue', target_id: issue2.id, target_iid: issue.iid } + expect(described_class.new(project, user, iid_params).target).to eq(issue2) + end + end end end -- cgit v1.2.1 From b5b5658868a8aaac0dc340f6d7c9a14842cab2e5 Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Sat, 15 Jun 2019 07:19:54 +0200 Subject: Separate conditions to increase readability --- app/finders/notes_finder.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 83bf015488f..06d01245e90 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -36,7 +36,8 @@ class NotesFinder target_id = @params[:target_id] target_iid = @params[:target_iid] - return @target = nil unless target_type && (target_id || target_iid) + return @target = nil unless target_type + return @target = nil unless (target_id || target_iid) @target = if target_type == "commit" -- cgit v1.2.1 From 9079085fe7b03a330a50ddee638990af42b22bda Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Sat, 15 Jun 2019 09:05:45 +0200 Subject: Remove unneeded parentheses --- app/finders/notes_finder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 06d01245e90..8f610d7dddb 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -37,7 +37,7 @@ class NotesFinder target_iid = @params[:target_iid] return @target = nil unless target_type - return @target = nil unless (target_id || target_iid) + return @target = nil unless target_id || target_iid @target = if target_type == "commit" -- cgit v1.2.1 From 5469d21d02d2969f7b1c629ebd5e5696c664736c Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Wed, 19 Jun 2019 11:07:17 +0200 Subject: Simplify result of find_noteable --- lib/api/helpers/notes_helpers.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index c444766249d..b03ac7deb71 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -78,9 +78,7 @@ module API noteable = NotesFinder.new(user_project, current_user, params).target noteable = nil unless can?(current_user, noteable_read_ability_name(noteable), noteable) - return not_found!(noteable_type) unless noteable - - noteable + noteable || not_found!(noteable_type) end def params_by_noteable_type_and_id(type, id) -- cgit v1.2.1