Fixes #38076 - Sanitize content_view repository_ids param #11253
Conversation
quba42
force-pushed
the
repository_ids_api_consistency
branch
2 times, most recently
from
9f74ae3 to
672adf4
Compare
There was a problem hiding this comment.
Even though, as the PR author, I can only comment, I am going to state here that I think this is ready to be merged. The commit was created by @m-bucher, so I am already a second pair of eyes.
In addition I have fully tested having vs. not having this change together with the rolling CV PR #11240
Without this PR included adding repos to a rolling CV that already contains repos via Hammer does not work (while the same action via the UI does work). This is because using Hammer has the effect of sending a list of strings rather than a list of integers, which is then compared to a list of integers as retrieved from the DB (list1 - list2 does not work as intended if the value types do not match).
Adding this PR fixes the issue, the tests are green, and the code changes are easily comprehensible. I have also discussed this PR with @ianballou who was not opposed to the general approach of sanitizing in the API controller.
=> IMHO this can be merged.
|
I'm still good with the approach here 👍 |
quba42
force-pushed
the
repository_ids_api_consistency
branch
from
672adf4 to
59edaf7
Compare
What are the changes introduced in this pull request?
For the content_view APIs, the repository_ids param is now cast to a list of integers even if it is supplied as a list of strings before being passed to the backend. This ensures the backend always gets consistent data and can perform list comparisons with data taken from the DB.
Considerations taken when implementing this change?
We could of course implement the needed handling within the affected actions instead of the API controller, however, this seems like a less robust design approach, since such special handling can be easily forgotten for the next action that needs it.
What are the testing steps for this pull request?
This PR is most easily tested in combination with these PRs:
Simply create a rolling CV, add a repo to it using Hammer, and then add another repo to it using Hammer. => Everything works.
Now revert this commit and repeat these steps. => Chaos and errors ensue. (Because Hammer sends a list of strings to the API, taking the list difference with the repository_ids already in the rolling CV, will cause Katello to attempt adding the same repo to the rolling CV multiple times in a single task).