Fixes #37587 - cast page and perPage to Number#10301
Conversation
ekohl
left a comment
There was a problem hiding this comment.
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.
| 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.
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
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
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.
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):
pageandperPagebeing 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_is in the backend - I'm not sure they made a difference, but think they should stay so that things are more consistent.