From 50c307df4dfa3be2b3eceb4e354d021883d2cc10 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 2 Oct 2024 16:42:05 +0100 Subject: [PATCH 01/33] Add more view calculation tests, and implement count distinct. --- packages/backend-core/src/sql/sql.ts | 143 ++++++++--- .../api/controllers/row/ExternalRequest.ts | 3 +- .../src/api/controllers/view/viewsV2.ts | 30 ++- .../src/api/routes/tests/viewV2.spec.ts | 229 +++++++++++++++++- .../src/sdk/app/rows/search/internal/sqs.ts | 27 ++- packages/server/src/sdk/app/views/index.ts | 12 +- packages/types/src/documents/app/view.ts | 24 +- packages/types/src/sdk/row.ts | 25 +- 8 files changed, 440 insertions(+), 53 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 3585dacbed..ed8dc929d6 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -139,29 +139,61 @@ class InternalBuilder { return this.table.schema[column] } - // Takes a string like foo and returns a quoted string like [foo] for SQL Server - // and "foo" for Postgres. - private quote(str: string): string { + private quoteChars(): [string, string] { switch (this.client) { - case SqlClient.SQL_LITE: case SqlClient.ORACLE: case SqlClient.POSTGRES: - return `"${str}"` + return ['"', '"'] case SqlClient.MS_SQL: - return `[${str}]` + return ["[", "]"] case SqlClient.MARIADB: case SqlClient.MY_SQL: - return `\`${str}\`` + case SqlClient.SQL_LITE: + return ["`", "`"] } } - // Takes a string like a.b.c and returns a quoted identifier like [a].[b].[c] - // for SQL Server and `a`.`b`.`c` for MySQL. - private quotedIdentifier(key: string): string { - return key - .split(".") - .map(part => this.quote(part)) - .join(".") + // Takes a string like foo and returns a quoted string like [foo] for SQL Server + // and "foo" for Postgres. + private quote(str: string): string { + const [start, end] = this.quoteChars() + return `${start}${str}${end}` + } + + private isQuoted(key: string): boolean { + const [start, end] = this.quoteChars() + return key.startsWith(start) && key.endsWith(end) + } + + // Takes a string like a.b.c or an array like ["a", "b", "c"] and returns a + // quoted identifier like [a].[b].[c] for SQL Server and `a`.`b`.`c` for + // MySQL. + private quotedIdentifier(key: string | string[]): string { + if (!Array.isArray(key)) { + key = this.splitIdentifier(key) + } + return key.map(part => this.quote(part)).join(".") + } + + // Turns an identifier like a.b.c or `a`.`b`.`c` into ["a", "b", "c"] + private splitIdentifier(key: string): string[] { + const [start, end] = this.quoteChars() + if (this.isQuoted(key)) { + return key.slice(1, -1).split(`${end}.${start}`) + } + return key.split(".") + } + + private qualifyIdentifier(key: string): string { + const tableName = this.getTableName() + const parts = this.splitIdentifier(key) + if (parts[0] !== tableName) { + parts.unshift(tableName) + } + if (this.isQuoted(key)) { + return this.quotedIdentifier(parts) + } + return parts.join(".") } private isFullSelectStatementRequired(): boolean { @@ -231,8 +263,13 @@ class InternalBuilder { // OracleDB can't use character-large-objects (CLOBs) in WHERE clauses, // so when we use them we need to wrap them in to_char(). This function // converts a field name to the appropriate identifier. - private convertClobs(field: string): string { - const parts = field.split(".") + private convertClobs(field: string, opts?: { forSelect?: boolean }): string { + if (this.client !== SqlClient.ORACLE) { + throw new Error( + "you've called convertClobs on a DB that's not Oracle, this is a mistake" + ) + } + const parts = this.splitIdentifier(field) const col = parts.pop()! const schema = this.table.schema[col] let identifier = this.quotedIdentifier(field) @@ -244,7 +281,11 @@ class InternalBuilder { schema.type === FieldType.OPTIONS || schema.type === FieldType.BARCODEQR ) { - identifier = `to_char(${identifier})` + if (opts?.forSelect) { + identifier = `to_char(${identifier}) as ${this.quotedIdentifier(col)}` + } else { + identifier = `to_char(${identifier})` + } } return identifier } @@ -859,28 +900,58 @@ class InternalBuilder { const fields = this.query.resource?.fields || [] const tableName = this.getTableName() if (fields.length > 0) { - query = query.groupBy(fields.map(field => `${tableName}.${field}`)) - query = query.select(fields.map(field => `${tableName}.${field}`)) + const qualifiedFields = fields.map(field => this.qualifyIdentifier(field)) + if (this.client === SqlClient.ORACLE) { + const groupByFields = qualifiedFields.map(field => + this.convertClobs(field) + ) + const selectFields = qualifiedFields.map(field => + this.convertClobs(field, { forSelect: true }) + ) + query = query + .groupByRaw(groupByFields.join(", ")) + .select(this.knex.raw(selectFields.join(", "))) + } else { + query = query.groupBy(qualifiedFields).select(qualifiedFields) + } } for (const aggregation of aggregations) { const op = aggregation.calculationType - const field = `${tableName}.${aggregation.field} as ${aggregation.name}` - switch (op) { - case CalculationType.COUNT: - query = query.count(field) - break - case CalculationType.SUM: - query = query.sum(field) - break - case CalculationType.AVG: - query = query.avg(field) - break - case CalculationType.MIN: - query = query.min(field) - break - case CalculationType.MAX: - query = query.max(field) - break + if (op === CalculationType.COUNT) { + if ("distinct" in aggregation && aggregation.distinct) { + if (this.client === SqlClient.ORACLE) { + const field = this.convertClobs(`${tableName}.${aggregation.field}`) + query = query.select( + this.knex.raw( + `COUNT(DISTINCT ${field}) as ${this.quotedIdentifier( + aggregation.name + )}` + ) + ) + } else { + query = query.countDistinct( + `${tableName}.${aggregation.field} as ${aggregation.name}` + ) + } + } else { + query = query.count(`* as ${aggregation.name}`) + } + } else { + const field = `${tableName}.${aggregation.field} as ${aggregation.name}` + switch (op) { + case CalculationType.SUM: + query = query.sum(field) + break + case CalculationType.AVG: + query = query.avg(field) + break + case CalculationType.MIN: + query = query.min(field) + break + case CalculationType.MAX: + query = query.max(field) + break + } } } return query diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 4ce326c35f..ed8836626c 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -696,9 +696,8 @@ export class ExternalRequest { const calculationFields = helpers.views.calculationFields(this.source) for (const [key, field] of Object.entries(calculationFields)) { aggregations.push({ + ...field, name: key, - field: field.field, - calculationType: field.calculationType, }) } } diff --git a/packages/server/src/api/controllers/view/viewsV2.ts b/packages/server/src/api/controllers/view/viewsV2.ts index 7f6f638541..22a4b725ee 100644 --- a/packages/server/src/api/controllers/view/viewsV2.ts +++ b/packages/server/src/api/controllers/view/viewsV2.ts @@ -11,14 +11,40 @@ import { ViewCalculationFieldMetadata, RelationSchemaField, ViewFieldMetadata, + CalculationType, } from "@budibase/types" import { builderSocket, gridSocket } from "../../../websockets" import { helpers } from "@budibase/shared-core" function stripUnknownFields( - field: BasicViewFieldMetadata -): RequiredKeys { + field: ViewFieldMetadata +): RequiredKeys { if (helpers.views.isCalculationField(field)) { + if (field.calculationType === CalculationType.COUNT) { + if ("distinct" in field && field.distinct) { + return { + order: field.order, + width: field.width, + visible: field.visible, + readonly: field.readonly, + icon: field.icon, + distinct: field.distinct, + calculationType: field.calculationType, + field: field.field, + columns: field.columns, + } + } else { + return { + order: field.order, + width: field.width, + visible: field.visible, + readonly: field.readonly, + icon: field.icon, + calculationType: field.calculationType, + columns: field.columns, + } + } + } const strippedField: RequiredKeys = { order: field.order, width: field.width, diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 669d35ba5b..477645b8f2 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -25,7 +25,7 @@ import { ViewFieldMetadata, FeatureFlag, BBReferenceFieldSubType, - ViewCalculationFieldMetadata, + NumericCalculationFieldMetadata, } from "@budibase/types" import { generator, mocks } from "@budibase/backend-core/tests" import { DatabaseName, getDatasource } from "../../../integrations/tests/utils" @@ -557,13 +557,13 @@ describe.each([ expect(Object.keys(view.schema!)).toHaveLength(1) - let sum = view.schema!.sum as ViewCalculationFieldMetadata + let sum = view.schema!.sum as NumericCalculationFieldMetadata expect(sum).toBeDefined() expect(sum.calculationType).toEqual(CalculationType.SUM) expect(sum.field).toEqual("Price") view = await config.api.viewV2.get(view.id) - sum = view.schema!.sum as ViewCalculationFieldMetadata + sum = view.schema!.sum as NumericCalculationFieldMetadata expect(sum).toBeDefined() expect(sum.calculationType).toEqual(CalculationType.SUM) expect(sum.field).toEqual("Price") @@ -864,6 +864,185 @@ describe.each([ } ) }) + + !isLucene && + describe("calculation views", () => { + let table: Table + let view: ViewV2 + + beforeEach(async () => { + table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + name: "name", + type: FieldType.STRING, + constraints: { + presence: true, + }, + }, + country: { + name: "country", + type: FieldType.STRING, + }, + age: { + name: "age", + type: FieldType.NUMBER, + }, + }, + }) + ) + + view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + schema: { + country: { + visible: true, + }, + age: { + visible: true, + calculationType: CalculationType.SUM, + field: "age", + }, + }, + }) + + await config.api.row.bulkImport(table._id!, { + rows: [ + { + name: "Steve", + age: 30, + country: "UK", + }, + { + name: "Jane", + age: 31, + country: "UK", + }, + { + name: "Ruari", + age: 32, + country: "USA", + }, + { + name: "Alice", + age: 33, + country: "USA", + }, + ], + }) + }) + + it("returns the expected rows prior to modification", async () => { + const { rows } = await config.api.row.search(view.id) + expect(rows).toHaveLength(2) + expect(rows).toEqual( + expect.arrayContaining([ + { + country: "USA", + age: 65, + }, + { + country: "UK", + age: 61, + }, + ]) + ) + }) + + it("can remove a group by field", async () => { + delete view.schema!.country + await config.api.viewV2.update(view) + + const { rows } = await config.api.row.search(view.id) + expect(rows).toHaveLength(1) + expect(rows).toEqual( + expect.arrayContaining([ + { + age: 126, + }, + ]) + ) + }) + + it("can remove a calculation field", async () => { + delete view.schema!.age + await config.api.viewV2.update(view) + + const { rows } = await config.api.row.search(view.id) + expect(rows).toHaveLength(4) + + // Because the removal of the calculation field actually makes this + // no longer a calculation view, these rows will now have _id and + // _rev fields. + expect(rows).toEqual( + expect.arrayContaining([ + expect.objectContaining({ country: "UK" }), + expect.objectContaining({ country: "UK" }), + expect.objectContaining({ country: "USA" }), + expect.objectContaining({ country: "USA" }), + ]) + ) + }) + + it("can add a new group by field", async () => { + view.schema!.name = { visible: true } + await config.api.viewV2.update(view) + + const { rows } = await config.api.row.search(view.id) + expect(rows).toHaveLength(4) + expect(rows).toEqual( + expect.arrayContaining([ + { + name: "Steve", + age: 30, + country: "UK", + }, + { + name: "Jane", + age: 31, + country: "UK", + }, + { + name: "Ruari", + age: 32, + country: "USA", + }, + { + name: "Alice", + age: 33, + country: "USA", + }, + ]) + ) + }) + + it("can add a new calculation field", async () => { + view.schema!.count = { + visible: true, + calculationType: CalculationType.COUNT, + } + await config.api.viewV2.update(view) + + const { rows } = await config.api.row.search(view.id) + expect(rows).toHaveLength(2) + expect(rows).toEqual( + expect.arrayContaining([ + { + country: "USA", + age: 65, + count: 2, + }, + { + country: "UK", + age: 61, + count: 2, + }, + ]) + ) + }) + }) }) describe("delete", () => { @@ -2573,6 +2752,50 @@ describe.each([ expect(actual).toEqual(expected) } }) + + it("should be able to do a COUNT(DISTINCT)", async () => { + const table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + name: "name", + type: FieldType.STRING, + }, + }, + }) + ) + + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + schema: { + count: { + visible: true, + calculationType: CalculationType.COUNT, + distinct: true, + field: "name", + }, + }, + }) + + await config.api.row.bulkImport(table._id!, { + rows: [ + { + name: "John", + }, + { + name: "John", + }, + { + name: "Sue", + }, + ], + }) + + const { rows } = await config.api.row.search(view.id) + expect(rows).toHaveLength(1) + expect(rows[0].count).toEqual(2) + }) }) !isLucene && diff --git a/packages/server/src/sdk/app/rows/search/internal/sqs.ts b/packages/server/src/sdk/app/rows/search/internal/sqs.ts index f0e47ce2b7..3847eb8f31 100644 --- a/packages/server/src/sdk/app/rows/search/internal/sqs.ts +++ b/packages/server/src/sdk/app/rows/search/internal/sqs.ts @@ -1,5 +1,6 @@ import { Aggregation, + CalculationType, Datasource, DocumentType, FieldType, @@ -369,11 +370,27 @@ export async function search( continue } - aggregations.push({ - name: key, - field: mapToUserColumn(field.field), - calculationType: field.calculationType, - }) + if (field.calculationType === CalculationType.COUNT) { + if ("distinct" in field && field.distinct) { + aggregations.push({ + name: key, + distinct: true, + field: mapToUserColumn(field.field), + calculationType: field.calculationType, + }) + } else { + aggregations.push({ + name: key, + calculationType: field.calculationType, + }) + } + } else { + aggregations.push({ + name: key, + field: mapToUserColumn(field.field), + calculationType: field.calculationType, + }) + } } } diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 2bd90822c7..08d7c55d07 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -1,4 +1,5 @@ import { + CalculationType, FieldType, PermissionLevel, RelationSchemaField, @@ -65,6 +66,15 @@ async function guardCalculationViewSchema( const calculationFields = helpers.views.calculationFields(view) for (const calculationFieldName of Object.keys(calculationFields)) { const schema = calculationFields[calculationFieldName] + const isCount = schema.calculationType === CalculationType.COUNT + const isDistinct = isCount && "distinct" in schema && schema.distinct + + // Count fields that aren't distinct don't need to reference another field, + // so we don't validate it. + if (isCount && !isDistinct) { + continue + } + const targetSchema = table.schema[schema.field] if (!targetSchema) { throw new HTTPError( @@ -73,7 +83,7 @@ async function guardCalculationViewSchema( ) } - if (!helpers.schema.isNumeric(targetSchema)) { + if (!isCount && !helpers.schema.isNumeric(targetSchema)) { throw new HTTPError( `Calculation field "${calculationFieldName}" references field "${schema.field}" which is not a numeric field`, 400 diff --git a/packages/types/src/documents/app/view.ts b/packages/types/src/documents/app/view.ts index a957564039..24995d79e4 100644 --- a/packages/types/src/documents/app/view.ts +++ b/packages/types/src/documents/app/view.ts @@ -42,11 +42,31 @@ export interface RelationSchemaField extends UIFieldMetadata { readonly?: boolean } -export interface ViewCalculationFieldMetadata extends BasicViewFieldMetadata { - calculationType: CalculationType +export interface NumericCalculationFieldMetadata + extends BasicViewFieldMetadata { + calculationType: + | CalculationType.MIN + | CalculationType.MAX + | CalculationType.SUM + | CalculationType.AVG field: string } +export interface CountCalculationFieldMetadata extends BasicViewFieldMetadata { + calculationType: CalculationType.COUNT +} + +export interface CountDistinctCalculationFieldMetadata + extends CountCalculationFieldMetadata { + distinct: true + field: string +} + +export type ViewCalculationFieldMetadata = + | NumericCalculationFieldMetadata + | CountCalculationFieldMetadata + | CountDistinctCalculationFieldMetadata + export type ViewFieldMetadata = | BasicViewFieldMetadata | ViewCalculationFieldMetadata diff --git a/packages/types/src/sdk/row.ts b/packages/types/src/sdk/row.ts index 0b63e88229..ea46e82508 100644 --- a/packages/types/src/sdk/row.ts +++ b/packages/types/src/sdk/row.ts @@ -3,12 +3,33 @@ import { SearchFilters } from "./search" import { CalculationType, Row } from "../documents" import { WithRequired } from "../shared" -export interface Aggregation { +export interface BaseAggregation { name: string - calculationType: CalculationType +} + +export interface NumericAggregation extends BaseAggregation { + calculationType: + | CalculationType.AVG + | CalculationType.MAX + | CalculationType.MIN + | CalculationType.SUM field: string } +export interface CountAggregation extends BaseAggregation { + calculationType: CalculationType.COUNT +} + +export interface CountDistinctAggregation extends CountAggregation { + distinct: true + field: string +} + +export type Aggregation = + | NumericAggregation + | CountAggregation + | CountDistinctAggregation + export interface SearchParams { tableId?: string viewId?: string From 7ed28593fb0efea21330c5af19e2df33e0c3b634 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 2 Oct 2024 16:47:53 +0100 Subject: [PATCH 02/33] Add a test for a count distinct column that references a non-existent field. --- .../src/api/routes/tests/viewV2.spec.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 477645b8f2..bda9930156 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -2796,6 +2796,30 @@ describe.each([ expect(rows).toHaveLength(1) expect(rows[0].count).toEqual(2) }) + + it("should not be able to COUNT(DISTINCT ...) against a non-existent field", async () => { + await config.api.viewV2.create( + { + tableId: table._id!, + name: generator.guid(), + schema: { + count: { + visible: true, + calculationType: CalculationType.COUNT, + distinct: true, + field: "does not exist oh no", + }, + }, + }, + { + status: 400, + body: { + message: + 'Calculation field "count" references field "does not exist oh no" which does not exist in the table schema', + }, + } + ) + }) }) !isLucene && From b004ef5448376c3d5d7868b38ee27b3c2294d13d Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 3 Oct 2024 09:42:08 +0100 Subject: [PATCH 03/33] Pull the error object out of isolated-vm when a user script throws an error. --- .../src/jsRunner/tests/jsRunner.spec.ts | 2 +- .../server/src/jsRunner/vm/isolated-vm.ts | 22 ++++++++++++++++++- .../src/helpers/javascript.ts | 3 +++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/packages/server/src/jsRunner/tests/jsRunner.spec.ts b/packages/server/src/jsRunner/tests/jsRunner.spec.ts index 525f6d865e..2b9fc08041 100644 --- a/packages/server/src/jsRunner/tests/jsRunner.spec.ts +++ b/packages/server/src/jsRunner/tests/jsRunner.spec.ts @@ -44,7 +44,7 @@ describe("jsRunner (using isolated-vm)", () => { const output = await processJS( `return this.constructor.constructor("return process.env")()` ) - expect(output).toBe("Error while executing JS") + expect(output).toBe("ReferenceError: process is not defined") }) describe("helpers", () => { diff --git a/packages/server/src/jsRunner/vm/isolated-vm.ts b/packages/server/src/jsRunner/vm/isolated-vm.ts index 87d5b966c2..e502b799bd 100644 --- a/packages/server/src/jsRunner/vm/isolated-vm.ts +++ b/packages/server/src/jsRunner/vm/isolated-vm.ts @@ -17,6 +17,15 @@ class ExecutionTimeoutError extends Error { } } +class UserScriptError extends Error { + constructor(readonly userScriptError: Error) { + super( + `error while running user-supplied JavaScript: ${userScriptError.message}`, + { cause: userScriptError } + ) + } +} + export class IsolatedVM implements VM { private isolate: ivm.Isolate private vm: ivm.Context @@ -29,6 +38,7 @@ export class IsolatedVM implements VM { private readonly resultKey = "results" private runResultKey: string + private runErrorKey: string constructor({ memoryLimit, @@ -47,6 +57,7 @@ export class IsolatedVM implements VM { this.jail.setSync("global", this.jail.derefInto()) this.runResultKey = crypto.randomUUID() + this.runErrorKey = crypto.randomUUID() this.addToContext({ [this.resultKey]: { [this.runResultKey]: "" }, }) @@ -216,7 +227,13 @@ export class IsolatedVM implements VM { } } - code = `results['${this.runResultKey}']=${this.codeWrapper(code)}` + code = ` + try { + results['${this.runResultKey}']=${this.codeWrapper(code)} + } catch (e) { + results['${this.runErrorKey}']=e + } + ` const script = this.isolate.compileScriptSync(code) @@ -227,6 +244,9 @@ export class IsolatedVM implements VM { // We can't rely on the script run result as it will not work for non-transferable values const result = this.getFromContext(this.resultKey) + if (result[this.runErrorKey]) { + throw new UserScriptError(result[this.runErrorKey]) + } return result[this.runResultKey] } diff --git a/packages/string-templates/src/helpers/javascript.ts b/packages/string-templates/src/helpers/javascript.ts index 3e16d8a07b..27a71da834 100644 --- a/packages/string-templates/src/helpers/javascript.ts +++ b/packages/string-templates/src/helpers/javascript.ts @@ -100,6 +100,9 @@ export function processJS(handlebars: string, context: any) { if (error.name === "ExecutionTimeoutError") { return "Request JS execution limit hit" } + if ("userScriptError" in error) { + return error.userScriptError.toString() + } return "Error while executing JS" } } From fdbe633b02be6adfe6956a3c08347dcf193d06cb Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 3 Oct 2024 12:43:57 +0100 Subject: [PATCH 04/33] Get errors working on the client side as well. --- .../bindings/EvaluationSidePanel.svelte | 27 ++++++++++++------- packages/string-templates/src/index.ts | 16 +++++++++++ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte b/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte index 2c4e6a0991..3b71fad715 100644 --- a/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte +++ b/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte @@ -8,22 +8,29 @@ export let evaluating = false export let expression = null - $: error = expressionResult === "Error while executing JS" + $: error = expressionResult && expressionResult.error != null $: empty = expression == null || expression?.trim() === "" $: success = !error && !empty $: highlightedResult = highlight(expressionResult) - const highlight = json => { - if (json == null) { + const highlight = result => { + if (result == null) { return "" } - // Attempt to parse and then stringify, in case this is valid JSON - try { - json = JSON.stringify(JSON.parse(json), null, 2) - } catch (err) { - // Ignore + + let str + if (result.error) { + str = result.error.toString() + } else { + // Attempt to parse and then stringify, in case this is valid result + try { + str = JSON.stringify(JSON.parse(result.result), null, 2) + } catch (err) { + // Ignore + } } - return formatHighlight(json, { + + return formatHighlight(str, { keyColor: "#e06c75", numberColor: "#e5c07b", stringColor: "#98c379", @@ -34,7 +41,7 @@ } const copy = () => { - let clipboardVal = expressionResult + let clipboardVal = expressionResult.result if (typeof clipboardVal === "object") { clipboardVal = JSON.stringify(clipboardVal, null, 2) } diff --git a/packages/string-templates/src/index.ts b/packages/string-templates/src/index.ts index 8d5fe4c16d..69ca05ef76 100644 --- a/packages/string-templates/src/index.ts +++ b/packages/string-templates/src/index.ts @@ -463,6 +463,22 @@ export function defaultJSSetup() { setTimeout: undefined, } createContext(context) + + js = ` + result = { + result: null, + error: null, + }; + + try { + result.result = ${js}; + } catch (e) { + result.error = e.toString(); + } + + result; + ` + return runInNewContext(js, context, { timeout: 1000 }) }) } else { From df242cc2ad25978786b5da80045579fde989241c Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 3 Oct 2024 13:07:22 +0100 Subject: [PATCH 05/33] Don't break the fact that processStringSync returns a string. --- .../common/bindings/BindingPanel.svelte | 16 ++++++--- .../bindings/EvaluationSidePanel.svelte | 33 +++++++++++-------- .../server/src/jsRunner/vm/isolated-vm.ts | 2 ++ .../src/utilities/rowProcessor/utils.ts | 9 ++++- packages/string-templates/src/errors.ts | 10 ++++++ .../src/helpers/javascript.ts | 4 +-- packages/string-templates/src/index.ts | 14 ++++++-- 7 files changed, 64 insertions(+), 24 deletions(-) diff --git a/packages/builder/src/components/common/bindings/BindingPanel.svelte b/packages/builder/src/components/common/bindings/BindingPanel.svelte index d8edf0cbb1..2f39dc6f53 100644 --- a/packages/builder/src/components/common/bindings/BindingPanel.svelte +++ b/packages/builder/src/components/common/bindings/BindingPanel.svelte @@ -66,6 +66,7 @@ let insertAtPos let targetMode = null let expressionResult + let expressionError let evaluating = false $: useSnippets = allowSnippets && !$licensing.isFreePlan @@ -142,10 +143,16 @@ } const debouncedEval = Utils.debounce((expression, context, snippets) => { - expressionResult = processStringSync(expression || "", { - ...context, - snippets, - }) + try { + expressionError = null + expressionResult = processStringSync(expression || "", { + ...context, + snippets, + }) + } catch (err) { + expressionResult = null + expressionError = err + } evaluating = false }, 260) @@ -370,6 +377,7 @@ {:else if sidePanel === SidePanels.Evaluation} diff --git a/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte b/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte index 3b71fad715..2c5c1d4587 100644 --- a/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte +++ b/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte @@ -5,32 +5,35 @@ import { fade } from "svelte/transition" export let expressionResult + export let expressionError export let evaluating = false export let expression = null - $: error = expressionResult && expressionResult.error != null + $: error = expressionError != null $: empty = expression == null || expression?.trim() === "" $: success = !error && !empty $: highlightedResult = highlight(expressionResult) - const highlight = result => { - if (result == null) { + const formatError = err => { + if (err.code === "USER_SCRIPT_ERROR") { + return err.userScriptError.toString() + } + return err.toString() + } + + const highlight = json => { + if (json == null) { return "" } - let str - if (result.error) { - str = result.error.toString() - } else { - // Attempt to parse and then stringify, in case this is valid result - try { - str = JSON.stringify(JSON.parse(result.result), null, 2) - } catch (err) { - // Ignore - } + // Attempt to parse and then stringify, in case this is valid result + try { + json = JSON.stringify(JSON.parse(json), null, 2) + } catch (err) { + // Ignore } - return formatHighlight(str, { + return formatHighlight(json, { keyColor: "#e06c75", numberColor: "#e5c07b", stringColor: "#98c379", @@ -80,6 +83,8 @@
{#if empty} Your expression will be evaluated here + {:else if error} + {formatError(expressionError)} {:else} {@html highlightedResult} diff --git a/packages/server/src/jsRunner/vm/isolated-vm.ts b/packages/server/src/jsRunner/vm/isolated-vm.ts index e502b799bd..9262687925 100644 --- a/packages/server/src/jsRunner/vm/isolated-vm.ts +++ b/packages/server/src/jsRunner/vm/isolated-vm.ts @@ -11,6 +11,7 @@ import { iifeWrapper } from "@budibase/string-templates" import environment from "../../environment" class ExecutionTimeoutError extends Error { + code = "ERR_SCRIPT_EXECUTION_TIMEOUT" constructor(message: string) { super(message) this.name = "ExecutionTimeoutError" @@ -18,6 +19,7 @@ class ExecutionTimeoutError extends Error { } class UserScriptError extends Error { + code = "USER_SCRIPT_ERROR" constructor(readonly userScriptError: Error) { super( `error while running user-supplied JavaScript: ${userScriptError.message}`, diff --git a/packages/server/src/utilities/rowProcessor/utils.ts b/packages/server/src/utilities/rowProcessor/utils.ts index 0fa6f62807..3a1ce08ece 100644 --- a/packages/server/src/utilities/rowProcessor/utils.ts +++ b/packages/server/src/utilities/rowProcessor/utils.ts @@ -81,7 +81,14 @@ export async function processFormulas( ...row, [column]: tracer.trace("processStringSync", {}, span => { span?.addTags({ table_id: table._id, column, static: isStatic }) - return processStringSync(formula, context) + try { + return processStringSync(formula, context) + } catch (err: any) { + if (err.code === "USER_SCRIPT_ERROR") { + return err.userScriptError.toString() + } + throw err + } }), } } diff --git a/packages/string-templates/src/errors.ts b/packages/string-templates/src/errors.ts index 79a8a525ef..4c7cbdb360 100644 --- a/packages/string-templates/src/errors.ts +++ b/packages/string-templates/src/errors.ts @@ -1,3 +1,13 @@ export class JsErrorTimeout extends Error { code = "ERR_SCRIPT_EXECUTION_TIMEOUT" } + +export class UserScriptError extends Error { + code = "USER_SCRIPT_ERROR" + + constructor(readonly userScriptError: Error) { + super( + `error while running user-supplied JavaScript: ${userScriptError.message}` + ) + } +} diff --git a/packages/string-templates/src/helpers/javascript.ts b/packages/string-templates/src/helpers/javascript.ts index 27a71da834..e1d7f3e8a3 100644 --- a/packages/string-templates/src/helpers/javascript.ts +++ b/packages/string-templates/src/helpers/javascript.ts @@ -100,8 +100,8 @@ export function processJS(handlebars: string, context: any) { if (error.name === "ExecutionTimeoutError") { return "Request JS execution limit hit" } - if ("userScriptError" in error) { - return error.userScriptError.toString() + if (error.code === "USER_SCRIPT_ERROR") { + throw error } return "Error while executing JS" } diff --git a/packages/string-templates/src/index.ts b/packages/string-templates/src/index.ts index 69ca05ef76..aceea1da7e 100644 --- a/packages/string-templates/src/index.ts +++ b/packages/string-templates/src/index.ts @@ -16,6 +16,7 @@ import { removeJSRunner, setJSRunner } from "./helpers/javascript" import manifest from "./manifest.json" import { ProcessOptions } from "./types" +import { UserScriptError } from "./errors" export { helpersToRemoveForJs, getJsHelperList } from "./helpers/list" export { FIND_ANY_HBS_REGEX } from "./utilities" @@ -230,6 +231,9 @@ export function processStringSync( return process(string) } } catch (err) { + if (err.code === "USER_SCRIPT_ERROR") { + throw err + } return input } } @@ -448,7 +452,7 @@ export function convertToJS(hbs: string) { return `${varBlock}${js}` } -export { JsErrorTimeout } from "./errors" +export { JsErrorTimeout, UserScriptError } from "./errors" export function defaultJSSetup() { if (!isBackendService()) { @@ -473,13 +477,17 @@ export function defaultJSSetup() { try { result.result = ${js}; } catch (e) { - result.error = e.toString(); + result.error = e; } result; ` - return runInNewContext(js, context, { timeout: 1000 }) + const result = runInNewContext(js, context, { timeout: 1000 }) + if (result.error) { + throw new UserScriptError(result.error) + } + return result.result }) } else { removeJSRunner() From f9ccbbe081b447c36ae8b472dc64124833fb8c79 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 3 Oct 2024 13:11:01 +0100 Subject: [PATCH 06/33] Fix jsRunner.spec.ts. --- packages/server/src/jsRunner/tests/jsRunner.spec.ts | 7 ++++--- packages/server/src/jsRunner/vm/isolated-vm.ts | 2 +- packages/string-templates/src/errors.ts | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/server/src/jsRunner/tests/jsRunner.spec.ts b/packages/server/src/jsRunner/tests/jsRunner.spec.ts index 2b9fc08041..7d2df17c1f 100644 --- a/packages/server/src/jsRunner/tests/jsRunner.spec.ts +++ b/packages/server/src/jsRunner/tests/jsRunner.spec.ts @@ -41,10 +41,11 @@ describe("jsRunner (using isolated-vm)", () => { }) it("should prevent sandbox escape", async () => { - const output = await processJS( - `return this.constructor.constructor("return process.env")()` + await expect( + processJS(`return this.constructor.constructor("return process.env")()`) + ).rejects.toThrow( + "error while running user-supplied JavaScript: ReferenceError: process is not defined" ) - expect(output).toBe("ReferenceError: process is not defined") }) describe("helpers", () => { diff --git a/packages/server/src/jsRunner/vm/isolated-vm.ts b/packages/server/src/jsRunner/vm/isolated-vm.ts index 9262687925..0a9e93f475 100644 --- a/packages/server/src/jsRunner/vm/isolated-vm.ts +++ b/packages/server/src/jsRunner/vm/isolated-vm.ts @@ -22,7 +22,7 @@ class UserScriptError extends Error { code = "USER_SCRIPT_ERROR" constructor(readonly userScriptError: Error) { super( - `error while running user-supplied JavaScript: ${userScriptError.message}`, + `error while running user-supplied JavaScript: ${userScriptError.toString()}`, { cause: userScriptError } ) } diff --git a/packages/string-templates/src/errors.ts b/packages/string-templates/src/errors.ts index 4c7cbdb360..d68461561e 100644 --- a/packages/string-templates/src/errors.ts +++ b/packages/string-templates/src/errors.ts @@ -7,7 +7,7 @@ export class UserScriptError extends Error { constructor(readonly userScriptError: Error) { super( - `error while running user-supplied JavaScript: ${userScriptError.message}` + `error while running user-supplied JavaScript: ${userScriptError.toString()}` ) } } From 795b10c4c9145d15c78ae708b51af44de179b924 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 3 Oct 2024 13:11:35 +0100 Subject: [PATCH 07/33] Fix types in string-templates. --- packages/string-templates/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/string-templates/src/index.ts b/packages/string-templates/src/index.ts index aceea1da7e..8fd2e7bfc4 100644 --- a/packages/string-templates/src/index.ts +++ b/packages/string-templates/src/index.ts @@ -230,7 +230,7 @@ export function processStringSync( } else { return process(string) } - } catch (err) { + } catch (err: any) { if (err.code === "USER_SCRIPT_ERROR") { throw err } From 646fc5e6bdde971b5c08b62bb0b529022740f9de Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 3 Oct 2024 15:56:19 +0100 Subject: [PATCH 08/33] Respond to PR comment. --- .../bindings/EvaluationSidePanel.svelte | 3 ++- .../server/src/jsRunner/vm/isolated-vm.ts | 26 +++++-------------- .../src/utilities/rowProcessor/utils.ts | 4 +-- packages/string-templates/src/errors.ts | 4 +-- .../src/helpers/javascript.ts | 8 +++--- packages/string-templates/src/index.ts | 6 ++--- 6 files changed, 18 insertions(+), 33 deletions(-) diff --git a/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte b/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte index 2c5c1d4587..ffb8f45297 100644 --- a/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte +++ b/packages/builder/src/components/common/bindings/EvaluationSidePanel.svelte @@ -3,6 +3,7 @@ import { Icon, ProgressCircle, notifications } from "@budibase/bbui" import { copyToClipboard } from "@budibase/bbui/helpers" import { fade } from "svelte/transition" + import { UserScriptError } from "@budibase/string-templates" export let expressionResult export let expressionError @@ -15,7 +16,7 @@ $: highlightedResult = highlight(expressionResult) const formatError = err => { - if (err.code === "USER_SCRIPT_ERROR") { + if (err.code === UserScriptError.code) { return err.userScriptError.toString() } return err.toString() diff --git a/packages/server/src/jsRunner/vm/isolated-vm.ts b/packages/server/src/jsRunner/vm/isolated-vm.ts index 0a9e93f475..87478a2f8a 100644 --- a/packages/server/src/jsRunner/vm/isolated-vm.ts +++ b/packages/server/src/jsRunner/vm/isolated-vm.ts @@ -7,27 +7,13 @@ import querystring from "querystring" import { BundleType, loadBundle } from "../bundles" import { Snippet, VM } from "@budibase/types" -import { iifeWrapper } from "@budibase/string-templates" +import { + iifeWrapper, + JsErrorTimeout, + UserScriptError, +} from "@budibase/string-templates" import environment from "../../environment" -class ExecutionTimeoutError extends Error { - code = "ERR_SCRIPT_EXECUTION_TIMEOUT" - constructor(message: string) { - super(message) - this.name = "ExecutionTimeoutError" - } -} - -class UserScriptError extends Error { - code = "USER_SCRIPT_ERROR" - constructor(readonly userScriptError: Error) { - super( - `error while running user-supplied JavaScript: ${userScriptError.toString()}`, - { cause: userScriptError } - ) - } -} - export class IsolatedVM implements VM { private isolate: ivm.Isolate private vm: ivm.Context @@ -223,7 +209,7 @@ export class IsolatedVM implements VM { if (this.isolateAccumulatedTimeout) { const cpuMs = Number(this.isolate.cpuTime) / 1e6 if (cpuMs > this.isolateAccumulatedTimeout) { - throw new ExecutionTimeoutError( + throw new JsErrorTimeout( `CPU time limit exceeded (${cpuMs}ms > ${this.isolateAccumulatedTimeout}ms)` ) } diff --git a/packages/server/src/utilities/rowProcessor/utils.ts b/packages/server/src/utilities/rowProcessor/utils.ts index 3a1ce08ece..29a5de221f 100644 --- a/packages/server/src/utilities/rowProcessor/utils.ts +++ b/packages/server/src/utilities/rowProcessor/utils.ts @@ -1,5 +1,5 @@ import { AutoFieldDefaultNames } from "../../constants" -import { processStringSync } from "@budibase/string-templates" +import { processStringSync, UserScriptError } from "@budibase/string-templates" import { AutoColumnFieldMetadata, FieldSchema, @@ -84,7 +84,7 @@ export async function processFormulas( try { return processStringSync(formula, context) } catch (err: any) { - if (err.code === "USER_SCRIPT_ERROR") { + if (err.code === UserScriptError.code) { return err.userScriptError.toString() } throw err diff --git a/packages/string-templates/src/errors.ts b/packages/string-templates/src/errors.ts index d68461561e..7a300ac863 100644 --- a/packages/string-templates/src/errors.ts +++ b/packages/string-templates/src/errors.ts @@ -1,9 +1,9 @@ export class JsErrorTimeout extends Error { - code = "ERR_SCRIPT_EXECUTION_TIMEOUT" + static code = "ERR_SCRIPT_EXECUTION_TIMEOUT" } export class UserScriptError extends Error { - code = "USER_SCRIPT_ERROR" + static code = "USER_SCRIPT_ERROR" constructor(readonly userScriptError: Error) { super( diff --git a/packages/string-templates/src/helpers/javascript.ts b/packages/string-templates/src/helpers/javascript.ts index e1d7f3e8a3..059c8a95f5 100644 --- a/packages/string-templates/src/helpers/javascript.ts +++ b/packages/string-templates/src/helpers/javascript.ts @@ -3,6 +3,7 @@ import cloneDeep from "lodash/fp/cloneDeep" import { LITERAL_MARKER } from "../helpers/constants" import { getJsHelperList } from "./list" import { iifeWrapper } from "../iife" +import { JsErrorTimeout, UserScriptError } from "../errors" // The method of executing JS scripts depends on the bundle being built. // This setter is used in the entrypoint (either index.js or index.mjs). @@ -94,13 +95,10 @@ export function processJS(handlebars: string, context: any) { } catch (error: any) { onErrorLog && onErrorLog(error) - if (error.code === "ERR_SCRIPT_EXECUTION_TIMEOUT") { + if (error.code === JsErrorTimeout.code) { return "Timed out while executing JS" } - if (error.name === "ExecutionTimeoutError") { - return "Request JS execution limit hit" - } - if (error.code === "USER_SCRIPT_ERROR") { + if (error.code === UserScriptError.code) { throw error } return "Error while executing JS" diff --git a/packages/string-templates/src/index.ts b/packages/string-templates/src/index.ts index 8fd2e7bfc4..fe2b4c261c 100644 --- a/packages/string-templates/src/index.ts +++ b/packages/string-templates/src/index.ts @@ -231,7 +231,7 @@ export function processStringSync( return process(string) } } catch (err: any) { - if (err.code === "USER_SCRIPT_ERROR") { + if (err.code === UserScriptError.code) { throw err } return input @@ -468,7 +468,7 @@ export function defaultJSSetup() { } createContext(context) - js = ` + const wrappedJs = ` result = { result: null, error: null, @@ -483,7 +483,7 @@ export function defaultJSSetup() { result; ` - const result = runInNewContext(js, context, { timeout: 1000 }) + const result = runInNewContext(wrappedJs, context, { timeout: 1000 }) if (result.error) { throw new UserScriptError(result.error) } From 03c514be4c6e8effb1ec657282d6c5415eaf5e78 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 3 Oct 2024 16:16:42 +0100 Subject: [PATCH 09/33] Fix tests. --- .../server/src/api/routes/tests/row.spec.ts | 17 +++++++++-------- packages/server/src/jsRunner/index.ts | 4 ++-- packages/server/src/jsRunner/vm/isolated-vm.ts | 13 +++++++------ packages/string-templates/src/errors.ts | 11 +++++++++-- .../string-templates/src/helpers/javascript.ts | 9 ++++++--- packages/string-templates/src/index.ts | 2 +- 6 files changed, 34 insertions(+), 22 deletions(-) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index f751942df9..7b590befc0 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -49,6 +49,7 @@ import * as uuid from "uuid" import { Knex } from "knex" import { InternalTables } from "../../../db/utils" import { withEnv } from "../../../environment" +import { JsTimeoutError } from "@budibase/string-templates/src/errors" const timestamp = new Date("2023-01-26T11:48:57.597Z").toISOString() tk.freeze(timestamp) @@ -76,13 +77,13 @@ async function waitForEvent( } describe.each([ - ["lucene", undefined], + // ["lucene", undefined], ["sqs", undefined], - [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], - [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], + // [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], + // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + // [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("/rows (%s)", (providerType, dsProvider) => { const isInternal = dsProvider === undefined const isLucene = providerType === "lucene" @@ -3013,7 +3014,7 @@ describe.each([ let i = 0 for (; i < 10; i++) { const row = rows[i] - if (row.formula !== "Timed out while executing JS") { + if (row.formula !== JsTimeoutError.message) { break } } @@ -3027,7 +3028,7 @@ describe.each([ for (; i < 10; i++) { const row = rows[i] expect(row.text).toBe("foo") - expect(row.formula).toBe("Request JS execution limit hit") + expect(row.formula).toStartWith("CPU time limit exceeded ") } } } diff --git a/packages/server/src/jsRunner/index.ts b/packages/server/src/jsRunner/index.ts index ccfca7cb4c..e17529a687 100644 --- a/packages/server/src/jsRunner/index.ts +++ b/packages/server/src/jsRunner/index.ts @@ -1,7 +1,7 @@ import { serializeError } from "serialize-error" import env from "../environment" import { - JsErrorTimeout, + JsTimeoutError, setJSRunner, setOnErrorLog, } from "@budibase/string-templates" @@ -40,7 +40,7 @@ export function init() { return vm.withContext(rest, () => vm.execute(js)) } catch (error: any) { if (error.message === "Script execution timed out.") { - throw new JsErrorTimeout() + throw new JsTimeoutError() } throw error } diff --git a/packages/server/src/jsRunner/vm/isolated-vm.ts b/packages/server/src/jsRunner/vm/isolated-vm.ts index 87478a2f8a..b8ed90aa23 100644 --- a/packages/server/src/jsRunner/vm/isolated-vm.ts +++ b/packages/server/src/jsRunner/vm/isolated-vm.ts @@ -7,13 +7,14 @@ import querystring from "querystring" import { BundleType, loadBundle } from "../bundles" import { Snippet, VM } from "@budibase/types" -import { - iifeWrapper, - JsErrorTimeout, - UserScriptError, -} from "@budibase/string-templates" +import { iifeWrapper, UserScriptError } from "@budibase/string-templates" import environment from "../../environment" +export class JsRequestTimeoutError extends Error { + static code = "JS_REQUEST_TIMEOUT_ERROR" + code = JsRequestTimeoutError.code +} + export class IsolatedVM implements VM { private isolate: ivm.Isolate private vm: ivm.Context @@ -209,7 +210,7 @@ export class IsolatedVM implements VM { if (this.isolateAccumulatedTimeout) { const cpuMs = Number(this.isolate.cpuTime) / 1e6 if (cpuMs > this.isolateAccumulatedTimeout) { - throw new JsErrorTimeout( + throw new JsRequestTimeoutError( `CPU time limit exceeded (${cpuMs}ms > ${this.isolateAccumulatedTimeout}ms)` ) } diff --git a/packages/string-templates/src/errors.ts b/packages/string-templates/src/errors.ts index 7a300ac863..77526c1b96 100644 --- a/packages/string-templates/src/errors.ts +++ b/packages/string-templates/src/errors.ts @@ -1,9 +1,16 @@ -export class JsErrorTimeout extends Error { - static code = "ERR_SCRIPT_EXECUTION_TIMEOUT" +export class JsTimeoutError extends Error { + static message = "Timed out while executing JS" + static code = "JS_TIMEOUT_ERROR" + code: string = JsTimeoutError.code + + constructor() { + super(JsTimeoutError.message) + } } export class UserScriptError extends Error { static code = "USER_SCRIPT_ERROR" + code: string = UserScriptError.code constructor(readonly userScriptError: Error) { super( diff --git a/packages/string-templates/src/helpers/javascript.ts b/packages/string-templates/src/helpers/javascript.ts index 059c8a95f5..879cd29fa6 100644 --- a/packages/string-templates/src/helpers/javascript.ts +++ b/packages/string-templates/src/helpers/javascript.ts @@ -3,7 +3,7 @@ import cloneDeep from "lodash/fp/cloneDeep" import { LITERAL_MARKER } from "../helpers/constants" import { getJsHelperList } from "./list" import { iifeWrapper } from "../iife" -import { JsErrorTimeout, UserScriptError } from "../errors" +import { JsTimeoutError, UserScriptError } from "../errors" // The method of executing JS scripts depends on the bundle being built. // This setter is used in the entrypoint (either index.js or index.mjs). @@ -95,8 +95,11 @@ export function processJS(handlebars: string, context: any) { } catch (error: any) { onErrorLog && onErrorLog(error) - if (error.code === JsErrorTimeout.code) { - return "Timed out while executing JS" + if (error.code === "JS_REQUEST_TIMEOUT_ERROR") { + return error.message + } + if (error.code === JsTimeoutError.code) { + return JsTimeoutError.message } if (error.code === UserScriptError.code) { throw error diff --git a/packages/string-templates/src/index.ts b/packages/string-templates/src/index.ts index fe2b4c261c..0f5326374f 100644 --- a/packages/string-templates/src/index.ts +++ b/packages/string-templates/src/index.ts @@ -452,7 +452,7 @@ export function convertToJS(hbs: string) { return `${varBlock}${js}` } -export { JsErrorTimeout, UserScriptError } from "./errors" +export { JsTimeoutError, UserScriptError } from "./errors" export function defaultJSSetup() { if (!isBackendService()) { From 15a30b1d9ef30b14770ef8ab2ade298bcdee98ab Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 3 Oct 2024 16:32:14 +0100 Subject: [PATCH 10/33] Fix yet more tests. --- .../server/src/api/routes/tests/row.spec.ts | 14 +++++------ .../src/helpers/javascript.ts | 23 +++++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 7b590befc0..5222069460 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -49,7 +49,7 @@ import * as uuid from "uuid" import { Knex } from "knex" import { InternalTables } from "../../../db/utils" import { withEnv } from "../../../environment" -import { JsTimeoutError } from "@budibase/string-templates/src/errors" +import { JsTimeoutError } from "@budibase/string-templates" const timestamp = new Date("2023-01-26T11:48:57.597Z").toISOString() tk.freeze(timestamp) @@ -77,13 +77,13 @@ async function waitForEvent( } describe.each([ - // ["lucene", undefined], + ["lucene", undefined], ["sqs", undefined], - // [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], - // [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], + [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], + [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("/rows (%s)", (providerType, dsProvider) => { const isInternal = dsProvider === undefined const isLucene = providerType === "lucene" diff --git a/packages/string-templates/src/helpers/javascript.ts b/packages/string-templates/src/helpers/javascript.ts index 879cd29fa6..d5183ef3ed 100644 --- a/packages/string-templates/src/helpers/javascript.ts +++ b/packages/string-templates/src/helpers/javascript.ts @@ -95,15 +95,38 @@ export function processJS(handlebars: string, context: any) { } catch (error: any) { onErrorLog && onErrorLog(error) + // The error handling below is quite messy, because it has fallen to + // string-templates to handle a variety of types of error specific to usages + // above it in the stack. It would be nice some day to refactor this to + // allow each user of processStringSync to handle errors in the way they see + // fit. + + // This is to catch the error vm.runInNewContext() throws when the timeout + // is exceeded. + if (error.code === "ERR_SCRIPT_EXECUTION_TIMEOUT") { + return "Timed out while executing JS" + } + + // This is to catch the JsRequestTimeoutError we throw when we detect a + // timeout across an entire request in the backend. We use a magic string + // because we can't import from the backend into string-templates. if (error.code === "JS_REQUEST_TIMEOUT_ERROR") { return error.message } + + // This is to catch the JsTimeoutError we throw when we detect a timeout in + // a single JS execution. if (error.code === JsTimeoutError.code) { return JsTimeoutError.message } + + // This is to catch the error that happens if a user-supplied JS script + // throws for reasons introduced by the user. if (error.code === UserScriptError.code) { throw error } + + // If all else fails, generic error message. return "Error while executing JS" } } From 5431becf01e0d53b75e366819b9f70dd083829c4 Mon Sep 17 00:00:00 2001 From: Christos Alexiou Date: Thu, 3 Oct 2024 18:44:41 +0300 Subject: [PATCH 11/33] DEPLOY expects boolean on the other end --- .github/workflows/deploy-featurebranch.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy-featurebranch.yml b/.github/workflows/deploy-featurebranch.yml index a676fe75f0..463074e836 100644 --- a/.github/workflows/deploy-featurebranch.yml +++ b/.github/workflows/deploy-featurebranch.yml @@ -23,7 +23,7 @@ jobs: PAYLOAD_BRANCH: ${{ github.head_ref }} PAYLOAD_PR_NUMBER: ${{ github.event.pull_request.number }} PAYLOAD_LICENSE_TYPE: "free" - PAYLOAD_DEPLOY: "true" + PAYLOAD_DEPLOY: true with: repository: budibase/budibase-deploys event: featurebranch-qa-deploy From 831c81a99c3aa701d817ffa878d67f3073560497 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 4 Oct 2024 09:31:42 +0100 Subject: [PATCH 12/33] Fix automation tests. --- packages/server/src/api/controllers/script.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/server/src/api/controllers/script.ts b/packages/server/src/api/controllers/script.ts index 4317f5fcde..0565b30f74 100644 --- a/packages/server/src/api/controllers/script.ts +++ b/packages/server/src/api/controllers/script.ts @@ -1,11 +1,18 @@ import { Ctx } from "@budibase/types" import { IsolatedVM } from "../../jsRunner/vm" -import { iifeWrapper } from "@budibase/string-templates" +import { iifeWrapper, UserScriptError } from "@budibase/string-templates" export async function execute(ctx: Ctx) { const { script, context } = ctx.request.body const vm = new IsolatedVM() - ctx.body = vm.withContext(context, () => vm.execute(iifeWrapper(script))) + try { + ctx.body = vm.withContext(context, () => vm.execute(iifeWrapper(script))) + } catch (err: any) { + if (err.code === UserScriptError.code) { + throw err.userScriptError + } + throw err + } } export async function save(ctx: Ctx) { From 3cb14d596ad64049a63ed254cbbaf764bc9199d9 Mon Sep 17 00:00:00 2001 From: Peter Clement Date: Fri, 4 Oct 2024 10:18:27 +0100 Subject: [PATCH 13/33] fixes an issue where id and revision weren't passed to row action automations --- packages/server/src/sdk/app/rowActions.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/server/src/sdk/app/rowActions.ts b/packages/server/src/sdk/app/rowActions.ts index 21c256eacb..0fbd7f664e 100644 --- a/packages/server/src/sdk/app/rowActions.ts +++ b/packages/server/src/sdk/app/rowActions.ts @@ -220,6 +220,8 @@ export async function run(tableId: any, rowActionId: any, rowId: string) { automation, { fields: { + id: row._id, + revision: row._rev, row, table, }, From 28e6a039292e65b1df9347671999aa858253747c Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 4 Oct 2024 10:32:33 +0100 Subject: [PATCH 14/33] Include syntax errors in processJS --- packages/string-templates/src/helpers/javascript.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/string-templates/src/helpers/javascript.ts b/packages/string-templates/src/helpers/javascript.ts index d5183ef3ed..ed8d4ad6d7 100644 --- a/packages/string-templates/src/helpers/javascript.ts +++ b/packages/string-templates/src/helpers/javascript.ts @@ -126,7 +126,10 @@ export function processJS(handlebars: string, context: any) { throw error } - // If all else fails, generic error message. + if (error.name === "SyntaxError") { + return error.toString() + } + return "Error while executing JS" } } From 9a47db4a13dec6785a04606cffcaf66a858d08d9 Mon Sep 17 00:00:00 2001 From: Peter Clement Date: Fri, 4 Oct 2024 10:39:17 +0100 Subject: [PATCH 15/33] fix test --- packages/server/src/api/routes/tests/rowAction.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index 4fe248984a..a58053d6cd 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -698,6 +698,8 @@ describe("/rowsActions", () => { inputs: null, outputs: { fields: {}, + id: rowId, + revision: (await config.api.row.get(tableId, rowId))._rev, row: await config.api.row.get(tableId, rowId), table: { ...(await config.api.table.get(tableId)), From 696b2c38db48100506edff971a8aa24545b67fdd Mon Sep 17 00:00:00 2001 From: andz-bb Date: Fri, 4 Oct 2024 10:54:14 +0100 Subject: [PATCH 16/33] persist app sort method selection by saving it against the user --- .../pages/builder/portal/apps/index.svelte | 3 +- packages/builder/src/stores/portal/apps.js | 89 +++++++++++-------- packages/builder/src/stores/portal/index.js | 2 +- packages/types/src/api/web/auth.ts | 1 + packages/types/src/documents/global/user.ts | 1 + .../worker/src/api/routes/validation/users.ts | 1 + 6 files changed, 58 insertions(+), 39 deletions(-) diff --git a/packages/builder/src/pages/builder/portal/apps/index.svelte b/packages/builder/src/pages/builder/portal/apps/index.svelte index 9a073d041f..ddd37dc4a3 100644 --- a/packages/builder/src/pages/builder/portal/apps/index.svelte +++ b/packages/builder/src/pages/builder/portal/apps/index.svelte @@ -26,6 +26,7 @@ licensing, environment, enrichedApps, + sortBy, } from "stores/portal" import { goto } from "@roxi/routify" import AppRow from "components/start/AppRow.svelte" @@ -247,7 +248,7 @@