Feat(RHOAIENG-56731): type guards strengthening #7204
Merged
openshift-merge-bot[bot] merged 4 commits intoopendatahub-io:mainfrom Apr 16, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds defensive normalization and stricter serialization plus tests across backend and frontend. Backend: MaaS client and repositories ensure slices are non-nil and normalize nested nils; subscription model JSON tags now always emit Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Actionable IssuesCWE-391: Unchecked Error Condition — packages/maas/bff/internal/repositories/policies.go
CWE-253: Incorrect Check for Missing Optional Data — packages/maas/bff/internal/integrations/maas/maas_client.go and frontend guards
CWE-1025: Comparison Using Wrong Factors — packages/maas/frontend/src/app/api/api-keys.ts and subscriptions.ts
CWE-772: Missing Release of Resource after Effective Lifetime — packages/maas/bff/internal/integrations/maas/maas_client_test.go
CWE-330: Use of Insufficiently Random Values — frontend tests/mocks
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/maas/bff/internal/repositories/subscriptions.go (1)
362-373:⚠️ Potential issue | 🟠 MajorHandle
unstructured.NestedSliceerrors instead of silently treating malformed specs as empty lists.Both
convertUnstructuredToSubscriptionandconvertUnstructuredToAuthPolicydeclare error return types but never propagate errors fromunstructured.Nested*calls. When CR fields have wrong types, conversion silently degrades to emptygroups/modelRefs, masking invalid data and returning misleading responses.Proposed fix pattern
- ownerGroups, _, _ := unstructured.NestedSlice(content, "spec", "owner", "groups") - for _, g := range ownerGroups { + ownerGroups, found, err := unstructured.NestedSlice(content, "spec", "owner", "groups") + if err != nil { + return nil, fmt.Errorf("invalid spec.owner.groups: %w", err) + } + if found { + for _, g := range ownerGroups { if gMap, ok := g.(map[string]interface{}); ok { if name, ok := gMap["name"].(string); ok { sub.Owner.Groups = append(sub.Owner.Groups, models.GroupReference{Name: name}) } } - } + } + }Apply the same
found/errhandling to:
spec.modelRefs(line 373)spec.modelRefsandspec.subjects.groupsinconvertUnstructuredToAuthPolicy(lines 456, 471)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/bff/internal/repositories/subscriptions.go` around lines 362 - 373, convertUnstructuredToSubscription and convertUnstructuredToAuthPolicy currently ignore errors from unstructured.NestedSlice, causing malformed CR fields to be treated as empty slices; update each NestedSlice call (e.g., the ownerGroups/modelRefs calls that populate sub.Owner.Groups and sub.ModelRefs, and the subjects.groups calls used in convertUnstructuredToAuthPolicy) to capture the returned found/err values, check err and return a descriptive error from the conversion function if err != nil or if found is false but the field exists with a wrong type, and only iterate the slice when no error occurred; ensure you reference the same variable names (ownerGroups, modelRefs, subjectsGroups, sub.Owner.Groups, sub.ModelRefs) so the added checks are placed exactly around those conversions.
🧹 Nitpick comments (2)
packages/maas/bff/internal/models/subscription_test.go (1)
13-31: Add explicit zero-value (nilslice) serialization coverage forgroups.Current tests only assert behavior for explicitly initialized empty slices. Add assertions for
OwnerSpec{}andSubjectSpec{}sonullvs[]behavior is locked intentionally and future tag/normalization changes are caught early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/bff/internal/models/subscription_test.go` around lines 13 - 31, Add tests to assert nil (zero-value) slice serialization for groups so it yields "groups":[] rather than null: for OwnerSpec and SubjectSpec marshal the zero-value structs (OwnerSpec{} and SubjectSpec{}) using json.Marshal and assert the resulting JSON contains `"groups":[]` (mirror the existing TestOwnerSpec_EmptyGroupsSerializesAsArray and TestSubjectSpec_EmptyGroupsSerializesAsArray patterns). Name the new tests clearly (e.g., TestOwnerSpec_NilGroupsSerializesAsArray and TestSubjectSpec_NilGroupsSerializesAsArray) and reuse the same error handling and strings.Contains checks to fail on unexpected marshal errors or wrong output.packages/maas/frontend/src/app/api/subscriptions.ts (1)
166-167: Add diagnostics when filtering invalid response items.Both list endpoints now silently drop invalid entries. Add warning/metric emission with dropped-count so backend schema drift is visible instead of quietly degrading data.
As per coding guidelines, "REVIEW PRIORITIES: ... Bug-prone patterns and error handling gaps".
Also applies to: 288-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/frontend/src/app/api/subscriptions.ts` around lines 166 - 167, When filtering response.data in the isModArchResponse check, count how many items are invalid (use Array.isArray(response.data), compute valid = response.data.filter(isMaaSSubscription) and dropped = response.data.length - valid.length), then return valid.map(normalizeSubscription) but first emit a warning and a metric when dropped > 0 (e.g., logger.warn("Dropped %d subscription items due to schema mismatch", dropped) and metrics.increment("maas.subscriptions.dropped_items", dropped)); apply the same change to the other occurrence that uses isModArchResponse/isMaaSSubscription/normalizeSubscription so backend schema drift is visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/maas/bff/internal/repositories/policies.go`:
- Around line 52-53: The current loop in the repository that logs conversion
failures (where you call r.logger.Warn("Failed to convert MaaSAuthPolicy",
slog.String("name", item.GetName()), slog.Any("error", err)) and continue) can
return an empty slice and nil error if every item fails; change the logic in the
policy conversion function (the method performing the loop that references
r.logger.Warn and item.GetName()) to track successful conversions (e.g., append
to a results slice or increment a success counter) and after the loop, if the
input list was non-empty but the results slice is empty, return a non-nil error
indicating all policy conversions failed so callers can surface schema/uptream
breakage; keep the current behavior of skipping individual bad items when at
least one conversion succeeds.
In `@packages/maas/frontend/src/app/api/api-keys.ts`:
- Around line 22-26: The type guard isAPIKey currently only checks id, name, and
status loosely; update it to validate all required APIKey fields (e.g., ensure
creationDate exists and is a string (and optionally a valid ISO date), ensure
status is one of the allowed enum values such as 'active'|'revoked'|'pending'),
and keep the isRecord(v) check; modify the predicate for isAPIKey to explicitly
verify these properties (id, name, creationDate, status) and their
types/accepted values so malformed API payloads are rejected before reaching UI
logic.
In `@packages/maas/frontend/src/app/api/subscriptions.ts`:
- Around line 50-51: isGroupsSpec currently allows v.groups == null which can
let null flow into consumers typed as OwnerSpec/SubjectSpec; update the
predicate to require an array (remove the v.groups == null branch) so it only
returns true when Array.isArray(v.groups) && v.groups.every(isGroupRef), thereby
guaranteeing callers receive a groups array, or alternatively, if you must
accept null here, add an explicit normalization step where the validated object
is converted into OwnerSpec/SubjectSpec (replace null/undefined groups with [])
before returning or casting to the typed object.
---
Outside diff comments:
In `@packages/maas/bff/internal/repositories/subscriptions.go`:
- Around line 362-373: convertUnstructuredToSubscription and
convertUnstructuredToAuthPolicy currently ignore errors from
unstructured.NestedSlice, causing malformed CR fields to be treated as empty
slices; update each NestedSlice call (e.g., the ownerGroups/modelRefs calls that
populate sub.Owner.Groups and sub.ModelRefs, and the subjects.groups calls used
in convertUnstructuredToAuthPolicy) to capture the returned found/err values,
check err and return a descriptive error from the conversion function if err !=
nil or if found is false but the field exists with a wrong type, and only
iterate the slice when no error occurred; ensure you reference the same variable
names (ownerGroups, modelRefs, subjectsGroups, sub.Owner.Groups, sub.ModelRefs)
so the added checks are placed exactly around those conversions.
---
Nitpick comments:
In `@packages/maas/bff/internal/models/subscription_test.go`:
- Around line 13-31: Add tests to assert nil (zero-value) slice serialization
for groups so it yields "groups":[] rather than null: for OwnerSpec and
SubjectSpec marshal the zero-value structs (OwnerSpec{} and SubjectSpec{}) using
json.Marshal and assert the resulting JSON contains `"groups":[]` (mirror the
existing TestOwnerSpec_EmptyGroupsSerializesAsArray and
TestSubjectSpec_EmptyGroupsSerializesAsArray patterns). Name the new tests
clearly (e.g., TestOwnerSpec_NilGroupsSerializesAsArray and
TestSubjectSpec_NilGroupsSerializesAsArray) and reuse the same error handling
and strings.Contains checks to fail on unexpected marshal errors or wrong
output.
In `@packages/maas/frontend/src/app/api/subscriptions.ts`:
- Around line 166-167: When filtering response.data in the isModArchResponse
check, count how many items are invalid (use Array.isArray(response.data),
compute valid = response.data.filter(isMaaSSubscription) and dropped =
response.data.length - valid.length), then return
valid.map(normalizeSubscription) but first emit a warning and a metric when
dropped > 0 (e.g., logger.warn("Dropped %d subscription items due to schema
mismatch", dropped) and metrics.increment("maas.subscriptions.dropped_items",
dropped)); apply the same change to the other occurrence that uses
isModArchResponse/isMaaSSubscription/normalizeSubscription so backend schema
drift is visible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 00d6d664-2a4a-421f-986f-ce440eff8d7b
📒 Files selected for processing (11)
packages/maas/bff/internal/integrations/maas/maas_client.gopackages/maas/bff/internal/integrations/maas/maas_client_test.gopackages/maas/bff/internal/models/subscription.gopackages/maas/bff/internal/models/subscription_test.gopackages/maas/bff/internal/repositories/policies.gopackages/maas/bff/internal/repositories/subscriptions.gopackages/maas/bff/internal/repositories/subscriptions_mock.gopackages/maas/frontend/src/app/api/__tests__/api-keys.spec.tspackages/maas/frontend/src/app/api/__tests__/subscriptions.spec.tspackages/maas/frontend/src/app/api/api-keys.tspackages/maas/frontend/src/app/api/subscriptions.ts
claudialphonse78
force-pushed
the
feat/RHOAIENG-56731-type-guards
branch
from
176a6cf to
04549a5
Compare
claudialphonse78
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7204 +/- ##
==========================================
- Coverage 64.89% 64.88% -0.02%
==========================================
Files 2442 2448 +6
Lines 76015 76069 +54
Branches 19171 19184 +13
==========================================
+ Hits 49330 49355 +25
- Misses 26685 26714 +29
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
|
My subscription with |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dpanshug
approved these changes
Apr 15, 2026
Contributor
dpanshug
left a comment
dpanshug
left a comment
There was a problem hiding this comment.
/lgtm This fixes null field crash scenarios properly and tested well with the unit tests. Great work!
claudialphonse78
force-pushed
the
feat/RHOAIENG-56731-type-guards
branch
from
e68dcb5 to
789d125
Compare
Contributor
|
/retest |
Contributor
|
All PipelineRuns for this commit have already succeeded. Use |
Contributor
|
/retest |
Contributor
|
/approve |
claudialphonse78
force-pushed
the
feat/RHOAIENG-56731-type-guards
branch
from
789d125 to
13cfd6a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/maas/frontend/src/app/api/subscriptions.ts (1)
166-167: Emit diagnostics when malformed entries are dropped.Line 166-167 and Line 288-289 silently discard invalid items via
.filter(...). Add a dropped-count warning/metric so backend contract regressions are visible during triage.Also applies to: 288-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/frontend/src/app/api/subscriptions.ts` around lines 166 - 167, When filtering response.data you currently drop invalid items silently; compute droppedCount as response.data.length minus filtered.length where you call isMaaSSubscription, log or emit a metric (e.g., call a diagnostics/metrics helper like metrics.increment or processLogger.warn) with context (endpoint/response id and droppedCount) and then map normalizeSubscription over the filtered items; apply the same change in the second occurrence that uses isMaaSSubscription (lines ~288-289) so backend contract regressions are visible during triage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/maas/bff/internal/integrations/maas/maas_client_test.go`:
- Line 21: The test currently ignores the error returned by url.Parse(ts.URL);
update the test setup to capture and handle the error (e.g., u, err :=
url.Parse(ts.URL) and if err != nil { t.Fatalf("parse test server URL: %v", err)
}) so errcheck passes; update the code around the url.Parse call in
maas_client_test.go (where variable u and ts are used) to fail the test on parse
failure.
In `@packages/maas/frontend/src/app/api/subscriptions.ts`:
- Around line 130-131: The validator currently allows
ModelRefInfo.token_rate_limits to be null (via the condition v.token_rate_limits
== null) which permits null to flow into typed UserSubscription payloads; change
the validation so token_rate_limits must be either undefined or an array of
valid items (use Array.isArray(v.token_rate_limits) &&
v.token_rate_limits.every(isTokenRateLimitInfo) and remove the acceptance of
null), and also normalize any outgoing payloads where UserSubscription objects
are constructed (the code that returns/filter objects around token_rate_limits)
to coerce null/undefined into an empty array and filter with
isTokenRateLimitInfo (e.g., set token_rate_limits = (Array.isArray(value) ?
value.filter(isTokenRateLimitInfo) : [])) so no null escapes.
---
Nitpick comments:
In `@packages/maas/frontend/src/app/api/subscriptions.ts`:
- Around line 166-167: When filtering response.data you currently drop invalid
items silently; compute droppedCount as response.data.length minus
filtered.length where you call isMaaSSubscription, log or emit a metric (e.g.,
call a diagnostics/metrics helper like metrics.increment or processLogger.warn)
with context (endpoint/response id and droppedCount) and then map
normalizeSubscription over the filtered items; apply the same change in the
second occurrence that uses isMaaSSubscription (lines ~288-289) so backend
contract regressions are visible during triage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a4840ff9-e4b2-40d5-a16b-7a2ba4881492
📒 Files selected for processing (11)
packages/maas/bff/internal/integrations/maas/maas_client.gopackages/maas/bff/internal/integrations/maas/maas_client_test.gopackages/maas/bff/internal/models/subscription.gopackages/maas/bff/internal/models/subscription_test.gopackages/maas/bff/internal/repositories/policies.gopackages/maas/bff/internal/repositories/subscriptions.gopackages/maas/bff/internal/repositories/subscriptions_mock.gopackages/maas/frontend/src/app/api/__tests__/api-keys.spec.tspackages/maas/frontend/src/app/api/__tests__/subscriptions.spec.tspackages/maas/frontend/src/app/api/api-keys.tspackages/maas/frontend/src/app/api/subscriptions.ts
✅ Files skipped from review due to trivial changes (4)
- packages/maas/bff/internal/models/subscription_test.go
- packages/maas/bff/internal/repositories/subscriptions_mock.go
- packages/maas/frontend/src/app/api/tests/subscriptions.spec.ts
- packages/maas/bff/internal/integrations/maas/maas_client.go
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/maas/bff/internal/repositories/policies.go
- packages/maas/frontend/src/app/api/api-keys.ts
- packages/maas/bff/internal/repositories/subscriptions.go
- packages/maas/bff/internal/models/subscription.go
- packages/maas/frontend/src/app/api/tests/api-keys.spec.ts
| t.Helper() | ||
| ts := httptest.NewServer(handler) | ||
| t.Cleanup(ts.Close) | ||
| u, _ := url.Parse(ts.URL) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate golangci config:"
fd -a '.golangci.yml' '.golangci.yaml' || true
echo
echo "Inspect ignored Parse errors in Go files:"
rg -nP --type go 'url\.Parse\(' -C2
rg -nP --type go ':\=\s*[^,\n]+,\s*_\s*:=?\s*url\.Parse\(' -C2 || trueRepository: opendatahub-io/odh-dashboard
Length of output: 9529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for golangci config in maas package ==="
find packages/maas -name '.golangci.yaml' -o -name '.golangci.yml' 2>/dev/null | head -20
echo
echo "=== Check if there's a linter config at package root ==="
ls -la packages/maas/ | grep -i config || true
echo
echo "=== Read the test file to understand context ==="
head -50 packages/maas/bff/internal/integrations/maas/maas_client_test.go
echo
echo "=== Check if this test is actually running url.Parse and relying on success ==="
sed -n '15,35p' packages/maas/bff/internal/integrations/maas/maas_client_test.goRepository: opendatahub-io/odh-dashboard
Length of output: 2529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read the .golangci.yaml in packages/maas/bff ==="
cat packages/maas/bff/.golangci.yaml
echo
echo "=== Check if errcheck is enabled ==="
grep -i "errcheck" packages/maas/bff/.golangci.yaml || echo "errcheck not found in config"Repository: opendatahub-io/odh-dashboard
Length of output: 528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check golangci-lint documentation for std-error-handling preset ==="
echo "The 'std-error-handling' preset in golangci-lint typically enables:"
echo "- errcheck (checks for unchecked errors)"
echo "- errchkjson (checks for json.Unmarshal unchecked errors)"
echo "- nilerr (checks for explicit nil returns)"
echo
echo "=== Verify the actual usage pattern - is ts.URL guaranteed valid? ==="
grep -n "httptest.NewServer" packages/maas/bff/internal/integrations/maas/maas_client_test.go
echo
echo "=== Check if NewMaasClient expects a non-nil *url.URL ==="
grep -A5 "func NewMaasClient" packages/maas/bff/internal/integrations/maas/maas_client.go | head -20Repository: opendatahub-io/odh-dashboard
Length of output: 736
Handle url.Parse error in test setup to comply with errcheck linter.
Line 21 ignores the error return from url.Parse(). The std-error-handling preset is enabled in .golangci.yaml, which activates errcheck. Even though httptest.NewServer produces a valid URL, explicit error handling is required for linter compliance and consistency with the rest of the codebase.
Suggested fix
- u, _ := url.Parse(ts.URL)
+ u, err := url.Parse(ts.URL)
+ if err != nil {
+ t.Fatalf("failed to parse test server URL: %v", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| u, _ := url.Parse(ts.URL) | |
| u, err := url.Parse(ts.URL) | |
| if err != nil { | |
| t.Fatalf("failed to parse test server URL: %v", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/maas/bff/internal/integrations/maas/maas_client_test.go` at line 21,
The test currently ignores the error returned by url.Parse(ts.URL); update the
test setup to capture and handle the error (e.g., u, err := url.Parse(ts.URL)
and if err != nil { t.Fatalf("parse test server URL: %v", err) }) so errcheck
passes; update the code around the url.Parse call in maas_client_test.go (where
variable u and ts are used) to fail the test on parse failure.
Comment on lines
+130
to
131
| (v.token_rate_limits == null || | ||
| (Array.isArray(v.token_rate_limits) && v.token_rate_limits.every(isTokenRateLimitInfo))); |
Contributor
There was a problem hiding this comment.
Prevent null from escaping as ModelRefInfo.token_rate_limits.
Line 130 accepts token_rate_limits == null, and Line 288-289 returns filtered objects without normalization. That allows null to flow in typed UserSubscription payloads and can still break array consumers.
Suggested fix
+const normalizeUserSubscription = (sub: UserSubscription): UserSubscription => ({
+ ...sub,
+ model_refs: sub.model_refs.map((ref) => ({
+ ...ref,
+ token_rate_limits: Array.isArray(ref.token_rate_limits) ? ref.token_rate_limits : [],
+ })),
+});
+
export const listUserSubscriptions =
(hostPath = '') =>
(opts: APIOptions): Promise<UserSubscription[]> =>
handleRestFailures(
restGET(hostPath, `${URL_PREFIX}/api/${BFF_API_VERSION}/subscriptions`, {}, opts),
).then((response) => {
if (isModArchResponse<unknown>(response) && Array.isArray(response.data)) {
- return response.data.filter(isUserSubscription);
+ return response.data.filter(isUserSubscription).map(normalizeUserSubscription);
}
throw new Error('Invalid response format');
});As per coding guidelines, "Validate all API responses before rendering".
Also applies to: 288-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/maas/frontend/src/app/api/subscriptions.ts` around lines 130 - 131,
The validator currently allows ModelRefInfo.token_rate_limits to be null (via
the condition v.token_rate_limits == null) which permits null to flow into typed
UserSubscription payloads; change the validation so token_rate_limits must be
either undefined or an array of valid items (use
Array.isArray(v.token_rate_limits) &&
v.token_rate_limits.every(isTokenRateLimitInfo) and remove the acceptance of
null), and also normalize any outgoing payloads where UserSubscription objects
are constructed (the code that returns/filter objects around token_rate_limits)
to coerce null/undefined into an empty array and filter with
isTokenRateLimitInfo (e.g., set token_rate_limits = (Array.isArray(value) ?
value.filter(isTokenRateLimitInfo) : [])) so no null escapes.
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dpanshug, Griffin-Sullivan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 4fadc71
into
opendatahub-io:main
54 checks passed
katieperry4
pushed a commit
to katieperry4/odh-dashboard
that referenced
this pull request
Apr 17, 2026
* fix:added guards checks on the backend for subscriptions, policies and test cases * fix:added guards checks on frontend and test case changes * fix:added test case fixes * review:code rabbit review changes
openshift-merge-bot Bot
pushed a commit
that referenced
this pull request
Apr 17, 2026
* fix:added guards checks on the backend for subscriptions, policies and test cases * fix:added guards checks on frontend and test case changes * fix:added test case fixes * review:code rabbit review changes Co-authored-by: Claudia Alphonse <claudialphonse@gmail.com>
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.
Closes RHOAIENG-56731
Description
Addresses UI-breaking bugs caused by
nullor missing array fields in APIresponses, both from the upstream MaaS API and from K8s-backed resources.
Previously a single malformed item or unexpected
nullfield was enough to crash an entire list page.The fix is applied at two levels:
BFF — send clean data to the frontend
nullarray fields from the upstream MaaS API (model_refs,token_rate_limits,data) to empty arrays before forwarding to the frontendMaaSSubscription,MaaSAuthPolicy) always haveinitialized slices for
owner.groups,modelRefs, andsubjects.groups— nevernullomitemptyfromOwnerSpec.GroupsandSubjectSpec.Groupsso an emptygroup list always serializes as
[]rather than being dropped from the JSONListPoliciesskip a malformed policy and log a warning instead of returninga 500 for the entire list (same pattern already used for model refs)
Frontend — handle the unexpected gracefully
every() + throwpattern in subscription list fetching with.filter()so one bad item is dropped silently instead of taking down the whole page
isAPIKeyguard to also validatestatussince it's shown in thetable
nullacceptance to a few guards (isGroupsSpec,isModelRefInfo) as asafety net in case the BFF ever sends unexpected data
How Has This Been Tested?
omitemptyto thegroup fields
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
Bug Fixes
Tests