Permalink
Browse files

refactor page number checking, add offset validation

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

references #115
  • Loading branch information...
1 parent e8205b5 commit 05fc8345a28aeb086d927d633436c362ac0d9b7b @mislav committed Aug 2, 2011
View
7 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
View
10 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
View
32 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
View
5 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
View
36 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
View
30 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
View
7 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

0 comments on commit 05fc834

Please sign in to comment.