Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Per 'nil' is still using limit #356

Closed
d3vkit opened this Issue · 6 comments

4 participants

@d3vkit

I am sure this is user error on my part, but I just don't know what the error could be. I am using Rails 3.2.11, Ruby 1.9.3, Kamanari 0.14.1, and postgres 9.1.7:

# Count all orders in DB
irb(main):011:0> Order.count
   (0.6ms)  SELECT COUNT(*) FROM "orders" 
=> 100
# Try with page but no limit, uses default fine
irb(main):012:0> Order.page(1).count
   (0.6ms)  SELECT COUNT(count_column) FROM (SELECT 1 AS count_column FROM "orders" LIMIT 25 OFFSET 0) subquery_for_count 
=> 25
# Try with page and limit, uses limit fine
irb(main):014:0> Order.page(1).per(50).count
   (0.6ms)  SELECT COUNT(count_column) FROM (SELECT 1 AS count_column FROM "orders" LIMIT 50 OFFSET 0) subquery_for_count 
=> 50
# Try with nil, expect to return all - but uses default limit
irb(main):015:0> Order.page(1).per(nil).count
   (0.6ms)  SELECT COUNT(count_column) FROM (SELECT 1 AS count_column FROM "orders" LIMIT 25 OFFSET 0) subquery_for_count 
=> 25

I'm not sure if I read the doc wrong, but I thought passing nil to page would return all. For now I will just be passing some huge number in. But any help would be appreciated - not sure if this is a bug or an error on my part, so sorry if this is the wrong place to report this.

@jdeff

I'm having the same issue with 0.14.1, rails 3.2.9, and ruby 1.9.3-p194

@jdeff

Pretty sure that this is because 0.14.1 was released 5+ months ago and this feature was implemented 3 months ago. Any word on when 0.14.2 or 0.15.0 will be released?

@bricker

@d3vkit @jdeff Correct, per(nil) hasn't been released yet. Here's the commit if you want to point your Gemfile at it: d63ac01 This commit includes per(nil) and a small fix related to it.

@yuki24
Collaborator

As @bricker said, The master branch on GitHub contains solve the fix.
However, after I read the comment from @arfl, I started wondering whether this is the right design. When we need pagination in a real system, we would probably have a line like this:

Model.page(params[:page]).per(params[:per_page])

And basically we would expect #per to use default_per_page value if params[:per_page] is nil. Also, I think if you want to retrieve every record from the DB, then the right way would be not to paginate.

By the way, the next version would be 0.15.0 since it includes several additional options.

@bricker

@yuki24 I see what you mean and I agree with you, but I think there still needs to be a way to specify "no limit". The reason I did it using nil is just because that's how both will_paginate and ActiveRecord work and I thought the consistency was nice.

I wonder if, instead of nil, we should accept a special case. Something like:

Model.page(params[:page]).per(:all)

What do you think? If you like this idea, I will implement it. /cc @amatsuda

@bricker

Thinking about this more - should we add a configuration option to let the developer decide behavior on per(nil)? I think it's important to keep the behavior consistent with ActiveRecord, but I definitely see the need to allow a per_page parameter (which could of course be nil). We could add something like:

Kaminari,configure do |config|
  config.limit_on_nil = :no_limit # options could be :no_limit (opt-in), or :default_per_page (default)
end

... yeah it feels a little dirty but I think, perhaps selfishly, that it's important to support both. The developer could of course also decide what to do in the case of nil:

class PostsController < ApplicationController
  before_filter :set_per_page

  def index
    @posts = Post.page(params[:page]).per(@per_page)
  end
  # ...

  private
  def set_per_page
    per_page = params[:per_page].to_i
    @per_page = per_page > 0 ? per_page : 25
  end
end

... something like that.

@yuki24 yuki24 closed this in 03fe8ba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.