From 6fc3263e15b71830e6f1b2a66891da5f4c055137 Mon Sep 17 00:00:00 2001 From: Sebastian Ziebell Date: Wed, 13 Feb 2013 15:48:52 +0100 Subject: [PATCH] API: extracted helper method to provide 400 bad request error with description Extracted a method for 400 error (Bad request) and adjusted code accordingly. The name of the missing attribute is used to show which one was missing from the request. It is used to give an appropriate message in the json response. --- lib/api/helpers.rb | 6 ++++++ lib/api/merge_requests.rb | 6 +++--- lib/api/milestones.rb | 2 +- lib/api/notes.rb | 2 +- lib/api/projects.rb | 24 ++++++++++++------------ spec/requests/api/merge_requests_spec.rb | 5 +++++ 6 files changed, 28 insertions(+), 17 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 6bd8111c..becb3bce 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -55,6 +55,12 @@ module Gitlab render_api_error!('403 Forbidden', 403) end + def bad_request!(attribute) + message = ["400 (Bad request)"] + message << "\"" + attribute.to_s + "\" not given" + render_api_error!(message.join(' '), 400) + end + def not_found!(resource = nil) message = ["404"] message << resource if resource diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index a0ca3026..0c18ece3 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -13,9 +13,9 @@ module Gitlab # def handle_merge_request_error(merge_request_errors) if merge_request_errors[:target_branch].any? - error!(merge_request_errors[:target_branch], 400) + bad_request!(:target_branch) elsif merge_request_errors[:source_branch].any? - error!(merge_request_errors[:source_branch], 400) + bad_request!(:source_branch) elsif merge_request_errors[:base].any? error!(merge_request_errors[:base], 422) end @@ -129,7 +129,7 @@ module Gitlab present note, with: Entities::MRNote else if note.errors[:note].any? - error!(note.errors[:note], 400) + bad_request!(:note) end not_found! end diff --git a/lib/api/milestones.rb b/lib/api/milestones.rb index 1f7d0876..cdb0e146 100644 --- a/lib/api/milestones.rb +++ b/lib/api/milestones.rb @@ -13,7 +13,7 @@ module Gitlab # def handle_milestone_errors(milestone_errors) if milestone_errors[:title].any? - error!(milestone_errors[:title], 400) + bad_request!(:title) end end end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 47dead9d..56de6e09 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -44,7 +44,7 @@ module Gitlab present @note, with: Entities::Note else # :note is exposed as :body, but :note is set on error - error!(@note.errors[:note], 400) if @note.errors[:note].any? + bad_request!(:note) if @note.errors[:note].any? not_found! end end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 24761cd5..ecd3401f 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -45,7 +45,7 @@ module Gitlab # Example Request # POST /projects post do - error!("Name is required", 400) if !params.has_key? :name + bad_request!(:name) if !params.has_key? :name attrs = attributes_for_keys [:name, :description, :default_branch, @@ -101,8 +101,8 @@ module Gitlab post ":id/members" do authorize! :admin_project, user_project - error!("User id not given", 400) if !params.has_key? :user_id - error!("Access level not given", 400) if !params.has_key? :access_level + bad_request!(:user_id) if !params.has_key? :user_id + bad_request!(:access_level) if !params.has_key? :access_level # either the user is already a team member or a new one team_member = user_project.team_member_by_id(params[:user_id]) @@ -133,8 +133,8 @@ module Gitlab authorize! :admin_project, user_project team_member = user_project.users_projects.find_by_user_id(params[:user_id]) - error!("Access level not given", 400) if !params.has_key? :access_level - error!("User can not be found", 404) if team_member.nil? + bad_request!(:access_level) if !params.has_key? :access_level + not_found!("User can not be found") if team_member.nil? if team_member.update_attributes(project_access: params[:access_level]) @member = team_member.user @@ -196,7 +196,7 @@ module Gitlab post ":id/hooks" do authorize! :admin_project, user_project - error!("Url not given", 400) unless params.has_key? :url + bad_request!(:url) unless params.has_key? :url @hook = user_project.hooks.new({"url" => params[:url]}) if @hook.save @@ -218,7 +218,7 @@ module Gitlab @hook = user_project.hooks.find(params[:hook_id]) authorize! :admin_project, user_project - error!("Url not given", 400) unless params.has_key? :url + bad_request!(:url) unless params.has_key? :url attrs = attributes_for_keys [:url] if @hook.update_attributes attrs @@ -237,7 +237,7 @@ module Gitlab # DELETE /projects/:id/hooks delete ":id/hooks" do authorize! :admin_project, user_project - error!("Hook id not given", 400) unless params.has_key? :hook_id + bad_request!(:hook_id) unless params.has_key? :hook_id begin @hook = ProjectHook.find(params[:hook_id]) @@ -368,9 +368,9 @@ module Gitlab post ":id/snippets" do authorize! :write_snippet, user_project - error!("Title not given", 400) if !params[:title].present? - error!("Filename not given", 400) if !params[:file_name].present? - error!("Code not given", 400) if !params[:code].present? + bad_request!(:title) if !params[:title].present? + bad_request!(:file_name) if !params[:file_name].present? + bad_request!(:code) if !params[:code].present? attrs = attributes_for_keys [:title, :file_name] attrs[:expires_at] = params[:lifetime] if params[:lifetime].present? @@ -451,7 +451,7 @@ module Gitlab get ":id/repository/commits/:sha/blob" do authorize! :download_code, user_project - error!("Filepath must be specified", 400) if !params.has_key? :filepath + bad_request!(:filepath) if !params.has_key? :filepath ref = params[:sha] diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 4e7a84df..431aae7c 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -32,6 +32,11 @@ describe Gitlab::API do response.status.should == 200 json_response['title'].should == merge_request.title end + + it "should return a 404 error if merge_request_id not found" do + get api("/projects/#{project.id}/merge_request/999", user) + response.status.should == 404 + end end describe "POST /projects/:id/merge_requests" do