Commit 4d543541 authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

Merge branch 'jubianchi-api/improve-error-reporting' into 'master'

Merge jubianchi-api/improve-error-reporting

Closes https://github.com/gitlabhq/gitlabhq/pull/7502

See merge request !1112
parents 7dba4ece d0e3ab58
......@@ -122,9 +122,11 @@ class MergeRequest < ActiveRecord::Base
if opened? || reopened?
similar_mrs = self.target_project.merge_requests.where(source_branch: source_branch, target_branch: target_branch, source_project_id: source_project.id).opened
similar_mrs = similar_mrs.where('id not in (?)', self.id) if self.id
if similar_mrs.any?
errors.add :base, "Cannot Create: This merge request already exists: #{similar_mrs.pluck(:title)}"
errors.add :validate_branches,
"Cannot Create: This merge request already exists: #{
similar_mrs.pluck(:title)
}"
end
end
end
......@@ -140,7 +142,8 @@ class MergeRequest < ActiveRecord::Base
if source_project.forked_from?(target_project)
true
else
errors.add :base, "Source project is not a fork of target project"
errors.add :validate_fork,
'Source project is not a fork of target project'
end
end
end
......
......@@ -80,6 +80,7 @@ Return values:
- `404 Not Found` - A resource could not be accessed, e.g. an ID for a resource could not be found
- `405 Method Not Allowed` - The request is not supported
- `409 Conflict` - A conflicting resource already exists, e.g. creating a project with a name that already exists
- `422 Unprocessable` - The entity could not be processed
- `500 Server Error` - While handling the request something went wrong on the server side
## Sudo
......@@ -144,3 +145,52 @@ Issue:
- iid - is unique only in scope of a single project. When you browse issues or merge requests with Web UI, you see iid.
So if you want to get issue with api you use `http://host/api/v3/.../issues/:id.json`. But when you want to create a link to web page - use `http:://host/project/issues/:iid.json`
## Data validation and error reporting
When working with the API you may encounter validation errors. In such case, the API will answer with an HTTP `400` status.
Such errors appear in two cases:
* A required attribute of the API request is missing, e.g. the title of an issue is not given
* An attribute did not pass the validation, e.g. user bio is too long
When an attribute is missing, you will get something like:
HTTP/1.1 400 Bad Request
Content-Type: application/json
{
"message":"400 (Bad request) \"title\" not given"
}
When a validation error occurs, error messages will be different. They will hold all details of validation errors:
HTTP/1.1 400 Bad Request
Content-Type: application/json
{
"message": {
"bio": [
"is too long (maximum is 255 characters)"
]
}
}
This makes error messages more machine-readable. The format can be described as follow:
{
"message": {
"<property-name>": [
"<error-message>",
"<error-message>",
...
],
"<embed-entity>": {
"<property-name>": [
"<error-message>",
"<error-message>",
...
],
}
}
}
......@@ -58,7 +58,7 @@ module API
if key.valid? && user_project.deploy_keys << key
present key, with: Entities::SSHKey
else
not_found!
render_validation_error!(key)
end
end
......
......@@ -155,7 +155,17 @@ module API
end
def not_allowed!
render_api_error!('Method Not Allowed', 405)
render_api_error!('405 Method Not Allowed', 405)
end
def conflict!(message = nil)
render_api_error!(message || '409 Conflict', 409)
end
def render_validation_error!(model)
unless model.valid?
render_api_error!(model.errors.messages || '400 Bad Request', 400)
end
end
def render_api_error!(message, status)
......
......@@ -109,7 +109,7 @@ module API
present issue, with: Entities::Issue
else
not_found!
render_validation_error!(issue)
end
end
......@@ -149,7 +149,7 @@ module API
present issue, with: Entities::Issue
else
not_found!
render_validation_error!(issue)
end
end
......
......@@ -30,16 +30,14 @@ module API
attrs = attributes_for_keys [:name, :color]
label = user_project.find_label(attrs[:name])
if label
return render_api_error!('Label already exists', 409)
end
conflict!('Label already exists') if label
label = user_project.labels.create(attrs)
if label.valid?
present label, with: Entities::Label
else
render_api_error!(label.errors.full_messages.join(', '), 400)
render_validation_error!(label)
end
end
......@@ -56,9 +54,7 @@ module API
required_attributes! [:name]
label = user_project.find_label(params[:name])
if !label
return render_api_error!('Label not found', 404)
end
not_found!('Label') unless label
label.destroy
end
......@@ -66,10 +62,11 @@ module API
# Updates an existing label. At least one optional parameter is required.
#
# Parameters:
# id (required) - The ID of a project
# name (optional) - The name of the label to be deleted
# color (optional) - Color of the label given in 6-digit hex
# notation with leading '#' sign (e.g. #FFAABB)
# id (required) - The ID of a project
# name (required) - The name of the label to be deleted
# new_name (optional) - The new name of the label
# color (optional) - Color of the label given in 6-digit hex
# notation with leading '#' sign (e.g. #FFAABB)
# Example Request:
# PUT /projects/:id/labels
put ':id/labels' do
......@@ -77,16 +74,14 @@ module API
required_attributes! [:name]
label = user_project.find_label(params[:name])
if !label
return render_api_error!('Label not found', 404)
end
not_found!('Label not found') unless label
attrs = attributes_for_keys [:new_name, :color]
if attrs.empty?
return render_api_error!('Required parameters "name" or "color" ' \
'missing',
400)
render_api_error!('Required parameters "new_name" or "color" ' \
'missing',
400)
end
# Rename new name to the actual label attribute name
......@@ -95,7 +90,7 @@ module API
if label.update(attrs)
present label, with: Entities::Label
else
render_api_error!(label.errors.full_messages.join(', '), 400)
render_validation_error!(label)
end
end
end
......
......@@ -10,8 +10,13 @@ module API
error!(errors[:project_access], 422)
elsif errors[:branch_conflict].any?
error!(errors[:branch_conflict], 422)
elsif errors[:validate_fork].any?
error!(errors[:validate_fork], 422)
elsif errors[:validate_branches].any?
conflict!(errors[:validate_branches])
end
not_found!
render_api_error!(errors, 400)
end
end
......@@ -228,7 +233,7 @@ module API
if note.save
present note, with: Entities::MRNote
else
not_found!
render_validation_error!(note)
end
end
end
......
......@@ -56,7 +56,7 @@ module API
if @snippet.save
present @snippet, with: Entities::ProjectSnippet
else
not_found!
render_validation_error!(@snippet)
end
end
......@@ -80,7 +80,7 @@ module API
if @snippet.update_attributes attrs
present @snippet, with: Entities::ProjectSnippet
else
not_found!
render_validation_error!(@snippet)
end
end
......@@ -97,6 +97,7 @@ module API
authorize! :modify_project_snippet, @snippet
@snippet.destroy
rescue
not_found!('Snippet')
end
end
......
......@@ -111,7 +111,7 @@ module API
if @project.errors[:limit_reached].present?
error!(@project.errors[:limit_reached], 403)
end
not_found!
render_validation_error!(@project)
end
end
......@@ -149,7 +149,7 @@ module API
if @project.saved?
present @project, with: Entities::Project
else
not_found!
render_validation_error!(@project)
end
end
......
......@@ -42,7 +42,8 @@ module API
# Parameters:
# email (required) - Email
# password (required) - Password
# name - Name
# name (required) - Name
# username (required) - Name
# skype - Skype ID
# linkedin - Linkedin
# twitter - Twitter account
......@@ -65,7 +66,15 @@ module API
if user.save
present user, with: Entities::UserFull
else
not_found!
conflict!('Email has already been taken') if User.
where(email: user.email).
count > 0
conflict!('Username has already been taken') if User.
where(username: user.username).
count > 0
render_validation_error!(user)
end
end
......@@ -92,14 +101,23 @@ module API
attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :website_url, :projects_limit, :username, :extern_uid, :provider, :bio, :can_create_group, :admin]
user = User.find(params[:id])
not_found!("User not found") unless user
not_found!('User') unless user
admin = attrs.delete(:admin)
user.admin = admin unless admin.nil?
conflict!('Email has already been taken') if attrs[:email] &&
User.where(email: attrs[:email]).
where.not(id: user.id).count > 0
conflict!('Username has already been taken') if attrs[:username] &&
User.where(username: attrs[:username]).
where.not(id: user.id).count > 0
if user.update_attributes(attrs)
present user, with: Entities::UserFull
else
not_found!
render_validation_error!(user)
end
end
......@@ -113,13 +131,15 @@ module API
# POST /users/:id/keys
post ":id/keys" do
authenticated_as_admin!
required_attributes! [:title, :key]
user = User.find(params[:id])
attrs = attributes_for_keys [:title, :key]
key = user.keys.new attrs
if key.save
present key, with: Entities::SSHKey
else
not_found!
render_validation_error!(key)
end
end
......@@ -132,11 +152,9 @@ module API
get ':uid/keys' do
authenticated_as_admin!
user = User.find_by(id: params[:uid])
if user
present user.keys, with: Entities::SSHKey
else
not_found!
end
not_found!('User') unless user
present user.keys, with: Entities::SSHKey
end
# Delete existing ssh key of a specified user. Only available to admin
......@@ -150,15 +168,13 @@ module API
delete ':uid/keys/:id' do
authenticated_as_admin!
user = User.find_by(id: params[:uid])
if user
begin
key = user.keys.find params[:id]
key.destroy
rescue ActiveRecord::RecordNotFound
not_found!
end
else
not_found!
not_found!('User') unless user
begin
key = user.keys.find params[:id]
key.destroy
rescue ActiveRecord::RecordNotFound
not_found!('Key')
end
end
......@@ -173,7 +189,7 @@ module API
if user
user.destroy
else
not_found!
not_found!('User')
end
end
end
......@@ -219,7 +235,7 @@ module API
if key.save
present key, with: Entities::SSHKey
else
not_found!
render_validation_error!(key)
end
end
......
......@@ -169,6 +169,15 @@ describe API::API, api: true do
response.status.should == 400
json_response['message']['labels']['?']['title'].should == ['is invalid']
end
it 'should return 400 if title is too long' do
post api("/projects/#{project.id}/issues", user),
title: 'g' * 256
response.status.should == 400
json_response['message']['title'].should == [
'is too long (maximum is 255 characters)'
]
end
end
describe "PUT /projects/:id/issues/:issue_id to update only title" do
......@@ -237,6 +246,15 @@ describe API::API, api: true do
json_response['labels'].should include 'label_bar'
json_response['labels'].should include 'label/bar'
end
it 'should return 400 if title is too long' do
put api("/projects/#{project.id}/issues/#{issue.id}", user),
title: 'g' * 256
response.status.should == 400
json_response['message']['title'].should == [
'is too long (maximum is 255 characters)'
]
end
end
describe "PUT /projects/:id/issues/:issue_id to update state and label" do
......
......@@ -47,7 +47,7 @@ describe API::API, api: true do
name: 'Foo',
color: '#FFAA'
response.status.should == 400
json_response['message'].should == 'Color is invalid'
json_response['message']['color'].should == ['is invalid']
end
it 'should return 400 for too long color code' do
......@@ -55,7 +55,7 @@ describe API::API, api: true do
name: 'Foo',
color: '#FFAAFFFF'
response.status.should == 400
json_response['message'].should == 'Color is invalid'
json_response['message']['color'].should == ['is invalid']
end
it 'should return 400 for invalid name' do
......@@ -63,7 +63,7 @@ describe API::API, api: true do
name: '?',
color: '#FFAABB'
response.status.should == 400
json_response['message'].should == 'Title is invalid'
json_response['message']['title'].should == ['is invalid']
end
it 'should return 409 if label already exists' do
......@@ -84,7 +84,7 @@ describe API::API, api: true do
it 'should return 404 for non existing label' do
delete api("/projects/#{project.id}/labels", user), name: 'label2'
response.status.should == 404
json_response['message'].should == 'Label not found'
json_response['message'].should == '404 Label Not Found'
end
it 'should return 400 for wrong parameters' do
......@@ -132,11 +132,14 @@ describe API::API, api: true do
it 'should return 400 if no label name given' do
put api("/projects/#{project.id}/labels", user), new_name: 'label2'
response.status.should == 400
json_response['message'].should == '400 (Bad request) "name" not given'
end
it 'should return 400 if no new parameters given' do
put api("/projects/#{project.id}/labels", user), name: 'label1'
response.status.should == 400
json_response['message'].should == 'Required parameters '\
'"new_name" or "color" missing'
end
it 'should return 400 for invalid name' do
......@@ -145,7 +148,7 @@ describe API::API, api: true do
new_name: '?',
color: '#FFFFFF'
response.status.should == 400
json_response['message'].should == 'Title is invalid'
json_response['message']['title'].should == ['is invalid']
end
it 'should return 400 for invalid name' do
......@@ -153,7 +156,7 @@ describe API::API, api: true do
name: 'label1',
color: '#FF'
response.status.should == 400
json_response['message'].should == 'Color is invalid'
json_response['message']['color'].should == ['is invalid']
end
it 'should return 400 for too long color code' do
......@@ -161,7 +164,7 @@ describe API::API, api: true do
name: 'Foo',
color: '#FFAAFFFF'
response.status.should == 400
json_response['message'].should == 'Color is invalid'
json_response['message']['color'].should == ['is invalid']
end
end
end
......@@ -163,6 +163,28 @@ describe API::API, api: true do
json_response['message']['labels']['?']['title'].should ==
['is invalid']
end
context 'with existing MR' do
before do
post api("/projects/#{project.id}/merge_requests", user),
title: 'Test merge_request',
source_branch: 'stable',
target_branch: 'master',
author: user
@mr = MergeRequest.all.last
end
it 'should return 409 when MR already exists for source/target' do
expect do
post api("/projects/#{project.id}/merge_requests", user),
title: 'New test merge_request',
source_branch: 'stable',
target_branch: 'master',
author: user
end.to change { MergeRequest.count }.by(0)
response.status.should == 409
end
end
end
context 'forked projects' do
......@@ -210,16 +232,26 @@ describe API::API, api: true do
response.status.should == 400
end
it "should return 404 when target_branch is specified and not a forked project" do
post api("/projects/#{project.id}/merge_requests", user),
title: 'Test merge_request', target_branch: 'master', source_branch: 'stable', author: user, target_project_id: fork_project.id
response.status.should == 404
end
context 'when target_branch is specified' do
it 'should return 422 if not a forked project' do
post api("/projects/#{project.id}/merge_requests", user),
title: 'Test merge_request',
target_branch: 'master',
source_branch: 'stable',
author: user,
target_project_id: fork_project.id
response.status.should == 422
end
it "should return 404 when target_branch is specified and for a different fork" do
post api("/projects/#{fork_project.id}/merge_requests", user2),
title: 'Test merge_request', target_branch: 'master', source_branch: 'stable', author: user2, target_project_id: unrelated_project.id
response.status.should == 404
it 'should return 422 if targeting a different fork' do
post api("/projects/#{fork_project.id}/merge_requests", user2),
title: 'Test merge_request',
target_branch: 'master',
source_branch: 'stable',
author: user2,
target_project_id: unrelated_project.id
response.status.should == 422
end
end
it "should return 201 when target_branch is specified and for the same project" do
......@@ -256,7 +288,7 @@ describe API::API, api: true do
merge_request.close
put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user)
response.status.should == 405
json_response['message'].should == 'Method Not Allowed'
json_response['message'].should == '405 Method Not Allowed'
end
it "should return 401 if user has no permissions to merge" do
......@@ -316,7 +348,8 @@ describe API::API, api: true do
end
it "should return 404 if note is attached to non existent merge request" do
post api("/projects/#{project.id}/merge_request/111/comments", user), note: "My comment"
post api("/projects/#{project.id}/merge_request/404/comments", user),
note: 'My comment'
response.status.should == 404
end
end
......
......@@ -188,9 +188,24 @@ describe API::API, api: true do
response.status.should == 201
end
it "should respond with 404 on failure" do
it 'should respond with 400 on failure' do
post api("/projects/user/#{user.id}", admin)
response.status.should == 404
response.status.should == 400
json_response['message']['creator'].should == ['can\'t be blank']
json_response['message']['namespace'].should == ['can\'t be blank']
json_response['message']['name'].should == [
'can\'t be blank',
'is too short (minimum is 0 characters)',
'can contain only letters, digits, \'_\', \'-\' and \'.\' and '\
'space. It must start with letter, digit or \'_\'.'
]
json_response['message']['path'].should == [
'can\'t be blank',
'is too short (minimum is 0 characters)',
'can contain only letters, digits, \'_\', \'-\' and \'.\'. It must '\
'start with letter, digit or \'_\', optionally preceeded by \'.\'. '\
'It must not end in \'.git\'.'
]
end
it "should assign attributes to project" do
......@@ -410,9 +425,9 @@ describe API::API, api: true do
response.status.should == 200
end
it "should return success when deleting unknown snippet id" do
it 'should return 404 when deleting unknown snippet id' do
delete api("/projects/#{project.id}/snippets/1234", user)
response.status.should == 200
response.status.should == 404
end
end
......@@ -459,7 +474,21 @@ describe API::API, api: true do
describe "POST /projects/:id/keys" do
it "should not create an invalid ssh key" do
post api("/projects/#{project.id}/keys", user), { title: "invalid key" }
response.status.should == 404
response.status.should == 400
json_response['message']['key'].should == [
'can\'t be blank',
'is too short (minimum is 0 characters)',
'is invalid'
]
end
it 'should not create a key without title' do
post api("/projects/#{project.id}/keys", user), key: 'some key'
response.status.should == 400
json_response['message']['title'].should == [
'can\'t be blank',
'is too short (minimum is 0 characters)'
]
end
it "should create new ssh key" do
......
This diff is collapsed.
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