From e6633a4534ceef4b16531bb8b7e0049dd636c542 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Wed, 21 Feb 2024 10:06:41 -0600 Subject: [PATCH] dest api: Clean up error API (#3698) * standardize error codes * reorganize * cleanup errors on other components * remove special case for dest type err * rename/simplify split helper * update form * update test * fix type issue --- graphql2/generated.go | 3 +- graphql2/graph/errorcodes.graphqls | 25 +++++ graphql2/graphqlapp/app.go | 8 +- graphql2/graphqlapp/destinationvalidation.go | 14 +-- graphql2/models_gen.go | 52 +++++++++ .../selection/DestinationField.stories.tsx | 12 ++- web/src/app/selection/DestinationField.tsx | 10 +- .../UserContactMethodFormDest.stories.tsx | 15 ++- .../app/users/UserContactMethodFormDest.tsx | 44 ++++---- web/src/app/util/errtypes.ts | 65 +++++++++++ web/src/app/util/errutil.test.ts | 20 ++-- web/src/app/util/errutil.ts | 101 +++++------------- web/src/errors.d.ts | 27 ----- web/src/schema.d.ts | 2 + 14 files changed, 235 insertions(+), 163 deletions(-) create mode 100644 graphql2/graph/errorcodes.graphqls create mode 100644 web/src/app/util/errtypes.ts delete mode 100644 web/src/errors.d.ts diff --git a/graphql2/generated.go b/graphql2/generated.go index 34deef4371..208980a2ec 100644 --- a/graphql2/generated.go +++ b/graphql2/generated.go @@ -4760,7 +4760,7 @@ func (ec *executionContext) introspectType(name string) (*introspection.Type, er return introspection.WrapTypeFromDef(ec.Schema(), ec.Schema().Types[name]), nil } -//go:embed "schema.graphql" "graph/_Mutation.graphqls" "graph/_Query.graphqls" "graph/_directives.graphqls" "graph/destinations.graphqls" "graph/escalationpolicy.graphqls" "graph/gqlapikeys.graphqls" +//go:embed "schema.graphql" "graph/_Mutation.graphqls" "graph/_Query.graphqls" "graph/_directives.graphqls" "graph/destinations.graphqls" "graph/errorcodes.graphqls" "graph/escalationpolicy.graphqls" "graph/gqlapikeys.graphqls" var sourcesFS embed.FS func sourceData(filename string) string { @@ -4777,6 +4777,7 @@ var sources = []*ast.Source{ {Name: "graph/_Query.graphqls", Input: sourceData("graph/_Query.graphqls"), BuiltIn: false}, {Name: "graph/_directives.graphqls", Input: sourceData("graph/_directives.graphqls"), BuiltIn: false}, {Name: "graph/destinations.graphqls", Input: sourceData("graph/destinations.graphqls"), BuiltIn: false}, + {Name: "graph/errorcodes.graphqls", Input: sourceData("graph/errorcodes.graphqls"), BuiltIn: false}, {Name: "graph/escalationpolicy.graphqls", Input: sourceData("graph/escalationpolicy.graphqls"), BuiltIn: false}, {Name: "graph/gqlapikeys.graphqls", Input: sourceData("graph/gqlapikeys.graphqls"), BuiltIn: false}, } diff --git a/graphql2/graph/errorcodes.graphqls b/graphql2/graph/errorcodes.graphqls new file mode 100644 index 0000000000..dd41c7844a --- /dev/null +++ b/graphql2/graph/errorcodes.graphqls @@ -0,0 +1,25 @@ + + +""" +Known error codes that the server can return. + +These values will be returned in the `extensions.code` field of the error response. +""" +enum ErrorCode { + + """ + The input value is invalid, the `path` field will contain the exact path to the invalid input. + + A separate error will be returned for each invalid field. + """ + INVALID_INPUT_VALUE + + """ + The `path` field contains the exact path to the DestinationInput that is invalid. + + The `extensions.fieldID` field contains the ID of the input field that is invalid. + + A separate error will be returned for each invalid field. + """ + INVALID_DEST_FIELD_VALUE +} diff --git a/graphql2/graphqlapp/app.go b/graphql2/graphqlapp/app.go index 848f9bab0b..c200e629ed 100644 --- a/graphql2/graphqlapp/app.go +++ b/graphql2/graphqlapp/app.go @@ -148,6 +148,11 @@ func isGQLValidation(gqlErr *gqlerror.Error) bool { return false } + _, ok := gqlErr.Extensions["code"].(graphql2.ErrorCode) + if ok { + return true + } + code, ok := gqlErr.Extensions["code"].(string) if !ok { return false @@ -157,9 +162,6 @@ func isGQLValidation(gqlErr *gqlerror.Error) bool { case errcode.ValidationFailed, errcode.ParseFailed: // These are gqlgen validation errors. return true - case ErrCodeInvalidDestType, ErrCodeInvalidDestValue: - // These are destination validation errors. - return true } return false diff --git a/graphql2/graphqlapp/destinationvalidation.go b/graphql2/graphqlapp/destinationvalidation.go index 2ea11baf9c..b786542997 100644 --- a/graphql2/graphqlapp/destinationvalidation.go +++ b/graphql2/graphqlapp/destinationvalidation.go @@ -17,11 +17,6 @@ import ( "github.com/vektah/gqlparser/v2/gqlerror" ) -const ( - ErrCodeInvalidDestType = "INVALID_DESTINATION_TYPE" - ErrCodeInvalidDestValue = "INVALID_DESTINATION_FIELD_VALUE" -) - func errReason(err error) string { var fErr validation.FieldError if errors.As(err, &fErr) { @@ -48,16 +43,13 @@ func addDestFieldError(ctx context.Context, parentField, fieldID string, err err for _, part := range parentParts { p = append(p, ast.PathName(part)) } - p = append(p, - ast.PathName("values"), // DestinationInput.Values - ast.PathName(fieldID), - ) graphql.AddError(ctx, &gqlerror.Error{ Message: errReason(err), Path: p, Extensions: map[string]interface{}{ - "code": ErrCodeInvalidDestValue, + "code": graphql2.ErrorCodeInvalidDestFieldValue, + "fieldID": fieldID, }, }) @@ -186,7 +178,7 @@ func (a *App) ValidateDestination(ctx context.Context, fieldName string, dest *g Message: "unsupported destination type", Path: p, Extensions: map[string]interface{}{ - "code": ErrCodeInvalidDestType, + "code": graphql2.ErrorCodeInvalidInputValue, }, }) diff --git a/graphql2/models_gen.go b/graphql2/models_gen.go index bf47d05580..59209e648e 100644 --- a/graphql2/models_gen.go +++ b/graphql2/models_gen.go @@ -971,6 +971,58 @@ func (e ConfigType) MarshalGQL(w io.Writer) { fmt.Fprint(w, strconv.Quote(e.String())) } +// Known error codes that the server can return. +// +// These values will be returned in the `extensions.code` field of the error response. +type ErrorCode string + +const ( + // The input value is invalid, the `path` field will contain the exact path to the invalid input. + // + // A separate error will be returned for each invalid field. + ErrorCodeInvalidInputValue ErrorCode = "INVALID_INPUT_VALUE" + // The `path` field contains the exact path to the DestinationInput that is invalid. + // + // The `extensions.fieldID` field contains the ID of the input field that is invalid. + // + // A separate error will be returned for each invalid field. + ErrorCodeInvalidDestFieldValue ErrorCode = "INVALID_DEST_FIELD_VALUE" +) + +var AllErrorCode = []ErrorCode{ + ErrorCodeInvalidInputValue, + ErrorCodeInvalidDestFieldValue, +} + +func (e ErrorCode) IsValid() bool { + switch e { + case ErrorCodeInvalidInputValue, ErrorCodeInvalidDestFieldValue: + return true + } + return false +} + +func (e ErrorCode) String() string { + return string(e) +} + +func (e *ErrorCode) UnmarshalGQL(v interface{}) error { + str, ok := v.(string) + if !ok { + return fmt.Errorf("enums must be strings") + } + + *e = ErrorCode(str) + if !e.IsValid() { + return fmt.Errorf("%s is not a valid ErrorCode", str) + } + return nil +} + +func (e ErrorCode) MarshalGQL(w io.Writer) { + fmt.Fprint(w, strconv.Quote(e.String())) +} + type IntegrationKeyType string const ( diff --git a/web/src/app/selection/DestinationField.stories.tsx b/web/src/app/selection/DestinationField.stories.tsx index 86e3cbb3d6..eb6507c620 100644 --- a/web/src/app/selection/DestinationField.stories.tsx +++ b/web/src/app/selection/DestinationField.stories.tsx @@ -135,12 +135,20 @@ export const FieldError: Story = { disabled: false, destFieldErrors: [ { - path: 'third-field', + path: ['input', 'dest'], message: 'This is an error message (third)', + extensions: { + code: 'INVALID_DEST_FIELD_VALUE', + fieldID: 'third-field', + }, }, { - path: 'first-field', + path: ['input', 'dest'], message: 'This is an error message (first)', + extensions: { + code: 'INVALID_DEST_FIELD_VALUE', + fieldID: 'first-field', + }, }, ], }, diff --git a/web/src/app/selection/DestinationField.tsx b/web/src/app/selection/DestinationField.tsx index 0e0d7e0c12..5334c652c2 100644 --- a/web/src/app/selection/DestinationField.tsx +++ b/web/src/app/selection/DestinationField.tsx @@ -4,7 +4,7 @@ import DestinationInputDirect from './DestinationInputDirect' import { useDestinationType } from '../util/RequireConfig' import DestinationSearchSelect from './DestinationSearchSelect' import { Grid } from '@mui/material' -import { InputFieldError } from '../util/errutil' +import { DestFieldValueError } from '../util/errtypes' export type DestinationFieldProps = { value: FieldValueInput[] @@ -13,7 +13,7 @@ export type DestinationFieldProps = { disabled?: boolean - destFieldErrors?: InputFieldError[] + destFieldErrors?: DestFieldValueError[] } function capFirstLetter(s: string): string { @@ -46,13 +46,9 @@ export default function DestinationField( props.onChange(newValues) } - // getFieldID returns the last segment of the path. - const getFieldID = (err: InputFieldError): string => - err.path.split('.').pop() || '' - const fieldErrMsg = capFirstLetter( props.destFieldErrors?.find( - (err) => getFieldID(err) === field.fieldID, + (err) => err.extensions.fieldID === field.fieldID, )?.message || '', ) diff --git a/web/src/app/users/UserContactMethodFormDest.stories.tsx b/web/src/app/users/UserContactMethodFormDest.stories.tsx index d58554a600..7edc8e1dbe 100644 --- a/web/src/app/users/UserContactMethodFormDest.stories.tsx +++ b/web/src/app/users/UserContactMethodFormDest.stories.tsx @@ -110,10 +110,14 @@ export const ErrorSingleField: Story = { statusUpdates: false, }, disabled: false, - fieldErrors: [ + errors: [ { message: 'number is too short', // note: the 'n' is lowercase - path: 'dest.values.phone-number', + path: ['input', 'dest'], + extensions: { + code: 'INVALID_DEST_FIELD_VALUE', + fieldID: 'phone-number', + }, }, ], }, @@ -153,10 +157,13 @@ export const ErrorMultiField: Story = { statusUpdates: false, }, disabled: false, - fieldErrors: [ + errors: [ { - path: 'name', + path: ['input', 'name'], message: 'must begin with a letter', + extensions: { + code: 'INVALID_INPUT_VALUE', + }, }, ], }, diff --git a/web/src/app/users/UserContactMethodFormDest.tsx b/web/src/app/users/UserContactMethodFormDest.tsx index d7c4c849b0..62b8629835 100644 --- a/web/src/app/users/UserContactMethodFormDest.tsx +++ b/web/src/app/users/UserContactMethodFormDest.tsx @@ -5,9 +5,14 @@ import React from 'react' import { DestinationInput } from '../../schema' import { FormContainer, FormField } from '../forms' import { renderMenuItem } from '../selection/DisableableMenuItem' -import { InputFieldError, SimpleError } from '../util/errutil' import DestinationField from '../selection/DestinationField' import { useContactMethodTypes } from '../util/RequireConfig' +import { + isInputFieldError, + isDestFieldError, + KnownError, + DestFieldValueError, +} from '../util/errtypes' export type Value = { name: string @@ -18,8 +23,7 @@ export type Value = { export type UserContactMethodFormProps = { value: Value - typeError?: SimpleError - fieldErrors?: Array + errors?: Array disabled?: boolean edit?: boolean @@ -27,10 +31,16 @@ export type UserContactMethodFormProps = { onChange?: (CMValue: Value) => void } +export const errorPaths = (prefix = '*'): string[] => [ + `${prefix}.name`, + `${prefix}.dest.type`, + `${prefix}.dest`, +] + export default function UserContactMethodFormDest( props: UserContactMethodFormProps, ): JSX.Element { - const { value, edit = false, ...other } = props + const { value, edit = false, errors = [], ...other } = props const destinationTypes = useContactMethodTypes() const currentType = destinationTypes.find((d) => d.type === value.dest.type) @@ -47,28 +57,14 @@ export default function UserContactMethodFormDest( statusUpdateChecked = false } - const formErrors = [] - if (props.typeError) { - formErrors.push({ - // the old form code requires this shape to map errors to the correct field - message: props.typeError.message, - field: 'dest.type', - }) - } - const nameErr = props.fieldErrors?.find( - (err) => err.path.split('.').pop() === 'name', - ) - if (nameErr) { - formErrors.push({ - message: nameErr.message, - field: 'name', - }) - } - return ( ({ + // need to convert to FormContainer's error format + message: e.message, + field: e.path[e.path.length - 1].toString(), + }))} value={value} mapOnChangeValue={(newValue: Value): Value => { if (newValue.dest.type === value.dest.type) { @@ -119,7 +115,7 @@ export default function UserContactMethodFormDest( destType={value.dest.type} component={DestinationField} disabled={edit} - destFieldErrors={props.fieldErrors} + destFieldErrors={errors.filter(isDestFieldError)} /> diff --git a/web/src/app/util/errtypes.ts b/web/src/app/util/errtypes.ts new file mode 100644 index 0000000000..f1c3eaf64f --- /dev/null +++ b/web/src/app/util/errtypes.ts @@ -0,0 +1,65 @@ +import { GraphQLError } from 'graphql' +import { ErrorCode } from '../../schema' + +export interface BaseError { + message: string +} + +export interface KnownError extends BaseError { + readonly path: ReadonlyArray + extensions: { + code: ErrorCode + } +} + +export interface InputFieldError extends KnownError { + extensions: { + code: 'INVALID_INPUT_VALUE' + } +} + +export interface DestFieldValueError extends KnownError { + extensions: { + code: 'INVALID_DEST_FIELD_VALUE' + fieldID: string + } +} + +function assertNever(x: never): void { + console.log('unhandled error code', x) +} + +function isKnownErrorCode(code: ErrorCode): code is ErrorCode { + switch (code) { + case 'INVALID_INPUT_VALUE': + return true + case 'INVALID_DEST_FIELD_VALUE': + return true + default: + assertNever(code) // ensure we handle all error codes + return false + } +} + +function isGraphQLError(err: unknown): err is GraphQLError { + if (!err) return false + if (!Object.prototype.hasOwnProperty.call(err, 'path')) return false + if (!Object.prototype.hasOwnProperty.call(err, 'extensions')) return false + return true +} + +export function isKnownError(err: unknown): err is KnownError { + if (!isGraphQLError(err)) return false + if (!Object.prototype.hasOwnProperty.call(err.extensions, 'code')) + return false + + return isKnownErrorCode(err.extensions.code as ErrorCode) +} +export function isDestFieldError(err: unknown): err is DestFieldValueError { + if (!isKnownError(err)) return false + return err.extensions.code === 'INVALID_DEST_FIELD_VALUE' +} +export function isInputFieldError(err: unknown): err is InputFieldError { + if (!isKnownError(err)) return false + return err.extensions.code === 'INVALID_INPUT_VALUE' +} diff --git a/web/src/app/util/errutil.test.ts b/web/src/app/util/errutil.test.ts index 9701e7f856..a79c6465d1 100644 --- a/web/src/app/util/errutil.test.ts +++ b/web/src/app/util/errutil.test.ts @@ -1,35 +1,35 @@ import { GraphQLError } from 'graphql' -import { getInputFieldErrors } from './errutil' +import { splitErrorsByPath } from './errutil' -describe('getInputFieldErrors', () => { +describe('splitErrorsByPath', () => { it('should split errors by path', () => { const resp = [ { message: 'test1', path: ['foo', 'bar', 'dest', 'type'], extensions: { - code: 'INVALID_DESTINATION_TYPE', + code: 'INVALID_INPUT_VALUE', }, }, { message: 'test2', path: ['foo', 'bar', 'dest', 'values', 'example-field'], extensions: { - code: 'INVALID_DESTINATION_FIELD_VALUE', + code: 'INVALID_INPUT_VALUE', }, }, ] as unknown as GraphQLError[] - const [inputFieldErrors, otherErrors] = getInputFieldErrors( - ['foo.bar.dest.type', 'foo.bar.dest.values.example-field'], - resp, - ) + const [inputFieldErrors, otherErrors] = splitErrorsByPath(resp, [ + 'foo.bar.dest.type', + 'foo.bar.dest.values.example-field', + ]) expect(inputFieldErrors).toHaveLength(2) expect(inputFieldErrors[0].message).toEqual('test1') - expect(inputFieldErrors[0].path).toEqual('foo.bar.dest.type') + expect(inputFieldErrors[0].path.join('.')).toEqual('foo.bar.dest.type') expect(inputFieldErrors[1].message).toEqual('test2') - expect(inputFieldErrors[1].path).toEqual( + expect(inputFieldErrors[1].path.join('.')).toEqual( 'foo.bar.dest.values.example-field', ) expect(otherErrors).toHaveLength(0) diff --git a/web/src/app/util/errutil.ts b/web/src/app/util/errutil.ts index a504509da8..dc1a63d4d2 100644 --- a/web/src/app/util/errutil.ts +++ b/web/src/app/util/errutil.ts @@ -2,11 +2,7 @@ import _ from 'lodash' import { ApolloError } from '@apollo/client' import { GraphQLError } from 'graphql/error' import { CombinedError } from 'urql' -import { - INVALID_DESTINATION_FIELD_VALUE, - INVALID_DESTINATION_TYPE, -} from '../../errors.d' -import { useDestinationType } from './RequireConfig' +import { BaseError, isKnownError, KnownError } from './errtypes' const mapName = (name: string): string => _.camelCase(name).replace(/Id$/, 'ID') @@ -39,91 +35,48 @@ export function nonFieldErrors(err?: ApolloError | CombinedError): Error[] { ) } -export type SimpleError = { - message: string -} - -export type InputFieldError = { - message: string - path: string -} - -function isGraphQLError(e: SimpleError | GraphQLError): e is GraphQLError { - return !!(e as GraphQLError).extensions -} - /** - * getInputFieldErrors returns a list of input field errors and other errors from a CombinedError. - * Any errors that are not input field errors (or are not in the filterPaths list) will be returned as other errors. + * splitErrorsByPath returns a list of known errors and other errors from a CombinedError or array of errors. + * + * Any errors that are not known errors (or are not in the filterPaths list) will be returned as other errors. * - * @param filterPaths - a list of paths to filter errors by * @param err - the CombinedError to filter + * @param paths - a list of paths to filter errors by, paths can be exact or begin with a wildcard (*) + * @returns a tuple of known errors and other errors */ -export function getInputFieldErrors( - filterPaths: string[], - errs: (GraphQLError | SimpleError)[] | undefined | null, -): [InputFieldError[], SimpleError[]] { - if (!errs) return [[], []] - const inputFieldErrors = [] as InputFieldError[] - const otherErrors = [] as SimpleError[] - errs.forEach((err) => { - if (!isGraphQLError(err)) { - otherErrors.push(err) - return - } - if (!err.path) { - otherErrors.push(err) - return - } - const code = err.extensions?.code - if ( - // only support known error codes - code !== INVALID_DESTINATION_TYPE && - code !== INVALID_DESTINATION_FIELD_VALUE - ) { +export function splitErrorsByPath( + err: CombinedError | BaseError[] | undefined | null, + paths: string[], +): [KnownError[], BaseError[]] { + if (!err) return [[], []] + const knownErrors: KnownError[] = [] + const otherErrors: BaseError[] = [] + + const errors = Array.isArray(err) ? err : err.graphQLErrors + + errors.forEach((err) => { + if (!isKnownError(err)) { otherErrors.push(err) return } const fullPath = err.path.join('.') + const matches = paths.some((p) => { + if (p.startsWith('*')) { + return fullPath.endsWith(p.slice(1)) + } + return fullPath === p + }) - if (!filterPaths.includes(fullPath)) { + if (!matches) { otherErrors.push(err) return } - inputFieldErrors.push({ message: err.message, path: fullPath }) + knownErrors.push(err) }) - return [inputFieldErrors, otherErrors] -} - -/** - * useErrorsForDest returns the errors for a destination type and field path from a CombinedError. - * The first return value is the error for the destination type, if any. - * The second return value is a list of errors for the destination fields, if any. - * The third return value is a list of other errors, if any. - */ -export function useErrorsForDest( - err: CombinedError | undefined | null, - destType: string, - destFieldPath: string, // the path of the DestinationInput field -): [SimpleError | undefined, InputFieldError[], SimpleError[]] { - const cfg = useDestinationType(destType) // need to call hook before conditional return - if (!err) return [undefined, [], []] - - const [destTypeErrs, nonDestTypeErrs] = getInputFieldErrors( - [destFieldPath + '.type'], - err.graphQLErrors, - ) - - const paths = cfg.requiredFields.map( - (f) => `${destFieldPath}.values.${f.fieldID}`, - ) - - const [destFieldErrs, otherErrs] = getInputFieldErrors(paths, nonDestTypeErrs) - - return [destTypeErrs[0] || undefined, destFieldErrs, otherErrs] + return [knownErrors, otherErrors] } export interface FieldError extends Error { diff --git a/web/src/errors.d.ts b/web/src/errors.d.ts deleted file mode 100644 index 91971c9843..0000000000 --- a/web/src/errors.d.ts +++ /dev/null @@ -1,27 +0,0 @@ -/** - * INVALID_DESTINATION_TYPE is returned when the selected destination type is not valid, or is not allowed. - */ -export const INVALID_DESTINATION_TYPE = 'INVALID_DESTINATION_TYPE' - -/** - * INVALID_DESTINATION_FIELD_VALUE is returned when the value of a field on a destination is invalid. - */ -export const INVALID_DESTINATION_FIELD_VALUE = 'INVALID_DESTINATION_FIELD_VALUE' - -type KnownErrorCode = INVALID_DESTINATION_TYPE | INVALID_DESTINATION_FIELD_VALUE - -export type InvalidDestTypeError = { - message: string - path: readonly (string | number)[] - extensions: { - code: INVALID_DESTINATION_TYPE - } -} - -export type InvalidFieldValueError = { - message: string - path: readonly (string | number)[] - extensions: { - code: INVALID_DESTINATION_FIELD_VALUE - } -} diff --git a/web/src/schema.d.ts b/web/src/schema.d.ts index 96b9e89775..d9de9a6d53 100644 --- a/web/src/schema.d.ts +++ b/web/src/schema.d.ts @@ -406,6 +406,8 @@ export interface DestinationTypeInfo { userDisclaimer: string } +export type ErrorCode = 'INVALID_DEST_FIELD_VALUE' | 'INVALID_INPUT_VALUE' + export interface EscalationPolicy { assignedTo: Target[] description: string