Merge pull request #9644 from tchak/graphql-fix-pagination-with-order-desc

fix(graphql): fix pagination with order desc

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
-    #
+    def limit_and_inverted(first: nil, last: nil, after: nil, before: nil)
-    # 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)
       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)

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(:start_cursor) { cursor_for(dossier3, order_column) }
-              let(:end_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][:hasNextPage]).to be_truthy
-                expect(gql_data[:demarche][:dossiers][:pageInfo][:hasPreviousPage]).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

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 } }