From 0b02d1dd615c97d68bdc22178cd1280c42d5ab96 Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 3 May 2022 21:07:59 +0200 Subject: [PATCH 1/6] 3216 parsing jq stack to show error message --- src/blocks/transformers/jq.ts | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/blocks/transformers/jq.ts b/src/blocks/transformers/jq.ts index e720e8b745..c437d92d12 100644 --- a/src/blocks/transformers/jq.ts +++ b/src/blocks/transformers/jq.ts @@ -22,6 +22,8 @@ import { isNullOrBlank } from "@/utils"; import { InputValidationError } from "@/blocks/errors"; import { isErrorObject } from "@/errors"; +const jqStacktraceRegexp = /jq: error \(at \:0\): (?.*)/; + export class JQTransformer extends Transformer { override async isPure(): Promise { return true; @@ -66,13 +68,28 @@ export class JQTransformer extends Transformer { // eslint-disable-next-line @typescript-eslint/return-await -- Type is `any`, it throws the rule off return await jq.promised.json(input, filter); } catch (error) { - if (!isErrorObject(error) || !error.message.includes("compile error")) { + // The message length check is there because the JQ error message sometimes is cut and if it is we try to parse the stacktrace + // See https://github.com/pixiebrix/pixiebrix-extension/issues/3216 + if ( + !isErrorObject(error) || + (error.message.length > 13 && !error.message.includes("compile error")) + ) { throw error; } - const message = error.stack.includes("unexpected $end") - ? "Unexpected end of jq filter, are you missing a parentheses, brace, and/or quote mark?" - : "Invalid jq filter, see error log for details"; + let message: string; + if (error.stack.includes("unexpected $end")) { + message = + "Unexpected end of jq filter, are you missing a parentheses, brace, and/or quote mark?"; + } else { + const jqMessageFromStack = jqStacktraceRegexp.exec(error.stack)?.groups + ?.message; + if (jqMessageFromStack == null) { + message = "Invalid jq filter, see error log for details"; + } else { + message = jqMessageFromStack.trim(); + } + } throw new InputValidationError( message, From 9b0b87688453f56636487f5082f090dd9b002cb4 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 5 May 2022 16:02:04 +0200 Subject: [PATCH 2/6] 3216 serializing the cause of error --- src/background/logging.ts | 5 +++-- src/errors.test.ts | 21 +++++++++++++++++++++ src/errors.ts | 12 +++++++++++- 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/background/logging.ts b/src/background/logging.ts index 417da2627b..616c701adc 100644 --- a/src/background/logging.ts +++ b/src/background/logging.ts @@ -19,7 +19,7 @@ import { uuidv4 } from "@/types/helpers"; import { getRollbar } from "@/telemetry/initRollbar"; import { MessageContext, SerializedError, UUID } from "@/core"; import { Except, JsonObject } from "type-fest"; -import { deserializeError, serializeError } from "serialize-error"; +import { deserializeError } from "serialize-error"; import { DBSchema, openDB } from "idb/with-async-ittr"; import { isEmpty, once, sortBy } from "lodash"; import { allowsTrack } from "@/telemetry/dnt"; @@ -32,6 +32,7 @@ import { IGNORED_ERROR_PATTERNS, isAxiosError, isContextError, + serializePixiebrixError, } from "@/errors"; import { expectContext, forbidContext } from "@/utils/expectContext"; import { isAppRequest, selectAbsoluteUrl } from "@/services/requestErrorUtils"; @@ -333,7 +334,7 @@ export async function recordError( data, // Ensure it's serialized - error: serializeError(maybeSerializedError), + error: serializePixiebrixError(maybeSerializedError as Error), }), ]); } catch (recordError) { diff --git a/src/errors.test.ts b/src/errors.test.ts index 50b0c06762..3b1c34111b 100644 --- a/src/errors.test.ts +++ b/src/errors.test.ts @@ -27,11 +27,13 @@ import { MultipleElementsFoundError, NoElementsFoundError, selectError, + serializePixiebrixError, } from "@/errors"; import { range } from "lodash"; import { deserializeError, serializeError } from "serialize-error"; import { InputValidationError, OutputValidationError } from "@/blocks/errors"; import { matchesAnyPattern } from "@/utils"; +import { isPlainObject } from "@reduxjs/toolkit"; const TEST_MESSAGE = "Test message"; @@ -291,3 +293,22 @@ describe("selectError", () => { ); }); }); + +describe("serializatin", () => { + test("serializes error cause", () => { + const inputValidationError = new InputValidationError( + "test input validation error", + null, + null, + [] + ); + const contextError = new ContextError("text context error", { + cause: inputValidationError, + }); + + const serializedError = serializePixiebrixError(contextError); + + expect(isPlainObject(serializedError)).toBeTruthy(); + expect(isPlainObject(serializedError.cause)).toBeTruthy(); + }); +}); diff --git a/src/errors.ts b/src/errors.ts index 19e798d39d..c963140a28 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -16,7 +16,7 @@ */ import { MessageContext } from "@/core"; -import { deserializeError, ErrorObject } from "serialize-error"; +import { deserializeError, ErrorObject, serializeError } from "serialize-error"; import { AxiosError, AxiosResponse } from "axios"; import { isObject, matchesAnyPattern } from "@/utils"; import safeJsonStringify from "json-stringify-safe"; @@ -250,6 +250,16 @@ export function isContextError(error: unknown): error is ContextError { ); } +// The serializeError preserves custom properties that effectively means the "cause" is skipped by the serializer +export function serializePixiebrixError(error: Error): ErrorObject { + const serializedError = serializeError(error); + if ("cause" in error) { + serializedError.cause = serializePixiebrixError(error.cause as Error); + } + + return serializedError; +} + /** * Returns true iff the root cause of the error was a CancelError. * @param error the error object From 40bca1187250aa43f8176b4314e91df16c53f2fc Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 5 May 2022 16:19:59 +0200 Subject: [PATCH 3/6] 3216 Optimizing jqStacktraceRegexp --- src/blocks/transformers/jq.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/blocks/transformers/jq.ts b/src/blocks/transformers/jq.ts index c437d92d12..3b357bdb47 100644 --- a/src/blocks/transformers/jq.ts +++ b/src/blocks/transformers/jq.ts @@ -22,7 +22,7 @@ import { isNullOrBlank } from "@/utils"; import { InputValidationError } from "@/blocks/errors"; import { isErrorObject } from "@/errors"; -const jqStacktraceRegexp = /jq: error \(at \:0\): (?.*)/; +const jqStacktraceRegexp = /jq: error \(at :0\): (?.*)/; export class JQTransformer extends Transformer { override async isPure(): Promise { From c8a2fd00ebdd9b2e8c947369c1a928caebe2cb5d Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 6 May 2022 16:10:31 +0200 Subject: [PATCH 4/6] 3216 PR comments --- src/blocks/transformers/jq.ts | 17 ++++------------- src/errors.test.ts | 6 +++--- src/errors.ts | 8 +++++--- 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/src/blocks/transformers/jq.ts b/src/blocks/transformers/jq.ts index 3b357bdb47..a2064877db 100644 --- a/src/blocks/transformers/jq.ts +++ b/src/blocks/transformers/jq.ts @@ -77,19 +77,10 @@ export class JQTransformer extends Transformer { throw error; } - let message: string; - if (error.stack.includes("unexpected $end")) { - message = - "Unexpected end of jq filter, are you missing a parentheses, brace, and/or quote mark?"; - } else { - const jqMessageFromStack = jqStacktraceRegexp.exec(error.stack)?.groups - ?.message; - if (jqMessageFromStack == null) { - message = "Invalid jq filter, see error log for details"; - } else { - message = jqMessageFromStack.trim(); - } - } + const message = error.stack.includes("unexpected $end") + ? "Unexpected end of jq filter, are you missing a parentheses, brace, and/or quote mark?" + : jqStacktraceRegexp.exec(error.stack)?.groups?.message?.trim() ?? + "Invalid jq filter, see error log for details"; throw new InputValidationError( message, diff --git a/src/errors.test.ts b/src/errors.test.ts index 3b1c34111b..fdba227d04 100644 --- a/src/errors.test.ts +++ b/src/errors.test.ts @@ -285,16 +285,16 @@ describe("selectError", () => { it("wraps primitive from PromiseRejectionEvent", () => { const errorEvent = new PromiseRejectionEvent( "error", - createUncaughtRejection("It’s a non-error") + createUncaughtRejection("It's a non-error") ); expect(selectError(errorEvent)).toMatchInlineSnapshot( - "[Error: It’s a non-error]" + "[Error: It's a non-error]" ); }); }); -describe("serializatin", () => { +describe("serialization", () => { test("serializes error cause", () => { const inputValidationError = new InputValidationError( "test input validation error", diff --git a/src/errors.ts b/src/errors.ts index c963140a28..3369d89e2d 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -251,10 +251,12 @@ export function isContextError(error: unknown): error is ContextError { } // The serializeError preserves custom properties that effectively means the "cause" is skipped by the serializer -export function serializePixiebrixError(error: Error): ErrorObject { +export function serializePixiebrixError(error: unknown): ErrorObject { const serializedError = serializeError(error); - if ("cause" in error) { - serializedError.cause = serializePixiebrixError(error.cause as Error); + if (typeof error === "object" && "cause" in error) { + serializedError.cause = serializePixiebrixError( + (error as { cause: unknown }).cause + ); } return serializedError; From 1fadfdf18ee102abd343fb864595168bbd4956b4 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Fri, 6 May 2022 17:13:38 -0400 Subject: [PATCH 5/6] #3216: add tests; clarify use of helper method --- src/background/logging.ts | 8 ++-- src/blocks/transformers/jq.test.ts | 75 ++++++++++++++++++++++++++++++ src/blocks/transformers/jq.ts | 7 ++- src/errors.test.ts | 6 ++- src/errors.ts | 9 ++-- 5 files changed, 94 insertions(+), 11 deletions(-) create mode 100644 src/blocks/transformers/jq.test.ts diff --git a/src/background/logging.ts b/src/background/logging.ts index 616c701adc..51f6436c8f 100644 --- a/src/background/logging.ts +++ b/src/background/logging.ts @@ -32,7 +32,7 @@ import { IGNORED_ERROR_PATTERNS, isAxiosError, isContextError, - serializePixiebrixError, + serializeErrorAndProperties, } from "@/errors"; import { expectContext, forbidContext } from "@/utils/expectContext"; import { isAppRequest, selectAbsoluteUrl } from "@/services/requestErrorUtils"; @@ -332,9 +332,9 @@ export async function recordError( context: flatContext, message, data, - - // Ensure it's serialized - error: serializePixiebrixError(maybeSerializedError as Error), + // Ensure the object is fully serialized. Required because it will be stored in IDB and flow through the Redux + // state. Can be converted to serializeError after https://github.com/sindresorhus/serialize-error/issues/74 + error: serializeErrorAndProperties(maybeSerializedError), }), ]); } catch (recordError) { diff --git a/src/blocks/transformers/jq.test.ts b/src/blocks/transformers/jq.test.ts new file mode 100644 index 0000000000..004e962226 --- /dev/null +++ b/src/blocks/transformers/jq.test.ts @@ -0,0 +1,75 @@ +/* + * Copyright (C) 2022 PixieBrix, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import { JQTransformer } from "@/blocks/transformers/jq"; +import { unsafeAssumeValidArg } from "@/runtime/runtimeTypes"; +import ConsoleLogger from "@/utils/ConsoleLogger"; +import { InputValidationError } from "@/blocks/errors"; +import { BusinessError } from "@/errors"; + +describe("parse compile error", () => { + test("invalid fromdate", async () => { + // https://github.com/pixiebrix/pixiebrix-extension/issues/3216 + const promise = new JQTransformer().transform( + unsafeAssumeValidArg({ filter: '"" | fromdate', input: {} }), + { + ctxt: {}, + root: null, + logger: new ConsoleLogger(), + } + ); + + await expect(promise).rejects.toThrowError(InputValidationError); + await expect(promise).rejects.toThrowError( + 'date "" does not match format "%Y-%m-%dT%H:%M:%SZ"' + ); + }); + + test("missing brace", async () => { + // https://github.com/pixiebrix/pixiebrix-extension/issues/3216 + const promise = new JQTransformer().transform( + unsafeAssumeValidArg({ filter: "{", input: {} }), + { + ctxt: {}, + root: null, + logger: new ConsoleLogger(), + } + ); + + await expect(promise).rejects.toThrowError(InputValidationError); + await expect(promise).rejects.toThrowError( + "Unexpected end of jq filter, are you missing a parentheses, brace, and/or quote mark?" + ); + }); + + test("null iteration", async () => { + // https://github.com/pixiebrix/pixiebrix-extension/issues/3216 + const promise = new JQTransformer().transform( + unsafeAssumeValidArg({ filter: ".foo[]", input: {} }), + { + ctxt: {}, + root: null, + logger: new ConsoleLogger(), + } + ); + + await expect(promise).rejects.toThrowError(BusinessError); + await expect(promise).rejects.toThrowError( + "Cannot iterate over null (null)" + ); + }); +}); diff --git a/src/blocks/transformers/jq.ts b/src/blocks/transformers/jq.ts index a2064877db..0ee24c7fc8 100644 --- a/src/blocks/transformers/jq.ts +++ b/src/blocks/transformers/jq.ts @@ -20,7 +20,7 @@ import { BlockArg, BlockOptions, Schema } from "@/core"; import { propertiesToSchema } from "@/validators/generic"; import { isNullOrBlank } from "@/utils"; import { InputValidationError } from "@/blocks/errors"; -import { isErrorObject } from "@/errors"; +import { BusinessError, isErrorObject } from "@/errors"; const jqStacktraceRegexp = /jq: error \(at :0\): (?.*)/; @@ -74,6 +74,11 @@ export class JQTransformer extends Transformer { !isErrorObject(error) || (error.message.length > 13 && !error.message.includes("compile error")) ) { + // Unless there's bug in jq itself, if there's an error at this point, it's business error + if (isErrorObject(error)) { + throw new BusinessError(error.message, { cause: error }); + } + throw error; } diff --git a/src/errors.test.ts b/src/errors.test.ts index fdba227d04..9ad8afcf88 100644 --- a/src/errors.test.ts +++ b/src/errors.test.ts @@ -27,7 +27,7 @@ import { MultipleElementsFoundError, NoElementsFoundError, selectError, - serializePixiebrixError, + serializeErrorAndProperties, } from "@/errors"; import { range } from "lodash"; import { deserializeError, serializeError } from "serialize-error"; @@ -306,8 +306,10 @@ describe("serialization", () => { cause: inputValidationError, }); - const serializedError = serializePixiebrixError(contextError); + const serializedError = serializeErrorAndProperties(contextError); + // Use the isPlainObject from "@reduxjs/toolkit" because it's Redux that requires the object in the state to be + // serializable. We want it to be serializable and especially serializable for redux. expect(isPlainObject(serializedError)).toBeTruthy(); expect(isPlainObject(serializedError.cause)).toBeTruthy(); }); diff --git a/src/errors.ts b/src/errors.ts index 3369d89e2d..6c56804f79 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -250,11 +250,12 @@ export function isContextError(error: unknown): error is ContextError { ); } -// The serializeError preserves custom properties that effectively means the "cause" is skipped by the serializer -export function serializePixiebrixError(error: unknown): ErrorObject { - const serializedError = serializeError(error); +// `serialize-error/serializeError` preserves custom properties, so "cause" in our errors, e.g., ContextError is skipped +// https://github.com/sindresorhus/serialize-error/issues/74 +export function serializeErrorAndProperties(error: unknown): ErrorObject { + const serializedError = serializeError(error, { useToJSON: false }); if (typeof error === "object" && "cause" in error) { - serializedError.cause = serializePixiebrixError( + serializedError.cause = serializeErrorAndProperties( (error as { cause: unknown }).cause ); } From 86a7f929b2fb300e54ddfa2424a1e0adb2b768f7 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Fri, 6 May 2022 17:18:14 -0400 Subject: [PATCH 6/6] #3216: add smoke tests --- src/blocks/transformers/jq.test.ts | 36 +++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/blocks/transformers/jq.test.ts b/src/blocks/transformers/jq.test.ts index 004e962226..2b514230d8 100644 --- a/src/blocks/transformers/jq.test.ts +++ b/src/blocks/transformers/jq.test.ts @@ -21,11 +21,41 @@ import ConsoleLogger from "@/utils/ConsoleLogger"; import { InputValidationError } from "@/blocks/errors"; import { BusinessError } from "@/errors"; +describe("smoke tests", () => { + test("passes input to filter", async () => { + const promise = new JQTransformer().transform( + unsafeAssumeValidArg({ filter: ".foo", data: { foo: 42 } }), + { + ctxt: {}, + root: null, + logger: new ConsoleLogger(), + } + ); + + await expect(promise).resolves.toStrictEqual(42); + }); +}); + +describe("ctxt", () => { + test.each([[null], [""]])("pass context if data is %s", async (data) => { + const promise = new JQTransformer().transform( + unsafeAssumeValidArg({ filter: ".foo", data }), + { + ctxt: { foo: 42 }, + root: null, + logger: new ConsoleLogger(), + } + ); + + await expect(promise).resolves.toStrictEqual(42); + }); +}); + describe("parse compile error", () => { test("invalid fromdate", async () => { // https://github.com/pixiebrix/pixiebrix-extension/issues/3216 const promise = new JQTransformer().transform( - unsafeAssumeValidArg({ filter: '"" | fromdate', input: {} }), + unsafeAssumeValidArg({ filter: '"" | fromdate', data: {} }), { ctxt: {}, root: null, @@ -42,7 +72,7 @@ describe("parse compile error", () => { test("missing brace", async () => { // https://github.com/pixiebrix/pixiebrix-extension/issues/3216 const promise = new JQTransformer().transform( - unsafeAssumeValidArg({ filter: "{", input: {} }), + unsafeAssumeValidArg({ filter: "{", data: {} }), { ctxt: {}, root: null, @@ -59,7 +89,7 @@ describe("parse compile error", () => { test("null iteration", async () => { // https://github.com/pixiebrix/pixiebrix-extension/issues/3216 const promise = new JQTransformer().transform( - unsafeAssumeValidArg({ filter: ".foo[]", input: {} }), + unsafeAssumeValidArg({ filter: ".foo[]", data: {} }), { ctxt: {}, root: null,