RHINENG-26547: re-apply workspace related code changes#2231
Merged
TenSt merged 2 commits intoJun 11, 2026
Merged
Conversation
Reviewer's GuideMigrate inventory workspace handling from JSON-based 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:
- ApplyInventoryWorkspaceFilter currently always applies
WHERE si.workspace_id IN (?)even whenworkspaceIDsis nil/empty; sincefindInventoryWorkspacesreturns nil for “all workspaces” this will unintentionally filter out all systems instead of leaving the query unbounded—consider treatingnil(or empty) as no workspace restriction and only adding the WHERE clause when there is at least one ID. - The RBAC middleware still sets
KeyInventoryGroupsin the Gin context, but all call sites now readKeyInventoryWorkspacesinstead; you can simplify and avoid confusion by removing theKeyInventoryGroupsconstant and the associatedc.Setcall if groups are no longer used anywhere.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ApplyInventoryWorkspaceFilter currently always applies `WHERE si.workspace_id IN (?)` even when `workspaceIDs` is nil/empty; since `findInventoryWorkspaces` returns nil for “all workspaces” this will unintentionally filter out all systems instead of leaving the query unbounded—consider treating `nil` (or empty) as no workspace restriction and only adding the WHERE clause when there is at least one ID.
- The RBAC middleware still sets `KeyInventoryGroups` in the Gin context, but all call sites now read `KeyInventoryWorkspaces` instead; you can simplify and avoid confusion by removing the `KeyInventoryGroups` constant and the associated `c.Set` call if groups are no longer used anywhere.
## Individual Comments
### Comment 1
<location path="base/database/utils.go" line_range="244-248" />
<code_context>
- db := DB.Where("si.workspaces @> ANY (?::jsonb[])", groups[utils.KeyGrouped])
- if _, ok := groups[utils.KeyUngrouped]; ok {
- db = db.Or("si.workspaces = '[]'")
+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")
}
- return tx.Where(db)
+ return tx.Where("si.workspace_id IN (?)", workspaceIDs)
}
</code_context>
<issue_to_address>
**issue:** Applying `IN (?)` with an empty workspaceID slice can lead to invalid SQL or unintended filtering.
With an empty `workspaceIDs`, this still runs `WHERE si.workspace_id IN (?)` with no values. Depending on the dialect/driver this may produce invalid SQL (`IN ()`), or `IN (NULL)` that filters out all rows, which is both unpredictable and implicit. Since you already log for the empty case, consider returning `tx` unchanged (or an explicit hard-deny) when `len(workspaceIDs) == 0` so the behavior is well-defined and intentional.
</issue_to_address>
### Comment 2
<location path="manager/controllers/systems_advisories_view.go" line_range="159-160" />
<code_context>
Distinct(systemsAdvisoriesSelect).
Joins("LEFT JOIN system_advisories sa ON am.id = sa.advisory_id AND sa.rh_account_id = ? AND sa.status_id = 0", acc)
if len(systems) > 0 {
- query = query.Joins(fmt.Sprintf("%s AND si.inventory_id in (?)", siJoin), systems)
+ query = query.Joins(fmt.Sprintf("%s AND si.inventory_id in (?::uuid)", siJoin), systems)
} else {
query = query.Joins(siJoin)
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `IN (?::uuid)` with a slice of `SystemID` strings may generate invalid SQL or cast issues.
Because `systems` is a slice of `SystemID` (a `string` alias), GORM will expand `IN (?)` into multiple placeholders. With `(?::uuid)`, the `::uuid` will only apply to the last placeholder, yielding invalid SQL. Since the Go type is already `string`, the cast is usually unnecessary if the DB column is UUID. Either drop the `::uuid` or switch to a single array parameter with `si.inventory_id = ANY (?::uuid[])` if you truly need an explicit cast.
</issue_to_address>
### Comment 3
<location path="manager/middlewares/kessel_test.go" line_range="69" />
<code_context>
utils.CoreCfg.KesselAuthClientSecret = originalKesselAuthClientSecret
}
-func TestProcessWorkspaces(t *testing.T) {
- id1 := "aaaaaaaa-0000-0000-0000-000000000001"
- id2 := "bbbbbbbb-0000-0000-0000-000000000002"
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for the new Kessel workspace handling now that processWorkspaces is removed
With `processWorkspaces` and `TestProcessWorkspaces` removed, `hasPermissionKessel` now builds the workspace slice inline and sets `utils.KeyInventoryWorkspaces`. To keep equivalent coverage for the new behavior, please:
- Add a test where the mocked Kessel client returns multiple workspaces and assert the full slice of workspace IDs on the context.
- Add a test where the mocked Kessel client returns zero workspaces and assert a 401 response and that `KeyInventoryWorkspaces` is not set.
These can live alongside `TestHasPermissionKessel` to fully exercise the new representation.
Suggested implementation:
```golang
"app/base/utils"
"net/http"
"net/http/httptest"
"testing"
"google.golang.org/grpc"
```
```golang
utils.CoreCfg.KesselAuthClientSecret = originalKesselAuthClientSecret
}
func TestHasPermissionKessel_MultipleWorkspacesSetsContext(t *testing.T) {
// arrange: mocked Kessel client returns multiple workspaces
workspaces := []string{
"aaaaaaaa-0000-0000-0000-000000000001",
"bbbbbbbb-0000-0000-0000-000000000002",
}
mockClient := &MockKesselClient{
WorkspacesToReturn: workspaces,
}
// newKesselMiddlewareWithClient should follow the same pattern used by existing tests
middleware := newKesselMiddlewareWithClient(mockClient)
handlerCalled := false
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
handlerCalled = true
got := r.Context().Value(utils.KeyInventoryWorkspaces)
if got == nil {
t.Fatalf("expected %q to be set on context, got nil", utils.KeyInventoryWorkspaces)
}
gotSlice, ok := got.([]string)
if !ok {
t.Fatalf("expected context value type []string, got %T", got)
}
if len(gotSlice) != len(workspaces) {
t.Fatalf("expected %d workspaces, got %d", len(workspaces), len(gotSlice))
}
for i, id := range workspaces {
if gotSlice[i] != id {
t.Errorf("workspace[%d]: expected %q, got %q", i, id, gotSlice[i])
}
}
})
req := httptest.NewRequest(http.MethodGet, "/some/path", nil)
rr := httptest.NewRecorder()
// act
middleware(handler).ServeHTTP(rr, req)
// assert
if !handlerCalled {
t.Errorf("expected wrapped handler to be called")
}
if rr.Code != http.StatusOK {
t.Errorf("expected status %d, got %d", http.StatusOK, rr.Code)
}
}
func TestHasPermissionKessel_NoWorkspacesReturnsUnauthorized(t *testing.T) {
// arrange: mocked Kessel client returns zero workspaces
mockClient := &MockKesselClient{
WorkspacesToReturn: []string{},
}
middleware := newKesselMiddlewareWithClient(mockClient)
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Fatalf("handler should not be called when there are no workspaces")
})
req := httptest.NewRequest(http.MethodGet, "/some/path", nil)
rr := httptest.NewRecorder()
// act
middleware(handler).ServeHTTP(rr, req)
// assert
if rr.Code != http.StatusUnauthorized {
t.Fatalf("expected status %d when no workspaces, got %d", http.StatusUnauthorized, rr.Code)
}
// If the middleware only sets KeyInventoryWorkspaces for authorized requests,
// then the absence of a downstream handler invocation is enough to guarantee
// it won't be visible to handlers. If the middleware mutates req.Context()
// even on unauthorized paths, adjust this assertion accordingly.
if got := req.Context().Value(utils.KeyInventoryWorkspaces); got != nil {
t.Fatalf("expected %q not to be set on the original request context, got %#v", utils.KeyInventoryWorkspaces, got)
}
}
```
1. Replace `MockKesselClient` and its `WorkspacesToReturn` field with whatever mock type and configuration your existing `TestHasPermissionKessel` uses to control the list of workspaces returned by the Kessel client.
2. Implement `newKesselMiddlewareWithClient(mockClient)` (or rename this call) to match the constructor/helper already used in your current Kessel middleware tests so the middleware under test is configured with the mocked client.
3. If `hasPermissionKessel` sets `utils.KeyInventoryWorkspaces` onto the context using a different type than `[]string` (e.g. `[]uuid.UUID` or a custom struct), adjust the type assertion and expected slice accordingly in `TestHasPermissionKessel_MultipleWorkspacesSetsContext`.
4. For the “no workspaces” test, if the middleware mutates the request context even on unauthorized responses, instead of checking `req.Context()`, you should assert the absence of `utils.KeyInventoryWorkspaces` in whatever request object the middleware actually passes to the next handler (e.g. by having the handler record the context if it is ever invoked, or by exposing a helper used in other tests).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
TenSt
force-pushed
the
stepan/RHINENG-26547-re-apply-workspace-related-code-changes
branch
from
5f4a256 to
65478f5
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2231 +/- ##
==========================================
- Coverage 58.56% 58.34% -0.22%
==========================================
Files 140 140
Lines 8973 8967 -6
==========================================
- Hits 5255 5232 -23
- Misses 3166 3187 +21
+ Partials 552 548 -4
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:
|
MichaelMraka
approved these changes
Jun 11, 2026
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
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.
This PR:
Summary by Sourcery
Replace inventory group-based access and filtering with workspace ID/name columns across API, database, and RBAC, and simplify workspace filtering using the new indexed workspace_id field.
Enhancements:
Build:
Deployment:
Tests: