Some API requests return a 404 when some other error code would be appropriate
Created by: Xylakant
Some calls to the API return a 404 when some other status code would be justified, for example the attempt to add a project member that already exists returns 404. In general, this pattern seems to be used quite often:
if users_project.save
@member = users_project.user
present @member, with: Entities::ProjectMember, project: user_project
else
not_found!Returning 404 in this case is not correct. If you really want to throw an error, 409 conflict might be appropriate, but given that this operation can be made idempotent, a 201 Created might be more appropriate in this case.
Activity
Created by: m4tthumphrey
This has concerned me for a while now. The big one that stuck out for me was if you try and create a new repo where the name already exists, it returns a 404. Not helpful at all.
By Administrator on 2013-01-30T10:47:28 (imported from GitLab project)
By Administrator on 2013-01-30T10:47:28 (imported from GitLab)
Created by: Xylakant
I knew about the docs, but had not found the specs. Thanks for the pointer. We can provide a patch for the issue and related ones (#2832 (closed)) if there's interest but I don't want to trample on somebody elses work. So I'd appreciate feedback.
Walking through the specs I see the following return codes as problematic (or currently unspecced):
- failure to create or update a merge request: (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/merge_requests.rb#L63 / https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/merge_requests.rb#L91)
- failure to add a note to a merge request (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/merge_requests.rb#L112)
- failure to create or update a milestone (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/milestones.rb#L50 / https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/milestones.rb#L73)
- failure to create notes (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/notes.rb#L46 / https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/notes.rb#L100)
- failure to create a project (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/projects.rb#L50) [1]
- failure to add or modify a project member (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/projects.rb#L101 / https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/projects.rb#L121)
- failure to create or update a project hook (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/projects.rb#L176 / https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/projects.rb#L197)
- failure to create or update a project snippet (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/projects.rb#L308 / https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/projects.rb#L308)
- failure to create a user (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/users.rb#L46) [2]
- failure to add a key for a user (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/users.rb#L90)
For theses cases I would propose the following behavior:
- PUT requests (updates) in general should be idempotent, so multiple put request with the same payload should return 200 OK (haven't tested, but judging from the code that's the case)
- If the ressource that should be modified cannot be found, 404 is legitimate. So if the user is trying to add a snippet to a project that does not exist, 404. If he tries to modify a snippet that does not exist 404. Actually, if the user doesn't have permissions to see the project, 404 would be best as well, akin to what github does. Return a description of why the operation failed ("Trying to add a note on a project that does not exist").
- If the ressource cannot be saved due to a conflict, such as having two projects with the same name, 409 Conflict seems appropriate. Try to give a reason "A project with this name already exists."
- If something else fails, it's the servers fault, return a generic 500.
These will currently blow up and raise a 500
- Trying to remove a project member that's not member any more (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/projects.rb#L135)
- Trying to delete a hook that's not there (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/projects.rb#L208)
- Trying to delete a snippet that's not there (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/projects.rb#L349)
- Trying to delete a key that doesn't exist (https://github.com/gitlabhq/gitlabhq/blob/master/lib/api/users.rb#L102)
These cases should return 200 OK, even if the ressource in question does not exist. The caller doesn't actually care wether they existed before or not, just that they're gone. This makes the API more robust over slow or unreliable connections, where operations can fail due to network errors. The client doesn't need to keep track of failed/succeeded, he can just try again. The return value would be slightly different in case the ressource didn't exist since currently the call returns the deleted ressource.
Sorry for the long post, appreciate if you got this far.
[1] side-note: I didn't find an api endpoint to update a project. Oversight or on purpose? [2] nor could I find the endpoint to update a user.
By Administrator on 2013-01-30T18:07:21 (imported from GitLab project)
By Administrator on 2013-01-30T18:07:21 (imported from GitLab)
Created by: Xylakant
403 would leak information: You now know that the project exists, but you don't have access. That's the reason that github et. al. use 404 when you try to access a private repository that you don't have permissions to see.
By Administrator on 2013-01-31T14:33:14 (imported from GitLab project)
By Administrator on 2013-01-31T14:33:14 (imported from GitLab)
Created by: Xylakant
What about cases where we strictly speaking are not creating the ressource, but it doesn't matter for the client? Such as adding a user to a project that already is a team member with the same access level as given in the call? I'd return 200 OK here since the result is as if the operation succeeded. However, if the current access level and the given access level differ, the result should be a conflict.
By Administrator on 2013-01-31T15:06:03 (imported from GitLab project)
By Administrator on 2013-01-31T15:06:03 (imported from GitLab)
