Skip to content

Coerce pagination string params #110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Coerce pagination string params #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af4a01a
Coerce pagination string params
ryansch May 8, 2014
68c8dea
#limit and #offset validate value param
ryansch May 13, 2014
c48e1c1
0 is valid for offset, return self for chaining
ryansch May 11, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions elasticsearch-model/lib/elasticsearch/model/response/pagination.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,11 @@ def offset_value
# Set the "limit" (`size`) value
#
def limit(value)
return self if value.to_i <= 0
@results = nil
@records = nil
@response = nil
@per_page = value
@per_page = value.to_i
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may introduce some confusion as any value other than a string representation of a number is likely going to return 0 by calling the #to_i:

"35".to_i   # good
> 35     

"ab".to_i  # not good
> 0

This will cause the next few lines to succeed, with a size of 0 and a per_page of 0, and likely add confusion. I think it should fail loudly. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know of a way to get ruby to throw up when converting a String to a Fixnum if the conversion doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever we decide here needs to also be applied to the #page method above in the same file (it already uses #to_i.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if instead of just coercing we validated it as well?

@per_page.to_i must be >= 1

If it is < 1, then it has no affect on the query and neither does the page argument. Thus returning all results. The page number is only valid when there is a proper per_page limit.

When I put in a page number that doesn't make sense as a possible value say -1 or boom kaminari just gives me page 1. So following that I would really want it to be ignored and fall back to a default value. Default value for @page is easy. Picking a default value for @per_page would be arbitrary.

Also, I wouldn't want to throw an exception because someone decided to put ?per_page=boom in their address bar; I get enough email as it is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fail loudly:

@per_page = Integer(value)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AaronRustad That's entirely too obvious. 😏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like @morganick's idea of testing for >=1 since that also deals with negative integers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AaronRustad @morganick I've added code/tests for invalid string params.


search.definition.update :size => @per_page
search.definition.update :from => @per_page * (@page - 1) if @page
Expand All @@ -79,11 +80,12 @@ def limit(value)
# Set the "offset" (`from`) value
#
def offset(value)
return self if value.to_i < 0
@results = nil
@records = nil
@response = nil
@page = nil
search.definition.update :from => value
search.definition.update :from => value.to_i
self
end

Expand Down
22 changes: 22 additions & 0 deletions elasticsearch-model/test/unit/response_pagination_kaminari_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,17 @@ def self.document_type; 'bar'; end
assert_nil @response.instance_variable_get(:@records)
assert_nil @response.instance_variable_get(:@results)
end

should 'coerce string parameters' do
@response.limit("35")
assert_equal 35, @response.search.definition[:size]
end

should 'ignore invalid string parameters' do
@response.limit(35)
@response.limit("asdf")
assert_equal 35, @response.search.definition[:size]
end
end

context "with the page() and limit() methods" do
Expand Down Expand Up @@ -151,6 +162,17 @@ def self.document_type; 'bar'; end
assert_nil @response.instance_variable_get(:@records)
assert_nil @response.instance_variable_get(:@results)
end

should 'coerce string parameters' do
@response.offset("35")
assert_equal 35, @response.search.definition[:from]
end

should 'coerce invalid string parameters' do
@response.offset(35)
@response.offset("asdf")
assert_equal 0, @response.search.definition[:from]
end
end

context "total" do
Expand Down