Fix project_path sorting scopes in Packages::Package model

Package Registry pagination returns empty results on group pages when ordered by project_path. In this MR, we fix the sorting scopes in the Packages::Package model to return correct results.

Changelog: fixed

app/models/packages/package.rb

@@ -208,15 +208,23 @@ class Packages::Package < ApplicationRecord
   scope :order_by_package_file, -> { joins(:package_files).order('packages_package_files.created_at ASC') }
 
   scope :order_project_path, -> do
-    keyset_order = keyset_pagination_order(join_class: Project, column_name: :path, direction: :asc)
-
-    joins(:project).reorder(keyset_order)
+    build_keyset_order_on_joined_column(
+      scope: joins(:project),
+      attribute_name: 'project_path',
+      column: Project.arel_table[:path],
+      direction: :asc,
+      nullable: :nulls_last
+    )
   end
 
   scope :order_project_path_desc, -> do
-    keyset_order = keyset_pagination_order(join_class: Project, column_name: :path, direction: :desc)
-
-    joins(:project).reorder(keyset_order)
+    build_keyset_order_on_joined_column(
+      scope: joins(:project),
+      attribute_name: 'project_path',
+      column: Project.arel_table[:path],
+      direction: :desc,
+      nullable: :nulls_first
+    )
   end
 
   def self.inheritance_column = 'package_type'
@@ -294,33 +302,6 @@ def self.sort_by_attribute(method)
     end
   end
 
-  def self.keyset_pagination_order(join_class:, column_name:, direction: :asc)
-    join_table = join_class.table_name
-    asc_order_expression = join_class.arel_table[column_name].asc.nulls_last
-    desc_order_expression = join_class.arel_table[column_name].desc.nulls_first
-    order_direction = direction == :asc ? asc_order_expression : desc_order_expression
-    reverse_order_direction = direction == :asc ? desc_order_expression : asc_order_expression
-    arel_order_classes = ::Gitlab::Pagination::Keyset::ColumnOrderDefinition::AREL_ORDER_CLASSES.invert
-
-    ::Gitlab::Pagination::Keyset::Order.build(
-      [
-        ::Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
-          attribute_name: "#{join_table}_#{column_name}",
-          column_expression: join_class.arel_table[column_name],
-          order_expression: order_direction,
-          reversed_order_expression: reverse_order_direction,
-          order_direction: direction,
-          distinct: false,
-          add_to_projections: true
-        ),
-        ::Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
-          attribute_name: 'id',
-          order_expression: arel_order_classes[direction].new(Packages::Package.arel_table[:id]),
-          add_to_projections: true
-        )
-      ])
-  end
-
   def versions
     project.packages
            .preload_pipelines

spec/graphql/resolvers/group_packages_resolver_spec.rb

@@ -2,10 +2,9 @@
 
 require 'spec_helper'
 
-RSpec.describe 'Resolvers::GroupPackagesResolver' do
+RSpec.describe Resolvers::GroupPackagesResolver, feature_category: :package_registry do
   include GraphqlHelpers
 
-  let_it_be(:described_class) { Resolvers::GroupPackagesResolver }
   let_it_be(:user) { create(:user) }
   let_it_be(:group) { create(:group, :public) }
   let_it_be(:project) { create(:project, :public, group: group, path: 'a') }
@@ -26,16 +25,16 @@
       let_it_be(:package3) { create(:package, project: project) }
       let_it_be(:package4) { create(:package, project: project2) }
 
-      context 'filter by package_name' do
+      context 'when sorting desc' do
         let(:args) { { sort: 'PROJECT_PATH_DESC' } }
 
         it { is_expected.to eq([package4, package2, package3, package]) }
       end
 
-      context 'filter by package_type' do
+      context 'when sorting asc' do
         let(:args) { { sort: 'PROJECT_PATH_ASC' } }
 
-        it { is_expected.to eq([package, package3, package2, package4]) }
+        it { is_expected.to eq([package3, package, package4, package2]) }
       end
     end
   end

spec/models/packages/package_spec.rb

@@ -88,6 +88,8 @@
     RSpec.shared_examples 'package sorting by attribute' do |order_by|
       subject { described_class.where(id: packages.map(&:id)).sort_by_attribute("#{order_by}_#{sort}").to_a }
 
+      let(:packages_desc) { packages.reverse }
+
       context "sorting by #{order_by}" do
         context 'ascending order' do
           let(:sort) { 'asc' }
@@ -98,7 +100,7 @@
         context 'descending order' do
           let(:sort) { 'desc' }
 
-          it { is_expected.to eq packages.reverse }
+          it { is_expected.to eq(packages_desc) }
         end
       end
     end
@@ -123,7 +125,8 @@
       let_it_be(:another_project) { create(:project, :public, namespace: group, name: 'project B', path: 'project-b') }
       let_it_be(:package4) { create(:npm_package, project: another_project, version: '3.1.0', name: "@#{project.root_namespace.path}/bar") }
 
-      let(:packages) { [package1, package2, package3, package4] }
+      let(:packages) { [package3, package2, package1, package4] }
+      let(:packages_desc) { [package4, package3, package2, package1] }
     end
   end
 
@@ -1056,8 +1059,8 @@
   end
 
   context 'sorting' do
-    let_it_be(:project) { create(:project, name: 'aaa') }
-    let_it_be(:project2) { create(:project, name: 'bbb') }
+    let_it_be(:project) { create(:project, path: 'aaa') }
+    let_it_be(:project2) { create(:project, path: 'bbb') }
     let_it_be(:package1) { create(:package, project: project) }
     let_it_be(:package2) { create(:package, project: project2) }
 
@@ -1073,20 +1076,13 @@
       let_it_be(:package3) { create(:package, project: project2) }
       let_it_be(:package4) { create(:package, project: project) }
 
-      shared_examples 'order_project_path scope' do
-        it 'orders packages by their projects path asc, then package id asc' do
-          expect(described_class.order_project_path).to eq([package1, package4, package2, package3])
-        end
+      it 'orders packages by their projects path asc, then package id desc' do
+        expect(described_class.order_project_path).to eq([package4, package1, package3, package2])
       end
 
-      shared_examples 'order_project_path_desc scope' do
-        it 'orders packages by their projects path desc, then package id desc' do
-          expect(described_class.order_project_path_desc).to eq([package3, package2, package4, package1])
-        end
+      it 'orders packages by their projects path desc, then package id desc' do
+        expect(described_class.order_project_path_desc).to eq([package3, package2, package4, package1])
       end
-
-      it_behaves_like 'order_project_path scope'
-      it_behaves_like 'order_project_path_desc scope'
     end
   end
 
@@ -1104,33 +1100,6 @@
     end
   end
 
-  describe '.keyset_pagination_order' do
-    let(:join_class) { nil }
-    let(:column_name) { nil }
-    let(:direction) { nil }
-
-    subject { described_class.keyset_pagination_order(join_class: join_class, column_name: column_name, direction: direction) }
-
-    it { expect { subject }.to raise_error(NoMethodError) }
-
-    context 'with valid params' do
-      let(:join_class) { Project }
-      let(:column_name) { :name }
-
-      context 'ascending direction' do
-        let(:direction) { :asc }
-
-        it { is_expected.to eq('"projects"."name" ASC NULLS LAST, "packages_packages"."id" ASC') }
-      end
-
-      context 'descending direction' do
-        let(:direction) { :desc }
-
-        it { is_expected.to eq('"projects"."name" DESC NULLS FIRST, "packages_packages"."id" DESC') }
-      end
-    end
-  end
-
   describe '.preload_tags' do
     let_it_be(:package) { create(:npm_package) }
     let_it_be(:tags) { create_list(:packages_tag, 2, package: package) }

spec/requests/api/group_packages_spec.rb

@@ -57,7 +57,8 @@
         let(:another_project) { create(:project, :public, namespace: group, name: 'project B') }
         let!(:package4) { create(:npm_package, project: another_project, version: '3.1.0', name: "@#{project.root_namespace.path}/bar") }
 
-        let(:packages) { [package1, package2, package3, package4] }
+        let(:packages) { [package3, package2, package1, package4] }
+        let(:package_ids_desc) { [package4.id, package3.id, package2.id, package1.id] }
       end
     end
 

spec/support/shared_examples/services/packages_shared_examples.rb

@@ -131,6 +131,8 @@
 RSpec.shared_examples 'package sorting' do |order_by|
   subject { get api(url), params: { sort: sort, order_by: order_by } }
 
+  let(:package_ids_desc) { packages.reverse.map(&:id) }
+
   context "sorting by #{order_by}" do
     context 'ascending order' do
       let(:sort) { 'asc' }
@@ -148,7 +150,7 @@
       it 'returns the sorted packages' do
         subject
 
-        expect(json_response.pluck('id')).to eq(packages.reverse.map(&:id))
+        expect(json_response.pluck('id')).to eq(package_ids_desc)
       end
     end
   end