From 33f5a553b626b17c65b60b5d9d142aea59d419ff Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Thu, 26 Oct 2023 10:38:04 +0200 Subject: [PATCH] fix(graphql): fix pagination with order desc --- app/graphql/connections/cursor_connection.rb | 39 +++++++++++------- .../graphql_controller_stored_queries_spec.rb | 40 +++++++++++++++++-- .../connections/cursor_connection_spec.rb | 12 ------ 3 files changed, 60 insertions(+), 31 deletions(-) diff --git a/app/graphql/connections/cursor_connection.rb b/app/graphql/connections/cursor_connection.rb index e9471a6f3..fa6df5e9d 100644 --- a/app/graphql/connections/cursor_connection.rb +++ b/app/graphql/connections/cursor_connection.rb @@ -42,23 +42,10 @@ module Connections # -> d5, d6 # # si after ou before present, last ou first donne juste limit - # - # order: - # order, ne sert rien si after ou before - # - # first: 2, order: desc => last: 2 - # -> d5, d6 - # - # last: 2, order: desc => first 2 - # -> d1, d2 - def limit_and_inverted(first: nil, last: nil, after: nil, before: nil, order: nil) + def limit_and_inverted(first: nil, last: nil, after: nil, before: nil) limit = [first, last, max_page_size].compact.min + 1 inverted = last.present? || before.present? - if order == :desc && after.nil? && before.nil? - inverted = !inverted - end - [limit, inverted] end @@ -73,8 +60,10 @@ module Connections def load_nodes @nodes ||= begin ensure_valid_params + limit, inverted = limit_and_inverted(first:, last:, after:, before:) + + return load_nodes_deprecated_order(limit, inverted) if @deprecated_order == :desc - limit, inverted = limit_and_inverted(first:, last:, after:, before:, order: @deprecated_order) expected_size = limit - 1 nodes = resolve_nodes(limit:, before:, after:, inverted:) @@ -89,6 +78,22 @@ module Connections end end + def load_nodes_deprecated_order(limit, inverted) + expected_size = limit - 1 + + if @deprecated_order == :desc && before.nil? + inverted = !inverted + end + + nodes = resolve_nodes(limit:, before: after, after: before, inverted:) + + result_size = nodes.size + @has_next_page = previous_page?(before, result_size, limit, inverted) + @has_previous_page = next_page?(after, result_size, limit, inverted) + + nodes.first(expected_size) + end + def ensure_valid_params if first.present? && last.present? raise GraphQL::ExecutionError.new('Arguments "first" and "last" are exclusive', extensions: { code: :bad_request }) @@ -105,6 +110,10 @@ module Connections if last.present? && last < 0 raise GraphQL::ExecutionError.new('Argument "last" must be a non-negative integer', extensions: { code: :bad_request }) end + + if last.present? && @deprecated_order == :desc + raise GraphQL::ExecutionError.new('Argument "last" is not supported with order "desc"', extensions: { code: :bad_request }) + end end def timestamp_and_id_from_cursor(cursor) diff --git a/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb b/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb index 1e9776844..13d88a767 100644 --- a/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb +++ b/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb @@ -197,17 +197,49 @@ describe API::V2::GraphqlController do context 'with deprecated order' do let(:variables) { { demarcheNumber: procedure.id, includeDossiers: true, first: 2, order: 'DESC' } } - let(:start_cursor) { cursor_for(dossier2, order_column) } - let(:end_cursor) { cursor_for(dossier3, order_column) } + let(:start_cursor) { cursor_for(dossier3, order_column) } + let(:end_cursor) { cursor_for(dossier2, order_column) } it { expect(gql_errors).to be_nil expect(gql_data[:demarche][:dossiers][:nodes].size).to eq(2) - expect(gql_data[:demarche][:dossiers][:pageInfo][:hasNextPage]).to be_falsey - expect(gql_data[:demarche][:dossiers][:pageInfo][:hasPreviousPage]).to be_truthy + expect(gql_data[:demarche][:dossiers][:pageInfo][:hasNextPage]).to be_truthy + expect(gql_data[:demarche][:dossiers][:pageInfo][:hasPreviousPage]).to be_falsey expect(gql_data[:demarche][:dossiers][:pageInfo][:startCursor]).to eq(start_cursor) expect(gql_data[:demarche][:dossiers][:pageInfo][:endCursor]).to eq(end_cursor) } + + context 'after' do + let(:variables) { { demarcheNumber: procedure.id, includeDossiers: true, first: 2, after: current_cursor, order: 'DESC' } } + let(:current_cursor) { cursor_for(dossier2, order_column) } + let(:start_cursor) { cursor_for(dossier1, order_column) } + let(:end_cursor) { cursor_for(dossier, order_column) } + + it { + expect(gql_errors).to be_nil + expect(gql_data[:demarche][:dossiers][:nodes].size).to eq(2) + expect(gql_data[:demarche][:dossiers][:pageInfo][:hasNextPage]).to be_falsey + expect(gql_data[:demarche][:dossiers][:pageInfo][:hasPreviousPage]).to be_truthy + expect(gql_data[:demarche][:dossiers][:pageInfo][:startCursor]).to eq(start_cursor) + expect(gql_data[:demarche][:dossiers][:pageInfo][:endCursor]).to eq(end_cursor) + } + end + + context 'before' do + let(:variables) { { demarcheNumber: procedure.id, includeDossiers: true, first: 2, before: current_cursor, order: 'DESC' } } + let(:current_cursor) { cursor_for(dossier1, order_column) } + let(:start_cursor) { cursor_for(dossier3, order_column) } + let(:end_cursor) { cursor_for(dossier2, order_column) } + + it { + expect(gql_errors).to be_nil + expect(gql_data[:demarche][:dossiers][:nodes].size).to eq(2) + expect(gql_data[:demarche][:dossiers][:pageInfo][:hasNextPage]).to be_truthy + expect(gql_data[:demarche][:dossiers][:pageInfo][:hasPreviousPage]).to be_falsey + expect(gql_data[:demarche][:dossiers][:pageInfo][:startCursor]).to eq(start_cursor) + expect(gql_data[:demarche][:dossiers][:pageInfo][:endCursor]).to eq(end_cursor) + } + end end context 'after' do diff --git a/spec/graphql/connections/cursor_connection_spec.rb b/spec/graphql/connections/cursor_connection_spec.rb index 6952eb8fc..8d88fc4fb 100644 --- a/spec/graphql/connections/cursor_connection_spec.rb +++ b/spec/graphql/connections/cursor_connection_spec.rb @@ -21,12 +21,6 @@ RSpec.describe Connections::CursorConnection do it { is_expected.to eq(limit: 3, inverted: false) } end - context 'when asked for 2 first elements, in order desc' do - let(:args) { { first: 2, order: :desc } } - - it { is_expected.to eq(limit: 3, inverted: true) } - end - context 'when exceeding the max_page_size' do let(:args) { { first: max_page_size + 1 } } @@ -39,12 +33,6 @@ RSpec.describe Connections::CursorConnection do it { is_expected.to eq(limit: 3, inverted: true) } end - context 'when asked for 2 last elements, in order desc' do - let(:args) { { last: 2, order: :desc } } - - it { is_expected.to eq(limit: 3, inverted: false) } - end - context '' do let(:args) { { after: :after, first: 2 } }