RHINENG-26450: replace string inventory IDs with uuid.UUID#2217
Open
katarinazaprazna wants to merge 1 commit into
Open
RHINENG-26450: replace string inventory IDs with uuid.UUID#2217katarinazaprazna wants to merge 1 commit into
katarinazaprazna wants to merge 1 commit into
Conversation
Reviewer's GuideSwitches inventory and system identifiers from string to uuid.UUID across core models, queries, controllers, Kafka/mqueue events, evaluator logic, middleware, and tests, ensuring end-to-end UUID-typed handling while preserving external string representations at the API boundary where needed. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
katarinazaprazna
changed the title
katarinazaprazna
force-pushed
the
update-inventory-id-types
branch
from
7a25fdd to
6ca7a66
Compare
TenSt
reviewed
May 29, 2026
TenSt
reviewed
May 29, 2026
TenSt
reviewed
May 29, 2026
TenSt
reviewed
May 29, 2026
TenSt
reviewed
May 29, 2026
TenSt
reviewed
May 29, 2026
TenSt
reviewed
May 29, 2026
katarinazaprazna
force-pushed
the
update-inventory-id-types
branch
from
fc52ece to
4964ee4
Compare
katarinazaprazna
force-pushed
the
update-inventory-id-types
branch
2 times, most recently
from
43f8366 to
03ab857
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2217 +/- ##
==========================================
+ Coverage 59.00% 59.07% +0.06%
==========================================
Files 137 137
Lines 8831 8821 -10
==========================================
Hits 5211 5211
+ Misses 3072 3064 -8
+ Partials 548 546 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 found 2 issues, and left some high level feedback:
- In
getSubscribedSystem, the DB result is read into a string and then passed touuid.Parsewith the error intentionally ignored; if the DB ever contains an invalid UUID this will be silently treated as “not found” — consider checking and logging the parse error (and returning a 500) to avoid masking data corruption or unexpected values. - Several places sort slices of UUIDs using
bytes.Compare(x[i][:], x[j][:])(e.g. incheckInventoryIDs); it may be clearer and less error-prone to centralize this into a small helper (e.g.lessUUID(a, b uuid.UUID) bool) or useuuid.UUID.String()for comparison to keep the sorting logic consistent and self-documenting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getSubscribedSystem`, the DB result is read into a string and then passed to `uuid.Parse` with the error intentionally ignored; if the DB ever contains an invalid UUID this will be silently treated as “not found” — consider checking and logging the parse error (and returning a 500) to avoid masking data corruption or unexpected values.
- Several places sort slices of UUIDs using `bytes.Compare(x[i][:], x[j][:])` (e.g. in `checkInventoryIDs`); it may be clearer and less error-prone to centralize this into a small helper (e.g. `lessUUID(a, b uuid.UUID) bool`) or use `uuid.UUID.String()` for comparison to keep the sorting logic consistent and self-documenting.
## Individual Comments
### Comment 1
<location path="manager/controllers/template_subscribed_systems_update.go" line_range="82-83" />
<code_context>
+ return 0, "", uuid.Nil, err
}
- if inventoryID == "" {
+ inventoryID, _ := uuid.Parse(inventoryIDStr)
+ if inventoryID == uuid.Nil {
err := errors.Errorf("System %s not found", systemCn)
utils.LogAndRespNotFound(c, err, err.Error())
</code_context>
<issue_to_address>
**issue:** Consider handling uuid.Parse errors explicitly instead of ignoring them
`uuid.Parse(inventoryIDStr)` errors are ignored and only `uuid.Nil` is checked. If `inventory_id` is malformed, this will incorrectly become a 404 and lose the parse error context. Please handle the parse error explicitly (e.g., log and return a 500) so corrupted `system_inventory.inventory_id` values are distinguishable from a genuinely missing system.
</issue_to_address>
### Comment 2
<location path="manager/middlewares/rbac.go" line_range="176-180" />
<code_context>
continue
}
- group, err := utils.ParseInventoryGroup(v, nil)
+ id, err := uuid.Parse(*v)
+ if err != nil {
+ continue
+ }
+ group, err := utils.ParseInventoryGroup(&id, nil)
if err != nil {
- // couldn't marshal inventory group to json
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Skipping non-UUID inventory groups silently may hide misconfigured RBAC attributes
With this change, any `inventory_group.id` that fails `uuid.Parse` is now dropped silently, whereas previously non-UUID IDs were still passed to `ParseInventoryGroup`. If upstream RBAC data ever includes non-UUID IDs (e.g., due to misconfig), those groups will disappear with no signal. Please add at least debug/warn logging on parse failures so these issues are detectable.
Suggested implementation:
```golang
id, err := uuid.Parse(*v)
if err != nil {
// log invalid inventory_group.id so RBAC misconfigurations are visible
log.WithError(err).
WithField("inventory_group_id", *v).
Warn("skipping inventory group with invalid UUID in RBAC access")
continue
}
group, err := utils.ParseInventoryGroup(&id, nil)
```
1. Ensure that `log` is a valid logger in this package:
- If the project uses `logrus`, confirm that `log` is the configured `*logrus.Entry` (or adjust the call to match your logger, e.g. `logging.Log` or `logger`).
- If no logger is available yet in this file, add an appropriate logger import (e.g. `github.com/sirupsen/logrus`) and initialize or use the project-standard logging facade.
2. Optionally, if your logging guidelines prefer debug over warn for this case, change `.Warn(...)` to `.Debug(...)` or use the appropriate level/method.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+82
to
+83
| inventoryID, _ := uuid.Parse(inventoryIDStr) | ||
| if inventoryID == uuid.Nil { |
There was a problem hiding this comment.
issue: Consider handling uuid.Parse errors explicitly instead of ignoring them
uuid.Parse(inventoryIDStr) errors are ignored and only uuid.Nil is checked. If inventory_id is malformed, this will incorrectly become a 404 and lose the parse error context. Please handle the parse error explicitly (e.g., log and return a 500) so corrupted system_inventory.inventory_id values are distinguishable from a genuinely missing system.
Comment on lines
+176
to
+180
| id, err := uuid.Parse(*v) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| group, err := utils.ParseInventoryGroup(&id, nil) |
There was a problem hiding this comment.
suggestion (bug_risk): Skipping non-UUID inventory groups silently may hide misconfigured RBAC attributes
With this change, any inventory_group.id that fails uuid.Parse is now dropped silently, whereas previously non-UUID IDs were still passed to ParseInventoryGroup. If upstream RBAC data ever includes non-UUID IDs (e.g., due to misconfig), those groups will disappear with no signal. Please add at least debug/warn logging on parse failures so these issues are detectable.
Suggested implementation:
id, err := uuid.Parse(*v)
if err != nil {
// log invalid inventory_group.id so RBAC misconfigurations are visible
log.WithError(err).
WithField("inventory_group_id", *v).
Warn("skipping inventory group with invalid UUID in RBAC access")
continue
}
group, err := utils.ParseInventoryGroup(&id, nil)- Ensure that
logis a valid logger in this package:- If the project uses
logrus, confirm thatlogis the configured*logrus.Entry(or adjust the call to match your logger, e.g.logging.Logorlogger). - If no logger is available yet in this file, add an appropriate logger import (e.g.
github.com/sirupsen/logrus) and initialize or use the project-standard logging facade.
- If the project uses
- Optionally, if your logging guidelines prefer debug over warn for this case, change
.Warn(...)to.Debug(...)or use the appropriate level/method.
katarinazaprazna
force-pushed
the
update-inventory-id-types
branch
from
03ab857 to
de8eec6
Compare
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
4 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.
Summary
Replace string inventory IDs with
uuid.UUIDacross the entire backend, starting from the listener entry point (Host model) through internal models, evaluator, manager controllers, and middleware (RBAC/Kessel)Ticket: https://redhat.atlassian.net/browse/RHINENG-26450
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Switch inventory and system identifier handling from strings to uuid.UUID across models, message payloads, controllers, and background tasks.
Enhancements:
Summary by Sourcery
Switch inventory/system identifiers across backend services from strings to uuid.UUID, updating models, queries, handlers, and tests while preserving external string-based representations where required.
Enhancements: