Commit ee0e5ff5 authored by Stan Hu's avatar Stan Hu

Fix ambiguous column errors when sorting dashboard with a CTE

In https://gitlab.com/gitlab-org/gitlab/merge_requests/23754 and
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24408, we
added a CTE optimization fence to ensure loading a user's projects is
always fast when the `/dashboard/projects` path is loaded.

However, when sorting by a different column type (e.g. last created),
PostgreSQL would raise an ambiguous column error because it wasn't clear
which `created_at` column was needed.

To fix this problem, we should be explicit that we want to order by
`projects.created_at`.

Foo
parent 961ffdb3
......@@ -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
......
......@@ -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'
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment