Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

per(0) should still returns scope / array with no elements, fixes #296 #305

Closed
wants to merge 1 commit into from

4 participants

@gs
gs commented

No description provided.

@amatsuda
Owner

Well, I actually don't have any strong opinion whether per(0) should "not limit" or "limit 0", but why do you think it "should" return an empty list?

@gs
gs commented

Hey,
personally I think that this should not break chain of scopes and I think that this should reflect sql behavior.
So when you do a query with LIMIT 0 - empty result set is returned.

Br,
Grzegorz

@yuki24
Collaborator

I don't think per should limit because it makes it impossible to calculate how many pages there are.
Let's say you have 100 records and call per(0), which is equivalent to limit(0). In this case, we will possibly encounter a "Division by zero" error.

@yuki24
Collaborator

I'm closing to avoid the division by zero error.

@yuki24 yuki24 closed this
@sfcgeorge

Hi, I agree with @gs that .per(0) should return 0 results. Zero means zero seems completely logical. My reason for wanting this is I have an API where the user can specify how many results they want (with an upper limit of course), but as that endpoint also returns metadata there are times when you want only the metadata and no actual content. Rather than making a separate API endpoint it makes sense to allow per(0). Furthermore, in the current implementation if a user supplies params[:per] = 0 then they could DOS your site as suddenly everything is returned. If you want all results then leave off the per or perhaps add another option of per(false) or per(:all). To patch my API quickly I've added this in a Rails initializer:

module Kaminari
  module PageScopeMethodsExtensions
    def per(num)
      if num.to_i <= 0
        limit(0)
      else
        super
      end
    end

    def total_pages
      if limit_value == 0
        0
      else
        super
      end
    end
  end
end

module Kaminari
  module PageScopeMethods
    prepend Kaminari::PageScopeMethodsExtensions
  end
end

If you would change your stance on this and consider my reasoning then I'd be happy to write a pull request, including prevention of divide by zero. Otherwise I'll just keep using my patch.

@yuki24
Collaborator

@sfcgeorge allowing per(0) (meaning limit(0)) would break almost all the helper methods as it would become impossible to calculate total_pages. total_pages shouldn't be 0 when it's actually incomputable (or mathematically infinity).

It makes sense per(0) returns an empty array but pagination is not only about models. If your API needs to return only metadata,

  • Use HTTP HEAD and only return RFC 5988-aware headers without a message body
  • Return 400 Bad Request when params[:per] = 0 (or something equivalent)

makes more sense to me.

@sfcgeorge

@yuki24 Thank you for the fast and thoughtful comment. It's given me a lot to think about.

You are right about total_pages, making it 0 isn't right, and making it :infinity or something seems weird. I suppose you could also conceptualise it as if someone is hitting an endpoint designed to return something then they probably shouldn't ask for nothing. Moving all meta information into headers and issuing a HEAD for when that's all you want seems most 'correct'. So I think you're right and Kaminari shouldn't allow 0.

For my case I am very tempted to go down the HEAD route. However, due to caching and ETags even a HEAD request in Rails still hits the database. So, if the whole point of asking for just the headers is performance then HEAD doesn't actually help, unfortunately. Also forcing consumers to parse both headers and content to get what they need adds a fair bit of complexity. There are tradeoffs either way, but I think I'm going to go for simplicity over correctness and continue monkeypatching. I will keep re-evaluating though.

I still think 0 returning everything is a denial of service risk though and a really unexpected behaviour for the developer. Perhaps Kaminari should raise an exception instead.

@yuki24
Collaborator

So what if we make the following changes?

  • Change Model.per(0) to actually call limit(0)
  • total_pages and current_pages will raise Kaminari::ZeroDivisionError when they're called and limit_value is 0
  • Kaminari::ZeroDivisionError inherits ZeroDivisionError

Maybe Kaminari::ZeroDivisionError isn't a good name though...

@sfcgeorge

Ah actually that is quite a good way of improving it. By doing that, if someone wants to create an API that allows per(0) then instead of returning total_pages it can return total_items leaving the client to work out the paging.

How about Kaminari::ZeroPerDivisionError? It's a little more descriptive, though maybe it could be clearer. Kaminari::ZeroPerWithPagesError describes the reason more than the error operation. I'm not sure. Overall it sounds like a good way forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 3, 2012
  1. @gs
This page is out of date. Refresh to see the latest.
View
2  lib/kaminari/models/page_scope_methods.rb
@@ -4,7 +4,7 @@ module PageScopeMethods
# Model.page(3).per(10)
def per(num)
if (n = num.to_i) <= 0
- self
+ limit(0)
elsif max_per_page && max_per_page < n
limit(max_per_page).offset(offset_value / limit_value * max_per_page)
else
View
11 spec/models/active_record/scopes_spec.rb
@@ -66,6 +66,11 @@
it { should have(5).users }
its('first.name') { should == 'user002' }
end
+
+ context 'page 1 per 0 padding 1' do
+ subject { model_class.page(1).per(0).padding(1) }
+ it { should == [] }
+ end
end
describe '#total_pages' do
@@ -86,17 +91,17 @@
context 'per 0 (using default)' do
subject { model_class.page(50).per(0) }
- its(:total_pages) { should == 4 }
+ it { should == [] }
end
context 'per -1 (using default)' do
subject { model_class.page(5).per(-1) }
- its(:total_pages) { should == 4 }
+ it { should == [] }
end
context 'per "String value that can not be converted into Number" (using default)' do
subject { model_class.page(5).per('aho') }
- its(:total_pages) { should == 4 }
+ it { should == [] }
end
end
View
6 spec/models/array_spec.rb
@@ -74,17 +74,17 @@
context 'per 0 (using default)' do
subject { array.page(50).per(0) }
- its(:total_pages) { should == 4 }
+ it { should == [] }
end
context 'per -1 (using default)' do
subject { array.page(5).per(-1) }
- its(:total_pages) { should == 4 }
+ it { should == [] }
end
context 'per "String value that can not be converted into Number" (using default)' do
subject { array.page(5).per('aho') }
- its(:total_pages) { should == 4 }
+ it { should == [] }
end
end
Something went wrong with that request. Please try again.