-
Notifications
You must be signed in to change notification settings - Fork 293
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 - ensure page and per_page are integers #11124
Fixes #37587 - ensure page and per_page are integers #11124
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.
It surprises me Katello overrides these. I see there's a def_param_group
that is very similar in Foreman and also shows you can compose them:
https://github.com/theforeman/foreman/blob/cd1ea006eb8b4bc7910e86cf2d182b1b7b63c45f/app/controllers/api/v2/base_controller.rb#L14-L23
I'd think this works:
def_param_group :search do
param_group :pagination, ::Api::V2::BaseController
param :full_result, :bool, :desc => N_("Whether or not to show all results")
param :sort_by, String, :desc => N_("Field to sort the results on")
param :sort_order, String, :desc => N_("How to order the sorted results (e.g. ASC for ascending)")
end
But then you also need to enhance the API to properly handle per_page=all
. I see Katello has invented full_result
in its API. Perhaps a chance to align APIs as well?
When I further look at it, it appears at some fundamental level the APIs are just slightly different. For example, it defines all these scoped_search_*
helpers where the base class in Foreman already defines helpers like resource_scope_for_index
that do all of those.
Katello also has sort_by
as a parameter where Foreman just has order
.
I ended up writing #11126 but I'm afraid it's a can of worms that I don't want to open right now.
d3169e1
to
d5cefc8
Compare
4bf3696
to
5d2577f
Compare
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.
I was able to verify that pagination in the packages wizard on the all hosts page works as expected regardless of user input. I was also able to verify that Katello always returns integers for the modified endpoint. The unit test appears to be appropriate and valid. Approving the PR :)
I wonder if this needs a release note since it's an API change. Probably never intended and always a bug, but still |
* Fixes #37587 - ensure page and per_page are integers (cherry picked from commit baa0512)
* Fixes #37587 - ensure page and per_page are integers (cherry picked from commit baa0512)
What are the changes introduced in this pull request?
(First attempt was theforeman/foreman#10301 - see theforeman/foreman#10301 (comment))
Ensure
page
andper_page
always get sent back from API as integers. Right now they can be sent back as strings if you send them as URL query params.Considerations taken when implementing this change?
This is to fix the immediate issue and release blocker. We can do more extensive refactoring like #11126 as well, but let's get this in for now.
What are the testing steps for this pull request?
Follow reproduction steps in the redmine