-
Notifications
You must be signed in to change notification settings - Fork 5
[Schema] - Validate integration field API response shape before Configure step #4190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from 5 commits
897fe7c
7509f19
7b1f337
f0cfdd5
459b16c
6ce8830
5b4c988
cd90b87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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.", | ||||||||||||||||||||||||||
|
geodem127 marked this conversation as resolved.
geodem127 marked this conversation as resolved.
|
||||||||||||||||||||||||||
| buttonLabel: "Try Again", | ||||||||||||||||||||||||||
| buttonIcon: <AutorenewRoundedIcon fontSize="small" sx={{ fontSize: 40 }} />, | ||||||||||||||||||||||||||
|
geodem127 marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Same issue exists on the
Suggested change
|
||||||||||||||||||||||||||
| variant: "contained", | ||||||||||||||||||||||||||
| color: "primary", | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
|
geodem127 marked this conversation as resolved.
|
||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const ConnectToApi = ({ | ||||||||||||||||||||||||||
|
|
@@ -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); | ||||||||||||||||||||||||||
|
|
@@ -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 | ||||||||||||||||||||||||||
| : CONNECTION_STATUSES[status].subTitle} | ||||||||||||||||||||||||||
|
geodem127 marked this conversation as resolved.
geodem127 marked this conversation as resolved.
Comment on lines
+430
to
+432
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Comment on lines
+430
to
+432
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dead fallback — unreachable code.
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
|
||||||||||||||||||||||||||
| </Typography> | ||||||||||||||||||||||||||
| </Box> | ||||||||||||||||||||||||||
| <Button | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| 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; | ||
| }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file duplicates The simpler path: export |
||
| 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/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, | ||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: If
Suggested change
Also worth adding |
||||||||||||||||||||||||||||||||||||||||||||
| reason: `API returned an array of ${typeof apiData[0]}s. Expected an array of objects.`, | ||||||||||||||||||||||||||||||||||||||||||||
|
geodem127 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+29
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two issues here worth fixing together: 1. Self-contradictory message on
Suggested change
2. Unsafe
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @geodem127 is point 1 valid?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, point 1 is valid — confirmed. |
||||||||||||||||||||||||||||||||||||||||||||
| 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 }; | ||||||||||||||||||||||||||||||||||||||||||||
|
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>(""); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -23,6 +96,7 @@ const useIntegrationField = () => { | |||||||||||||||||||||||||||||||||||||||||||
| currentEndpointRef.current = endpoint; | ||||||||||||||||||||||||||||||||||||||||||||
| setStatus("connecting"); | ||||||||||||||||||||||||||||||||||||||||||||
| setApiData(null); | ||||||||||||||||||||||||||||||||||||||||||||
| setInvalidReason(""); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||
| const response: any = await getExternalApi({ | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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 { | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -56,12 +137,14 @@ const useIntegrationField = () => { | |||||||||||||||||||||||||||||||||||||||||||
| currentEndpointRef.current = ""; | ||||||||||||||||||||||||||||||||||||||||||||
| setApiData(null); | ||||||||||||||||||||||||||||||||||||||||||||
| setStatus(null); | ||||||||||||||||||||||||||||||||||||||||||||
| setInvalidReason(""); | ||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||
| }, []); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||
| data: apiData, | ||||||||||||||||||||||||||||||||||||||||||||
| status, | ||||||||||||||||||||||||||||||||||||||||||||
| invalidReason, | ||||||||||||||||||||||||||||||||||||||||||||
| fetchApiData, | ||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.