Add compatibility for additionalPropertyMap for MCP servers#1255
Add compatibility for additionalPropertyMap for MCP servers#1255
Conversation
…andatory property error
e19166
requested review from
Krishanx92,
lasanthaS,
piyumaldk and
tharikaGitHub
as code owners
|
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical runtime error in the Publisher Lifecycle page where calling .find() on undefined would cause the page to hang when tenant-level mandatory additional API properties were configured for MCP Server APIs. The issue occurred because MCP servers use additionalPropertiesMap (an object) instead of additionalProperties (an array) for storing custom properties, but the validation code assumed additionalProperties would always be present as an array.
Changes:
- Added defensive checks to prevent calling
.find()on undefinedadditionalProperties - Implemented fallback logic to use
additionalPropertiesMapwhenadditionalPropertiesis undefined - Ensured consistent validation behavior across both data representations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| CustomizedStepper.jsx | Added defensive handling for undefined additionalProperties with fallback to additionalPropertiesMap in mandatory properties validation |
| LifeCycleUpdate.jsx | Added defensive handling for undefined additionalProperties with fallback to additionalPropertiesMap in mandatory properties validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
WalkthroughTwo API details components now support fallback validation for mandatory custom properties. Both Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
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/LifeCycle/LifeCycleUpdate.jsx (1)
265-271:⚠️ Potential issue | 🟠 Major
loadingis never reset tofalseon error — component hangs indefinitely
loadingis only cleared at line 263 inside the happy path ofAPI.getSettings().then(...). IfAPI.getSettings()rejects (inner catch, line 265) or ifPromise.allrejects (outer catch, line 269),loadingstaystrueand the component renders<Progress />forever.🐛 Proposed fix
.catch((error) => { console.error('Error fetching settings:', error); + this.setState({ loading: false }); }); }) .catch((error) => { console.error('Error fetching revisions:', error); + this.setState({ loading: false }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/LifeCycle/LifeCycleUpdate.jsx` around lines 265 - 271, The component leaves the loading state true on errors; ensure you call setLoading(false) in both error handlers so the spinner stops when API.getSettings or the outer Promise.all rejects: add setLoading(false) inside the inner catch for API.getSettings(error) and inside the outer catch for the Promise.all rejection (where revisions fetch errors are handled), keeping the existing console.error logging and any contextual error handling around API.getSettings and Promise.all.
🧹 Nitpick comments (1)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/LifeCycle/LifeCycleUpdate.jsx (1)
242-255: Extract the shared mandatory-property validation into a utility functionThe same two-branch validation logic (array
.find()vs. map lookup with!!coercion) now lives in bothLifeCycleUpdate.jsx(lines 242–255) andCustomizedStepper.jsx(lines 306–318). A single shared helper prevents future drift between the two implementations.♻️ Suggested utility (e.g., in a shared validation module)
+// shared utility, e.g. AppComponents/Shared/Utils/apiValidation.js +export function checkMandatoryPropertiesFilled(api, requiredPropertyNames) { + if (Array.isArray(api.additionalProperties)) { + return requiredPropertyNames.every(name => { + const prop = api.additionalProperties.find(p => p.name === name); + return !!(prop && prop.value !== ''); + }); + } + const map = api.additionalPropertiesMap || {}; + return requiredPropertyNames.every(name => { + const prop = map[name]; + return !!(prop && prop.value !== ''); + }); +}Then both call sites become a one-liner:
- if (api.additionalProperties !== undefined) { - isMandatoryPropertiesAvailable = requiredPropertyNames.every(propertyName => { - const property = api.additionalProperties.find( - (prop) => prop.name === propertyName); - return !!(property && property.value !== ''); - }); - } else { - const addPropsMap = api.additionalPropertiesMap || {}; - isMandatoryPropertiesAvailable = requiredPropertyNames.every(propertyName => { - const property = addPropsMap[propertyName]; - return !!(property && property.value !== ''); - }); - } + isMandatoryPropertiesAvailable = checkMandatoryPropertiesFilled(api, requiredPropertyNames);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/LifeCycle/LifeCycleUpdate.jsx` around lines 242 - 255, Extract the duplicated validation into a shared utility like isMandatoryPropertiesAvailable(requiredPropertyNames, api) that encapsulates both branches (api.additionalProperties array with .find and api.additionalPropertiesMap lookup) and returns a boolean; then replace the inline blocks in LifeCycleUpdate.jsx (the setState/validation block using requiredPropertyNames and api.additionalProperties/api.additionalPropertiesMap) and in CustomizedStepper.jsx (the similar every()/find/map logic) with a single call to that helper, ensuring the helper uses the same !! coercion and checks property.value !== '' to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/LifeCycle/LifeCycleUpdate.jsx`:
- Around line 243-254: The guard using "api.additionalProperties !== undefined"
permits null and causes .find() crashes; change the branch condition to use
Array.isArray(api.additionalProperties) so the array path only runs when
additionalProperties is a real array, and fall back to the map path
(api.additionalPropertiesMap or {}) otherwise; update the logic that sets
isMandatoryPropertiesAvailable in LifeCycleUpdate.jsx (the
api.additionalProperties check and the requiredPropertyNames.every(...) block)
to use Array.isArray(api.additionalProperties) and keep the existing map
handling for non-array values.
In
`@portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/NewOverview/CustomizedStepper.jsx`:
- Around line 306-311: The current check uses "api.additionalProperties !==
undefined" which still allows null and causes .find() to throw; update the guard
to validate it's an array (use Array.isArray(api.additionalProperties)) before
calling setIsMandatoryPropertiesAvailable(...) so the
requiredPropertyNames.every callback only runs when api.additionalProperties is
an actual array; keep the existing logic that uses
api.additionalProperties.find(...) and requiredPropertyNames to determine the
boolean.
---
Outside diff comments:
In
`@portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/LifeCycle/LifeCycleUpdate.jsx`:
- Around line 265-271: The component leaves the loading state true on errors;
ensure you call setLoading(false) in both error handlers so the spinner stops
when API.getSettings or the outer Promise.all rejects: add setLoading(false)
inside the inner catch for API.getSettings(error) and inside the outer catch for
the Promise.all rejection (where revisions fetch errors are handled), keeping
the existing console.error logging and any contextual error handling around
API.getSettings and Promise.all.
---
Nitpick comments:
In
`@portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/LifeCycle/LifeCycleUpdate.jsx`:
- Around line 242-255: Extract the duplicated validation into a shared utility
like isMandatoryPropertiesAvailable(requiredPropertyNames, api) that
encapsulates both branches (api.additionalProperties array with .find and
api.additionalPropertiesMap lookup) and returns a boolean; then replace the
inline blocks in LifeCycleUpdate.jsx (the setState/validation block using
requiredPropertyNames and api.additionalProperties/api.additionalPropertiesMap)
and in CustomizedStepper.jsx (the similar every()/find/map logic) with a single
call to that helper, ensuring the helper uses the same !! coercion and checks
property.value !== '' to preserve behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/LifeCycle/LifeCycleUpdate.jsxportals/publisher/src/main/webapp/source/src/app/components/Apis/Details/NewOverview/CustomizedStepper.jsx
...blisher/src/main/webapp/source/src/app/components/Apis/Details/LifeCycle/LifeCycleUpdate.jsx
Outdated
Show resolved
Hide resolved
...her/src/main/webapp/source/src/app/components/Apis/Details/NewOverview/CustomizedStepper.jsx
Outdated
Show resolved
Hide resolved
|
e19166
changed the title
4 participants
Purpose
This PR fixes a bug where the Publisher Lifecycle page would hang in a loading state for MCP Server APIs when tenant-level mandatory additional API properties are configured. The root cause was calling .find() on api.additionalProperties when that field was undefined for MCP servers (REST APIs always had an array). The PR adds defensive handling for both representations (array and additionalPropertiesMap) and ensures mandatory property validation no longer throws. It also includes: revision/deployment detection was improved to properly detect approved deployments (including the special handling used for API Products and null statuses), avoiding false negatives when determining whether the Publish actions should be enabled.
Goals
Preventing direct .find() calls on potentially undefined values stops the JS runtime error and allows the Lifecycle page to finish loading.
Accurately detecting whether deployments exist (and whether one is APPROVED) ensures the UI enables or disables Publish controls correctly.
Approach
Resolves: wso2/api-manager#4681
Summary by CodeRabbit