Repository navigation
Guard Resources page against unresolved $ref in parameters/requestBody - #1407
ShavinAnjithaAlpha wants to merge 5 commits into
Conversation
resolvedSpec.paths can be incomplete if SwaggerClient.resolve() partially fails (circular or unreachable $ref), causing the Resources page to crash when indexing into paths[target][verb] for referenced parameters/requestBody. Add the same defensive checks used in the original swaggerValidationBehaviour='updated' hotfix. Related: wso2/api-manager#1354
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Guarded parameter and request-body rendering portals/publisher/.../ListParameters.jsx |
Parameter mapping now requires resolvedSpec.paths; request-body rows are rendered from resolved path content for references and directly from operation.requestBody.content otherwise. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Suggested reviewers: ashera96, krishanx92, tharikagithub
🚥 Pre-merge checks | ✅ 5
-
✅ Passed checks (5 passed)
-
Check name Status Explanation Title check ✅ Passed The title clearly summarizes the main change: guarding Resources against unresolved $ref in parameters and requestBody. Description check ✅ Passed The description matches the code changes and explains the ListParameters hardening against incomplete spec resolution. Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
-
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Resources/components/operationComponents/ListParameters.jsx (1)
129-133: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard the resolved operation, not only
resolvedSpec.paths.The new checks are insufficient for incomplete resolution:
resolvedSpec.paths[target],[verb],parameters, orrequestBody.contentmay still be missing, so these accesses can still crash. Conversely, gating the entire parameter map onresolvedSpec.pathshides valid inline parameters. Map inline parameters independently, resolve$refentries with nested guards, and skip unresolved request-body content safely.Proposed fix
- {operation.parameters && resolvedSpec.paths + {operation.parameters && operation.parameters.map((parameter, index) => { const isRefParam = isRef(parameter); const paramCopy = isRefParam - ? resolvedSpec.paths[target][verb].parameters[index] : parameter; + ? resolvedSpec.paths?.[target]?.[verb]?.parameters?.[index] : parameter; + if (!paramCopy) { + return null; + } ... - {operation.requestBody && isRef(operation.requestBody) && resolvedSpec.paths - && Object.entries(resolvedSpec.paths[target][verb].requestBody.content).map( + {operation.requestBody && isRef(operation.requestBody) + && Object.entries( + resolvedSpec.paths?.[target]?.[verb]?.requestBody?.content || {}, + ).map(Also applies to: 205-206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Resources/components/operationComponents/ListParameters.jsx` around lines 129 - 133, Update the parameter and request-body handling around the operation rendering so inline parameters remain mapped even when resolvedSpec.paths is unavailable. Guard each nested lookup through resolvedSpec.paths[target], the verb operation, and its parameters before resolving $ref entries, falling back safely for unresolved references; likewise guard requestBody.content and skip unresolved content instead of accessing missing properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Resources/components/operationComponents/ListParameters.jsx`:
- Around line 129-133: Update the parameter and request-body handling around the
operation rendering so inline parameters remain mapped even when
resolvedSpec.paths is unavailable. Guard each nested lookup through
resolvedSpec.paths[target], the verb operation, and its parameters before
resolving $ref entries, falling back safely for unresolved references; likewise
guard requestBody.content and skip unresolved content instead of accessing
missing properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6aa9f974-9b5f-4f49-bf34-1150c4550205
📒 Files selected for processing (1)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Resources/components/operationComponents/ListParameters.jsx
|
|
Purpose
Harden
ListParameters.jsxin the Publisher Resources page so it can't crash whenresolvedSpecdoesn't contain a fully$ref-resolved spec for the operation being rendered.Related issue
Fixes (proactively) the failure mode reported in wso2/api-manager#1354 — "Resources page break if the API definition has references."
Background / root cause
#1354 was originally caused by the Publisher's
updatedswagger-validation mode:Resources.jsxbuiltresolvedSpecfrom thevalidate-openapiREST endpoint's response, which returns a stripped-down spec (no fullpaths).ListParameters.jsxthen indexed intoresolvedSpec.paths[target][verb]to resolve$refparameters/requestBody, which threw whenpathswas missing.That specific code path (
swaggerValidationBehaviour: 'updated', and the branch that substituted the stripped REST response forresolvedSpec) no longer exists in this repo —Resources.jsxnow always resolves the spec viaSwaggerClient.resolve()before rendering, so the original trigger is gone.However,
ListParameters.jsxnever picked up the defensive guards that were added alongside that original fix (in the oldcarbon-apimgtpublisher.featuremodule, support hotfix branchsupport-9.0.174.x-full).SwaggerClient.resolve()can still fail to fully resolve a spec (circular refs, an unreachable external$ref, a malformed pointer), in which caseresolvedSpec.paths[target][verb]can be undefined or incomplete whileresolvedSpec.specitself is non-empty — which would reproduce the same crash shape through a different trigger. This PR ports those guards.What changed
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Resources/components/operationComponents/ListParameters.jsxoperation.parameterswhenresolvedSpec.pathsis actually present, instead of assuming it's always populated.resolvedSpec.pathsbefore indexing into it, so a missing/partial resolve can no longer throw.