feat: repo file fields sanitization - #1859
Open
pindo696 wants to merge 7 commits into
Open
Conversation
Reviewer's GuideAdds centralized validation and sanitization for repository metadata (packages, errata, references, architectures), wires failures into Prometheus metrics and Grafana dashboards, and extends tests plus fixtures to cover validation behavior and repository basearch handling. Sequence diagram for updateinfo errata and package validationsequenceDiagram
participant UpdateInfoMD
participant XMLUpdate as update_elem
participant Validators as metadata_validators
participant Metrics as VALIDATION_FAILED_ITEMS
participant Logger
UpdateInfoMD->>XMLUpdate: iterparse(filename)
XMLUpdate-->>UpdateInfoMD: <update> end event
UpdateInfoMD->>UpdateInfoMD: _parse_update(elem)
UpdateInfoMD->>UpdateInfoMD: _process_package(pkg, module)
UpdateInfoMD->>UpdateInfoMD: _validate(name, name)
UpdateInfoMD->>Validators: validate_field(value, field_type)
alt validation fails
Validators-->>UpdateInfoMD: raise ValidationError
UpdateInfoMD->>Metrics: labels(metadata_type=updateinfo, field=field_type).inc()
UpdateInfoMD->>Logger: warning("Validation failed, skipped: %s", err)
UpdateInfoMD-->>UpdateInfoMD: skip current update
else validation succeeds
Validators-->>UpdateInfoMD: validated value
UpdateInfoMD-->>UpdateInfoMD: append package to update["pkglist"]
UpdateInfoMD-->>UpdateInfoMD: append update to updates
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
pindo696
force-pushed
the
repo-sanitization-checks
branch
2 times, most recently
from
July 15, 2026 08:22
f25ce11 to
e12bd9a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1859 +/- ##
==========================================
- Coverage 62.98% 55.00% -7.98%
==========================================
Files 70 93 +23
Lines 6800 6328 -472
==========================================
- Hits 4283 3481 -802
- Misses 2517 2822 +305
- Partials 0 25 +25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pindo696
force-pushed
the
repo-sanitization-checks
branch
5 times, most recently
from
July 20, 2026 13:01
bfe5a69 to
087af13
Compare
Contributor
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The architecture validation relies on a hardcoded whitelist in
VALID_ARCHITECTURES; consider centralizing this list in a shared config or deriving it from an authoritative source (e.g., rpm/OS metadata) so new or uncommon architectures don’t start causing unexpected validation failures at runtime.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The architecture validation relies on a hardcoded whitelist in `VALID_ARCHITECTURES`; consider centralizing this list in a shared config or deriving it from an authoritative source (e.g., rpm/OS metadata) so new or uncommon architectures don’t start causing unexpected validation failures at runtime.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
pindo696
force-pushed
the
repo-sanitization-checks
branch
from
July 20, 2026 14:53
c0c9b32 to
22509f4
Compare
pindo696
marked this pull request as ready for review
July 20, 2026 14:57
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new architecture validation relies on
VALID_ARCHITECTURESbeing initialized viainit_validator_architectures; consider enforcing or asserting this initialization (or providing a safe fallback) beforePrimaryMD,UpdateInfoMD, andRepositoryControllerstart validating arches to avoid unexpectedUnknown architectureerrors in production. - In
_parse_updatethe code assumes<references>is always present (references.findall), which will raise if the element is missing; adding aNoneguard there would make updateinfo parsing more robust against slightly malformed metadata.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new architecture validation relies on `VALID_ARCHITECTURES` being initialized via `init_validator_architectures`; consider enforcing or asserting this initialization (or providing a safe fallback) before `PrimaryMD`, `UpdateInfoMD`, and `RepositoryController` start validating arches to avoid unexpected `Unknown architecture` errors in production.
- In `_parse_update` the code assumes `<references>` is always present (`references.findall`), which will raise if the element is missing; adding a `None` guard there would make updateinfo parsing more robust against slightly malformed metadata.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jdobes
reviewed
Jul 27, 2026
| def _validate(self, value, field_type): | ||
| """Validate a field and track metrics on failure.""" | ||
| try: | ||
| return validate_field(value, field_type) |
Member
There was a problem hiding this comment.
It lookups the validator function every single time for millions of items, the performace hit will add up
Contributor
Author
There was a problem hiding this comment.
The dict lookup is O(1), which is essentially free. So yes, its negligible. The real performance hit is inside each validator (regex matching or stripping,..)
I applied one more improvement by using .isdigit() instead of regex digit which should be faster.
Furthermore, implemented kind of cache for already seen validator values per field type defined, that turns most calls into a single set lookup, bypassing regex after the first occurrence in a tradeoff with memory.
| @@ -61,21 +63,10 @@ def list_repositories(self): | |||
| return repos | |||
|
|
|||
| def _import_basearch(self, basearch): | |||
Member
There was a problem hiding this comment.
Perhaps it could be removed/refactored, since it's not doing any import anymore.
Little sanitization for repo files during parsing process. Supporting test files taken and modified from vmaas-data repository. Part of RHINENG-13437
Grafana metrics to keep track of potential sanitization failures at the begginning. Also added REGISTRY property so worker metrics can be exposed upon update withount preinitialization.
Verify basearch field has one of allowed arch values. Part of RHINENG-13437
Instead of numeric counters reuse an existing vmaas reposcan errors plot to display validation failures.
Use a single source of truth for arch names based on database values. As we sanitize the archnames, the insert is no longer relevant to us.
pindo696
force-pushed
the
repo-sanitization-checks
branch
2 times, most recently
from
July 28, 2026 08:43
4a6d0bb to
fd3409a
Compare
Bypass regex validation by caching already validated values to improve the validator performance. Replace digit regex for python idigit which should perform better as well.
pindo696
force-pushed
the
repo-sanitization-checks
branch
from
July 28, 2026 09:08
fd3409a to
3b170d3
Compare
Arch is not a valid arch value for validation. We need valid arch so import repos can be tested, otherwise the sanitization will reject it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Little sanitization for repo files during parsing process. Supporting test files taken and modified from vmaas-data repository. Metrics for initial behavior observation. Modified tests. Part of RHINENG-13437
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Introduce validation and sanitization for repository metadata fields and expose new validation metrics for observability.
New Features:
Bug Fixes:
Enhancements:
Tests: