pagination: refactor to avoid counting pages in API.

Previously the page-based (numbered) paginator would always count the
total_pages, even in API calls when it wasn't needed. This could be very
slow in some cases. Refactor so that total_pages isn't calculated unless
it's called.

While we're at it, refactor to condense all the sequential vs. numbered
pagination logic into one module. This incidentally fixes a couple more
bugs:

* "page=b0" returned all pages rather than nothing.
* Bad parameters like "page=blaha123" and "page=a123blah" were accepted.

app/controllers/application_controller.rb

@@ -62,7 +62,7 @@ class ApplicationController < ActionController::Base
       render_error_page(405, exception)
     when ActionController::UnknownFormat, ActionView::MissingTemplate
       render_error_page(406, exception, message: "#{request.format.to_s} is not a supported format for this page")
-    when Danbooru::Paginator::PaginationError
+    when PaginationExtension::PaginationError
       render_error_page(410, exception, template: "static/pagination_error", message: "You cannot go beyond page #{Danbooru.config.max_numbered_pages}.")
     when Post::SearchError
       render_error_page(422, exception, template: "static/tag_limit_error", message: "You cannot search for more than #{CurrentUser.tag_query_limit} tags at a time.")

app/controllers/delayed_jobs_controller.rb

@@ -3,7 +3,7 @@ class DelayedJobsController < ApplicationController
   before_action :admin_only, except: [:index]
 
   def index
-    @delayed_jobs = Delayed::Job.order("run_at asc").paginate(params[:page], :limit => params[:limit])
+    @delayed_jobs = Delayed::Job.order("run_at asc").extending(PaginationExtension).paginate(params[:page], :limit => params[:limit])
     respond_with(@delayed_jobs)
   end
 

app/logical/danbooru/paginator/active_record_extension.rb

@@ -1,119 +0,0 @@
-require 'active_support/concern'
-
-module Danbooru
-  module Paginator
-    module ActiveRecordExtension
-      extend ActiveSupport::Concern
-
-      module ClassMethods
-        def paginate(page, options = {})
-          @paginator_options = options
-
-          if use_sequential_paginator?(page)
-            paginate_sequential(page)
-          else
-            paginate_numbered(page)
-          end
-        end
-
-        def use_sequential_paginator?(page)
-          page =~ /[ab]\d+/i
-        end
-
-        def paginate_sequential(page)
-          if page =~ /b(\d+)/
-            paginate_sequential_before($1)
-          elsif page =~ /a(\d+)/
-            paginate_sequential_after($1)
-          else
-            paginate_sequential_before
-          end
-        end
-
-        def paginate_sequential_before(before_id = nil)
-          c = limit(records_per_page + 1)
-
-          if before_id.to_i > 0
-            c = c.where("#{table_name}.id < ?", before_id.to_i)
-          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)
-          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)
-          page = [page.to_i, 1].max
-
-          if page > Danbooru.config.max_numbered_pages
-            raise ::Danbooru::Paginator::PaginationError
-          end
-
-          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
-              obj.total_pages = 1
-            end
-            obj.current_page = page
-          end
-        end
-
-        def records_per_page
-          option_for(:limit).to_i
-        end
-
-        # When paginating large tables, we want to avoid doing an expensive count query
-        # when the result won't even be used. So when calling paginate you can pass in
-        # an optional :search_count key which points to the search params. If these params
-        # exist, then assume we're doing a search and don't override the default count
-        # behavior. Otherwise, just return some large number so the paginator skips the
-        # count.
-        def option_for(key)
-          case key
-          when :limit
-            limit = @paginator_options.try(:[], :limit) || Danbooru.config.posts_per_page
-            if limit.to_i > 1_000
-              limit = 1_000
-            end
-            limit
-
-          when :count
-            if @paginator_options.has_key?(:search_count) && @paginator_options[:search_count].blank?
-              1_000_000
-            elsif @paginator_options[:count]
-              @paginator_options[:count]
-            else
-              nil
-            end
-
-          end
-        end
-
-        # taken from kaminari (https://github.com/amatsuda/kaminari)
-        def total_count
-          return option_for(:count) if option_for(:count)
-
-          c = except(:offset, :limit, :order)
-          c = c.reorder(nil)
-          c = c.count
-          c.respond_to?(:count) ? c.count : c
-        rescue ActiveRecord::StatementInvalid => e
-          if e.to_s =~ /statement timeout/
-            1_000_000
-          else
-            raise
-          end
-        end
-      end
-    end
-  end
-end

app/logical/danbooru/paginator/numbered_collection_extension.rb

@@ -1,15 +0,0 @@
-module Danbooru
-  module Paginator
-    module NumberedCollectionExtension
-      attr_accessor :current_page, :total_pages
-
-      def is_first_page?
-        current_page == 1
-      end
-
-      def is_last_page?
-        current_page >= total_pages
-      end
-    end
-  end
-end

app/logical/danbooru/paginator/pagination_error.rb

@@ -1,6 +0,0 @@
-module Danbooru
-  module Paginator
-    class PaginationError < Exception
-    end
-  end
-end

app/logical/danbooru/paginator/sequential_collection_extension.rb

@@ -1,34 +0,0 @@
-module Danbooru
-  module Paginator
-    module SequentialCollectionExtension
-      attr_accessor :sequential_paginator_mode
-
-      def is_first_page?
-        if sequential_paginator_mode == :before
-          false
-        else
-          size <= records_per_page
-        end
-      end
-
-      def is_last_page?
-        if sequential_paginator_mode == :after
-          false
-        else
-          size <= records_per_page
-        end
-      end
-
-      # 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
-          super.first(records_per_page).reverse
-        end
-      end
-    end
-  end
-end

app/logical/pagination_extension.rb

@@ -0,0 +1,91 @@
+module PaginationExtension
+  class PaginationError < Exception ; end
+
+  attr_accessor :current_page, :records_per_page, :paginator_count, :paginator_mode
+
+  def paginate(page, limit: nil, count: nil, search_count: nil)
+    @records_per_page = limit || Danbooru.config.posts_per_page
+    @records_per_page = @records_per_page.to_i.clamp(1, 1000)
+
+    if count.present?
+      @paginator_count = count
+    elsif !search_count.nil? && search_count.blank?
+      @paginator_count = 1_000_000
+    end
+
+    if page =~ /\Ab(\d+)\z/i
+      @paginator_mode = :sequential_before
+      paginate_sequential_before($1, records_per_page)
+    elsif page =~ /\Aa(\d+)\z/i
+      @paginator_mode = :sequential_after
+      paginate_sequential_after($1, records_per_page)
+    else
+      @paginator_mode = :numbered
+      @current_page = [page.to_i, 1].max
+      raise PaginationError if current_page > Danbooru.config.max_numbered_pages
+
+      paginate_numbered(current_page, records_per_page)
+    end
+  end
+
+  def paginate_sequential_before(before_id, limit)
+    where("#{table_name}.id < ?", before_id).reorder("#{table_name}.id desc").limit(limit + 1)
+  end
+
+  def paginate_sequential_after(after_id, limit)
+    where("#{table_name}.id > ?", after_id).reorder("#{table_name}.id asc").limit(limit + 1)
+  end
+
+  def paginate_numbered(page, limit)
+    offset((page - 1) * limit).limit(limit)
+  end
+
+  def is_first_page?
+    if paginator_mode == :numbered
+      current_page == 1
+    elsif paginator_mode == :sequential_before
+      false
+    elsif paginator_mode == :sequential_after
+      size <= records_per_page
+    end
+  end
+
+  def is_last_page?
+    if paginator_mode == :numbered
+      current_page >= total_pages
+    elsif paginator_mode == :sequential_before
+      size <= records_per_page
+    elsif paginator_mode == :sequential_after
+      false
+    end
+  end
+
+  # 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 paginator_mode == :sequential_before
+      super.first(records_per_page)
+    elsif paginator_mode == :sequential_after
+      super.first(records_per_page).reverse
+    elsif paginator_mode == :numbered
+      super
+    end
+  end
+
+  def total_pages
+    (total_count.to_f / records_per_page).ceil
+  end
+
+  # taken from kaminari (https://github.com/amatsuda/kaminari)
+  def total_count
+    @paginator_count ||= except(:offset, :limit, :order).reorder(nil).count
+  rescue ActiveRecord::StatementInvalid => e
+    if e.to_s =~ /statement timeout/
+      1_000_000
+    else
+      raise
+    end
+  end
+end

app/models/application_record.rb

@@ -1,7 +1,13 @@
 class ApplicationRecord < ActiveRecord::Base
   self.abstract_class = true
 
-  include Danbooru::Paginator::ActiveRecordExtension
+  concerning :PaginationMethods do
+    class_methods do
+      def paginate(*options)
+        extending(PaginationExtension).paginate(*options)
+      end
+    end
+  end
 
   concerning :SearchMethods do
     class_methods do

config/initializers/delayed_jobs.rb

@@ -1,5 +1,4 @@
 require 'delayed/plugin'
-require 'danbooru/paginator/active_record_extension'
 
 class DelayedJobTimeoutPlugin < ::Delayed::Plugin
   callbacks do |lifecycle|
@@ -13,4 +12,3 @@ Delayed::Worker.logger = Logger.new(STDOUT, level: :debug)
 Delayed::Worker.default_queue_name = "default"
 Delayed::Worker.destroy_failed_jobs = false
 Delayed::Worker.plugins << DelayedJobTimeoutPlugin
-Delayed::Job.include(Danbooru::Paginator::ActiveRecordExtension)

test/unit/paginator_test.rb

@@ -12,6 +12,11 @@ class PaginatorTest < ActiveSupport::TestCase
 
       assert_equal(expected_posts.map(&:id), posts.map(&:id))
     end
+
+    should "return nothing for b0" do
+      posts = Post.paginate("b0")
+      assert_empty(posts.map(&:id))
+    end
   end
 
   context "sequential pagination (after)" do
@@ -30,5 +35,30 @@ class PaginatorTest < ActiveSupport::TestCase
 
       assert_equal(expected_posts.map(&:id), posts.map(&:id))
     end
+
+    should "raise an error when exceeding the page limit" do
+      Danbooru.config.stubs(:max_numbered_pages).returns(5)
+      assert_raises(PaginationExtension::PaginationError) do
+        Post.paginate(10)
+      end
+    end
+
+    should "count pages correctly" do
+      assert_equal(5, Post.paginate(1, limit: 1).total_pages)
+      assert_equal(3, Post.paginate(1, limit: 2).total_pages)
+      assert_equal(2, Post.paginate(1, limit: 3).total_pages)
+      assert_equal(2, Post.paginate(1, limit: 4).total_pages)
+      assert_equal(1, Post.paginate(1, limit: 5).total_pages)
+    end
+
+    should "detect the first and last page correctly" do
+      assert(Post.paginate(0, limit: 1).is_first_page?)
+      assert(Post.paginate(1, limit: 1).is_first_page?)
+      refute(Post.paginate(1, limit: 1).is_last_page?)
+
+      refute(Post.paginate(5, limit: 1).is_first_page?)
+      assert(Post.paginate(5, limit: 1).is_last_page?)
+      assert(Post.paginate(6, limit: 1).is_last_page?)
+    end
   end
 end

test/unit/post_sets/intro_test.rb

@@ -1,5 +1,4 @@
 require 'test_helper'
-require "danbooru/paginator/pagination_error"
 
 module PostSets
   class IntroTest < ActiveSupport::TestCase

test/unit/post_sets/post_test.rb

@@ -1,5 +1,4 @@
 require 'test_helper'
-require "danbooru/paginator/pagination_error"
 
 module PostSets
   class PostTest < ActiveSupport::TestCase
@@ -86,7 +85,7 @@ module PostSets
         end
 
         should "fail" do
-          assert_raises(Danbooru::Paginator::PaginationError) do
+          assert_raises(PaginationExtension::PaginationError) do
             @set.posts
           end
         end