Merge pull request #3643 from evazion/fix-3642

Fix #3642: Issues with sequential pagination

app/logical/danbooru/paginator/active_record_extension.rb

@@ -37,17 +37,17 @@ module Danbooru
             c = c.where("#{table_name}.id < ?", before_id.to_i)
           end
 
-          c.reorder("#{table_name}.id desc").tap do |obj|
-            obj.extend(SequentialCollectionExtension)
-            obj.sequential_paginator_mode = :before
-          end
+          c = c.reorder("#{table_name}.id desc")
+          c = c.extending(SequentialCollectionExtension)
+          c.sequential_paginator_mode = :before
+          c
         end
 
         def paginate_sequential_after(after_id)
-          limit(records_per_page + 1).where("#{table_name}.id > ?", after_id.to_i).reorder("#{table_name}.id asc").tap do |obj|
-            obj.extend(SequentialCollectionExtension)
-            obj.sequential_paginator_mode = :after
-          end
+          c = limit(records_per_page + 1).where("#{table_name}.id > ?", after_id.to_i).reorder("#{table_name}.id asc")
+          c = c.extending(SequentialCollectionExtension)
+          c.sequential_paginator_mode = :after
+          c
         end
 
         def paginate_numbered(page)
@@ -57,8 +57,7 @@ module Danbooru
             raise ::Danbooru::Paginator::PaginationError.new("You cannot go beyond page #{Danbooru.config.max_numbered_pages}. Please narrow your search terms.")
           end
 
-          limit(records_per_page).offset((page - 1) * records_per_page).tap do |obj|
-            obj.extend(NumberedCollectionExtension)
+          extending(NumberedCollectionExtension).limit(records_per_page).offset((page - 1) * records_per_page).tap do |obj|
             if records_per_page > 0
               obj.total_pages = (obj.total_count.to_f / records_per_page).ceil
             else

app/logical/danbooru/paginator/sequential_collection_extension.rb

@@ -19,7 +19,10 @@ module Danbooru
         end
       end
 
-      def to_a
+      # XXX Hack: in sequential pagination we fetch one more record than we need
+      # so that we can tell when we're on the first or last page. Here we override
+      # a rails internal method to discard that extra record. See #2044, #3642.
+      def records
         if sequential_paginator_mode == :before
           super.first(records_per_page)
         else

test/unit/paginator_test.rb

@@ -0,0 +1,34 @@
+require 'test_helper'
+
+class PaginatorTest < ActiveSupport::TestCase
+  setup do
+    @posts = FactoryBot.create_list(:post, 5)
+  end
+
+  context "sequential pagination (before)" do
+    should "return the correct set of records" do
+      expected_posts = Post.limit(3).order(id: :desc)
+      posts = Post.paginate("b9999999", limit: 3)
+
+      assert_equal(expected_posts.map(&:id), posts.map(&:id))
+    end
+  end
+
+  context "sequential pagination (after)" do
+    should "return the correct set of records" do
+      expected_posts = Post.limit(3).order(id: :asc).reverse
+      posts = Post.paginate("a0", limit: 3)
+
+      assert_equal(expected_posts.map(&:id), posts.map(&:id))
+    end
+  end
+
+  context "numbered pagination" do
+    should "return the correct set of records" do
+      expected_posts = Post.limit(3).order(id: :desc)
+      posts = Post.order(id: :desc).paginate("1", limit: 3)
+
+      assert_equal(expected_posts.map(&:id), posts.map(&:id))
+    end
+  end
+end