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
5 changes: 3 additions & 2 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,
serializePixiebrixError,
} from "@/errors";
import { expectContext, forbidContext } from "@/utils/expectContext";
import { isAppRequest, selectAbsoluteUrl } from "@/services/requestErrorUtils";
Expand Down Expand Up @@ -333,7 +334,7 @@ export async function recordError(
data,

// Ensure it's serialized
error: serializeError(maybeSerializedError),
error: serializePixiebrixError(maybeSerializedError as Error),
Copy link
Contributor

Choose a reason for hiding this comment

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

serializeError must accept unknown; as Error is unsafely asserting that unknown is an Error without actually checking for it.

}),
]);
} catch (recordError) {
Expand Down
25 changes: 21 additions & 4 deletions src/blocks/transformers/jq.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import { isNullOrBlank } from "@/utils";
import { InputValidationError } from "@/blocks/errors";
import { isErrorObject } from "@/errors";

const jqStacktraceRegexp = /jq: error \(at \<stdin\>:0\): (?<message>.*)/;

export class JQTransformer extends Transformer {
override async isPure(): Promise<boolean> {
return true;
Expand Down Expand Up @@ -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
Copy link
Contributor

@fregante fregante May 5, 2022

Choose a reason for hiding this comment

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

You can write it this way to avoid the nested condition. Prettier will likely improve the wrapping further

Suggested change
const jqMessageFromStack = jqStacktraceRegexp.exec(error.stack)?.groups
message = jqStacktraceRegexp.exec(error.stack)?.groups?.message.trim()
?? "Invalid jq filter, see error log for details"

?.message;
if (jqMessageFromStack == null) {
message = "Invalid jq filter, see error log for details";
} else {
message = jqMessageFromStack.trim();
}
}

throw new InputValidationError(
message,
Expand Down
21 changes: 21 additions & 0 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,
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";
fregante marked this conversation as resolved.
Show resolved Hide resolved

const TEST_MESSAGE = "Test message";

Expand Down Expand Up @@ -291,3 +293,22 @@ describe("selectError", () => {
);
});
});

describe("serializatin", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe("serializatin", () => {
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 = serializePixiebrixError(contextError);

expect(isPlainObject(serializedError)).toBeTruthy();
expect(isPlainObject(serializedError.cause)).toBeTruthy();
});
});
12 changes: 11 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,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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we keep this function it needs to be renamed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a link to the issue here? sindresorhus/serialize-error#74

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
Expand Down