-
Notifications
You must be signed in to change notification settings - Fork 993
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.
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
Fixes #37587 - cast page and perPage to Number #10301
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My biggest issue with Ruby's .to_i
is that it silently ignores anything that doesn't fit. For example, 'asd'.to_i
becomes 0. That can lead to per_page
being 0
which IMHO is never valid.
I'd like to see some tests for this and a reproducer.
@@ -102,8 +102,8 @@ def metadata_page | |||
end | |||
|
|||
def metadata_per_page | |||
@per_page ||= Setting[:entries_per_page] if params[:per_page].blank? | |||
@per_page ||= params[:per_page] == 'all' ? metadata_total : params[:per_page].to_i | |||
@per_page ||= Setting[:entries_per_page].to_i if params[:per_page].blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this setting ever not a number? From the model I'd expect it to always be an integer:
foreman/app/registries/foreman/settings/general.rb
Lines 18 to 22 in 2ea2c46
setting('entries_per_page', | |
type: :integer, | |
description: N_("Number of records shown per page in Foreman"), | |
default: 20, | |
full_name: N_('Entries per page')) |
We already know that if you modify the default to a string via the API it becomes an integer:
foreman/test/controllers/api/v2/settings_controller_test.rb
Lines 92 to 96 in 2ea2c46
test "should parse string values to integers" do | |
put :update, params: { :id => 'entries_per_page', :setting => { :value => "100" } } | |
assert_response :success | |
assert_equal 100, Setting['entries_per_page'] | |
end |
If it's not, something is wrong and we need to figure out why. IMHO we shouldn't do this.
@per_page ||= Setting[:entries_per_page] if params[:per_page].blank? | ||
@per_page ||= params[:per_page] == 'all' ? metadata_total : params[:per_page].to_i | ||
@per_page ||= Setting[:entries_per_page].to_i if params[:per_page].blank? | ||
@per_page ||= params[:per_page] == 'all' ? metadata_total.to_i : params[:per_page].to_i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: if this isn't a number then we also print the total as a non-number in our JSON which is wrong.
I have a hunch the problem is in https://github.com/theforeman/foreman/blob/develop/app/views/api/v2/layouts/index_layout.json.erb but I didn't want to touch that, because I'm sure there are .. reasons that's the way it is 🤔 |
all I know is it always comes back from the API as a string. |
Could you come up with an integration test that reproduces it? Or at least the API request & response? |
Ok, I've looked into it some more and found something interesting. The page and per_page always come back as strings, but only (a) when you've sent them as URL query params, and (b) only from Katello endpoints. This led me to Katello/katello#11124, which seems to fix the issue without any Foreman changes (I tested on develop). @ekohl Let me know if you still want to keep any changes here; otherwise I think we can close this in favor of the Katello one. |
I'd prefer to keep it simple and avoid redundant conversions. |
New All Hosts page - On the bulk packages wizard, you get this error when you choose anything except 'Upgrade all' and then advance to the next wizard step (the packages table):
page
andperPage
being strings also cause the Pagination component to do crazy things like go from page 1 > 2 > 21 when you press the next page button.This casts them to Numbers in TableIndexPage. I also added some
.to_i
s in the backend - I'm not sure they made a difference, but think they should stay so that things are more consistent.