Add id as second sort parameter for group sort by name

Adding primary key to database query order rule
generates deterministic sort result and thus pagination.
This is needed because subgroups can have identical names.
Signed-off-by: Marko, Peter <peter.marko@siemens.com>

changelogs/unreleased/fix-groups-api-ordering.yml

+title: Fixed pagination of groups API
+merge_request: 19665
+author: Marko, Peter
+type: added

lib/api/groups.rb

@@ -46,7 +46,9 @@ module API
         groups = GroupsFinder.new(current_user, find_params).execute
         groups = groups.search(params[:search]) if params[:search].present?
         groups = groups.where.not(id: params[:skip_groups]) if params[:skip_groups].present?
-        groups = groups.reorder(params[:order_by] => params[:sort])
+        order_options = { params[:order_by] => params[:sort] }
+        order_options["id"] ||= "asc"
+        groups = groups.reorder(order_options)
 
         groups
       end

spec/requests/api/groups_spec.rb

@@ -138,12 +138,15 @@ describe API::Groups do
 
     context "when using sorting" do
       let(:group3) { create(:group, name: "a#{group1.name}", path: "z#{group1.path}") }
-      let(:group4) { create(:group, name: "z#{group1.name}", path: "y#{group1.path}") }
+      let(:group4) { create(:group, name: "same-name", path: "y#{group1.path}") }
+      let(:group5) { create(:group, name: "same-name") }
       let(:response_groups) { json_response.map { |group| group['name'] } }
+      let(:response_groups_ids) { json_response.map { |group| group['id'] } }
 
       before do
         group3.add_owner(user1)
         group4.add_owner(user1)
+        group5.add_owner(user1)
       end
 
       it "sorts by name ascending by default" do
@@ -181,6 +184,33 @@ describe API::Groups do
         expect(json_response).to be_an Array
         expect(response_groups).to eq(Group.visible_to_user(user1).order(:id).pluck(:name))
       end
+
+      it "sorts also by descending id with pagination fix" do
+        get api("/groups", user1), order_by: "id", sort: "desc"
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        expect(response_groups).to eq(Group.visible_to_user(user1).order(id: :desc).pluck(:name))
+      end
+
+      it "sorts identical keys by id for good pagination" do
+        get api("/groups", user1), search: "same-name", order_by: "name"
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        expect(response_groups_ids).to eq(Group.select { |group| group['name'] == 'same-name' }.map { |group| group['id'] }.sort)
+      end
+
+      it "sorts descending identical keys by id for good pagination" do
+        get api("/groups", user1), search: "same-name", order_by: "name", sort: "desc"
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        expect(response_groups_ids).to eq(Group.select { |group| group['name'] == 'same-name' }.map { |group| group['id'] }.sort)
+      end
     end
 
     context 'when using owned in the request' do