#156 ✓resolved
Grant Rodgers

[PATCH] Collection.create should handle nil page

Reported by Grant Rodgers | November 14th, 2007 @ 02:04 AM

(sorry, sql is oracle)

The paginate finder handles a nil value for page:

Company.paginate(:page => nil, :per_page => 10)
=> select * from (select raw_sql_.*, rownum raw_rnum_ from (SELECT * FROM companies ) raw_sql_ where rownum <= 10) where raw_rnum_ > 0

Collection.create doesn't:

WillPaginate::Collection.create(nil, 10) do |pager|
  pager.replace(Company.find(:all, :limit => pager.per_page, :offset => pager.offset))
=> select * from (select raw_sql_.*, rownum raw_rnum_ from (SELECT * FROM companies ) raw_sql_ where rownum <= 0) where raw_rnum_ > -10

wp_parse_options! sets page to 1 if nil, so Collection#initialize should do the same thing.

Comments and changes to this ticket

  • Chris Wanstrath

    Chris Wanstrath November 14th, 2007 @ 03:43 AM

    • Assigned user changed from “Chris Wanstrath” to “Mislav”
    • State changed from “new” to “open”
  • martin

    martin November 21st, 2007 @ 08:18 AM

    On a related note, both versions don't handle existing, non-nil but non-numerical values of page.

    E.g. if someone accesses /.../index?page=Wurstbrot this will give an invalid SQL statement - page.to_i returns 0 and that creates the LIMIT -10 statement.

    While this is mostly harmless, I don't think outside users should be able to generate this kind of error by simply surfing the site.

  • Brad Greenlee

    Brad Greenlee December 14th, 2007 @ 01:43 AM

    We just ran into this as well at Wesabe, when a bot checking for security holes started throwing all kinds of non-numeric values at page. Seems the easiest solution is to just throw this in collection.rb:

        def initialize(page, per_page, total)
           @current_page  = page.to_i
           @current_page = 1 if @current_page == 0

    although a less user-friendly solution would be to throw an ActiveRecord::RecordNotFound

  • Mislav

    Mislav December 19th, 2007 @ 03:37 PM

    • State changed from “open” to “invalid”

    In Ruby, try doing this:

    2 + "2"

    You'll get an error that String cannot be coerced into Fixnum. This is because Ruby is strictly typed and it won't try to do any guesswork - that's a feature, not a language flaw.

    This is how WP::Collection was designed: to be strict on the data it accepts. Other than converting input data to integers, it won't do absolutely any guesswork after that. So, if input data is "Schnitzel" instead of a valid number, errors should and will happen.

    AR::Base#paginate method was designed to be a smarter API than this. It accepts nil as current page and defaults to page 1 because that is the global convention with pagination on the Web.

    My mistake was not that I didn't implement more guesswork in these methods, but that I tolerated invalid input. In near future, I will change will_paginate to raise a WP::InvalidPage error whenever there is input such as "Schnitzel", zero, negative number or a blank string.

    To conclude: WP::Collection is a basic class that won't do any of your application logic. If you want to sanitize input or default to a certain page, you'll have to do that prior to calling it or its big sister, AR::Base#paginate.

  • Brad Greenlee

    Brad Greenlee December 19th, 2007 @ 10:12 PM

    Throwing an appropriate exception is fine. Letting it fall through to SQL as LIMIT -10 isn't good. Thanks for looking at it.

  • Mislav

    Mislav December 24th, 2007 @ 12:26 AM

    • State changed from “invalid” to “resolved”

    In revision [421], WillPaginate::InvalidPage error is thrown on invalid input in :page parameter. WillPaginate::Collection doesn't even accept nil as page value anymore, but paginate() API does and defaults to page 1.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

Everyone's favorite Ruby library for pagination of practically anything!

People watching this ticket