[Schema] - Validate integration field API response shape before Configure step#4190
Conversation
…ng past Connect The Connect step accepted any 200-OK + parseable JSON as success, even shapes the Configure step can't use (null, [], a bare object with no nested array, an array of primitives) — leaving the user on a silently empty, permanently-disabled Configure screen with no explanation. Classify the response right after connecting, reusing the same shape-walking helpers ConfigureDisplayOptions uses to derive its key-selector options (extracted to a shared keyPathHelpers.ts so the two can never diverge), and surface a specific reason on the existing Connect-step failure overlay instead of advancing. Refs #4092
…d Connect step Refs #4092
geodem127
added
bug
Review: [Schema] - Validate integration field API response shape before Configure stepSummary: Solid fix for a real silent-failure bug. The approach — extracting the helper functions, building a classifier that mirrors the downstream rendering logic, and adding a new status instead of overloading What's good
Issues1. Empty-object array produces a self-contradictory error message (logic bug)
2.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }; | ||
| } | ||
| if (!getObjectKeyPaths(apiData[0] as object).length) { | ||
| return { | ||
| ok: false, | ||
| reason: `API returned an array of ${typeof apiData[0]}s. Expected an array of objects.`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Two issues here worth fixing together:
1. Self-contradictory message on [{}]
getObjectKeyPaths({}) returns [], so [{}] fails this check with the message "API returned an array of objects. Expected an array of objects." — which is nonsensical. The case you want to reject here is "array items have no selectable leaf keys", not "array items are not objects". Consider:
| }; | |
| } | |
| if (!getObjectKeyPaths(apiData[0] as object).length) { | |
| return { | |
| ok: false, | |
| reason: `API returned an array of ${typeof apiData[0]}s. Expected an array of objects.`, | |
| }; | |
| } | |
| if (!isObj(apiData[0])) { | |
| return { | |
| ok: false, | |
| reason: `API returned an array of ${typeof apiData[0]}s. Expected an array of objects.`, | |
| }; | |
| } | |
| if (!getObjectKeyPaths(apiData[0]).length) { | |
| return { | |
| ok: false, | |
| reason: | |
| "API returned an array of objects with no selectable keys. Make sure each object has at least one scalar property.", | |
| }; | |
| } |
2. Unsafe as object cast
The original code calls getObjectKeyPaths(apiData[0] as object) without a prior isObj check. If apiData[0] is a primitive, for...in over a boxed primitive returns nothing and the correct result falls out by accident. The suggestion above fixes both by checking isObj explicitly first.
There was a problem hiding this comment.
Yes, point 1 is valid — confirmed. getObjectKeyPaths({}) returns [], so [{}] fell into the "not an object" branch and produced the self-contradictory "API returned an array of objects. Expected an array of objects." Fixed in 7b1f337 by splitting the isObj check from the selectable-keys check.
Coverage ReportOverall Coverage
Changed Files Coverage
|
| }; | ||
| } | ||
| if (!getObjectKeyPaths(apiData[0] as object).length) { | ||
| return { | ||
| ok: false, | ||
| reason: `API returned an array of ${typeof apiData[0]}s. Expected an array of objects.`, | ||
| }; | ||
| } |
…e keys
Splits the isObj check from the key-path check in classifyApiResponse so an
array of empty objects ([{}]) gets an accurate error instead of "API
returned an array of objects. Expected an array of objects."
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review: [Schema] - Validate integration field API response shape before Configure stepOverall this is a well-scoped fix. The Bug — single-object path accepts
|
…ield validation
- classifyApiResponse: the single-object branch accepted {items:[{}]} as
valid because getAllArrayKeyPaths only checks that an array contains
objects, not that they have selectable keys. Now verifies at least one
nested array's first item has selectable keys, using lodash get() to
resolve dotted/bracketed paths the same way ConfigureDisplayOptions does.
- ConnectToApi: the static "invalid" subTitle was dead code since the
render always used invalidReason. Now falls back to it if invalidReason
is ever empty.
- Cypress: added coverage for [{}] and {items:[{}]} invalid shapes.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed all outstanding review feedback:
Cypress
Pushed in f0cfdd5. |
Code ReviewSummary: This PR correctly fixes a silent failure in the integration field wizard where invalid API response shapes (null, empty array, bare object, array of primitives) would silently pass the Connect step and leave the Configure step blank with no explanation. The fix is well-structured: helper extraction into a shared module, a classifier that deliberately mirrors the downstream shape-walking logic, and a new Overall this is clean and well-reasoned work. A couple of minor issues worth addressing: Bug:
|
…tep-is-silently-empty-when
Review: [Schema] - Validate integration field API response shape before Configure stepSummary: This is a solid fix. The root cause was clear, the solution is well-scoped, and the key design decision — making Strengths
Issues1.
|
…tep-is-silently-empty-when # Conflicts: # src/shell/components/FieldTypeIntegration/Configure/ConfigureDisplayOptions.tsx
…d-configure-step-is-silently-empty-when' into fix/4092-schema-integration-field-configure-step-is-silently-empty-when
Code ReviewOverviewThis PR correctly fixes a silent failure in the integration field wizard: API responses that can't produce selectable keys in the Configure step were silently accepted, leaving the display-options panel blank with no explanation. The approach — classify the response using the same shape-walking logic the renderer uses, then surface a specific error — is the right design. Main concern: duplicate helper implementationsThe PR creates The fix is straightforward: export Other issuesDead-code fallback in the subtitle expression (ConnectToApi.tsx ~line 431): {status === "invalid"
? invalidReason || CONNECTION_STATUSES[status].subTitle
: CONNECTION_STATUSES[status].subTitle}
Oversized button icon in buttonIcon: <AutorenewRoundedIcon fontSize="small" sx={{ fontSize: 40 }} />,
Positive notes
|
| }; | ||
| walk(obj, ""); | ||
| return acc; | ||
| }; |
There was a problem hiding this comment.
This file duplicates getObjectKeyPaths and getAllArrayKeyPaths verbatim from keyPathResolution.ts (which already exports them after PR #4188). Two identical implementations means a future bug fix to one won't automatically reach the other — which breaks the "classifier uses the same helpers as the renderer" guarantee stated in the PR description.
The simpler path: export isObj from keyPathResolution.ts and import all three helpers from there in useIntegrationField.ts, then delete this file entirely. ConfigureDisplayOptions.tsx already imports from keyPathResolution.ts, so the single-source guarantee holds with no new file needed.
| subTitle: | ||
| "The API connected, but its response can't be used to configure this field.", | ||
| buttonLabel: "Try Again", | ||
| buttonIcon: <AutorenewRoundedIcon fontSize="small" sx={{ fontSize: 40 }} />, |
There was a problem hiding this comment.
The sx={{ fontSize: 40 }} overrides fontSize="small" (~20 px), producing a 40 px icon inside a size="small" button — visually too large. The same issue exists in the pre-existing failed entry (line 76). The sx prop should be dropped from both:
| buttonIcon: <AutorenewRoundedIcon fontSize="small" sx={{ fontSize: 40 }} />, | |
| buttonIcon: <AutorenewRoundedIcon fontSize="small" />, |
| {status === "invalid" | ||
| ? invalidReason || CONNECTION_STATUSES[status].subTitle | ||
| : CONNECTION_STATUSES[status].subTitle} |
There was a problem hiding this comment.
classifyApiResponse always returns a non-empty reason and fetchApiData always assigns it to invalidReason before setting status: "invalid", so invalidReason is never falsy here. The || CONNECTION_STATUSES[status].subTitle fallback is dead code. Simplify to:
| {status === "invalid" | |
| ? invalidReason || CONNECTION_STATUSES[status].subTitle | |
| : CONNECTION_STATUSES[status].subTitle} | |
| {status === "invalid" | |
| ? invalidReason | |
| : CONNECTION_STATUSES[status].subTitle} |
The dev merge renamed keyPathHelpers.ts to keyPathResolution.ts but the conflict resolution missed useIntegrationField.ts, which still imported the old module (and keyPathResolution.ts didn't export isObj, which useIntegrationField.ts needs).
Code ReviewOverall: Well-structured, targeted bug fix. The discriminated union return type on Bug:
|
| } | ||
| if (!isObj(apiData[0])) { | ||
| return { | ||
| ok: false, |
There was a problem hiding this comment.
Bug: typeof null === "object" produces a misleading message.
If apiData is [null], isObj(null) correctly returns false (the v !== null guard), but then typeof null is "object", so this message reads "API returned an array of objects" — the opposite of what happened.
| ok: false, | |
| const kind = apiData[0] === null ? "null" : typeof apiData[0]; | |
| return { | |
| ok: false, | |
| reason: `API returned an array of ${kind}s. Expected an array of objects.`, | |
| }; |
Also worth adding { name: "array of nulls", body: [null] } to the Cypress invalidShapes table to cover this case.
| {status === "invalid" | ||
| ? invalidReason || CONNECTION_STATUSES[status].subTitle | ||
| : CONNECTION_STATUSES[status].subTitle} |
There was a problem hiding this comment.
Dead fallback — unreachable code.
invalidReason is always non-empty when status === "invalid": setInvalidReason(classification.reason) and setStatus("invalid") are called in the same synchronous block, so React batches them into a single render. The || CONNECTION_STATUSES[status].subTitle branch is never reached.
Per the project's own convention ("Don't add error handling or fallbacks for scenarios that can't happen"), prefer the simpler form:
| {status === "invalid" | |
| ? invalidReason || CONNECTION_STATUSES[status].subTitle | |
| : CONNECTION_STATUSES[status].subTitle} | |
| {status === "invalid" | |
| ? invalidReason | |
| : CONNECTION_STATUSES[status].subTitle} |
| subTitle: | ||
| "The API connected, but its response can't be used to configure this field.", | ||
| buttonLabel: "Try Again", | ||
| buttonIcon: <AutorenewRoundedIcon fontSize="small" sx={{ fontSize: 40 }} />, |
There was a problem hiding this comment.
fontSize="small" is a no-op here. The sx={{ fontSize: 40 }} prop takes higher CSS specificity and overrides the class set by fontSize="small" (~20px → 40px). The explicit MUI prop is misleading since it has no effect.
Same issue exists on the failed entry at line 76 — worth fixing both while touching this file.
| buttonIcon: <AutorenewRoundedIcon fontSize="small" sx={{ fontSize: 40 }} />, | |
| buttonIcon: <AutorenewRoundedIcon sx={{ fontSize: 40 }} />, |
2 participants
RCA:
The Connect step of the Integration field wizard (
useIntegrationField.fetchApiData) treated any 200-OK + parseable JSON response as a successful connection, with no check on whether the response shape could actually be used. Four shapes silently broke the next step (null,[], a bare object with no nested array, an array of primitives):ConfigureDisplayOptions.tsx's key-selector effect derivesrootPathOptions/apiPathOptionsfrom the response, and for all four shapes those come back empty — leaving the "Select Keys to Display" panel completely blank, Done permanently disabled, and zero explanation of what went wrong.FIX:
useIntegrationField.ts— added an exportedclassifyApiResponse(apiData)that classifies a response as usable or not, deliberately built on the same shape-walking helpersConfigureDisplayOptionsuses to derive its own options, so a response classified "ok" is guaranteed to actually produce selectable options downstream (and vice versa).fetchApiDatanow runs this classification before settingstatus: "success"; on failure it sets a newstatus: "invalid"plus a specificinvalidReasonstring instead.Configure/keyPathHelpers.ts(new) — extractedisObj/getObjectKeyPaths/getAllArrayKeyPathsout ofConfigureDisplayOptions.tsx(previously inline/unexported) into a shared module so both the classifier and the renderer use one source of truth.Configure/ConfigureDisplayOptions.tsx— now imports the helpers from the shared module; behavior unchanged.Configure/ConnectToApi.tsx— added an"invalid"entry to the existingCONNECTION_STATUSESfailure-overlay map (reusing the existing UI pattern, no new UI) and renders the dynamicinvalidReasonas the subtitle. The existing button handler already keeps the user on the Connect step for any non-success status, so no other change was needed to block advancement.Regression test added:
cypress/e2e/schema/integration.spec.js—Add Field > Invalid Response Shape > blocks advancing and shows an error for: <shape>(parameterized over the four bad shapes).TESTING PLAN:
Prerequisites: none — the test mocks
**/get-url?url=*with each bad shape.null,[],{name:"x", id:1}, or[1,2,3]: confirm the overlay shows "Unsupported Response Format" with a shape-specific reason, and clicking the action button keeps you on the Connect step (does not advance to Display Type).Expected outcome: all four bad shapes show a specific, actionable error and block advancement; the happy path is unchanged — verified via manual browser testing (fetch-mocked, since the issue's originally-seeded QA endpoints have since been taken down) and the new Cypress spec.
RECORDING:
Related work:
PR #4188 (open) separately adds an "Edit API URL" re-entry point for editing existing integration fields and independently extracts the same three helper functions into its own
keyPathResolution.ts. The two PRs will produce one trivial, one-line import-path merge conflict inConfigureDisplayOptions.tsx— confirmed via a local test-merge, easily resolved by whichever merges second.Closes #4092
🤖 Generated with Claude Code