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 --- 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 ++-- 5 files changed, 32 insertions(+), 28 deletions(-) (limited to 'lib/api') 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 -- 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(-) (limited to 'lib/api') 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