From 042904c632abe0a7ca95e0a9170b169ee68d7502 Mon Sep 17 00:00:00 2001 From: adolfo-pd Date: Fri, 13 Dec 2024 13:57:41 -0800 Subject: [PATCH 1/7] Review and update all prop value validation for accuracy --- .../examples/nextjs/package-lock.json | 2 +- .../connect-react/src/hooks/form-context.tsx | 78 ++++---- packages/connect-react/src/hooks/use-app.tsx | 69 +------ packages/connect-react/src/utils/component.ts | 177 ++++++++++++++++++ 4 files changed, 216 insertions(+), 110 deletions(-) create mode 100644 packages/connect-react/src/utils/component.ts diff --git a/packages/connect-react/examples/nextjs/package-lock.json b/packages/connect-react/examples/nextjs/package-lock.json index c56ed62c8171b..c5246f24c095f 100644 --- a/packages/connect-react/examples/nextjs/package-lock.json +++ b/packages/connect-react/examples/nextjs/package-lock.json @@ -23,7 +23,7 @@ }, "../..": { "name": "@pipedream/connect-react", - "version": "1.0.0-preview.9", + "version": "1.0.0-preview.10", "license": "MIT", "dependencies": { "@pipedream/sdk": "workspace:^", diff --git a/packages/connect-react/src/hooks/form-context.tsx b/packages/connect-react/src/hooks/form-context.tsx index a13acc54e74b6..b559a5b2e1f73 100644 --- a/packages/connect-react/src/hooks/form-context.tsx +++ b/packages/connect-react/src/hooks/form-context.tsx @@ -9,7 +9,10 @@ import type { import { useFrontendClient } from "./frontend-client-context"; import type { ComponentFormProps } from "../components/ComponentForm"; import type { FormFieldContext } from "./form-field-context"; -import { appPropError } from "./use-app"; +import { + appPropErrors, arrayPropErrors, booleanPropErrors, integerPropErrors, + stringPropErrors, +} from "../utils/component"; export type DynamicProps = { id: string; configurableProps: T; }; // TODO @@ -19,6 +22,7 @@ export type FormContext = { configuredProps: ConfiguredProps; dynamicProps?: DynamicProps; // lots of calls require dynamicProps?.id, so need to expose dynamicPropsQueryIsFetching?: boolean; + errors: Record; fields: Record>; id: string; isValid: boolean; @@ -168,55 +172,44 @@ export const FormContextProvider = ({ // so can't rely on that base control form validation const propErrors = (prop: ConfigurableProp, value: unknown): string[] => { const errs: string[] = []; - if (value === undefined) { - if (!prop.optional) { - errs.push("required"); - } - } else if (prop.type === "integer") { // XXX type should be "number"? we don't support floats otherwise... - if (typeof value !== "number") { - errs.push("not a number"); - } else { - if (prop.min != null && value < prop.min) { - errs.push("number too small"); - } - if (prop.max != null && value > prop.max) { - errs.push("number too big"); - } - } - } else if (prop.type === "boolean") { - if (typeof value !== "boolean") { - errs.push("not a boolean"); - } - } else if (prop.type === "string") { - type StringProp = ConfigurableProp & { - min?: number; - max?: number; - } - const { - min = 1, max, - } = prop as StringProp; - if (typeof value !== "string") { - errs.push("not a string"); - } else { - if (value.length < min) { - errs.push(`string length must be at least ${min} characters`); - } - if (max && value.length > max) { - errs.push(`string length must not exceed ${max} characters`); - } - } - } else if (prop.type === "app") { + if (prop.optional || prop.hidden || prop.disabled) return [] + if (typeof value === "undefined") { + return [ + "required", + ] + } + if (prop.type === "app") { const field = fields[prop.name] if (field) { const app = field.extra.app - const err = appPropError({ + errs.push(...(appPropErrors({ + prop, value, app, - }) - if (err) errs.push(err) + }) ?? [])) } else { errs.push("field not registered") } + } else if (prop.type === "boolean") { + errs.push(...(booleanPropErrors({ + prop, + value, + }) ?? [])) + } else if (prop.type === "integer") { + errs.push(...(integerPropErrors({ + prop, + value, + }) ?? [])) + } else if (prop.type === "string") { + errs.push(...(stringPropErrors({ + prop, + value, + }) ?? [])) + } else if (prop.type === "string[]") { + errs.push(...(arrayPropErrors({ + prop, + value, + }) ?? [])) } return errs; }; @@ -377,6 +370,7 @@ export const FormContextProvider = ({ configuredProps, dynamicProps, dynamicPropsQueryIsFetching, + errors, fields, optionalPropIsEnabled, optionalPropSetEnabled, diff --git a/packages/connect-react/src/hooks/use-app.tsx b/packages/connect-react/src/hooks/use-app.tsx index 4dca554fb3246..8437bdb625593 100644 --- a/packages/connect-react/src/hooks/use-app.tsx +++ b/packages/connect-react/src/hooks/use-app.tsx @@ -1,16 +1,13 @@ import { useQuery, type UseQueryOptions, } from "@tanstack/react-query"; +import type { GetAppResponse } from "@pipedream/sdk"; import { useFrontendClient } from "./frontend-client-context"; -import type { - AppRequestResponse, AppResponse, ConfigurablePropApp, - PropValue, -} from "@pipedream/sdk"; /** * Get details about an app */ -export const useApp = (slug: string, opts?:{ useQueryOpts?: Omit, "queryKey" | "queryFn">;}) => { +export const useApp = (slug: string, opts?:{ useQueryOpts?: Omit, "queryKey" | "queryFn">;}) => { const client = useFrontendClient(); const query = useQuery({ queryKey: [ @@ -26,65 +23,3 @@ export const useApp = (slug: string, opts?:{ useQueryOpts?: Omit & { - oauth_access_token?: string -} - -function getCustomFields(app: AppResponse): AppCustomField[] { - const isOauth = app.auth_type === "oauth" - const userDefinedCustomFields = JSON.parse(app.custom_fields_json || "[]") - if ("extracted_custom_fields_names" in app && app.extracted_custom_fields_names) { - const extractedCustomFields = ((app as AppResponseWithExtractedCustomFields).extracted_custom_fields_names || []).map( - (name) => ({ - name, - }), - ) - userDefinedCustomFields.push(...extractedCustomFields) - } - return userDefinedCustomFields.map((cf: AppCustomField) => { - return { - ...cf, - // if oauth, treat all as optional (they are usually needed for getting access token) - optional: cf.optional || isOauth, - } - }) -} - -export function appPropError(opts: { value: any, app: AppResponse | undefined }): string | undefined { - const { app, value } = opts - if (!app) { - return "app field not registered" - } - if (!value) { - return "no app configured" - } - if (typeof value !== "object") { - return "not an app" - } - const _value = value as PropValue<"app"> - if ("authProvisionId" in _value && !_value.authProvisionId) { - if (app.auth_type) { - if (app.auth_type === "oauth" && !(_value as OauthAppPropValue).oauth_access_token) { - return "missing oauth token" - } - if (app.auth_type === "oauth" || app.auth_type === "keys") { - for (const cf of getCustomFields(app)) { - if (!cf.optional && !_value[cf.name]) { - return "missing custom field" - } - } - } - return "no auth provision configured" - } - } -} diff --git a/packages/connect-react/src/utils/component.ts b/packages/connect-react/src/utils/component.ts new file mode 100644 index 0000000000000..2b0892e9e2cbc --- /dev/null +++ b/packages/connect-react/src/utils/component.ts @@ -0,0 +1,177 @@ +import type { + App, ConfigurableProp, ConfigurablePropApp, ConfigurablePropBoolean, ConfigurablePropInteger, ConfigurablePropString, ConfigurablePropStringArray, PropValue, +} from "@pipedream/sdk"; + +export type PropOptionValue = { + __lv: { + value: T + } +} + +export function valueFromOption(value: T | PropOptionValue): T | undefined | null { + if (typeof value === "object" && value && "__lv" in value) { + return (value as PropOptionValue).__lv.value + } + return value +} + +export type PropOption = { + emitValue: T | PropOptionValue +} +export type PropOptions = { + selectedOptions: PropOption[] +} + +export function valuesFromOptions(value: unknown | T[] | PropOptions): T[] { + if (typeof value === "object" && value && "selectedOptions" in value && Array.isArray(value.selectedOptions)) { + const selectedOptions = value.selectedOptions as PropOption[] + const results = [] + for (const so of selectedOptions) { + if (typeof so === "object" && so && "emitValue" in so) { + const emitValue = so.emitValue as T | PropOptionValue + if (typeof emitValue === "object" && emitValue && "__lv" in emitValue) { + results.push(emitValue.__lv.value) + } + results.push(emitValue) + } + throw "unexpected value" + } + } + if (!Array.isArray(value)) + throw "unexpected value" + return value as T[] +} + +export type ValidationOpts = { + prop: T + value: unknown + app?: App +} + +export function arrayPropErrors(opts: ValidationOpts): string[] | undefined { + const _values = valuesFromOptions(opts.value) + if (typeof _values === "undefined") { + return [ + "required", + ] + } + if (Array.isArray(_values) && !_values.length) return [ + "empty array", + ] +} + +export function booleanPropErrors(opts: ValidationOpts): string[] | undefined { + const _value = valueFromOption(opts.value) + if (_value == null || typeof _value === "undefined") return [ + "required", + ] +} + +export function integerPropErrors(opts: ValidationOpts): string[] | undefined { + const { + prop, value: valueOpt, + } = opts + const value = valueFromOption(valueOpt) + + if (value == null || typeof value === "undefined") return [ + "required", + ] + + const _value: number = typeof value === "number" + ? value + : parseInt(String(value)) + + if (_value !== _value) return [ + "not a number", + ] // NaN + const errs = [] + if (typeof prop.min === "number" && _value < prop.min) errs.push("number too small") + if (typeof prop.max === "number" && _value > prop.max) errs.push("number too big") + return errs +} + +export function stringPropErrors(opts: ValidationOpts): string[] | undefined { + const _value = valueFromOption(opts.value) + if (!_value) return [ + "string must not be empty", + ] +} + +type AppWithExtractedCustomFields = App & { + extracted_custom_fields_names: string[] +} + +type AppCustomField = { + name: string + optional?: boolean +} + +type OauthAppPropValue = PropValue<"app"> & { + oauth_access_token: string +} + +type AppPropValueWithCustomFields = PropValue<"app"> & { + [K in T[number]["name"]]: T[number] +} + +function getCustomFields(app: App): AppCustomField[] { + const isOauth = app.auth_type === "oauth" + const userDefinedCustomFields = JSON.parse(app.custom_fields_json || "[]") + if ("extracted_custom_fields_names" in app && app.extracted_custom_fields_names) { + const extractedCustomFields = ((app as AppWithExtractedCustomFields).extracted_custom_fields_names || []).map( + (name) => ({ + name, + }), + ) + userDefinedCustomFields.push(...extractedCustomFields) + } + return userDefinedCustomFields.map((cf: AppCustomField) => { + return { + ...cf, + // if oauth, treat all as optional (they are usually needed for getting access token) + optional: cf.optional || isOauth, + } + }) +} + +export function appPropErrors(opts: ValidationOpts): string[] | undefined { + const { + app, value, + } = opts + if (!app) { + return [ + "app field not registered", + ] + } + if (!value) { + return [ + "no app configured", + ] + } + if (typeof value !== "object") { + return [ + "not an app", + ] + } + const _value = value as PropValue<"app"> + if ("authProvisionId" in _value && !_value.authProvisionId) { + if (app.auth_type) { + const errs = [] + if (app.auth_type === "oauth" && !(_value as OauthAppPropValue).oauth_access_token) { + errs.push("missing oauth token") + } + if (app.auth_type === "oauth" || app.auth_type === "keys") { + const customFields = getCustomFields(app) + const _valueWithCustomFields = _value as AppPropValueWithCustomFields + for (const cf of customFields) { + if (!cf.optional && !_valueWithCustomFields[cf.name]) { + errs.push(`missing custom field: ${cf.name}`) + } + } + } + if (app.auth_type !== "none") + errs.push("no auth provision configured") + return errs + } + } +} From a84b407b584a1477e207cac84f54039485ebd2e5 Mon Sep 17 00:00:00 2001 From: adolfo-pd Date: Fri, 13 Dec 2024 14:18:53 -0800 Subject: [PATCH 2/7] Ignore string prop values that don't match string type --- .../connect-react/src/hooks/form-context.tsx | 5 ----- packages/connect-react/src/utils/component.ts | 18 ++++++++++++------ 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/packages/connect-react/src/hooks/form-context.tsx b/packages/connect-react/src/hooks/form-context.tsx index b559a5b2e1f73..292be68c579d6 100644 --- a/packages/connect-react/src/hooks/form-context.tsx +++ b/packages/connect-react/src/hooks/form-context.tsx @@ -173,11 +173,6 @@ export const FormContextProvider = ({ const propErrors = (prop: ConfigurableProp, value: unknown): string[] => { const errs: string[] = []; if (prop.optional || prop.hidden || prop.disabled) return [] - if (typeof value === "undefined") { - return [ - "required", - ] - } if (prop.type === "app") { const field = fields[prop.name] if (field) { diff --git a/packages/connect-react/src/utils/component.ts b/packages/connect-react/src/utils/component.ts index 2b0892e9e2cbc..d7a887ebbace4 100644 --- a/packages/connect-react/src/utils/component.ts +++ b/packages/connect-react/src/utils/component.ts @@ -50,12 +50,12 @@ export type ValidationOpts = { export function arrayPropErrors(opts: ValidationOpts): string[] | undefined { const _values = valuesFromOptions(opts.value) - if (typeof _values === "undefined") { + if (!opts.prop.default && typeof _values === "undefined") { return [ "required", ] } - if (Array.isArray(_values) && !_values.length) return [ + if (!opts.prop.default && Array.isArray(_values) && !_values.length) return [ "empty array", ] } @@ -73,7 +73,7 @@ export function integerPropErrors(opts: ValidationOpts) } = opts const value = valueFromOption(valueOpt) - if (value == null || typeof value === "undefined") return [ + if (!prop.default && value == null || typeof value === "undefined") return [ "required", ] @@ -92,9 +92,15 @@ export function integerPropErrors(opts: ValidationOpts) export function stringPropErrors(opts: ValidationOpts): string[] | undefined { const _value = valueFromOption(opts.value) - if (!_value) return [ - "string must not be empty", - ] + + if (!opts.prop.default) { + if (typeof _value === "undefined" || _value == null) return [ + "required", + ] + if (!String(_value).length) return [ + "string must not be empty", + ] + } } type AppWithExtractedCustomFields = App & { From 98a2706b10c14bfd77269dba8e191636568ab986 Mon Sep 17 00:00:00 2001 From: adolfo-pd Date: Fri, 13 Dec 2024 14:40:16 -0800 Subject: [PATCH 3/7] bad error check --- packages/connect-react/src/utils/component.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/connect-react/src/utils/component.ts b/packages/connect-react/src/utils/component.ts index d7a887ebbace4..d22e6bbbbc4fa 100644 --- a/packages/connect-react/src/utils/component.ts +++ b/packages/connect-react/src/utils/component.ts @@ -33,8 +33,9 @@ export function valuesFromOptions(value: unknown | T[] | PropOptions): T[] results.push(emitValue.__lv.value) } results.push(emitValue) + } else { + throw "unexpected value" } - throw "unexpected value" } } if (!Array.isArray(value)) From 1cdf77ff7358b5dc48d6568b30a08244f6bc2b0c Mon Sep 17 00:00:00 2001 From: adolfo-pd Date: Fri, 13 Dec 2024 14:42:33 -0800 Subject: [PATCH 4/7] CR suggestions --- packages/connect-react/src/utils/component.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/connect-react/src/utils/component.ts b/packages/connect-react/src/utils/component.ts index d22e6bbbbc4fa..b88bf6c65ef20 100644 --- a/packages/connect-react/src/utils/component.ts +++ b/packages/connect-react/src/utils/component.ts @@ -74,7 +74,7 @@ export function integerPropErrors(opts: ValidationOpts) } = opts const value = valueFromOption(valueOpt) - if (!prop.default && value == null || typeof value === "undefined") return [ + if (!prop.default && (value == null || typeof value === "undefined")) return [ "required", ] @@ -82,9 +82,9 @@ export function integerPropErrors(opts: ValidationOpts) ? value : parseInt(String(value)) - if (_value !== _value) return [ + if (Number.isNaN(_value)) return [ "not a number", - ] // NaN + ] const errs = [] if (typeof prop.min === "number" && _value < prop.min) errs.push("number too small") if (typeof prop.max === "number" && _value > prop.max) errs.push("number too big") From c2ef751fb26bffb2da936ec1e456cd7e1c05e518 Mon Sep 17 00:00:00 2001 From: adolfo-pd Date: Fri, 13 Dec 2024 14:48:35 -0800 Subject: [PATCH 5/7] Another CR suggestion --- packages/connect-react/src/utils/component.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/connect-react/src/utils/component.ts b/packages/connect-react/src/utils/component.ts index b88bf6c65ef20..47a91ddd6fa8a 100644 --- a/packages/connect-react/src/utils/component.ts +++ b/packages/connect-react/src/utils/component.ts @@ -25,18 +25,19 @@ export type PropOptions = { export function valuesFromOptions(value: unknown | T[] | PropOptions): T[] { if (typeof value === "object" && value && "selectedOptions" in value && Array.isArray(value.selectedOptions)) { const selectedOptions = value.selectedOptions as PropOption[] - const results = [] + const results: T[] = [] for (const so of selectedOptions) { if (typeof so === "object" && so && "emitValue" in so) { const emitValue = so.emitValue as T | PropOptionValue if (typeof emitValue === "object" && emitValue && "__lv" in emitValue) { results.push(emitValue.__lv.value) } - results.push(emitValue) + results.push(emitValue as T) } else { throw "unexpected value" } } + return results } if (!Array.isArray(value)) throw "unexpected value" From 6805a6081e074ff8d2e90f3343bbe684854f532f Mon Sep 17 00:00:00 2001 From: adolfo-pd Date: Fri, 13 Dec 2024 14:53:28 -0800 Subject: [PATCH 6/7] Versioning --- packages/connect-react/CHANGELOG.md | 5 +++++ packages/connect-react/package.json | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/connect-react/CHANGELOG.md b/packages/connect-react/CHANGELOG.md index dc9225596572a..45a3f265aa9c3 100644 --- a/packages/connect-react/CHANGELOG.md +++ b/packages/connect-react/CHANGELOG.md @@ -1,6 +1,11 @@ # Changelog +# [1.0.0-preview.11] - 2024-12-13 + +- Make prop validation more consistent with app behavior +- Relax validation of string props when value is not a string + # [1.0.0-preview.10] - 2024-12-12 - Enforce string length limits diff --git a/packages/connect-react/package.json b/packages/connect-react/package.json index d6d7388c8d590..2cfd49d93cde1 100644 --- a/packages/connect-react/package.json +++ b/packages/connect-react/package.json @@ -1,6 +1,6 @@ { "name": "@pipedream/connect-react", - "version": "1.0.0-preview.10", + "version": "1.0.0-preview.11", "description": "Pipedream Connect library for React", "files": [ "dist" From 7c6e6c9fa8769dfae366ce9dc00ead3cf73809e5 Mon Sep 17 00:00:00 2001 From: adolfo-pd Date: Fri, 13 Dec 2024 14:57:17 -0800 Subject: [PATCH 7/7] Another bug with translating options --- packages/connect-react/src/utils/component.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/connect-react/src/utils/component.ts b/packages/connect-react/src/utils/component.ts index 47a91ddd6fa8a..7d30ef639687f 100644 --- a/packages/connect-react/src/utils/component.ts +++ b/packages/connect-react/src/utils/component.ts @@ -31,8 +31,9 @@ export function valuesFromOptions(value: unknown | T[] | PropOptions): T[] const emitValue = so.emitValue as T | PropOptionValue if (typeof emitValue === "object" && emitValue && "__lv" in emitValue) { results.push(emitValue.__lv.value) + } else { + results.push(emitValue as T) } - results.push(emitValue as T) } else { throw "unexpected value" }