API: Return 400 for all validation erros in the mebers API

changelogs/unreleased/unified-member-api-response.yml

@@ -0,0 +1,4 @@
+---
+title: 'API: Return 400 for all validation erros in the mebers API'
+merge_request: 9523
+author: Robert Schilling

doc/api/v3_to_v4.md

@@ -41,5 +41,6 @@ changes are in V4:
 - Renamed `branch_name` to `branch` on DELETE `id/repository/branches/:branch` response [!8936](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8936)
 - Remove `public` param from create and edit actions of projects [!8736](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8736)
 - Notes do not return deprecated field `upvote` and `downvote` [!9384](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9384)
+- Return HTTP status code `400` for all validation errors when creating or updating a member instead of sometimes `422` error. [!9523](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9523)
 - Remove `GET /groups/owned`. Use `GET /groups?owned=true` instead [!9505](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9505)
 - Return 202 with JSON body on async removals on V4 API (DELETE `/projects/:id/repository/merged_branches` and DELETE `/projects/:id`) [!9449](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9449)

lib/api/members.rb

@@ -55,7 +55,6 @@ module API
           authorize_admin_source!(source_type, source)
 
           member = source.members.find_by(user_id: params[:user_id])
-
           conflict!('Member already exists') if member
 
           member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
@@ -63,9 +62,6 @@ module API
           if member.persisted? && member.valid?
             present member.user, with: Entities::Member, member: member
           else
-            # This is to ensure back-compatibility but 400 behavior should be used
-            # for all validation errors in 9.0!
-            render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level)
             render_validation_error!(member)
           end
         end
@@ -87,9 +83,6 @@ module API
           if member.update_attributes(declared_params(include_missing: false))
             present member.user, with: Entities::Member, member: member
           else
-            # This is to ensure back-compatibility but 400 behavior should be used
-            # for all validation errors in 9.0!
-            render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level)
             render_validation_error!(member)
           end
         end

spec/requests/api/members_spec.rb

@@ -173,11 +173,11 @@ describe API::Members, api: true  do
         expect(response).to have_http_status(400)
       end
 
-      it 'returns 422 when access_level is not valid' do
+      it 'returns 400  when access_level is not valid' do
         post api("/#{source_type.pluralize}/#{source.id}/members", master),
              user_id: stranger.id, access_level: 1234
 
-        expect(response).to have_http_status(422)
+        expect(response).to have_http_status(400)
       end
     end
   end
@@ -230,11 +230,11 @@ describe API::Members, api: true  do
         expect(response).to have_http_status(400)
       end
 
-      it 'returns 422 when access level is not valid' do
+      it 'returns 400  when access level is not valid' do
         put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master),
             access_level: 1234
 
-        expect(response).to have_http_status(422)
+        expect(response).to have_http_status(400)
       end
     end
   end
@@ -342,7 +342,7 @@ describe API::Members, api: true  do
         post api("/projects/#{project.id}/members", master),
              user_id: stranger.id, access_level: Member::OWNER
 
-        expect(response).to have_http_status(422)
+        expect(response).to have_http_status(400)
       end.to change { project.members.count }.by(0)
     end
   end