Skip to content
Open
57 changes: 57 additions & 0 deletions cypress/e2e/schema/integration.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,63 @@ 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: [{}] },
{
name: "single object with nested array of empty objects",
body: { items: [{}] },
},
];

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 @@ -77,6 +77,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 @@ -103,7 +115,7 @@ const ConnectToApi = ({
keyPaths?: IntegrationKeyPaths | null;
}) => {
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 @@ -409,12 +421,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
: 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>
{keyPathsMismatch && (
<Typography
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { get } from "lodash";
import { IntegrationKeyPaths } from "../../../services/types";

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

export const getObjectKeyPaths = (obj: object): string[] => {
const acc: string[] = [];
Expand Down
85 changes: 84 additions & 1 deletion src/shell/components/FieldTypeIntegration/useIntegrationField.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,84 @@
import { useState, useEffect, useCallback, useRef } from "react";
import { get } from "lodash";
import { useGetExternalApiMutation } from "shell/services/cloudFunctions";
import {
isObj,
getObjectKeyPaths,
getAllArrayKeyPaths,
} from "./Configure/keyPathResolution";

// 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)) {
const arrayPaths = getAllArrayKeyPaths(apiData);
if (!arrayPaths.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.",
};
}
const hasUsableArray = arrayPaths.some((path) => {
const item = get(apiData, path)?.[0];
return isObj(item) && getObjectKeyPaths(item).length > 0;
});
if (!hasUsableArray) {
return {
ok: false,
reason:
"API returned an object whose nested arrays contain no selectable keys. Make sure at least one nested array item has scalar properties.",
};
}
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 +96,7 @@ const useIntegrationField = () => {
currentEndpointRef.current = endpoint;
setStatus("connecting");
setApiData(null);
setInvalidReason("");

try {
const response: any = await getExternalApi({
Expand All @@ -33,6 +107,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 +137,14 @@ const useIntegrationField = () => {
currentEndpointRef.current = "";
setApiData(null);
setStatus(null);
setInvalidReason("");
};
}, []);

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