Commit 92d72b41 authored by Felipe Artur's avatar Felipe Artur

Fix backward keyset pagination for epics start_date/end_date

This fixes the ActiveRecord::IrreversibleOrderError when paginating
backwards with epics ordered by start_date or end_date.
parent 2701da3d
......@@ -5,9 +5,9 @@ module Types
graphql_name 'EpicSort'
description 'Roadmap sort values'
value 'start_date_desc', 'Start date at descending order.'
value 'start_date_asc', 'Start date at ascending order.'
value 'end_date_desc', 'End date at descending order.'
value 'end_date_asc', 'End date at ascending order.'
value 'start_date_desc', 'Start date at descending order.', value: :start_date_desc
value 'start_date_asc', 'Start date at ascending order.', value: :start_date_asc
value 'end_date_desc', 'End date at descending order.', value: :end_date_desc
value 'end_date_asc', 'End date at ascending order.', value: :end_date_asc
end
end
......@@ -88,19 +88,27 @@ module EE
end
scope :order_start_date_asc, -> do
reorder(::Gitlab::Database.nulls_last_order('start_date'), 'id DESC')
keyset_order = keyset_pagination_for(column_name: :start_date)
reorder(keyset_order)
end
scope :order_start_date_desc, -> do
keyset_order = keyset_pagination_for(column_name: :start_date, direction: 'DESC')
reorder(keyset_order)
end
scope :order_end_date_asc, -> do
reorder(::Gitlab::Database.nulls_last_order('end_date'), 'id DESC')
keyset_order = keyset_pagination_for(column_name: :end_date)
reorder(keyset_order)
end
scope :order_end_date_desc, -> do
reorder(::Gitlab::Database.nulls_last_order('end_date', 'DESC'), 'id DESC')
end
keyset_order = keyset_pagination_for(column_name: :end_date, direction: 'DESC')
scope :order_start_date_desc, -> do
reorder(::Gitlab::Database.nulls_last_order('start_date', 'DESC'), 'id DESC')
reorder(keyset_order)
end
scope :order_closed_date_desc, -> { reorder(closed_at: :desc) }
......@@ -149,6 +157,20 @@ module EE
select(selection).in_parents(node.parent_ids)
end
# This is being overriden from Issuable to be able to use
# keyset pagination, allowing queries with these
# ordering statements to be reversible on GraphQL.
def self.sort_by_attribute(method, excluded_labels: [])
case method.to_s
when 'start_date_asc' then order_start_date_asc
when 'start_date_desc' then order_start_date_desc
when 'end_date_asc' then order_end_date_asc
when 'end_date_desc' then order_end_date_desc
else
super
end
end
private
def set_fixed_start_date
......@@ -230,10 +252,10 @@ module EE
def simple_sorts
super.merge(
{
'start_date_asc' => -> { order_start_date_asc.with_order_id_desc },
'start_date_desc' => -> { order_start_date_desc.with_order_id_desc },
'end_date_asc' => -> { order_end_date_asc.with_order_id_desc },
'end_date_desc' => -> { order_end_date_desc.with_order_id_desc }
'start_date_asc' => -> { order_start_date_asc },
'start_date_desc' => -> { order_start_date_desc },
'end_date_asc' => -> { order_end_date_asc },
'end_date_desc' => -> { order_end_date_desc }
}
)
end
......@@ -290,6 +312,26 @@ module EE
records.map { |record| record.attributes.with_indifferent_access }
end
def keyset_pagination_for(column_name:, direction: 'ASC')
reverse_direction = direction == 'ASC' ? 'DESC' : 'ASC'
::Gitlab::Pagination::Keyset::Order.build([
::Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
attribute_name: column_name.to_s,
column_expression: ::Epic.arel_table[column_name],
order_expression: ::Gitlab::Database.nulls_last_order(column_name, direction),
reversed_order_expression: ::Gitlab::Database.nulls_last_order(column_name, reverse_direction),
order_direction: direction,
distinct: false,
nullable: :nulls_last
),
::Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
attribute_name: 'id',
order_expression: ::Epic.arel_table[:id].desc
)
])
end
end
def resource_parent
......
---
title: Fix backward keyset pagination for epics start_date/end_date
merge_request: 50579
author:
type: fixed
......@@ -7,13 +7,13 @@ require 'spec_helper'
RSpec.describe 'Epics through GroupQuery' do
include GraphqlHelpers
let(:user) { create(:user) }
let(:group) { create(:group) }
let(:project) { create(:project, :public, group: group) }
let(:label) { create(:label) }
let(:epic) { create(:labeled_epic, group: group, labels: [label]) }
let(:epics_data) { graphql_data['group']['epics']['edges'] }
let(:epic_data) { graphql_data['group']['epic'] }
let(:user) { create(:user) }
let_it_be(:group) { create(:group) }
let(:project) { create(:project, :public, group: group) }
let(:label) { create(:label) }
let(:epic) { create(:labeled_epic, group: group, labels: [label]) }
let(:epics_data) { graphql_data['group']['epics']['edges'] }
let(:epic_data) { graphql_data['group']['epic'] }
# similar to GET /groups/:id/epics
describe 'Get list of epics from a group' do
......@@ -60,14 +60,69 @@ RSpec.describe 'Epics through GroupQuery' do
end
context 'with multiple epics' do
let(:user2) { create(:user) }
let!(:epic) { create(:epic, group: group, state: :closed, created_at: 3.days.ago, updated_at: 2.days.ago) }
let!(:epic2) { create(:epic, author: user2, group: group, title: 'foo', description: 'bar', created_at: 2.days.ago, updated_at: 3.days.ago) }
let_it_be(:user2) { create(:user) }
let_it_be(:epic) { create(:epic, group: group, state: :closed, created_at: 3.days.ago, updated_at: 2.days.ago, start_date: 2.days.ago, end_date: 4.days.ago) }
let_it_be(:epic2) { create(:epic, author: user2, group: group, title: 'foo', description: 'bar', created_at: 2.days.ago, updated_at: 3.days.ago, start_date: 3.days.ago, end_date: 3.days.ago ) }
before do
stub_licensed_features(epics: true)
end
context 'with sort and pagination' do
let_it_be(:epic3) { create(:epic, group: group, start_date: 4.days.ago, end_date: 7.days.ago ) }
let_it_be(:epic4) { create(:epic, group: group, start_date: 5.days.ago, end_date: 6.days.ago ) }
let(:current_user) { user }
let(:data_path) { [:group, :epics] }
def pagination_query(params)
query =
<<~QUERY
epics(#{params}) {
#{page_info}
nodes { id }
}
QUERY
graphql_query_for('group', { 'fullPath' => group.full_path }, ['epicsEnabled', query])
end
def global_ids(*epics)
epics.map { |epic| global_id_of(epic) }
end
context 'with start_date_asc' do
it_behaves_like 'sorted paginated query', is_reversible: true do
let(:sort_param) { :start_date_asc }
let(:first_param) { 2 }
let(:expected_results) { global_ids(epic4, epic3, epic2, epic) }
end
end
context 'with start_date_desc' do
it_behaves_like 'sorted paginated query', is_reversible: true do
let(:sort_param) { :start_date_desc }
let(:first_param) { 2 }
let(:expected_results) { global_ids(epic, epic2, epic3, epic4) }
end
end
context 'with end_date_asc' do
it_behaves_like 'sorted paginated query', is_reversible: true do
let(:sort_param) { :end_date_asc }
let(:first_param) { 2 }
let(:expected_results) { global_ids(epic3, epic4, epic, epic2) }
end
end
context 'with end_date_desc' do
it_behaves_like 'sorted paginated query', is_reversible: true do
let(:sort_param) { :end_date_desc }
let(:first_param) { 2 }
let(:expected_results) { global_ids(epic2, epic, epic4, epic3) }
end
end
end
it 'sorts by created_at descending by default' do
post_graphql(query, current_user: user)
......
......@@ -55,14 +55,14 @@ module Gitlab
# scope :created_at_ordered, -> {
# keyset_order = Gitlab::Pagination::Keyset::Order.build([
# Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
# attribute: :created_at,
# attribute_name: :created_at,
# column_expression: Project.arel_table[:created_at],
# order_expression: Project.arel_table[:created_at].asc,
# distinct: false, # values in the column are not unique
# nullable: :nulls_last # we might see NULL values (bottom)
# ),
# Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
# attribute: :id,
# attribute_name: :id,
# order_expression: Project.arel_table[:id].asc
# )
# ])
......
......@@ -44,7 +44,7 @@
# end
# end
#
RSpec.shared_examples 'sorted paginated query' do
RSpec.shared_examples 'sorted paginated query' do |conditions = {}|
# Provided as a convenience when constructing queries using string concatenation
let(:page_info) { 'pageInfo { startCursor endCursor }' }
# Convenience for using default implementation of pagination_results_data
......@@ -123,6 +123,16 @@ RSpec.shared_examples 'sorted paginated query' do
expect(results).to eq first_page
end
end
context 'when last and sort params are present', if: conditions[:is_reversible] do
let(:params) { sort_argument.merge(last: 1) }
it 'fetches last elements without error' do
post_graphql(pagination_query(params), current_user: current_user)
expect(results.first).to eq(expected_results.last)
end
end
end
end
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