RHINENG-26529: Revert RHINENG-25147 workspace changes (v3.8.260) #2203
Merged
Conversation
Reviewer's GuideReverts the previous "simplify workspaces" change by restoring 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 2 issues, and left some high level feedback:
- In
ApplyInventoryWorkspaceFilteryou build the subquery using the globalDBhandle instead of the passed-intx, which can unintentionally bypass transaction/replica routing and is safer to base offtx(e.g.,sub := tx.Session(&gorm.Session{NewDB:true}).Where(...)). - There is now a second path (
processWorkspaces) that constructs the grouped-inventory JSON usingParseInventoryGroup, largely duplicating the behavior already used in the RBAC middleware; consider centralizing this group-building logic to avoid future drift in how the JSONB array is formatted.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ApplyInventoryWorkspaceFilter` you build the subquery using the global `DB` handle instead of the passed-in `tx`, which can unintentionally bypass transaction/replica routing and is safer to base off `tx` (e.g., `sub := tx.Session(&gorm.Session{NewDB:true}).Where(...)`).
- There is now a second path (`processWorkspaces`) that constructs the grouped-inventory JSON using `ParseInventoryGroup`, largely duplicating the behavior already used in the RBAC middleware; consider centralizing this group-building logic to avoid future drift in how the JSONB array is formatted.
## Individual Comments
### Comment 1
<location path="base/database/utils.go" line_range="243" />
<code_context>
-func ApplyInventoryWorkspaceFilter(tx *gorm.DB, workspaceIDs []string) *gorm.DB {
- if len(workspaceIDs) == 0 {
- utils.LogWarn("there should always be some workspaces, at least root workspace")
+func ApplyInventoryWorkspaceFilter(tx *gorm.DB, groups map[string]string) *gorm.DB {
+ if _, ok := groups[utils.KeyGrouped]; !ok {
+ if _, ok := groups[utils.KeyUngrouped]; ok {
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the map-based grouping API with a typed filter struct and using only the local `tx` to build the workspace query to make the code more explicit and type-safe.
You can keep the new grouping behavior but simplify the API and query composition to reduce indirection and reliance on magic keys.
### 1. Replace `map[string]string` with a typed struct
Instead of passing `map[string]string` and depending on `utils.KeyGrouped` / `utils.KeyUngrouped`, introduce a small struct and update the call sites. This keeps behavior but makes usage explicit and type-safe.
```go
// new type
type InventoryGroupFilter struct {
Grouped []string
IncludeUngrouped bool
}
```
Update signatures:
```go
func Systems(tx *gorm.DB, accountID int, gf InventoryGroupFilter, joins ...join) *gorm.DB {
tx = tx.Table("system_inventory si").
Joins("JOIN system_patch spatch ON si.id = spatch.system_id AND si.rh_account_id = spatch.rh_account_id").
Where("si.rh_account_id = ?", accountID)
tx = (joinsT)(joins).apply(tx)
return ApplyInventoryWorkspaceFilter(tx, gf)
}
func SystemAdvisories(tx *gorm.DB, accountID int, gf InventoryGroupFilter, joins ...join) *gorm.DB {
tx = Systems(tx, accountID, gf).
Joins("JOIN system_advisories sa on sa.system_id = si.id AND sa.rh_account_id = ?", accountID)
return (joinsT)(joins).apply(tx)
}
func SystemPackages(tx *gorm.DB, accountID int, gf InventoryGroupFilter, joins ...join) *gorm.DB {
tx = Systems(tx, accountID, gf).
Joins("JOIN system_package2 spkg on spkg.system_id = si.id AND spkg.rh_account_id = ?", accountID).
Joins("JOIN package p on p.id = spkg.package_id").
Joins("JOIN package_name pn on pn.id = spkg.name_id")
return (joinsT)(joins).apply(tx)
}
func SystemAdvisoriesByInventoryID(
tx *gorm.DB,
accountID int,
gf InventoryGroupFilter,
inventoryID string,
joins ...join,
) *gorm.DB {
tx = SystemAdvisories(tx, accountID, gf).
Where("si.inventory_id = ?::uuid", inventoryID)
return (joinsT)(joins).apply(tx)
}
```
Where you currently build the `map[string]string`, convert to `InventoryGroupFilter` once at the boundary instead of scattering key lookups.
### 2. Simplify `ApplyInventoryWorkspaceFilter` and avoid global `DB`
Replace map-based control flow and usage of global `DB` with a straightforward, `tx`-only implementation:
```go
func ApplyInventoryWorkspaceFilter(tx *gorm.DB, gf InventoryGroupFilter) *gorm.DB {
// no grouped filters
if len(gf.Grouped) == 0 {
if gf.IncludeUngrouped {
return tx.Where("si.workspaces = '[]'")
}
return tx // no workspace condition
}
cond := tx.Where("si.workspaces @> ANY (?::jsonb[])", gf.Grouped)
if gf.IncludeUngrouped {
cond = cond.Or("si.workspaces = '[]'")
}
return tx.Where(cond)
}
```
This preserves:
- “only ungrouped” behavior when there are no grouped values but `IncludeUngrouped` is true,
- “no filter” behavior when both grouped slice is empty and ungrouped is false,
- combined grouped + ungrouped behavior when both are requested,
while removing:
- magic string keys and map presence checks,
- the subtle use of global `DB` during query composition.
</issue_to_address>
### Comment 2
<location path="manager/middlewares/kessel.go" line_range="38" />
<code_context>
return clientBuilder.Build()
}
+func processWorkspaces(workspaces []*kesselv2.StreamedListObjectsResponse) (map[string]string, error) {
+ defer func(start time.Time) {
+ utils.LogDebug("durationMs", time.Since(start).Milliseconds(), "processed workspaces")
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the stringly-typed map-based inventory group contract with a small typed filter struct and moving SQL-specific formatting into the DB layer to clarify responsibilities and types.
You can simplify this by replacing the stringly-typed `map[string]string` (with a Postgres array literal) with a small typed struct and deferring the SQL formatting to the DB layer. That keeps behavior the same but makes the contract explicit and localizes DB-specific knowledge.
For example:
```go
// utils/inventory_filter.go (or similar)
type InventoryGroupFilter struct {
Grouped []string
// Ungrouped []string // add if/when needed
}
```
Update `processWorkspaces` to return this type:
```go
func processWorkspaces(workspaces []*kesselv2.StreamedListObjectsResponse) (*utils.InventoryGroupFilter, error) {
defer func(start time.Time) {
utils.LogDebug("durationMs", time.Since(start).Milliseconds(), "processed workspaces")
}(time.Now())
groups := make([]string, 0, len(workspaces))
for _, workspace := range workspaces {
group, err := utils.ParseInventoryGroup(&workspace.Object.ResourceId, nil)
if err != nil {
// couldn't marshal inventory group to json
continue
}
groups = append(groups, group)
}
if len(groups) == 0 {
return nil, errors.New("no workspaces found")
}
return &utils.InventoryGroupFilter{Grouped: groups}, nil
}
```
Then store this struct directly in the context:
```go
inventoryGroups, err := processWorkspaces(workspaces)
if err != nil {
utils.LogWarn(err.Error())
c.AbortWithStatusJSON(http.StatusUnauthorized, utils.ErrorResponse{Error: "Missing permission"})
return
}
c.Set(utils.KeyInventoryGroups, inventoryGroups)
```
Finally, move the Postgres-array-literal/SQL formatting into `ApplyInventoryWorkspaceFilter` (or equivalent DB layer function):
```go
func ApplyInventoryWorkspaceFilter(q sq.SelectBuilder, filter *utils.InventoryGroupFilter) sq.SelectBuilder {
if filter == nil || len(filter.Grouped) == 0 {
return q
}
// Build Postgres array literal or use pq.Array as appropriate
groupedLiteral := fmt.Sprintf("{%s}", strings.Join(filter.Grouped, ","))
return q.Where("inventory_group = ANY (?)", groupedLiteral)
}
```
This keeps the same behavior (same eventual SQL), but:
- Removes the cross-layer `map[string]string` contract and magic keys.
- Makes the filter’s shape visible in the type system.
- Localizes the DB-specific representation to the DB layer, avoiding leaky abstractions.
</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 #2203 +/- ##
==========================================
+ Coverage 59.08% 59.23% +0.14%
==========================================
Files 137 137
Lines 8775 8795 +20
==========================================
+ Hits 5185 5210 +25
+ Misses 3042 3037 -5
Partials 548 548
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:
|
TenSt
approved these changes
May 21, 2026
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
Reintroduce JSONB-based workspaces on systems and wire inventory group filtering through the stack instead of workspace IDs.
Enhancements:
Build:
Tests: