[PATCH] Collection.create should handle nil page
(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))
end
=> 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.
-
handle_nil_page_in_create.diff
1.08 KB
-
Chris Wanstrath- Assigned user changed from Chris Wanstrath to Mislav
- State changed from new to open
-
martinOn 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
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 == 0although a less user-friendly solution would be to throw an ActiveRecord::RecordNotFound
-
Mislav- 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
Throwing an appropriate exception is fine. Letting it fall through to SQL as LIMIT -10 isn't good. Thanks for looking at it.
-
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.