Remove ARRAY_COERCION_LAMBDA in API coercions

Use `Array[Integer]` and coerce these to an integer arrays.

Note that if `Array` were used with this lambda, things worked work with
Grape v1.3. However, `Array[String]` did not. Standardize everything on
using `Validations::Types::CommaSeparatedToIntegerArray.coerce` with
`Array[Integer]`

ee/lib/api/helpers/project_approval_rules_helpers.rb

@@ -5,24 +5,22 @@ module API
     module ProjectApprovalRulesHelpers
       extend Grape::API::Helpers
 
-      ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
-
       params :create_project_approval_rule do
         requires :name, type: String, desc: 'The name of the approval rule'
         requires :approvals_required, type: Integer, desc: 'The number of required approvals for this rule'
         optional :rule_type, type: String, desc: 'The type of approval rule'
-        optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
-        optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
-        optional :protected_branch_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The protected branch ids for this rule'
+        optional :users, as: :user_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The user ids for this rule'
+        optional :groups, as: :group_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The group ids for this rule'
+        optional :protected_branch_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The protected branch ids for this rule'
       end
 
       params :update_project_approval_rule do
         requires :approval_rule_id, type: Integer, desc: 'The ID of an approval_rule'
         optional :name, type: String, desc: 'The name of the approval rule'
         optional :approvals_required, type: Integer, desc: 'The number of required approvals for this rule'
-        optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
-        optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
-        optional :protected_branch_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The protected branch ids for this rule'
+        optional :users, as: :user_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The user ids for this rule'
+        optional :groups, as: :group_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The group ids for this rule'
+        optional :protected_branch_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The protected branch ids for this rule'
         optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed'
       end
 

ee/lib/api/merge_request_approval_rules.rb

@@ -4,8 +4,6 @@ module API
   class MergeRequestApprovalRules < ::Grape::API::Instance
     before { authenticate_non_get! }
 
-    ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
-
     helpers do
       def find_merge_request_approval_rule(merge_request, id)
         merge_request.approval_rules.find_by_id!(id)
@@ -34,8 +32,8 @@ module API
           requires :name, type: String, desc: 'The name of the approval rule'
           requires :approvals_required, type: Integer, desc: 'The number of required approvals for this rule'
           optional :approval_project_rule_id, type: Integer, desc: 'The ID of a project-level approval rule'
-          optional :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
-          optional :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
+          optional :user_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The user ids for this rule'
+          optional :group_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The group ids for this rule'
         end
         post do
           merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
@@ -56,8 +54,8 @@ module API
             requires :approval_rule_id, type: Integer, desc: 'The ID of an approval rule'
             optional :name, type: String, desc: 'The name of the approval rule'
             optional :approvals_required, type: Integer, desc: 'The number of required approvals for this rule'
-            optional :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
-            optional :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
+            optional :user_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The user ids for this rule'
+            optional :group_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The group ids for this rule'
             optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed'
           end
           put do

ee/lib/api/merge_request_approvals.rb

@@ -4,8 +4,6 @@ module API
   class MergeRequestApprovals < ::Grape::API::Instance
     before { authenticate_non_get! }
 
-    ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
-
     helpers do
       def present_approval(merge_request)
         present merge_request.approval_state, with: ::EE::API::Entities::ApprovalState, current_user: current_user
@@ -109,8 +107,10 @@ module API
           success ::EE::API::Entities::ApprovalState
         end
         params do
-          requires :approver_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of User IDs to set as approvers.'
-          requires :approver_group_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of Group IDs to set as approvers.'
+          requires :approver_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce,
+            desc: 'Array of User IDs to set as approvers.'
+          requires :approver_group_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce,
+            desc: 'Array of Group IDs to set as approvers.'
         end
         put 'approvers' do
           Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/8883')

ee/lib/api/project_approvals.rb

@@ -5,8 +5,6 @@ module API
     before { authenticate! }
     before { authorize! :update_approvers, user_project }
 
-    ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
-
     helpers do
       def filter_forbidden_param!(permission, param)
         unless can?(current_user, permission, user_project)
@@ -67,8 +65,8 @@ module API
         success EE::API::Entities::ApprovalSettings
       end
       params do
-        requires :approver_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of User IDs to set as approvers.'
-        requires :approver_group_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of Group IDs to set as approvers.'
+        requires :approver_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Array of User IDs to set as approvers.'
+        requires :approver_group_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Array of Group IDs to set as approvers.'
       end
       put ':id/approvers' do
         result = ::Projects::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute

ee/spec/requests/api/merge_request_approval_rules_spec.rb

@@ -143,7 +143,9 @@ describe API::MergeRequestApprovalRules do
     let(:current_user) { user }
     let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_rules" }
     let(:approver) { create(:user) }
+    let(:other_approver) { create(:user) }
     let(:group) { create(:group) }
+    let(:other_group) { create(:group) }
     let(:approval_project_rule_id) { nil }
     let(:user_ids) { [] }
     let(:group_ids) { [] }
@@ -166,7 +168,9 @@ describe API::MergeRequestApprovalRules do
       before do
         project.update!(disable_overriding_approvers_per_merge_request: false)
         project.add_developer(approver)
+        project.add_developer(other_approver)
         group.add_developer(approver)
+        other_group.add_developer(other_approver)
 
         action
       end
@@ -179,7 +183,7 @@ describe API::MergeRequestApprovalRules do
 
         expect(rule['name']).to eq(params[:name])
         expect(rule['approvals_required']).to eq(params[:approvals_required])
-        expect(rule['rule_type']).to eq('regular')
+        expect(rule['rule_type']).to eq('any_approver')
         expect(rule['contains_hidden_groups']).to eq(false)
         expect(rule['source_rule']).to be_nil
         expect(rule['eligible_approvers']).to be_empty
@@ -188,24 +192,24 @@ describe API::MergeRequestApprovalRules do
       end
 
       context 'users are passed' do
-        let(:user_ids) { [approver.id] }
+        let(:user_ids) { "#{approver.id},#{other_approver.id}" }
 
         it 'includes users' do
           rule = json_response
 
-          expect(rule['eligible_approvers']).to match([hash_including('id' => approver.id)])
-          expect(rule['users']).to match([hash_including('id' => approver.id)])
+          expect(rule['eligible_approvers'].map { |approver| approver['id'] }).to contain_exactly(approver.id, other_approver.id)
+          expect(rule['users'].map { |user| user['id'] }).to contain_exactly(approver.id, other_approver.id)
         end
       end
 
       context 'groups are passed' do
-        let(:group_ids) { [group.id] }
+        let(:group_ids) { "#{group.id},#{other_group.id}" }
 
         it 'includes groups' do
           rule = json_response
 
-          expect(rule['eligible_approvers']).to match([hash_including('id' => approver.id)])
-          expect(rule['groups']).to match([hash_including('id' => group.id)])
+          expect(rule['eligible_approvers'].map { |approver| approver['id'] }).to contain_exactly(approver.id, other_approver.id)
+          expect(rule['groups'].map { |group| group['id'] }).to contain_exactly(group.id, other_group.id)
         end
       end
 
@@ -257,6 +261,8 @@ describe API::MergeRequestApprovalRules do
     let(:user_ids) { [] }
     let(:group_ids) { [] }
     let(:remove_hidden_groups) { nil }
+    let(:other_approver) { create(:user) }
+    let(:other_group) { create(:group) }
 
     let(:params) do
       {
@@ -277,8 +283,10 @@ describe API::MergeRequestApprovalRules do
         project.update!(disable_overriding_approvers_per_merge_request: false)
         project.add_developer(existing_approver)
         project.add_developer(new_approver)
+        project.add_developer(other_approver)
         existing_group.add_developer(existing_approver)
         new_group.add_developer(new_approver)
+        other_group.add_developer(other_approver)
 
         action
       end
@@ -302,24 +310,24 @@ describe API::MergeRequestApprovalRules do
       end
 
       context 'users are passed' do
-        let(:user_ids) { [new_approver.id] }
+        let(:user_ids) { "#{new_approver.id},#{existing_approver.id}" }
 
         it 'changes users' do
           rule = json_response
 
-          expect(rule['eligible_approvers']).to match([hash_including('id' => new_approver.id)])
-          expect(rule['users']).to match([hash_including('id' => new_approver.id)])
+          expect(rule['eligible_approvers'].map { |approver| approver['id'] }).to contain_exactly(new_approver.id, existing_approver.id)
+          expect(rule['users'].map { |user| user['id'] }).to contain_exactly(new_approver.id, existing_approver.id)
         end
       end
 
       context 'groups are passed' do
-        let(:group_ids) { [new_group.id] }
+        let(:group_ids) { "#{new_group.id},#{other_group.id}" }
 
         it 'changes groups' do
           rule = json_response
 
-          expect(rule['eligible_approvers']).to match([hash_including('id' => new_approver.id)])
-          expect(rule['groups']).to match([hash_including('id' => new_group.id)])
+          expect(rule['eligible_approvers'].map { |approver| approver['id'] }).to contain_exactly(new_approver.id, other_approver.id)
+          expect(rule['groups'].map { |group| group['id'] }).to contain_exactly(new_group.id, other_group.id)
         end
       end
 

ee/spec/requests/api/merge_request_approvals_spec.rb

@@ -321,7 +321,7 @@ describe API::MergeRequestApprovals do
         it 'does not allow overriding approvers' do
           expect do
             put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
-              params: { approver_ids: [approver.id], approver_group_ids: [group.id] }
+              params: { approver_ids: approver.id.to_s, approver_group_ids: group.id.to_s }
           end.to not_change { merge_request.approvers.count }.and not_change { merge_request.approver_groups.count }
         end
       end
@@ -334,12 +334,12 @@ describe API::MergeRequestApprovals do
         it 'allows overriding approvers' do
           expect do
             put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
-              params: { approver_ids: [approver.id], approver_group_ids: [group.id] }
-          end.to change { merge_request.approvers.count }.from(0).to(1)
+              params: { approver_ids: "#{approver.id},#{user2.id}", approver_group_ids: "#{group.id}" }
+          end.to change { merge_request.approvers.count }.from(0).to(2)
             .and change { merge_request.approver_groups.count }.from(0).to(1)
 
           expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response['approvers'][0]['user']['username']).to eq(approver.username)
+          expect(json_response['approvers'].map { |approver| approver['user'] }.map { |user| user['username'] }).to contain_exactly(approver.username, user2.username)
           expect(json_response['approver_groups'][0]['group']['name']).to eq(group.name)
         end
 
@@ -349,7 +349,7 @@ describe API::MergeRequestApprovals do
 
           expect do
             put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
-              params: { approver_ids: [], approver_group_ids: [] }.to_json, headers: { CONTENT_TYPE: 'application/json' }
+              params: { approver_ids: '', approver_group_ids: '' }.to_json, headers: { CONTENT_TYPE: 'application/json' }
           end.to change { merge_request.approvers.count }.from(1).to(0)
             .and change { merge_request.approver_groups.count }.from(1).to(0)
 

ee/spec/requests/api/project_approval_rules_spec.rb

@@ -9,6 +9,7 @@ describe API::ProjectApprovalRules do
   let_it_be(:admin) { create(:user, :admin) }
   let_it_be(:project) { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) }
   let_it_be(:approver) { create(:user) }
+  let_it_be(:other_approver) { create(:user) }
 
   describe 'GET /projects/:id/approval_rules' do
     let(:url) { "/projects/#{project.id}/approval_rules" }

ee/spec/requests/api/project_approval_settings_spec.rb

@@ -9,6 +9,7 @@ describe API::ProjectApprovalSettings do
   let_it_be(:admin) { create(:user, :admin) }
   let_it_be(:project) { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) }
   let_it_be(:approver) { create(:user) }
+  let_it_be(:other_approver) { create(:user) }
 
   describe 'GET /projects/:id/approval_settings' do
     let(:url) { "/projects/#{project.id}/approval_settings" }

ee/spec/support/shared_examples/requests/api/project_approval_rules_api_shared_examples.rb

@@ -79,6 +79,7 @@ RSpec.shared_examples 'an API endpoint for updating project approval rule' do
   shared_examples 'a user with access' do
     before do
       project.add_developer(approver)
+      project.add_developer(other_approver)
     end
 
     context 'when protected_branch_ids param is present' do
@@ -117,10 +118,10 @@ RSpec.shared_examples 'an API endpoint for updating project approval rule' do
 
     it 'sets approvers' do
       expect do
-        put api(url, current_user), params: { users: [approver.id] }
-      end.to change { approval_rule.users.count }.from(0).to(1)
+        put api(url, current_user), params: { users: "#{approver.id},#{other_approver.id}" }
+      end.to change { approval_rule.users.count }.from(0).to(2)
 
-      expect(approval_rule.users).to contain_exactly(approver)
+      expect(approval_rule.users).to contain_exactly(approver, other_approver)
       expect(approval_rule.groups).to be_empty
 
       expect(response).to have_gitlab_http_status(:ok)