Skip to content
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

#3216 JQ fromdate error gets cut off in page editor #3336

Merged
merged 8 commits into from
May 6, 2022
Merged
9 changes: 5 additions & 4 deletions src/background/logging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -32,6 +32,7 @@ import {
IGNORED_ERROR_PATTERNS,
isAxiosError,
isContextError,
serializeErrorAndProperties,
} from "@/errors";
import { expectContext, forbidContext } from "@/utils/expectContext";
import { isAppRequest, selectAbsoluteUrl } from "@/services/requestErrorUtils";
Expand Down Expand Up @@ -331,9 +332,9 @@ export async function recordError(
context: flatContext,
message,
data,

// Ensure it's serialized
error: serializeError(maybeSerializedError),
// 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) {
Expand Down
105 changes: 105 additions & 0 deletions src/blocks/transformers/jq.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/*
* 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 <http://www.gnu.org/licenses/>.
*/

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("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', data: {} }),
{
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: "{", data: {} }),
{
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[]", data: {} }),
{
ctxt: {},
root: null,
logger: new ConsoleLogger(),
}
);

await expect(promise).rejects.toThrowError(BusinessError);
await expect(promise).rejects.toThrowError(
"Cannot iterate over null (null)"
);
});
});
19 changes: 16 additions & 3 deletions src/blocks/transformers/jq.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ 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 <stdin>:0\): (?<message>.*)/;

export class JQTransformer extends Transformer {
override async isPure(): Promise<boolean> {
Expand Down Expand Up @@ -66,13 +68,24 @@ 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"))
) {
// 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;
}

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";
: jqStacktraceRegexp.exec(error.stack)?.groups?.message?.trim() ??
"Invalid jq filter, see error log for details";

throw new InputValidationError(
message,
Expand Down
27 changes: 25 additions & 2 deletions src/errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ import {
MultipleElementsFoundError,
NoElementsFoundError,
selectError,
serializeErrorAndProperties,
} 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";
fregante marked this conversation as resolved.
Show resolved Hide resolved

const TEST_MESSAGE = "Test message";

Expand Down Expand Up @@ -283,11 +285,32 @@ describe("selectError", () => {
it("wraps primitive from PromiseRejectionEvent", () => {
const errorEvent = new PromiseRejectionEvent(
"error",
createUncaughtRejection("Its a non-error")
createUncaughtRejection("It's a non-error")
);

expect(selectError(errorEvent)).toMatchInlineSnapshot(
"[Error: Its a non-error]"
"[Error: It's a non-error]"
);
});
});

describe("serialization", () => {
test("serializes error cause", () => {
const inputValidationError = new InputValidationError(
"test input validation error",
null,
null,
[]
);
const contextError = new ContextError("text context error", {
cause: inputValidationError,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this was a bug:

This is a temporary issue so I wouldn't spend too much time on it 👍

});

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();
});
});
15 changes: 14 additions & 1 deletion src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -250,6 +250,19 @@ export function isContextError(error: unknown): error is ContextError {
);
}

// `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 = serializeErrorAndProperties(
(error as { cause: unknown }).cause
);
}

return serializedError;
}

/**
* Returns true iff the root cause of the error was a CancelError.
* @param error the error object
Expand Down