RHINENG-21444: remove ref to inventory.hosts table#2099
Open
TenSt wants to merge 9 commits intoRedHatInsights:masterfrom
Open
RHINENG-21444: remove ref to inventory.hosts table#2099TenSt wants to merge 9 commits intoRedHatInsights:masterfrom
TenSt wants to merge 9 commits intoRedHatInsights:masterfrom
Conversation
Reviewer's GuideRefactors inventory-related functionality to stop depending on the inventory.hosts table by migrating group/workload/system-profile data into system_inventory, updating queries and models accordingly, and aligning tests and schema (including a new migration) with the new structure. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
SC Environment Impact AssessmentOverall Impact: 🟢 LOW View full reportSummary
Detailed Findings🟢 LOW ImpactEnvironment configuration change detected
Required Actions
This assessment was automatically generated. Please review carefully and consult with the ROSA Core team for critical/high impact changes. |
![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 4 issues, and left some high level feedback:
- There are leftover debug prints using fmt.Printf in non-test code (e.g. buildInventoryQuery in manager/controllers/utils.go and systemsAdvisoriesQuery in systems_advisories_view.go); these should be removed or replaced with structured logging before merging.
- The down migration 146_system_inventory_new_workspaces_field.down.sql has a syntax issue in the UPDATE statement (
SET workspaces = ARRAY(SELECT ...) , FROM inventory.hosts ih– note the extra comma before FROM) which will cause the rollback to fail and should be corrected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are leftover debug prints using fmt.Printf in non-test code (e.g. buildInventoryQuery in manager/controllers/utils.go and systemsAdvisoriesQuery in systems_advisories_view.go); these should be removed or replaced with structured logging before merging.
- The down migration 146_system_inventory_new_workspaces_field.down.sql has a syntax issue in the UPDATE statement (`SET workspaces = ARRAY(SELECT ...) , FROM inventory.hosts ih` – note the extra comma before FROM) which will cause the rollback to fail and should be corrected.
## Individual Comments
### Comment 1
<location path="manager/controllers/common_attributes.go" line_range="12" />
<code_context>
type OSAttributes struct {
- OS string `json:"os" csv:"os" query:"ih.system_profile->'operating_system'->>'name' || ' ' || coalesce(ih.system_profile->'operating_system'->>'major' || '.' || (ih.system_profile->'operating_system'->>'minor'), '')" order_query:"ih.system_profile->'operating_system'->>'name' || ' ' || coalesce(ih.system_profile->'operating_system'->>'major' || '.' || (ih.system_profile->'operating_system'->>'minor'), '') collate numeric" gorm:"column:os"`
- Rhsm string `json:"rhsm" csv:"rhsm" query:"ih.system_profile->'rhsm'->>'version'" gorm:"column:rhsm"`
+ OS string `json:"os" csv:"os" query:"si.os_name || ' ' || coalesce(si.os_major || '.' || coalesce(si.os_minor::text, 'x'), '')" order_query:"si.os_name || ' ' || coalesce(si.os_major || '.' || coalesce(si.os_minor::text, 'x'), '') collate numeric" gorm:"column:os"`
+ Rhsm string `json:"rhsm" csv:"rhsm" query:"si.rhsm_version" gorm:"column:rhsm"`
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Casting of `si.os_major` is missing in the concatenation expression and will likely fail at runtime.
In `OSAttributes` `query`/`order_query`, `si.os_major` is a `smallint` (`*int16` in `SystemInventory`) and is concatenated directly with text: `si.os_name || ' ' || coalesce(si.os_major || '.' || coalesce(si.os_minor::text, 'x'), '')`. Postgres will reject `smallint || text` without an explicit cast. Please cast `os_major` to text in both places, e.g. `si.os_name || ' ' || coalesce(si.os_major::text || '.' || coalesce(si.os_minor::text, 'x'), '')`.
</issue_to_address>
### Comment 2
<location path="manager/controllers/utils.go" line_range="415-424" />
<code_context>
var cmp string
val := []any{}
+ fmt.Printf("key: %s, values: %v\n", key, values)
switch {
case values[0] == "not_nil":
cmp = " is not null"
case strings.Contains(key, "[sap_sids"):
- cmp = "::jsonb @> ?::jsonb"
- bval, _ := sonic.Marshal(values)
- val = []any{string(bval)}
+ cmp = " && ?::text[]"
+ val = []any{pq.Array(values)}
default:
cmp = " = ?"
val = []any{values[0]}
}
subq := fmt.Sprintf("%s%s", nestedFilters[key], cmp)
+ fmt.Printf("subq: %s\n", subq)
return tx.Where(subq, val...)
}
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Leftover debug `fmt.Printf` calls will spam stdout and may leak filter details.
These `fmt.Printf` calls in `buildInventoryQuery` are fine for local debugging but will be noisy in production and may expose filter values (including `key`, `values`, and `subq`) in logs/stdout. Please remove them or replace them with structured logging behind an appropriate debug/trace log level.
Suggested implementation:
```golang
var cmp string
val := []any{}
```
```golang
switch {
```
```golang
subq := fmt.Sprintf("%s%s", nestedFilters[key], cmp)
return tx.Where(subq, val...)
}
```
If there are other `fmt.Printf` (or similar) debug calls elsewhere in `manager/controllers/utils.go` or related files used in `buildInventoryQuery`, it would be good to remove or gate them behind the project's structured logging mechanism as well. Without seeing the rest of the codebase, I can't update those here.
</issue_to_address>
### Comment 3
<location path="database_admin/migrations/146_system_inventory_new_workspaces_field.down.sql" line_range="10-11" />
<code_context>
+-- rename the old workspaces field to groups
+ALTER TABLE system_inventory ADD COLUMN workspaces TEXT ARRAY CHECK (array_length(workspaces,1) > 0 or workspaces is null);
+
+UPDATE system_inventory si
+SET workspaces = ARRAY(SELECT jsonb_array_elements(ih.groups)->>'id'),
+FROM inventory.hosts ih
+WHERE ih.id = si.inventory_id;
</code_context>
<issue_to_address>
**issue (bug_risk):** The DOWN migration has a syntax error in the UPDATE statement.
The trailing comma after the `SET` expression makes this `UPDATE` invalid SQL and will cause the migration to fail. Removing it should resolve the issue:
```sql
UPDATE system_inventory si
SET workspaces = ARRAY(SELECT jsonb_array_elements(ih.groups)->>'id')
FROM inventory.hosts ih
WHERE ih.id = si.inventory_id;
```
Also confirm that `ARRAY(SELECT ...)` behaves correctly when `ih.groups` is empty or null so the DOWN migration handles hosts with no groups as intended.
</issue_to_address>
### Comment 4
<location path="manager/controllers/systems_advisories_view.go" line_range="93" />
<code_context>
if err != nil {
return nil, nil, nil, err
} // Error handled by method itself
+ fmt.Printf("filters: %+v\n", filters)
sysq, _ = ApplyInventoryFilter(filters, sysq, "sp.inventory_id")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Leftover debug logging of `filters` in systems advisories query.
This will log all filter criteria on every systems-advisories request and is probably only useful during development. Please remove it, or replace it with structured logging at a guarded debug level to avoid noisy or potentially sensitive output.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
database_admin/migrations/146_system_inventory_new_workspaces_field.down.sql
Outdated
Show resolved
Hide resolved
TenSt
force-pushed
the
stepan/RHINENG-21444-remove-ref-to-ih-table
branch
from
736c59b to
7167e42
Compare
TenSt
force-pushed
the
stepan/RHINENG-21444-remove-ref-to-ih-table
branch
from
7167e42 to
77aa87e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2099 +/- ##
==========================================
- Coverage 59.40% 59.39% -0.01%
==========================================
Files 134 134
Lines 8707 8692 -15
==========================================
- Hits 5172 5163 -9
+ Misses 2989 2981 -8
- Partials 546 548 +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:
|
Collaborator
Author
|
/retest |
Dugowitch
reviewed
Mar 11, 2026
Contributor
There was a problem hiding this comment.
Hey Stepan, migration 142 (#1988) replaced ih.groups JSONB (json array of objects with id and name) with si.workspaces TEXT ARRAY containing just the ids. This was done because the names are not used anywhere. What's the reason for reverting it?
Collaborator
|
Unfortunatelly group names are used in search/filtering. It's a reason to introduce them back. |
Collaborator
Author
|
/retest |
1 similar comment
Collaborator
Author
|
/retest |
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.
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Migrate usage of inventory metadata from the external inventory.hosts table into the local system_inventory table, including workspaces/groups, workloads, tags, OS and timestamps, and update schema, data fixtures, and tests to reflect the new source of truth.
New Features:
Bug Fixes:
Enhancements:
Tests: