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>

3 files changed, 38 insertions, 2 deletions

diff --git a/changelogs/unreleased/fix-groups-api-ordering.yml b/changelogs/unreleased/fix-groups-api-ordering.yml
new file mode 100644
index 00000000000..3a6a7f84356
--- /dev/null
+++ b/changelogs/unreleased/fix-groups-api-ordering.yml
@@ -0,0 +1,4 @@
+title: Fixed pagination of groups API
+merge_request: 19665
+author: Marko, Peter
+type: added
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 9d0b55c35c7..c7f41aba854 100644
--- a/lib/api/groups.rb
+++ b/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
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index f5218ebfac2..da23fdd7dca 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/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