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.

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.
Sebastian Ziebell
1 parent 54ab9bb6
Showing 6 changed files with 28 additions and 17 deletions Show diff stats
lib/api/helpers.rb
lib/api/merge_requests.rb
lib/api/milestones.rb
lib/api/notes.rb
lib/api/projects.rb
spec/requests/api/merge_requests_spec.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
@@ -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
@@ -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
@@ -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
@@ -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]
  
@@ -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
...	...	@@ -55,6 +55,12 @@ module Gitlab
55	55	render_api_error!('403 Forbidden', 403)
56	56	end
57	57
	58	+ def bad_request!(attribute)
	59	+ message = ["400 (Bad request)"]
	60	+ message << "\"" + attribute.to_s + "\" not given"
	61	+ render_api_error!(message.join(' '), 400)
	62	+ end
	63	+
58	64	def not_found!(resource = nil)
59	65	message = ["404"]
60	66	message << resource if resource
...	...
...	...	@@ -13,9 +13,9 @@ module Gitlab
13	13	#
14	14	def handle_merge_request_error(merge_request_errors)
15	15	if merge_request_errors[:target_branch].any?
16		- error!(merge_request_errors[:target_branch], 400)
	16	+ bad_request!(:target_branch)
17	17	elsif merge_request_errors[:source_branch].any?
18		- error!(merge_request_errors[:source_branch], 400)
	18	+ bad_request!(:source_branch)
19	19	elsif merge_request_errors[:base].any?
20	20	error!(merge_request_errors[:base], 422)
21	21	end
...	...	@@ -129,7 +129,7 @@ module Gitlab
129	129	present note, with: Entities::MRNote
130	130	else
131	131	if note.errors[:note].any?
132		- error!(note.errors[:note], 400)
	132	+ bad_request!(:note)
133	133	end
134	134	not_found!
135	135	end
...	...
...	...	@@ -13,7 +13,7 @@ module Gitlab
13	13	#
14	14	def handle_milestone_errors(milestone_errors)
15	15	if milestone_errors[:title].any?
16		- error!(milestone_errors[:title], 400)
	16	+ bad_request!(:title)
17	17	end
18	18	end
19	19	end
...	...
...	...	@@ -44,7 +44,7 @@ module Gitlab
44	44	present @note, with: Entities::Note
45	45	else
46	46	# :note is exposed as :body, but :note is set on error
47		- error!(@note.errors[:note], 400) if @note.errors[:note].any?
	47	+ bad_request!(:note) if @note.errors[:note].any?
48	48	not_found!
49	49	end
50	50	end
...	...
...	...	@@ -45,7 +45,7 @@ module Gitlab
45	45	# Example Request
46	46	# POST /projects
47	47	post do
48		- error!("Name is required", 400) if !params.has_key? :name
	48	+ bad_request!(:name) if !params.has_key? :name
49	49	attrs = attributes_for_keys [:name,
50	50	:description,
51	51	:default_branch,
...	...	@@ -101,8 +101,8 @@ module Gitlab
101	101	post ":id/members" do
102	102	authorize! :admin_project, user_project
103	103
104		- error!("User id not given", 400) if !params.has_key? :user_id
105		- error!("Access level not given", 400) if !params.has_key? :access_level
	104	+ bad_request!(:user_id) if !params.has_key? :user_id
	105	+ bad_request!(:access_level) if !params.has_key? :access_level
106	106
107	107	# either the user is already a team member or a new one
108	108	team_member = user_project.team_member_by_id(params[:user_id])
...	...	@@ -133,8 +133,8 @@ module Gitlab
133	133	authorize! :admin_project, user_project
134	134
135	135	team_member = user_project.users_projects.find_by_user_id(params[:user_id])
136		- error!("Access level not given", 400) if !params.has_key? :access_level
137		- error!("User can not be found", 404) if team_member.nil?
	136	+ bad_request!(:access_level) if !params.has_key? :access_level
	137	+ not_found!("User can not be found") if team_member.nil?
138	138
139	139	if team_member.update_attributes(project_access: params[:access_level])
140	140	@member = team_member.user
...	...	@@ -196,7 +196,7 @@ module Gitlab
196	196	post ":id/hooks" do
197	197	authorize! :admin_project, user_project
198	198
199		- error!("Url not given", 400) unless params.has_key? :url
	199	+ bad_request!(:url) unless params.has_key? :url
200	200
201	201	@hook = user_project.hooks.new({"url" => params[:url]})
202	202	if @hook.save
...	...	@@ -218,7 +218,7 @@ module Gitlab
218	218	@hook = user_project.hooks.find(params[:hook_id])
219	219	authorize! :admin_project, user_project
220	220
221		- error!("Url not given", 400) unless params.has_key? :url
	221	+ bad_request!(:url) unless params.has_key? :url
222	222
223	223	attrs = attributes_for_keys [:url]
224	224	if @hook.update_attributes attrs
...	...	@@ -237,7 +237,7 @@ module Gitlab
237	237	# DELETE /projects/:id/hooks
238	238	delete ":id/hooks" do
239	239	authorize! :admin_project, user_project
240		- error!("Hook id not given", 400) unless params.has_key? :hook_id
	240	+ bad_request!(:hook_id) unless params.has_key? :hook_id
241	241
242	242	begin
243	243	@hook = ProjectHook.find(params[:hook_id])
...	...	@@ -368,9 +368,9 @@ module Gitlab
368	368	post ":id/snippets" do
369	369	authorize! :write_snippet, user_project
370	370
371		- error!("Title not given", 400) if !params[:title].present?
372		- error!("Filename not given", 400) if !params[:file_name].present?
373		- error!("Code not given", 400) if !params[:code].present?
	371	+ bad_request!(:title) if !params[:title].present?
	372	+ bad_request!(:file_name) if !params[:file_name].present?
	373	+ bad_request!(:code) if !params[:code].present?
374	374
375	375	attrs = attributes_for_keys [:title, :file_name]
376	376	attrs[:expires_at] = params[:lifetime] if params[:lifetime].present?
...	...	@@ -451,7 +451,7 @@ module Gitlab
451	451	get ":id/repository/commits/:sha/blob" do
452	452	authorize! :download_code, user_project
453	453
454		- error!("Filepath must be specified", 400) if !params.has_key? :filepath
	454	+ bad_request!(:filepath) if !params.has_key? :filepath
455	455
456	456	ref = params[:sha]
457	457
...	...
...	...	@@ -32,6 +32,11 @@ describe Gitlab::API do
32	32	response.status.should == 200
33	33	json_response['title'].should == merge_request.title
34	34	end
	35	+
	36	+ it "should return a 404 error if merge_request_id not found" do
	37	+ get api("/projects/#{project.id}/merge_request/999", user)
	38	+ response.status.should == 404
	39	+ end
35	40	end
36	41
37	42	describe "POST /projects/:id/merge_requests" do
...	...