Skip to content
Open
53 changes: 53 additions & 0 deletions cypress/e2e/schema/integration.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,59 @@ describe("Integration Field", () => {
.should("have.length", 1);
});
});

context("Invalid Response Shape", () => {
const invalidShapes = [
Comment thread
geodem127 marked this conversation as resolved.
{ name: "null", body: null },
{ name: "empty array", body: [] },
{
name: "single object with no nested array",
body: { name: "Solo", id: 1 },
},
{ name: "array of primitives", body: [1, 2, 3, 4, 5] },
{ name: "array of objects with no selectable keys", body: [{}] },
];

invalidShapes.forEach(({ name, body }) => {
it(`blocks advancing and shows an error for: ${name}`, () => {
cy.intercept("**/get-url?url=*", { statusCode: 200, body }).as(
"getUrl"
);

cy.visit(`/schema/${Cypress.env("modelZUID")}/fields`);
cy.getBySelector("AddFieldBtn").click();
cy.getBySelector("FieldItem_integration").click();
cy.getBySelector("integrationConfigureButton").click();
cy.getBySelector("integrationFormDialog").should("exist");
cy.getBySelector("integrationEndpointInput")
.find("input")
.clear()
.type(ENDPOINTS.generic);

cy.getBySelector("integrationConnectButton").click();
cy.wait("@getUrl");

cy.getBySelector("integrationConnectionStatusContainer").should(
"exist"
);
cy.getBySelector("integrationConnectionStatusLabel").should(
"contain",
"Unsupported Response Format"
);
cy.getBySelector("integrationConnectionStatusSubtitle")
.invoke("text")
.should("not.be.empty");

// The action button must keep the user on the Connect step, not
// advance to Display Type where the Configure dead-end would occur.
cy.getBySelector("integrationConnectionStatusButton").click();
cy.getBySelector("integrationEndpointInput").should("exist");
cy.getBySelector("integrationSelectDisplayOptionsDialog").should(
"not.exist"
);
});
});
});
});

describe("Item Selection", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,52 +30,7 @@ import { get } from "lodash";
import { IntegrationKeyPaths, IntegrationTypes } from "../../../services/types";
import KeyPathSelector from "./KeyPathSelector";
import DisplayCard from "../Shared/DisplayCard";

const isObj = (v: unknown): v is object => typeof v === "object" && v !== null;

const getObjectKeyPaths = (obj: object): string[] => {
const acc: string[] = [];
const walk = (node: object, prefix: string): void => {
const arr = Array.isArray(node);
for (const key in node) {
if (!Object.prototype.hasOwnProperty.call(node, key)) continue;
const value = (node as Record<string, unknown>)[key];
const path = prefix
? arr
? `${prefix}[${key}]`
: `${prefix}.${key}`
: key;
if (isObj(value)) walk(value, path);
else acc.push(path);
}
};
walk(obj, "");
return acc;
};

const getAllArrayKeyPaths = (obj: object): string[] => {
const acc: string[] = [];
const walk = (node: object, prefix: string): void => {
const arr = Array.isArray(node);
for (const key in node) {
if (!Object.prototype.hasOwnProperty.call(node, key)) continue;
const value = (node as Record<string, unknown>)[key];
const path = prefix
? arr
? `${prefix}[${key}]`
: `${prefix}.${key}`
: key;
if (Array.isArray(value)) {
if (value.some(isObj)) acc.push(path);
walk(value, path);
} else if (isObj(value)) {
walk(value, path);
}
}
};
walk(obj, "");
return acc;
};
import { getObjectKeyPaths, getAllArrayKeyPaths } from "./keyPathHelpers";

const ConfigureDisplayOptions = ({
type,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,18 @@ const CONNECTION_STATUSES: {
variant: "contained",
color: "primary",
},
invalid: {
icon: (
<InfoRoundedIcon fontSize="large" color="error" sx={{ fontSize: 40 }} />
),
title: "Unsupported Response Format",
subTitle:
"The API connected, but its response can't be used to configure this field.",
Comment thread
geodem127 marked this conversation as resolved.
Comment thread
geodem127 marked this conversation as resolved.
buttonLabel: "Try Again",
buttonIcon: <AutorenewRoundedIcon fontSize="small" sx={{ fontSize: 40 }} />,
Comment thread
geodem127 marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
buttonIcon: <AutorenewRoundedIcon fontSize="small" sx={{ fontSize: 40 }} />,
buttonIcon: <AutorenewRoundedIcon fontSize="small" />,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
buttonIcon: <AutorenewRoundedIcon fontSize="small" sx={{ fontSize: 40 }} />,
buttonIcon: <AutorenewRoundedIcon sx={{ fontSize: 40 }} />,

variant: "contained",
color: "primary",
},
Comment thread
geodem127 marked this conversation as resolved.
};

const ConnectToApi = ({
Expand All @@ -95,7 +107,7 @@ const ConnectToApi = ({
closeForm?: () => void;
}) => {
const focusRef = useRef<string>("url");
const { data, status, fetchApiData } = useIntegrationField();
const { data, status, invalidReason, fetchApiData } = useIntegrationField();

const [isValidUrl, setIsValidUrl] = useState(true);
const [reqAborted, setReqAborted] = useState<boolean>(false);
Expand Down Expand Up @@ -395,12 +407,15 @@ const ConnectToApi = ({
{CONNECTION_STATUSES[status].title}
</Typography>
<Typography
data-cy="integrationConnectionStatusSubtitle"
variant="body2"
color="text.primary"
fontWeight={400}
textAlign="center"
>
{CONNECTION_STATUSES[status].subTitle}
{status === "invalid"
? invalidReason
: CONNECTION_STATUSES[status].subTitle}
Comment thread
geodem127 marked this conversation as resolved.
Comment thread
geodem127 marked this conversation as resolved.
Comment on lines +430 to +432

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
{status === "invalid"
? invalidReason || CONNECTION_STATUSES[status].subTitle
: CONNECTION_STATUSES[status].subTitle}
{status === "invalid"
? invalidReason
: CONNECTION_STATUSES[status].subTitle}

Comment on lines +430 to +432

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
{status === "invalid"
? invalidReason || CONNECTION_STATUSES[status].subTitle
: CONNECTION_STATUSES[status].subTitle}
{status === "invalid"
? invalidReason
: CONNECTION_STATUSES[status].subTitle}

</Typography>
</Box>
<Button
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
export const isObj = (v: unknown): v is object =>
typeof v === "object" && v !== null;

export const getObjectKeyPaths = (obj: object): string[] => {
const acc: string[] = [];
const walk = (node: object, prefix: string): void => {
const arr = Array.isArray(node);
for (const key in node) {
if (!Object.prototype.hasOwnProperty.call(node, key)) continue;
const value = (node as Record<string, unknown>)[key];
const path = prefix
? arr
? `${prefix}[${key}]`
: `${prefix}.${key}`
: key;
if (isObj(value)) walk(value, path);
else acc.push(path);
}
};
walk(obj, "");
return acc;
};

export const getAllArrayKeyPaths = (obj: object): string[] => {
const acc: string[] = [];
const walk = (node: object, prefix: string): void => {
const arr = Array.isArray(node);
for (const key in node) {
if (!Object.prototype.hasOwnProperty.call(node, key)) continue;
const value = (node as Record<string, unknown>)[key];
const path = prefix
? arr
? `${prefix}[${key}]`
: `${prefix}.${key}`
: key;
if (Array.isArray(value)) {
if (value.some(isObj)) acc.push(path);
walk(value, path);
} else if (isObj(value)) {
walk(value, path);
}
}
};
walk(obj, "");
return acc;
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

72 changes: 71 additions & 1 deletion src/shell/components/FieldTypeIntegration/useIntegrationField.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,71 @@
import { useState, useEffect, useCallback, useRef } from "react";
import { useGetExternalApiMutation } from "shell/services/cloudFunctions";
import {
isObj,
getObjectKeyPaths,
getAllArrayKeyPaths,
} from "./Configure/keyPathHelpers";

// Mirrors the shape-walking that ConfigureDisplayOptions does when deriving
// its key-selector options, so a response classified "ok" here is guaranteed
// to actually produce selectable options there (and vice versa).
export function classifyApiResponse(
apiData: unknown
): { ok: true } | { ok: false; reason: string } {
if (apiData === null || apiData === undefined) {
return {
ok: false,
reason: "API returned no data. Expected a JSON array of objects.",
};
}

if (Array.isArray(apiData)) {
if (!apiData.length) {
return {
ok: false,
reason:
"API returned an empty array. Add at least one item to the response to configure display options.",
};
}
if (!isObj(apiData[0])) {
return {
ok: false,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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.

reason: `API returned an array of ${typeof apiData[0]}s. Expected an array of objects.`,
Comment thread
geodem127 marked this conversation as resolved.
};
}
Comment on lines +29 to +36

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
};
}
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geodem127 is point 1 valid?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.",
};
}
return { ok: true };
}

if (isObj(apiData)) {
if (!getAllArrayKeyPaths(apiData).length) {
return {
ok: false,
reason:
"API returned a single object with no array of objects nested inside. Expected either an array root or an object containing an array of objects.",
};
}
return { ok: true };
Comment thread
geodem127 marked this conversation as resolved.
}

return {
ok: false,
reason: `API returned a ${typeof apiData}. Expected a JSON array of objects.`,
};
}

const useIntegrationField = () => {
const [apiData, setApiData] = useState<any>(null);
const [status, setStatus] = useState<
"connecting" | "success" | "failed" | null
"connecting" | "success" | "failed" | "invalid" | null
>(null);
const [invalidReason, setInvalidReason] = useState<string>("");
const [getExternalApi, { data }] = useGetExternalApiMutation();

const currentEndpointRef = useRef<string>("");
Expand All @@ -23,6 +83,7 @@ const useIntegrationField = () => {
currentEndpointRef.current = endpoint;
setStatus("connecting");
setApiData(null);
setInvalidReason("");

try {
const response: any = await getExternalApi({
Expand All @@ -33,6 +94,13 @@ const useIntegrationField = () => {

if (!response?.error) {
const responseData = await response.data;
const classification = classifyApiResponse(responseData);
if (classification.ok === false) {
setApiData(null);
setInvalidReason(classification.reason);
setStatus("invalid");
return;
}
setApiData(responseData);
setStatus("success");
} else {
Expand All @@ -56,12 +124,14 @@ const useIntegrationField = () => {
currentEndpointRef.current = "";
setApiData(null);
setStatus(null);
setInvalidReason("");
};
}, []);

return {
data: apiData,
status,
invalidReason,
fetchApiData,
};
};
Expand Down
Loading