-
Notifications
You must be signed in to change notification settings - Fork 13
Fix/updateinfo bug fix #75
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
Conversation
elguero
left a comment
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.
Minor comments. Otherwise, functionality looks good and tests look good.
NeilHanlon
left a comment
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.
Excellent work as always @rockythorn :) Solid fix for both bugs that also improves test coverage. It's a Christmas miracle and it's only December!
The root cause explanation & its' fixes seem entirely logical and correct. NEVRA-based dedupe is the right approach for sure. Though I'm not a test expert, the tests appear extensive enough to help catch regressions in the future.
We may want to at some point take a pass at adding typing for the project, which would at least give the ability to lint a bit easier than waiting for CPython to throw a fit.
One small nit if I have to have one:
Docstring at line 104 in api_updateinfo.py mentions product_name_for_packages as "Optional", but it's technically not since it's required for V2's XML generation. Might be worth clarifying that role in a future pass, but again... a nit.
LGTM! Thanks again Sam. Happy holidays. 🎅🏼 🎁 🕎
| if supported_product_id is not None: | ||
| # v2: Filter by FK (normalized relational data) | ||
| filtered_packages = [ | ||
| pkg for pkg in advisory.packages | ||
| if pkg.supported_product_id == supported_product_id and pkg.repo_name == repo_name | ||
| ] | ||
| seen_nevras = set() | ||
| filtered_packages = [] | ||
| for pkg in advisory.packages: | ||
| if pkg.supported_product_id == supported_product_id and pkg.repo_name == repo_name: | ||
| if pkg.nevra not in seen_nevras: | ||
| seen_nevras.add(pkg.nevra) | ||
| filtered_packages.append(pkg) |
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.
This dedupe implementation is clean, but I'd be remiss (i.e., exiled by ##python on Libera.chat) if I didn't take the opportunity to mention a
possibly more "pythonic" way to do it:
if supported_product_id is not None:
filtered_packages = list({
pkg.nevra: pkg
for pkg in advisory.packages
if pkg.supported_product_id == supported_product_id
and pkg.repo_name == repo_name
}.values())As long as we definitely want the first match and don't need side effects or logging during the filtering, then using a dict naturally performs the
"keep the first NEVRA" like you want.
Another option would be to remove the nesting and perform the check in a single pass instead:
# ...
for pkg in advisory.packages:
if (
pkg.supported_product_id != supported_product_id
or pkg.repo_name != repo_name
or pkg.nevra in seen_nevras
):
continue
seen_nevras.add(pkg.nevra)
filtered_packages.append(pkg)Either way, the code is fine as is :)
This PR fixes two bugs identified in the new updateinfo_v2 endpoint and strengthens tests for updateinfo XML generation. See https://bugs.rockylinux.org/view.php?id=11320
1. Product Information Missing from Updateinfo Elements
Problem:
The and XML elements in the generated updateinfo were missing the expected product context, producing output such as:
Cause:
The v2 endpoint previously did not pass the necessary product_name_for_packages argument to the updateinfo XML string builder.
Solution:
Update the endpoint to correctly pass product_name_for_packages, ensuring that product information appears as expected in both and elements.
2. Duplicate Packages in pkglist Element
Problem:
Identical packages appeared multiple times within , such as:
Cause:
Because the v2 endpoint operates over multiple mirrors/products associated with the same advisory, duplicate package entries were not eliminated (unlike the original endpoint, which pre-filters by mirror/product).
Solution:
Added explicit deduplication logic for packages in the v2 endpoint’s pkglist generation.
3. Test Coverage
New and updated unit tests have been added to detect regressions and ensure that these XML generation issues are caught in the future.
Summary:
This PR improves the fidelity of updateinfo XML output for the v2 endpoint by restoring product context and eliminating duplicate packages, with accompanying tests for long-term stability.
Note:
The tests and some related changes in this PR were written with the assistance of Claude Code.