More robust error handling in payload parser

Adam Erb · svc-squareup-copybara · commit 88c25ca171a7 · 2025-04-01T19:12:09.000Z
GitOrigin-RevId: 6708daf1d3ef13708579f715baedc4fbe5ef0966
diff --git a/misk-admin-web-actions/src/index.html b/misk-admin-web-actions/src/index.html
@@ -26,6 +26,11 @@
         background-color: #e69a9a;
         position: absolute;
       }
+      .error-marker {
+        background-color: rgba(255, 0, 0, 0.2);
+        border-bottom: 2px solid red;
+        position: absolute;
+      }
     </style>
   </head>
   <body>
diff --git a/misk-admin-web-actions/src/index.tsx b/misk-admin-web-actions/src/index.tsx
@@ -12,6 +12,7 @@ import {
   Input,
   IconButton,
   Button,
+  useToast,
 } from '@chakra-ui/react';
 import { DeleteIcon, AddIcon } from '@chakra-ui/icons';
 import HelpPanel from '@web-actions/ui/HelpPanel';
@@ -43,6 +44,7 @@ function App() {
   }));
   const endPointSelectorRef = useRef<EndpointSelector>(null);
   const requestEditorRef = useRef<RequestEditor>(null);
+  const toast = useToast();
 
   const {
     submit: handleSubmitRequest,
@@ -55,7 +57,7 @@ function App() {
     () => requestEditorRef.current?.editor?.getValue() ?? '',
   );
 
-  useAppEvent<ActionGroup>(APP_EVENTS.ENDPOINT_SELECTED, (selectedAction) => {
+  useAppEvent(APP_EVENTS.ENDPOINT_SELECTED, (selectedAction: ActionGroup) => {
     const callables = selectedAction.getCallablesByMethod();
     const defaultCallable = callables[0];
 
@@ -83,6 +85,16 @@ function App() {
     endPointSelectorRef.current?.focusSelect();
   });
 
+  useAppEvent(APP_EVENTS.SHOW_ERROR_TOAST, () => {
+    toast({
+      title: 'Error',
+      description: 'There are syntax or field name errors in the JSON Request',
+      status: 'error',
+      duration: 5000,
+      isClosable: true,
+    });
+  });
+
   useEffect(() => {
     fetchCached<MiskMetadataResponse>(`/api/web-actions/metadata`).finally(
       () => {
diff --git a/misk-admin-web-actions/src/web-actions/events/appEvents.ts b/misk-admin-web-actions/src/web-actions/events/appEvents.ts
@@ -2,13 +2,15 @@ export type AppEventType =
   | 'focus-endpoint-selector'
   | 'toggle-help'
   | 'submit-request'
-  | 'endpoint-selected';
+  | 'endpoint-selected'
+  | 'show-error-toast';
 
 export const APP_EVENTS = {
   FOCUS_ENDPOINT_SELECTOR: 'focus-endpoint-selector' as const,
   TOGGLE_HELP: 'toggle-help' as const,
   SUBMIT_REQUEST: 'submit-request' as const,
   ENDPOINT_SELECTED: 'endpoint-selected' as const,
+  SHOW_ERROR_TOAST: 'show-error-toast' as const,
 };
 
 class AppEventBus {
diff --git a/misk-admin-web-actions/src/web-actions/parsing/CommandParser.ts b/misk-admin-web-actions/src/web-actions/parsing/CommandParser.ts
@@ -13,7 +13,7 @@ import Unexpected from '@web-actions/parsing/ast/Unexpected';
 import NumLiteral from '@web-actions/parsing/ast/NumLiteral';
 import BoolLiteral from '@web-actions/parsing/ast/BoolLiteral';
 
-export function parseDocument(text: string, cursorIndex: number): TopLevel {
+export function parseDocument(text: string, cursorIndex?: number): TopLevel {
   return new CommandParser(text, cursorIndex).parse();
 }
 
@@ -172,13 +172,13 @@ export class CommandParser {
         } else {
           const u = this.readUnexpected();
           if (u) {
-            unexpected.push(u);
+            arr.push(u);
           } else {
             break;
           }
         }
       }
-      return new Arr(arr, unexpected);
+      return new Arr(arr);
     });
   }
 
diff --git a/misk-admin-web-actions/src/web-actions/parsing/__test__/parser.spec.ts b/misk-admin-web-actions/src/web-actions/parsing/__test__/parser.spec.ts
@@ -2,6 +2,7 @@ import { CommandParser } from '@web-actions/parsing/CommandParser';
 import StrLiteral from '@web-actions/parsing/ast/StrLiteral';
 import Obj from '@web-actions/parsing/ast/Obj';
 import Arr from '@web-actions/parsing/ast/Arr';
+import Unexpected from 'src/web-actions/parsing/ast/Unexpected';
 
 function parser(text: string): CommandParser {
   return new CommandParser(text);
@@ -54,8 +55,7 @@ test('array', () => {
 test('array error', () => {
   const result = parser(`["a" ! ]`).parseArr();
 
-  expect(result?.values.length).toEqual(1);
+  expect(result?.values.length).toEqual(2);
   expect(result?.values[0].as<StrLiteral>()?.value).toEqual('a');
-  expect(result?.unexpected.length).toEqual(1);
-  expect(result?.unexpected[0].value).toEqual('!');
+  expect(result?.values[1].as<Unexpected>()?.value).toEqual('!');
 });
diff --git a/misk-admin-web-actions/src/web-actions/parsing/ast/Action.ts b/misk-admin-web-actions/src/web-actions/parsing/ast/Action.ts
diff --git a/misk-admin-web-actions/src/web-actions/parsing/ast/Arr.ts b/misk-admin-web-actions/src/web-actions/parsing/ast/Arr.ts
@@ -1,18 +1,16 @@
 import AstNode from '@web-actions/parsing/ast/AstNode';
 
 import JsonValue from '@web-actions/parsing/ast/JsonValue';
-import Unexpected from '@web-actions/parsing/ast/Unexpected';
 import MiskType from '@web-actions/api/MiskType';
 import { MiskObjectTypes } from '@web-actions/api/responseTypes';
 
 export default class Arr extends JsonValue {
   values: JsonValue[];
   type?: MiskType;
 
-  constructor(values: JsonValue[], unexpected: Unexpected[]) {
+  constructor(values: JsonValue[]) {
     super();
     this.values = values;
-    this.unexpected = unexpected;
 
     for (const value of values) {
       value.parent = this;
diff --git a/misk-admin-web-actions/src/web-actions/parsing/ast/AstNode.ts b/misk-admin-web-actions/src/web-actions/parsing/ast/AstNode.ts
@@ -1,11 +1,7 @@
-import Unexpected from '@web-actions/parsing/ast/Unexpected';
-
 export default class AstNode {
   start?: number;
   end?: number;
   hasCursor: boolean = false;
-  unexpected: Unexpected[] = [];
-
   parent?: AstNode;
 
   childNodes(): AstNode[] {
@@ -36,6 +32,18 @@ export default class AstNode {
     return null;
   }
 
+  findAll(predicate: (a: AstNode) => boolean): AstNode[] {
+    const arr: AstNode[] = [];
+    if (predicate(this)) {
+      arr.push(this);
+    }
+    for (const child of this.childNodes()) {
+      const result = child.findAll(predicate);
+      arr.push(...result);
+    }
+    return arr;
+  }
+
   render(): string {
     throw new Error('Method not implemented');
   }
diff --git a/misk-admin-web-actions/src/web-actions/parsing/ast/Field.ts b/misk-admin-web-actions/src/web-actions/parsing/ast/Field.ts
@@ -12,7 +12,7 @@ import MiskType from '@web-actions/api/MiskType';
 
 export default class Field extends AstNode {
   name: StrLiteral;
-  value: JsonValue | null;
+  value: JsonValue | Unexpected | null;
 
   colonIndex: number | null;
   cursorInValuePosition: boolean = false;
@@ -27,14 +27,7 @@ export default class Field extends AstNode {
     super();
     this.colonIndex = colonIndex;
     this.name = name;
-    if (value instanceof JsonValue) {
-      this.value = value ?? null;
-    } else if (value instanceof Unexpected) {
-      this.unexpected = [value];
-      this.value = null;
-    } else {
-      this.value = null;
-    }
+    this.value = value ?? null;
 
     if (this.value) {
       this.value.parent = this;
diff --git a/misk-admin-web-actions/src/web-actions/parsing/ast/Obj.ts b/misk-admin-web-actions/src/web-actions/parsing/ast/Obj.ts
@@ -13,6 +13,7 @@ import MiskType from '@web-actions/api/MiskType';
 export default class Obj extends JsonValue {
   fields: Field[];
   type?: MiskObjectType;
+  unexpected: Unexpected[];
 
   constructor(fields: Field[], unexpected: Unexpected[]) {
     super();
@@ -24,7 +25,7 @@ export default class Obj extends JsonValue {
   }
 
   childNodes(): AstNode[] {
-    return this.fields;
+    return [...this.fields, ...this.unexpected];
   }
 
   render(): string {
diff --git a/misk-admin-web-actions/src/web-actions/parsing/ast/TopLevel.ts b/misk-admin-web-actions/src/web-actions/parsing/ast/TopLevel.ts
@@ -5,18 +5,11 @@ import Obj from '@web-actions/parsing/ast/Obj';
 import MiskType from '@web-actions/api/MiskType';
 
 export default class TopLevel extends AstNode {
-  obj: Obj | null;
+  obj: Obj | Unexpected | null;
 
   constructor(obj: Obj | Unexpected | null) {
     super();
-    if (obj instanceof Obj) {
-      this.obj = obj;
-    } else if (obj instanceof Unexpected) {
-      this.unexpected = [obj];
-      this.obj = null;
-    } else {
-      this.obj = null;
-    }
+    this.obj = obj;
     this.hasCursor = true;
 
     if (this.obj) {
@@ -32,6 +25,14 @@ export default class TopLevel extends AstNode {
     return this.obj?.render() ?? '';
   }
 
+  firstError(): Unexpected | null {
+    return (
+      (this.findAll(
+        (node): node is Unexpected => node instanceof Unexpected,
+      )[0] as Unexpected) ?? null
+    );
+  }
+
   applyTypes(actionDefinition: MiskWebActionDefinition | null) {
     if (this.obj) {
       if (actionDefinition) {
diff --git a/misk-admin-web-actions/src/web-actions/parsing/ast/Unexpected.ts b/misk-admin-web-actions/src/web-actions/parsing/ast/Unexpected.ts
@@ -1,14 +1,22 @@
-import AstNode from '@web-actions/parsing/ast/AstNode';
+import JsonValue from 'src/web-actions/parsing/ast/JsonValue';
+import MiskType from 'src/web-actions/api/MiskType';
+import { MiskObjectTypes } from 'src/web-actions/api/responseTypes';
 
-export default class Unexpected extends AstNode {
+export default class Unexpected extends JsonValue {
   value: string;
 
   constructor(value: string) {
     super();
     this.value = value;
   }
 
-  render() {
-    return this.value;
+  onEvalCursor(index: number) {
+    this.hasCursor = false;
+  }
+
+  applyTypes(type: MiskType, types: MiskObjectTypes) {}
+
+  render(): string {
+    throw new Error(`Unexpected value ${this}`);
   }
 }
diff --git a/misk-admin-web-actions/src/web-actions/services/ApiService.ts b/misk-admin-web-actions/src/web-actions/services/ApiService.ts
@@ -1,5 +1,6 @@
-import { CommandParser } from '@web-actions/parsing/CommandParser';
+import { parseDocument } from '@web-actions/parsing/CommandParser';
 import { MiskWebActionDefinition } from '@web-actions/api/responseTypes';
+import { appEvents, APP_EVENTS } from '@web-actions/events/appEvents';
 
 export interface Header {
   key: string;
@@ -22,8 +23,16 @@ export class ApiService {
   }: RequestOptions): Promise<string> {
     const headers: Record<string, string> = {};
 
+    let body: string | undefined = undefined;
     if (action.httpMethod !== 'GET') {
       headers['Content-Type'] = 'application/json';
+
+      const topLevel = parseDocument(requestBody ?? '');
+      if (topLevel?.firstError() !== null) {
+        appEvents.emit(APP_EVENTS.SHOW_ERROR_TOAST);
+        return '';
+      }
+      body = topLevel.render();
     }
 
     additionalHeaders.forEach((header) => {
@@ -35,10 +44,7 @@ export class ApiService {
     const fetchOptions = {
       method: action.httpMethod,
       headers: headers,
-      body:
-        action.httpMethod !== 'GET'
-          ? new CommandParser(requestBody ?? '').parse()?.render()
-          : undefined,
+      body: body,
     };
 
     const response = await fetch(path, fetchOptions);
diff --git a/misk-admin-web-actions/src/web-actions/ui/RequestEditor.tsx b/misk-admin-web-actions/src/web-actions/ui/RequestEditor.tsx

Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@ import Unexpected from '@web-actions/parsing/ast/Unexpected';`
`13`	`13`	`import NumLiteral from '@web-actions/parsing/ast/NumLiteral';`
`14`	`14`	`import BoolLiteral from '@web-actions/parsing/ast/BoolLiteral';`
`15`	`15`
`16`		`-export function parseDocument(text: string, cursorIndex: number): TopLevel {`
	`16`	`+export function parseDocument(text: string, cursorIndex?: number): TopLevel {`
`17`	`17`	`return new CommandParser(text, cursorIndex).parse();`
`18`	`18`	`}`
`19`	`19`
`@@ -172,13 +172,13 @@ export class CommandParser {`
`172`	`172`	`} else {`
`173`	`173`	`const u = this.readUnexpected();`
`174`	`174`	`if (u) {`
`175`		`- unexpected.push(u);`
	`175`	`+ arr.push(u);`
`176`	`176`	`} else {`
`177`	`177`	`break;`
`178`	`178`	`}`
`179`	`179`	`}`
`180`	`180`	`}`
`181`		`- return new Arr(arr, unexpected);`
	`181`	`+ return new Arr(arr);`
`182`	`182`	`});`
`183`	`183`	`}`
`184`	`184`
Original file line number	Diff line number	Diff line change
`@@ -13,6 +13,7 @@ import MiskType from '@web-actions/api/MiskType';`
`13`	`13`	`export default class Obj extends JsonValue {`
`14`	`14`	`fields: Field[];`
`15`	`15`	`type?: MiskObjectType;`
	`16`	`+ unexpected: Unexpected[];`
`16`	`17`
`17`	`18`	`constructor(fields: Field[], unexpected: Unexpected[]) {`
`18`	`19`	`super();`
`@@ -24,7 +25,7 @@ export default class Obj extends JsonValue {`
`24`	`25`	`}`
`25`	`26`
`26`	`27`	`childNodes(): AstNode[] {`
`27`		`- return this.fields;`
	`28`	`+ return [...this.fields, ...this.unexpected];`
`28`	`29`	`}`
`29`	`30`
`30`	`31`	`render(): string {`