refactor page number checking, add offset validation

Raise WP::InvalidPage exception on offset values larger than SQL's BIGINT

references mislav#115

lib/will_paginate/active_record.rb

@@ -47,7 +47,7 @@ def per_page(value = nil)
       def limit(num)
         rel = super
         if rel.current_page
-          rel.offset((rel.current_page-1) * rel.limit_value)
+          rel.offset ::WillPaginate.calculate_offset(rel.current_page, rel.limit_value)
         else
           rel
         end
@@ -124,10 +124,9 @@ def paginate(options)
       end
 
       def page(num)
-        pagenum = num.nil? ? 1 : num.to_i
-        raise ::WillPaginate::InvalidPage, num, pagenum if pagenum < 1
         rel = scoped.extending(RelationMethods)
-        rel = rel.offset((pagenum-1) * (rel.limit_value || per_page))
+        pagenum, per_page, offset = ::WillPaginate.process_values(num, rel.limit_value || self.per_page)
+        rel = rel.offset(offset)
         rel = rel.limit(per_page) unless rel.limit_value
         rel.current_page = pagenum
         rel

lib/will_paginate/array.rb

@@ -22,11 +22,11 @@ class Array
   # McAdam}[http://www.desimcadam.com/archives/8] and later proved to be the
   # most useful method of will_paginate library.
   def paginate(options = {})
-    raise ArgumentError, "parameter hash expected (got #{options.inspect})" unless Hash === options
-
-    WillPaginate::Collection.create options[:page] || 1,
-                                    options[:per_page] || 30,
-                                    options[:total_entries] || self.length do |pager|
+    page     = options[:page] || 1
+    per_page = options[:per_page] || WillPaginate.per_page
+    total    = options[:total_entries] || self.length
+
+    WillPaginate::Collection.create(page, per_page, total) do |pager|
       pager.replace self[pager.offset, pager.per_page].to_a
     end
   end

lib/will_paginate/collection.rb

@@ -1,27 +1,6 @@
+require 'will_paginate/per_page'
+
 module WillPaginate
-  # = Invalid page number error
-  # This is an ArgumentError raised in case a page was requested that is either
-  # zero or negative number. You should decide how do deal with such errors in
-  # the controller.
-  #
-  # If you're using Rails 2, then this error will automatically get handled like
-  # 404 Not Found. The hook is in "will_paginate.rb":
-  #
-  #   ActionController::Base.rescue_responses['WillPaginate::InvalidPage'] = :not_found
-  #
-  # If you don't like this, use your preffered method of rescuing exceptions in
-  # public from your controllers to handle this differently. The +rescue_from+
-  # method is a nice addition to Rails 2.
-  #
-  # This error is *not* raised when a page further than the last page is
-  # requested. Use <tt>WillPaginate::Collection#out_of_bounds?</tt> method to
-  # check for those cases and manually deal with them as you see fit.
-  class InvalidPage < ArgumentError
-    def initialize(page, page_num) #:nodoc:
-      super "#{page.inspect} given as value, which translates to '#{page_num}' as page number"
-    end
-  end
-
   # = The key to pagination
   # Arrays returned from paginating finds are, in fact, instances of this little
   # class. You may think of WillPaginate::Collection as an ordinary array with
@@ -44,12 +23,9 @@ class Collection < Array
     # and the total number of entries. The last argument is optional because it
     # is best to do lazy counting; in other words, count *conditionally* after
     # populating the collection using the +replace+ method.
-    def initialize(page, per_page, total = nil)
-      @current_page = page.to_i
-      raise InvalidPage.new(page, @current_page) if @current_page < 1
+    def initialize(page, per_page = WillPaginate.per_page, total = nil)
+      @current_page = InvalidPage.validate(page, 'page')
       @per_page = per_page.to_i
-      raise ArgumentError, "`per_page` setting cannot be less than 1 (#{@per_page} given)" if @per_page < 1
-
       self.total_entries = total if total
     end
 

lib/will_paginate/data_mapper.rb

@@ -7,9 +7,8 @@ module WillPaginate
   module DataMapper
     module Pagination
       def page(num)
-        pagenum = num.nil? ? 1 : num.to_i
-        raise ::WillPaginate::InvalidPage, num, pagenum if pagenum < 1
-        options = {:offset => (pagenum-1) * (query.limit || per_page)}
+        pagenum, per_page, offset = ::WillPaginate.process_values(num, query.limit || self.per_page)
+        options = {:offset => offset}
         options[:limit] = per_page unless query.limit
         col = new_collection(query.merge(options))
         col.current_page = pagenum

lib/will_paginate/per_page.rb

@@ -24,4 +24,40 @@ def inherited(subclass)
 
   # default number of items per page
   self.per_page = 30
+
+  # these methods are used internally and are subject to change
+  module Calculation
+    def process_values(page, per_page)
+      page = page.nil? ? 1 : InvalidPage.validate(page, 'page')
+      per_page = per_page.to_i
+      offset = calculate_offset(page, per_page)
+      [page, per_page, offset]
+    end
+
+    def calculate_offset(page, per_page)
+      InvalidPage.validate((page - 1) * per_page, 'offset')
+    end
+  end
+
+  extend Calculation
+
+  # Raised by paginating methods in case `page` parameter is an invalid number.
+  #
+  # In Rails this error is automatically handled as 404 Not Found.
+  class InvalidPage < ArgumentError
+    # the maximum value for SQL BIGINT
+    BIGINT = 9223372036854775807
+
+    # Returns value cast to integer, raising self if invalid
+    def self.validate(value, name)
+      num = value.to_i
+    rescue NoMethodError
+      raise self, "#{name} cannot be converted to integer: #{value.inspect}"
+    else
+      if 'offset' == name ? (num < 0 or num > BIGINT) : num < 1
+        raise self, "invalid #{name}: #{value.inspect}"
+      end
+      return num
+    end
+  end
 end

spec/collection_spec.rb

@@ -29,17 +29,6 @@
     result.size.should == 30
   end
 
-  describe "old API" do
-    it "should fail with numeric params" do
-      Proc.new { [].paginate(2) }.should raise_error(ArgumentError)
-      Proc.new { [].paginate(2, 10) }.should raise_error(ArgumentError)
-    end
-
-    it "should fail with both options and numeric param" do
-      Proc.new { [].paginate({}, 5) }.should raise_error(ArgumentError)
-    end
-  end
-
   it "should give total_entries precedence over actual size" do
     %w(a b c).paginate(:total_entries => 5).total_entries.should == 5
   end
@@ -118,26 +107,29 @@
   end
 
   it "should raise WillPaginate::InvalidPage on invalid input" do
-    for bad_input in [0, -1, nil, '', 'Schnitzel']
-      Proc.new { create bad_input }.should raise_error(WillPaginate::InvalidPage)
+    [0, -1, nil, '', 'Schnitzel'].each do |bad_input|
+      lambda {
+        create bad_input
+      }.should raise_error(WillPaginate::InvalidPage, "invalid page: #{bad_input.inspect}")
     end
   end
 
-  it "should raise Argument error on invalid per_page setting" do
-    Proc.new { create(1, -1) }.should raise_error(ArgumentError)
-  end
-
   it "should not respond to page_count anymore" do
     Proc.new { create.page_count }.should raise_error(NoMethodError)
   end
 
+  it "inherits per_page from global value" do
+    collection = described_class.new(1)
+    collection.per_page.should == 30
+  end
+
   private
 
     def create(page = 2, limit = 5, total = nil, &block)
       if block_given?
-        WillPaginate::Collection.create(page, limit, total, &block)
+        described_class.create(page, limit, total, &block)
       else
-        WillPaginate::Collection.new(page, limit, total)
+        described_class.new(page, limit, total)
       end
     end
 

spec/finders/active_record_spec.rb

@@ -418,6 +418,13 @@
       Developer.paginate((1..8).to_a, :per_page => 3, :page => 2, :order => 'id')
     }.should raise_error(ArgumentError)
   end
+
+  it "errors out for invalid values" do |variable|
+    lambda {
+      # page that results in an offset larger than BIGINT
+      Project.page(307445734561825862)
+    }.should raise_error(WillPaginate::InvalidPage, "invalid offset: 9223372036854775830")
+  end
 
   protected