RHINENG-26121: update manager to use account_advisory table#2253
Open
rverdile wants to merge 1 commit into
Open
RHINENG-26121: update manager to use account_advisory table#2253rverdile wants to merge 1 commit into
rverdile wants to merge 1 commit into
Conversation
Reviewer's GuideThis PR introduces account_advisory-backed advisory queries, adds workspace-name filtering support, and centralizes query resolution logic for both list and export endpoints, with accompanying tests and configuration flags. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 3 issues, and left some high level feedback:
- In resolveAdvisoriesQuery when EnableAccountAdvisory is true and effectiveWorkspaceIDs is empty, you're building the account-advisory query and then immediately adding
Where("FALSE"); consider avoiding the extra query construction by early-returning an empty condition instead of building and then discarding the query. - resolveWorkspaceNameToIDs currently sets a
Select("DISTINCT si.workspace_id")and then callsPluck("workspace_id", &ids); the explicitSelectwith DISTINCT is redundant becausePluckalready selects the column, so you could move the DISTINCT into the query (e.g.Distinct("workspace_id")) or drop the manual select entirely.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In resolveAdvisoriesQuery when EnableAccountAdvisory is true and effectiveWorkspaceIDs is empty, you're building the account-advisory query and then immediately adding `Where("FALSE")`; consider avoiding the extra query construction by early-returning an empty condition instead of building and then discarding the query.
- resolveWorkspaceNameToIDs currently sets a `Select("DISTINCT si.workspace_id")` and then calls `Pluck("workspace_id", &ids)`; the explicit `Select` with DISTINCT is redundant because `Pluck` already selects the column, so you could move the DISTINCT into the query (e.g. `Distinct("workspace_id")`) or drop the manual select entirely.
## Individual Comments
### Comment 1
<location path="manager/controllers/advisories.go" line_range="217-219" />
<code_context>
}
+func resolveAdvisoriesQuery(db *gorm.DB, account int, workspaceIDs []string, filters Filters) (*gorm.DB, error) {
+ if config.EnableAccountAdvisory && !hasNonGroupInventoryFilter(filters) {
+ effectiveWorkspaceIDs := workspaceIDs
+ if groupNames := getGroupNameFilterValues(filters); len(groupNames) > 0 {
+ var err error
+ effectiveWorkspaceIDs, err = resolveWorkspaceNameToIDs(db, account, groupNames, workspaceIDs)
</code_context>
<issue_to_address>
**question:** Clarify behavior when both workspaceIDs and group_name filter are provided
This logic makes `effectiveWorkspaceIDs` the intersection of the explicit `workspaceIDs` and those resolved from `group_name`. That’s a specific choice and differs from "group_name overrides workspaces" or "workspaces only" semantics. Please confirm this intersection behavior is what we want when both filters are provided, as this is the point where an incorrect assumption would lead to subtle filtering bugs.
</issue_to_address>
### Comment 2
<location path="manager/controllers/advisories.go" line_range="289-298" />
<code_context>
+ return nil
+}
+
+func resolveWorkspaceNameToIDs(db *gorm.DB,
+ accountID int,
+ names []string,
+ allowedWorkspaceIDs []string) ([]string, error) {
+ var ids []string
+ query := db.Table("system_inventory si").
+ Select("DISTINCT si.workspace_id").
+ Where("si.rh_account_id = ?", accountID).
+ Where("si.workspace_name IN (?)", names)
+ if len(allowedWorkspaceIDs) > 0 {
+ query = query.Where("si.workspace_id IN (?)", allowedWorkspaceIDs)
+ }
+ err := query.Pluck("workspace_id", &ids).Error
+ return ids, err
+}
</code_context>
<issue_to_address>
**nitpick:** Avoid redundant SELECT clause when using Pluck
In `resolveWorkspaceNameToIDs`, the `Select("DISTINCT si.workspace_id")` is redundant because `Pluck("workspace_id", &ids)` issues its own SELECT and ignores the prior one. If you need distinct IDs, use `query.Distinct().Pluck("workspace_id", &ids)` or remove the explicit `Select` to keep the query clear.
</issue_to_address>
### Comment 3
<location path="manager/controllers/advisories.go" line_range="216" />
<code_context>
return query
}
+func resolveAdvisoriesQuery(db *gorm.DB, account int, workspaceIDs []string, filters Filters) (*gorm.DB, error) {
+ if config.EnableAccountAdvisory && !hasNonGroupInventoryFilter(filters) {
+ effectiveWorkspaceIDs := workspaceIDs
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `resolveAdvisoriesQuery` to separate workspace/filter normalization from query strategy selection and to hide special-case handling inside builders and higher-level filter helpers for clearer control flow.
The added functionality is solid, but `resolveAdvisoriesQuery` now mixes multiple concerns and conditional branches in one place, which makes it harder to reason about. You can reduce complexity without changing behavior by:
1. **Separating filter/workspace normalization from strategy selection**
2. **Moving the `len(effectiveWorkspaceIDs) == 0` special case into the builder**
3. **Encapsulating the filter-specific helpers behind higher-level names**
### 1) Split `resolveAdvisoriesQuery` into normalization + strategy selection
Right now `resolveAdvisoriesQuery` does both:
- Inspect filters (`hasNonGroupInventoryFilter`, `getGroupNameFilterValues`)
- Resolve workspace IDs from group names
- Choose between account advisory / cached counts / tagged query
You can keep behavior but make the decision tree clearer by extracting the workspace resolution into a small helper, and making `resolveAdvisoriesQuery` mostly about strategy selection:
```go
func resolveAdvisoriesQuery(db *gorm.DB, account int, workspaceIDs []string, filters Filters) (*gorm.DB, error) {
effectiveWorkspaceIDs, err := resolveEffectiveWorkspaceIDs(db, account, workspaceIDs, filters)
if err != nil {
return nil, err
}
if config.EnableAccountAdvisory && !hasNonGroupInventoryFilter(filters) {
middlewares.AdvisoryAccountDataCnt.WithLabelValues("hit").Inc()
return buildQueryAdvisoriesFromAccountAdvisory(db, account, effectiveWorkspaceIDs), nil
}
if !config.EnableAccountAdvisory && !config.DisableCachedCounts &&
!HasInventoryFilter(filters) && len(workspaceIDs) == 0 {
middlewares.AdvisoryAccountDataCnt.WithLabelValues("hit").Inc()
return buildQueryAdvisories(db, account), nil
}
middlewares.AdvisoryAccountDataCnt.WithLabelValues("miss").Inc()
return buildQueryAdvisoriesTagged(db, filters, account, workspaceIDs), nil
}
func resolveEffectiveWorkspaceIDs(db *gorm.DB, account int, workspaceIDs []string, filters Filters) ([]string, error) {
if groupNames := getGroupNameFilterValues(filters); len(groupNames) > 0 {
return resolveWorkspaceNameToIDs(db, account, groupNames, workspaceIDs)
}
return workspaceIDs, nil
}
```
This preserves the feature flags and behavior, but makes the conditional paths easier to follow.
### 2) Move “no workspace” handling into the account advisory builder
The special-case:
```go
if len(effectiveWorkspaceIDs) == 0 {
query := buildQueryAdvisoriesFromAccountAdvisory(db, account, effectiveWorkspaceIDs)
return query.Where("FALSE"), nil
}
```
forces callers to know that “no workspace” means “return nothing”. You can push that logic into the builder so callers don’t need to know the `Where("FALSE")` trick:
```go
func buildQueryAdvisoriesFromAccountAdvisory(db *gorm.DB, account int, workspaceIDs []string) *gorm.DB {
// No effective workspaces => intentionally empty result
if len(workspaceIDs) == 0 {
return database.AdvisoryMetadata(db).
Select(AdvisoriesSelect).
Where("FALSE")
}
subq := buildAccountAdvisorySubquery(db, account, workspaceIDs)
return database.AdvisoryMetadata(db).
Select(AdvisoriesSelect).
Joins("JOIN (?) aad ON am.id = aad.advisory_id", subq).
Joins("LEFT JOIN advisory_severity sev ON am.severity_id = sev.id")
}
```
Then `resolveAdvisoriesQuery` just calls the builder without extra branching.
### 3) Wrap filter helpers in higher-level semantics
Right now `resolveAdvisoriesQuery` needs to know about `"group_name"` and the concrete `TagFilter` / `InventoryFilter` types via:
```go
func hasNonGroupInventoryFilter(filters Filters) bool { ... }
func getGroupNameFilterValues(filters Filters) []string { ... }
```
You can keep these helpers but rename / group them to better express intent, e.g.:
```go
func filtersHaveNonGroupInventoryConstraints(filters Filters) bool {
return hasNonGroupInventoryFilter(filters)
}
func filtersGroupNames(filters Filters) []string {
return getGroupNameFilterValues(filters)
}
```
and use them in `resolveEffectiveWorkspaceIDs` / `resolveAdvisoriesQuery`:
```go
if config.EnableAccountAdvisory && !filtersHaveNonGroupInventoryConstraints(filters) {
// ...
}
if groupNames := filtersGroupNames(filters); len(groupNames) > 0 {
// ...
}
```
Alternatively, place them into a small “filter adapter” file so query resolution doesn’t directly deal with magic keys and raw filter internals.
These focused changes keep all existing feature behavior (including metrics and flags) but reduce the branching and responsibility concentration in `resolveAdvisoriesQuery`, making the code easier to follow and test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2253 +/- ##
==========================================
+ Coverage 58.77% 58.92% +0.14%
==========================================
Files 147 147
Lines 9235 9288 +53
==========================================
+ Hits 5428 5473 +45
- Misses 3235 3241 +6
- Partials 572 574 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rverdile
force-pushed
the
update-manager
branch
2 times, most recently
from
e7e67fa to
71a590c
Compare
TenSt
reviewed
Jul 2, 2026
TenSt
left a comment
TenSt
left a comment
Collaborator
There was a problem hiding this comment.
Looks good! Please address the comments from soucery-ai (resolve them if you don't want to implement them) and I'm ready to approve 👍
Contributor
Author
|
thanks @TenSt! removed the extra select from the query |
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.
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Switch advisory listing and export to use precomputed per-workspace advisory counts from the account_advisory table when enabled, while preserving legacy behavior as a fallback.
New Features:
Enhancements:
Tests: