Merge branch 'sh-fix-ambiguous-column-with-cte' into 'master'

Fix ambiguous column errors when sorting dashboard with a CTE

See merge request gitlab-org/gitlab!24562

app/models/concerns/sortable.rb

@@ -8,13 +8,13 @@ module Sortable
   extend ActiveSupport::Concern
 
   included do
-    scope :with_order_id_desc, -> { order(id: :desc) }
-    scope :order_id_desc, -> { reorder(id: :desc) }
-    scope :order_id_asc, -> { reorder(id: :asc) }
-    scope :order_created_desc, -> { reorder(created_at: :desc) }
-    scope :order_created_asc, -> { reorder(created_at: :asc) }
-    scope :order_updated_desc, -> { reorder(updated_at: :desc) }
-    scope :order_updated_asc, -> { reorder(updated_at: :asc) }
+    scope :with_order_id_desc, -> { order(self.arel_table['id'].desc) }
+    scope :order_id_desc, -> { reorder(self.arel_table['id'].desc) }
+    scope :order_id_asc, -> { reorder(self.arel_table['id'].asc) }
+    scope :order_created_desc, -> { reorder(self.arel_table['created_at'].desc) }
+    scope :order_created_asc, -> { reorder(self.arel_table['created_at'].asc) }
+    scope :order_updated_desc, -> { reorder(self.arel_table['updated_at'].desc) }
+    scope :order_updated_asc, -> { reorder(self.arel_table['updated_at'].asc) }
     scope :order_name_asc, -> { reorder(Arel::Nodes::Ascending.new(arel_table[:name].lower)) }
     scope :order_name_desc, -> { reorder(Arel::Nodes::Descending.new(arel_table[:name].lower)) }
   end

app/models/project.rb

@@ -395,11 +395,11 @@ class Project < ApplicationRecord
 
   # last_activity_at is throttled every minute, but last_repository_updated_at is updated with every push
   scope :sorted_by_activity, -> { reorder(Arel.sql("GREATEST(COALESCE(last_activity_at, '1970-01-01'), COALESCE(last_repository_updated_at, '1970-01-01')) DESC")) }
-  scope :sorted_by_stars_desc, -> { reorder(star_count: :desc) }
-  scope :sorted_by_stars_asc, -> { reorder(star_count: :asc) }
+  scope :sorted_by_stars_desc, -> { reorder(self.arel_table['star_count'].desc) }
+  scope :sorted_by_stars_asc, -> { reorder(self.arel_table['star_count'].asc) }
   scope :sorted_by_name_asc_limited, ->(limit) { reorder(name: :asc).limit(limit) }
   # Sometimes queries (e.g. using CTEs) require explicit disambiguation with table name
-  scope :projects_order_id_desc, -> { reorder("#{table_name}.id DESC") }
+  scope :projects_order_id_desc, -> { reorder(self.arel_table['id'].desc) }
 
   scope :in_namespace, ->(namespace_ids) { where(namespace_id: namespace_ids) }
   scope :personal, ->(user) { where(namespace_id: user.namespace_id) }
@@ -595,9 +595,9 @@ class Project < ApplicationRecord
         # pass a string to avoid AR adding the table name
         reorder('project_statistics.storage_size DESC, projects.id DESC')
       when 'latest_activity_desc'
-        reorder(last_activity_at: :desc)
+        reorder(self.arel_table['last_activity_at'].desc)
       when 'latest_activity_asc'
-        reorder(last_activity_at: :asc)
+        reorder(self.arel_table['last_activity_at'].asc)
       when 'stars_desc'
         sorted_by_stars_desc
       when 'stars_asc'

spec/controllers/dashboard/projects_controller_spec.rb

@@ -11,7 +11,14 @@ describe Dashboard::ProjectsController do
     end
 
     context 'user logged in' do
-      let(:user) { create(:user) }
+      let_it_be(:user) { create(:user) }
+      let_it_be(:project) { create(:project) }
+      let_it_be(:project2) { create(:project) }
+
+      before_all do
+        project.add_developer(user)
+        project2.add_developer(user)
+      end
 
       before do
         sign_in(user)
@@ -28,12 +35,7 @@ describe Dashboard::ProjectsController do
       end
 
       it 'orders the projects by last activity by default' do
-        project = create(:project)
-        project.add_developer(user)
         project.update!(last_repository_updated_at: 3.days.ago, last_activity_at: 3.days.ago)
-
-        project2 = create(:project)
-        project2.add_developer(user)
         project2.update!(last_repository_updated_at: 10.days.ago, last_activity_at: 10.days.ago)
 
         get :index
@@ -42,12 +44,27 @@ describe Dashboard::ProjectsController do
       end
 
       context 'project sorting' do
-        let(:project) { create(:project) }
-
         it_behaves_like 'set sort order from user preference' do
           let(:sorting_param) { 'created_asc' }
         end
       end
+
+      context 'with search and sort parameters' do
+        render_views
+
+        shared_examples 'search and sort parameters' do |sort|
+          it 'returns a single project with no ambiguous column errors' do
+            get :index, params: { name: project2.name, sort: sort }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(assigns(:projects)).to eq([project2])
+          end
+        end
+
+        %w[latest_activity_desc latest_activity_asc stars_desc stars_asc created_desc].each do |sort|
+          it_behaves_like 'search and sort parameters', sort
+        end
+      end
     end
   end
 

spec/models/concerns/sortable_spec.rb

@@ -4,17 +4,18 @@ require 'spec_helper'
 
 describe Sortable do
   describe '.order_by' do
+    let(:arel_table) { Group.arel_table }
     let(:relation) { Group.all }
 
     describe 'ordering by id' do
       it 'ascending' do
-        expect(relation).to receive(:reorder).with(id: :asc)
+        expect(relation).to receive(:reorder).with(arel_table['id'].asc)
 
         relation.order_by('id_asc')
       end
 
       it 'descending' do
-        expect(relation).to receive(:reorder).with(id: :desc)
+        expect(relation).to receive(:reorder).with(arel_table['id'].desc)
 
         relation.order_by('id_desc')
       end
@@ -22,19 +23,19 @@ describe Sortable do
 
     describe 'ordering by created day' do
       it 'ascending' do
-        expect(relation).to receive(:reorder).with(created_at: :asc)
+        expect(relation).to receive(:reorder).with(arel_table['created_at'].asc)
 
         relation.order_by('created_asc')
       end
 
       it 'descending' do
-        expect(relation).to receive(:reorder).with(created_at: :desc)
+        expect(relation).to receive(:reorder).with(arel_table['created_at'].desc)
 
         relation.order_by('created_desc')
       end
 
       it 'order by "date"' do
-        expect(relation).to receive(:reorder).with(created_at: :desc)
+        expect(relation).to receive(:reorder).with(arel_table['created_at'].desc)
 
         relation.order_by('created_date')
       end
@@ -66,13 +67,13 @@ describe Sortable do
 
     describe 'ordering by Updated Time' do
       it 'ascending' do
-        expect(relation).to receive(:reorder).with(updated_at: :asc)
+        expect(relation).to receive(:reorder).with(arel_table['updated_at'].asc)
 
         relation.order_by('updated_asc')
       end
 
       it 'descending' do
-        expect(relation).to receive(:reorder).with(updated_at: :desc)
+        expect(relation).to receive(:reorder).with(arel_table['updated_at'].desc)
 
         relation.order_by('updated_desc')
       end