From 5d319768359953035226f53d9ac5ad5f027dc8f6 Mon Sep 17 00:00:00 2001 From: Peter Clement Date: Mon, 30 Sep 2024 13:07:32 +0100 Subject: [PATCH 01/13] updated automation thread to use ids and test --- .../tests/scenarios/branching.spec.ts | 57 +++++++++++------ .../tests/utilities/AutomationTestBuilder.ts | 61 ++++++++++++------- .../server/src/definitions/automations.ts | 1 + packages/server/src/threads/automation.ts | 18 ++++-- 4 files changed, 91 insertions(+), 46 deletions(-) diff --git a/packages/server/src/automations/tests/scenarios/branching.spec.ts b/packages/server/src/automations/tests/scenarios/branching.spec.ts index ae89fc18b5..76e04afdd3 100644 --- a/packages/server/src/automations/tests/scenarios/branching.spec.ts +++ b/packages/server/src/automations/tests/scenarios/branching.spec.ts @@ -17,44 +17,65 @@ describe("Branching automations", () => { afterAll(setup.afterAll) it("should run a multiple nested branching automation", async () => { + const firstLogId = "11111111-1111-1111-1111-111111111111" + const branch1LogId = "22222222-2222-2222-2222-222222222222" + const branch2LogId = "33333333-3333-3333-3333-333333333333" + const branch2Id = "44444444-4444-4444-4444-444444444444" + const builder = createAutomationBuilder({ name: "Test Trigger with Loop and Create Row", }) const results = await builder .appAction({ fields: {} }) - .serverLog({ text: "Starting automation" }) + .serverLog( + { text: "Starting automation" }, + { stepName: "FirstLog", id: firstLogId } + ) .branch({ topLevelBranch1: { steps: stepBuilder => - stepBuilder.serverLog({ text: "Branch 1" }).branch({ - branch1: { - steps: stepBuilder => - stepBuilder.serverLog({ text: "Branch 1.1" }), - condition: { - equal: { "{{steps.1.success}}": true }, + stepBuilder + .serverLog( + { text: "Branch 1" }, + { id: "66666666-6666-6666-6666-666666666666" } + ) + .branch({ + branch1: { + steps: stepBuilder => + stepBuilder.serverLog( + { text: "Branch 1.1" }, + { id: branch1LogId } + ), + condition: { + equal: { [`{{ steps.${firstLogId}.success }}`]: true }, + }, }, - }, - branch2: { - steps: stepBuilder => - stepBuilder.serverLog({ text: "Branch 1.2" }), - condition: { - equal: { "{{steps.1.success}}": false }, + branch2: { + steps: stepBuilder => + stepBuilder.serverLog( + { text: "Branch 1.2" }, + { id: branch2LogId } + ), + condition: { + equal: { [`{{ steps.${firstLogId}.success }}`]: false }, + }, }, - }, - }), + }), condition: { - equal: { "{{steps.1.success}}": true }, + equal: { [`{{ steps.${firstLogId}.success }}`]: true }, }, }, topLevelBranch2: { - steps: stepBuilder => stepBuilder.serverLog({ text: "Branch 2" }), + steps: stepBuilder => + stepBuilder.serverLog({ text: "Branch 2" }, { id: branch2Id }), condition: { - equal: { "{{steps.1.success}}": false }, + equal: { [`{{ steps.${firstLogId}.success }}`]: false }, }, }, }) .run() + expect(results.steps[3].outputs.status).toContain("branch1 branch taken") expect(results.steps[4].outputs.message).toContain("Branch 1.1") }) diff --git a/packages/server/src/automations/tests/utilities/AutomationTestBuilder.ts b/packages/server/src/automations/tests/utilities/AutomationTestBuilder.ts index 2269f075b2..6af18cd27e 100644 --- a/packages/server/src/automations/tests/utilities/AutomationTestBuilder.ts +++ b/packages/server/src/automations/tests/utilities/AutomationTestBuilder.ts @@ -64,18 +64,18 @@ class BaseStepBuilder { stepId: TStep, stepSchema: Omit, inputs: AutomationStepInputs, - stepName?: string + opts?: { stepName?: string; id?: string } ): this { - const id = uuidv4() + const id = opts?.id || uuidv4() this.steps.push({ ...stepSchema, inputs: inputs as any, id, stepId, - name: stepName || stepSchema.name, + name: opts?.stepName || stepSchema.name, }) - if (stepName) { - this.stepNames[id] = stepName + if (opts?.stepName) { + this.stepNames[id] = opts.stepName } return this } @@ -95,7 +95,6 @@ class BaseStepBuilder { }) branchStepInputs.children![key] = stepBuilder.build() }) - const branchStep: AutomationStep = { ...definition, id: uuidv4(), @@ -106,80 +105,98 @@ class BaseStepBuilder { } // STEPS - createRow(inputs: CreateRowStepInputs, opts?: { stepName?: string }): this { + createRow( + inputs: CreateRowStepInputs, + opts?: { stepName?: string; id?: string } + ): this { return this.step( AutomationActionStepId.CREATE_ROW, BUILTIN_ACTION_DEFINITIONS.CREATE_ROW, inputs, - opts?.stepName + opts ) } - updateRow(inputs: UpdateRowStepInputs, opts?: { stepName?: string }): this { + updateRow( + inputs: UpdateRowStepInputs, + opts?: { stepName?: string; id?: string } + ): this { return this.step( AutomationActionStepId.UPDATE_ROW, BUILTIN_ACTION_DEFINITIONS.UPDATE_ROW, inputs, - opts?.stepName + opts ) } - deleteRow(inputs: DeleteRowStepInputs, opts?: { stepName?: string }): this { + deleteRow( + inputs: DeleteRowStepInputs, + opts?: { stepName?: string; id?: string } + ): this { return this.step( AutomationActionStepId.DELETE_ROW, BUILTIN_ACTION_DEFINITIONS.DELETE_ROW, inputs, - opts?.stepName + opts ) } sendSmtpEmail( inputs: SmtpEmailStepInputs, - opts?: { stepName?: string } + opts?: { stepName?: string; id?: string } ): this { return this.step( AutomationActionStepId.SEND_EMAIL_SMTP, BUILTIN_ACTION_DEFINITIONS.SEND_EMAIL_SMTP, inputs, - opts?.stepName + opts ) } executeQuery( inputs: ExecuteQueryStepInputs, - opts?: { stepName?: string } + opts?: { stepName?: string; id?: string } ): this { return this.step( AutomationActionStepId.EXECUTE_QUERY, BUILTIN_ACTION_DEFINITIONS.EXECUTE_QUERY, inputs, - opts?.stepName + opts ) } - queryRows(inputs: QueryRowsStepInputs, opts?: { stepName?: string }): this { + queryRows( + inputs: QueryRowsStepInputs, + opts?: { stepName?: string; id?: string } + ): this { return this.step( AutomationActionStepId.QUERY_ROWS, BUILTIN_ACTION_DEFINITIONS.QUERY_ROWS, inputs, - opts?.stepName + opts ) } - loop(inputs: LoopStepInputs, opts?: { stepName?: string }): this { + loop( + inputs: LoopStepInputs, + opts?: { stepName?: string; id?: string } + ): this { return this.step( AutomationActionStepId.LOOP, BUILTIN_ACTION_DEFINITIONS.LOOP, inputs, - opts?.stepName + opts ) } - serverLog(input: ServerLogStepInputs, opts?: { stepName?: string }): this { + serverLog( + input: ServerLogStepInputs, + opts?: { stepName?: string; id?: string } + ): this { return this.step( AutomationActionStepId.SERVER_LOG, BUILTIN_ACTION_DEFINITIONS.SERVER_LOG, input, - opts?.stepName + opts ) } diff --git a/packages/server/src/definitions/automations.ts b/packages/server/src/definitions/automations.ts index 6488e604e9..44758b727b 100644 --- a/packages/server/src/definitions/automations.ts +++ b/packages/server/src/definitions/automations.ts @@ -15,6 +15,7 @@ export interface TriggerOutput { export interface AutomationContext extends AutomationResults { steps: any[] + stepsById?: Record stepsByName?: Record env?: Record trigger: any diff --git a/packages/server/src/threads/automation.ts b/packages/server/src/threads/automation.ts index e2a5a1c192..7788744ea2 100644 --- a/packages/server/src/threads/automation.ts +++ b/packages/server/src/threads/automation.ts @@ -91,6 +91,7 @@ class Orchestrator { // step zero is never used as the template string is zero indexed for customer facing this.context = { steps: [{}], + stepsById: {}, stepsByName: {}, trigger: triggerOutput, } @@ -457,6 +458,7 @@ class Orchestrator { inputs: steps[stepToLoopIndex].inputs, }) + this.context.stepsById![steps[stepToLoopIndex].id] = tempOutput const stepName = steps[stepToLoopIndex].name || steps[stepToLoopIndex].id this.context.stepsByName![stepName] = tempOutput this.context.steps[this.context.steps.length] = tempOutput @@ -517,7 +519,10 @@ class Orchestrator { Object.entries(filter).forEach(([_, value]) => { Object.entries(value).forEach(([field, _]) => { const updatedField = field.replace("{{", "{{ literal ") - const fromContext = processStringSync(updatedField, this.context) + const fromContext = processStringSync( + updatedField, + this.processContext(this.context) + ) toFilter[field] = fromContext }) }) @@ -563,9 +568,9 @@ class Orchestrator { } const stepFn = await this.getStepFunctionality(step.stepId) - let inputs = await this.addContextAndProcess( + let inputs = await processObject( originalStepInput, - this.context + this.processContext(this.context) ) inputs = automationUtils.cleanInputValues(inputs, step.schema.inputs) @@ -594,16 +599,16 @@ class Orchestrator { return null } - private async addContextAndProcess(inputs: any, context: any) { + private processContext(context: AutomationContext) { const processContext = { ...context, steps: { ...context.steps, + ...context.stepsById, ...context.stepsByName, }, } - - return processObject(inputs, processContext) + return processContext } private handleStepOutput( @@ -623,6 +628,7 @@ class Orchestrator { } else { this.updateExecutionOutput(step.id, step.stepId, step.inputs, outputs) this.context.steps[this.context.steps.length] = outputs + this.context.stepsById![step.id] = outputs const stepName = step.name || step.id this.context.stepsByName![stepName] = outputs } From 987a24fabc85d76bb3332e364d877a4349083692 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 1 Oct 2024 11:48:14 +0100 Subject: [PATCH 02/13] wip --- packages/backend-core/src/sql/sql.ts | 87 ++++++++++++------- .../src/api/routes/tests/viewV2.spec.ts | 39 +++++++-- 2 files changed, 88 insertions(+), 38 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index b7bf5bc102..54ca5a0135 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -71,18 +71,6 @@ function prioritisedArraySort(toSort: string[], priorities: string[]) { }) } -function getTableName(table?: Table): string | undefined { - // SQS uses the table ID rather than the table name - if ( - table?.sourceType === TableSourceType.INTERNAL || - table?.sourceId === INTERNAL_TABLE_SOURCE_ID - ) { - return table?._id - } else { - return table?.name - } -} - function convertBooleans(query: SqlQuery | SqlQuery[]): SqlQuery | SqlQuery[] { if (Array.isArray(query)) { return query.map((q: SqlQuery) => convertBooleans(q) as SqlQuery) @@ -180,15 +168,13 @@ class InternalBuilder { } private generateSelectStatement(): (string | Knex.Raw)[] | "*" { - const { meta, endpoint, resource, tableAliases } = this.query + const { meta, endpoint, resource } = this.query if (!resource || !resource.fields || resource.fields.length === 0) { return "*" } - const alias = tableAliases?.[endpoint.entityId] - ? tableAliases?.[endpoint.entityId] - : endpoint.entityId + const alias = this.getTableName(endpoint.entityId) const schema = meta.table.schema if (!this.isFullSelectStatementRequired()) { return [this.knex.raw(`${this.quote(alias)}.*`)] @@ -813,17 +799,39 @@ class InternalBuilder { return query } + getTableName(t?: Table | string): string { + let table: Table + if (typeof t === "string") { + if (!this.query.meta.tables?.[t]) { + throw new Error(`Table ${t} not found`) + } + table = this.query.meta.tables[t] + } else if (t) { + table = t + } else { + table = this.table + } + + let name = table.name + if ( + (table.sourceType === TableSourceType.INTERNAL || + table.sourceId === INTERNAL_TABLE_SOURCE_ID) && + table._id + ) { + // SQS uses the table ID rather than the table name + name = table._id + } + const aliases = this.query.tableAliases || {} + return aliases[name] ? aliases[name] : name + } + addDistinctCount(query: Knex.QueryBuilder): Knex.QueryBuilder { - const primary = this.table.primary - const aliases = this.query.tableAliases - const aliased = - this.table.name && aliases?.[this.table.name] - ? aliases[this.table.name] - : this.table.name - if (!primary) { + if (!this.table.primary) { throw new Error("SQL counting requires primary key to be supplied") } - return query.countDistinct(`${aliased}.${primary[0]} as total`) + return query.countDistinct( + `${this.getTableName(this.table)}.${this.table.primary[0]} as total` + ) } addAggregations( @@ -831,8 +839,9 @@ class InternalBuilder { aggregations: Aggregation[] ): Knex.QueryBuilder { const fields = this.query.resource?.fields || [] + const tableName = this.getTableName() if (fields.length > 0) { - query = query.groupBy(fields.map(field => `${this.table.name}.${field}`)) + query = query.groupBy(fields.map(field => `${tableName}.${field}`)) } for (const aggregation of aggregations) { const op = aggregation.calculationType @@ -861,10 +870,7 @@ class InternalBuilder { addSorting(query: Knex.QueryBuilder): Knex.QueryBuilder { let { sort, resource } = this.query const primaryKey = this.table.primary - const tableName = getTableName(this.table) - const aliases = this.query.tableAliases - const aliased = - tableName && aliases?.[tableName] ? aliases[tableName] : this.table?.name + const aliased = this.getTableName() if (!Array.isArray(primaryKey)) { throw new Error("Sorting requires primary key to be specified for table") } @@ -1508,18 +1514,35 @@ class SqlQueryBuilder extends SqlTableQueryBuilder { return results.length ? results : [{ [operation.toLowerCase()]: true }] } + private getTableName( + table: Table, + aliases?: Record + ): string | undefined { + let name = table.name + if ( + table.sourceType === TableSourceType.INTERNAL || + table.sourceId === INTERNAL_TABLE_SOURCE_ID + ) { + if (!table._id) { + return + } + // SQS uses the table ID rather than the table name + name = table._id + } + return aliases?.[name] ? aliases[name] : name + } + convertJsonStringColumns>( table: Table, results: T[], aliases?: Record ): T[] { - const tableName = getTableName(table) + const tableName = this.getTableName(table, aliases) for (const [name, field] of Object.entries(table.schema)) { if (!this._isJsonColumn(field)) { continue } - const aliasedTableName = (tableName && aliases?.[tableName]) || tableName - const fullName = `${aliasedTableName}.${name}` + const fullName = `${tableName}.${name}` for (let row of results) { if (typeof row[fullName as keyof T] === "string") { row[fullName as keyof T] = JSON.parse(row[fullName]) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 09273abdce..0f4e6c961c 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -39,13 +39,13 @@ import { } from "@budibase/backend-core" describe.each([ - ["lucene", undefined], - ["sqs", 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.MYSQL, getDatasource(DatabaseName.MYSQL)], + // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + // [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("/v2/views (%s)", (name, dsProvider) => { const config = setup.getConfig() const isSqs = name === "sqs" @@ -2458,6 +2458,33 @@ describe.each([ expect("_id" in row).toBe(false) } }) + + it.only("should be able to group by a basic field", async () => { + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + schema: { + quantity: { + visible: true, + field: "quantity", + }, + "Total Price": { + visible: true, + calculationType: CalculationType.SUM, + field: "price", + }, + }, + }) + + const response = await config.api.viewV2.search(view.id, { + query: {}, + }) + + for (const row of response.rows) { + expect(row["quantity"]).toBeGreaterThan(0) + expect(row["Total Price"]).toBeGreaterThan(0) + } + }) }) }) From f00593ff26ca42e2e1521048aab079b29a3742d3 Mon Sep 17 00:00:00 2001 From: Peter Clement Date: Tue, 1 Oct 2024 12:25:41 +0100 Subject: [PATCH 03/13] pr comments --- .../tests/scenarios/branching.spec.ts | 10 +++++----- .../tests/utilities/AutomationTestBuilder.ts | 20 +++++++++---------- .../server/src/definitions/automations.ts | 4 ++-- packages/server/src/threads/automation.ts | 6 +++--- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/packages/server/src/automations/tests/scenarios/branching.spec.ts b/packages/server/src/automations/tests/scenarios/branching.spec.ts index 76e04afdd3..032b729e44 100644 --- a/packages/server/src/automations/tests/scenarios/branching.spec.ts +++ b/packages/server/src/automations/tests/scenarios/branching.spec.ts @@ -30,7 +30,7 @@ describe("Branching automations", () => { .appAction({ fields: {} }) .serverLog( { text: "Starting automation" }, - { stepName: "FirstLog", id: firstLogId } + { stepName: "FirstLog", stepId: firstLogId } ) .branch({ topLevelBranch1: { @@ -38,14 +38,14 @@ describe("Branching automations", () => { stepBuilder .serverLog( { text: "Branch 1" }, - { id: "66666666-6666-6666-6666-666666666666" } + { stepId: "66666666-6666-6666-6666-666666666666" } ) .branch({ branch1: { steps: stepBuilder => stepBuilder.serverLog( { text: "Branch 1.1" }, - { id: branch1LogId } + { stepId: branch1LogId } ), condition: { equal: { [`{{ steps.${firstLogId}.success }}`]: true }, @@ -55,7 +55,7 @@ describe("Branching automations", () => { steps: stepBuilder => stepBuilder.serverLog( { text: "Branch 1.2" }, - { id: branch2LogId } + { stepId: branch2LogId } ), condition: { equal: { [`{{ steps.${firstLogId}.success }}`]: false }, @@ -68,7 +68,7 @@ describe("Branching automations", () => { }, topLevelBranch2: { steps: stepBuilder => - stepBuilder.serverLog({ text: "Branch 2" }, { id: branch2Id }), + stepBuilder.serverLog({ text: "Branch 2" }, { stepId: branch2Id }), condition: { equal: { [`{{ steps.${firstLogId}.success }}`]: false }, }, diff --git a/packages/server/src/automations/tests/utilities/AutomationTestBuilder.ts b/packages/server/src/automations/tests/utilities/AutomationTestBuilder.ts index 6af18cd27e..6aaf22cd6a 100644 --- a/packages/server/src/automations/tests/utilities/AutomationTestBuilder.ts +++ b/packages/server/src/automations/tests/utilities/AutomationTestBuilder.ts @@ -64,9 +64,9 @@ class BaseStepBuilder { stepId: TStep, stepSchema: Omit, inputs: AutomationStepInputs, - opts?: { stepName?: string; id?: string } + opts?: { stepName?: string; stepId?: string } ): this { - const id = opts?.id || uuidv4() + const id = opts?.stepId || uuidv4() this.steps.push({ ...stepSchema, inputs: inputs as any, @@ -107,7 +107,7 @@ class BaseStepBuilder { // STEPS createRow( inputs: CreateRowStepInputs, - opts?: { stepName?: string; id?: string } + opts?: { stepName?: string; stepId?: string } ): this { return this.step( AutomationActionStepId.CREATE_ROW, @@ -119,7 +119,7 @@ class BaseStepBuilder { updateRow( inputs: UpdateRowStepInputs, - opts?: { stepName?: string; id?: string } + opts?: { stepName?: string; stepId?: string } ): this { return this.step( AutomationActionStepId.UPDATE_ROW, @@ -131,7 +131,7 @@ class BaseStepBuilder { deleteRow( inputs: DeleteRowStepInputs, - opts?: { stepName?: string; id?: string } + opts?: { stepName?: string; stepId?: string } ): this { return this.step( AutomationActionStepId.DELETE_ROW, @@ -143,7 +143,7 @@ class BaseStepBuilder { sendSmtpEmail( inputs: SmtpEmailStepInputs, - opts?: { stepName?: string; id?: string } + opts?: { stepName?: string; stepId?: string } ): this { return this.step( AutomationActionStepId.SEND_EMAIL_SMTP, @@ -155,7 +155,7 @@ class BaseStepBuilder { executeQuery( inputs: ExecuteQueryStepInputs, - opts?: { stepName?: string; id?: string } + opts?: { stepName?: string; stepId?: string } ): this { return this.step( AutomationActionStepId.EXECUTE_QUERY, @@ -167,7 +167,7 @@ class BaseStepBuilder { queryRows( inputs: QueryRowsStepInputs, - opts?: { stepName?: string; id?: string } + opts?: { stepName?: string; stepId?: string } ): this { return this.step( AutomationActionStepId.QUERY_ROWS, @@ -178,7 +178,7 @@ class BaseStepBuilder { } loop( inputs: LoopStepInputs, - opts?: { stepName?: string; id?: string } + opts?: { stepName?: string; stepId?: string } ): this { return this.step( AutomationActionStepId.LOOP, @@ -190,7 +190,7 @@ class BaseStepBuilder { serverLog( input: ServerLogStepInputs, - opts?: { stepName?: string; id?: string } + opts?: { stepName?: string; stepId?: string } ): this { return this.step( AutomationActionStepId.SERVER_LOG, diff --git a/packages/server/src/definitions/automations.ts b/packages/server/src/definitions/automations.ts index 44758b727b..9433075da7 100644 --- a/packages/server/src/definitions/automations.ts +++ b/packages/server/src/definitions/automations.ts @@ -15,8 +15,8 @@ export interface TriggerOutput { export interface AutomationContext extends AutomationResults { steps: any[] - stepsById?: Record - stepsByName?: Record + stepsById: Record + stepsByName: Record env?: Record trigger: any } diff --git a/packages/server/src/threads/automation.ts b/packages/server/src/threads/automation.ts index 7788744ea2..3b47634663 100644 --- a/packages/server/src/threads/automation.ts +++ b/packages/server/src/threads/automation.ts @@ -74,7 +74,7 @@ class Orchestrator { private job: Job private loopStepOutputs: LoopStep[] private stopped: boolean - private executionOutput: AutomationContext + private executionOutput: Omit constructor(job: AutomationJob) { let automation = job.data.automation @@ -458,9 +458,9 @@ class Orchestrator { inputs: steps[stepToLoopIndex].inputs, }) - this.context.stepsById![steps[stepToLoopIndex].id] = tempOutput + this.context.stepsById[steps[stepToLoopIndex].id] = tempOutput const stepName = steps[stepToLoopIndex].name || steps[stepToLoopIndex].id - this.context.stepsByName![stepName] = tempOutput + this.context.stepsByName[stepName] = tempOutput this.context.steps[this.context.steps.length] = tempOutput this.context.steps = this.context.steps.filter( item => !item.hasOwnProperty.call(item, "currentItem") From ae4f7ae4b45ab4e30778eddff0938017924a22d0 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 1 Oct 2024 15:04:01 +0100 Subject: [PATCH 04/13] Implement group by and add a test for it. --- packages/backend-core/src/sql/sql.ts | 19 +++++++++------ .../src/api/routes/tests/viewV2.spec.ts | 23 +++++++++++-------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 54ca5a0135..14e32623e3 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -799,6 +799,14 @@ class InternalBuilder { return query } + isSqs(t?: Table): boolean { + const table = t || this.table + return ( + table.sourceType === TableSourceType.INTERNAL || + table.sourceId === INTERNAL_TABLE_SOURCE_ID + ) + } + getTableName(t?: Table | string): string { let table: Table if (typeof t === "string") { @@ -813,11 +821,7 @@ class InternalBuilder { } let name = table.name - if ( - (table.sourceType === TableSourceType.INTERNAL || - table.sourceId === INTERNAL_TABLE_SOURCE_ID) && - table._id - ) { + if (this.isSqs(table) && table._id) { // SQS uses the table ID rather than the table name name = table._id } @@ -830,7 +834,7 @@ class InternalBuilder { throw new Error("SQL counting requires primary key to be supplied") } return query.countDistinct( - `${this.getTableName(this.table)}.${this.table.primary[0]} as total` + `${this.getTableName()}.${this.table.primary[0]} as total` ) } @@ -842,10 +846,11 @@ class InternalBuilder { const tableName = this.getTableName() if (fields.length > 0) { query = query.groupBy(fields.map(field => `${tableName}.${field}`)) + query = query.select(fields.map(field => `${tableName}.${field}`)) } for (const aggregation of aggregations) { const op = aggregation.calculationType - const field = `${this.table.name}.${aggregation.field} as ${aggregation.name}` + const field = `${tableName}.${aggregation.field} as ${aggregation.name}` switch (op) { case CalculationType.COUNT: query = query.count(field) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 0f4e6c961c..b03c445b78 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -39,13 +39,13 @@ import { } from "@budibase/backend-core" describe.each([ - // ["lucene", undefined], - // ["sqs", 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.MYSQL, getDatasource(DatabaseName.MYSQL)], + [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("/v2/views (%s)", (name, dsProvider) => { const config = setup.getConfig() const isSqs = name === "sqs" @@ -2459,7 +2459,7 @@ describe.each([ } }) - it.only("should be able to group by a basic field", async () => { + it("should be able to group by a basic field", async () => { const view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), @@ -2480,9 +2480,14 @@ describe.each([ query: {}, }) + const priceByQuantity: Record = {} + for (const row of rows) { + priceByQuantity[row.quantity] ??= 0 + priceByQuantity[row.quantity] += row.price + } + for (const row of response.rows) { - expect(row["quantity"]).toBeGreaterThan(0) - expect(row["Total Price"]).toBeGreaterThan(0) + expect(row["Total Price"]).toEqual(priceByQuantity[row.quantity]) } }) }) From addd54a8e8baab79811e6f71aab728f32239577b Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 1 Oct 2024 15:39:33 +0100 Subject: [PATCH 05/13] Fix generic-sql.spec.ts --- packages/backend-core/src/sql/sql.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 14e32623e3..0f72eea96e 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -811,7 +811,10 @@ class InternalBuilder { let table: Table if (typeof t === "string") { if (!this.query.meta.tables?.[t]) { - throw new Error(`Table ${t} not found`) + // This can legitimately happen in custom queries, where the user is + // querying against a table that may not have been imported into + // Budibase. + return t } table = this.query.meta.tables[t] } else if (t) { @@ -1547,12 +1550,12 @@ class SqlQueryBuilder extends SqlTableQueryBuilder { if (!this._isJsonColumn(field)) { continue } - const fullName = `${tableName}.${name}` + const fullName = `${tableName}.${name}` as keyof T for (let row of results) { - if (typeof row[fullName as keyof T] === "string") { - row[fullName as keyof T] = JSON.parse(row[fullName]) + if (typeof row[fullName] === "string") { + row[fullName] = JSON.parse(row[fullName]) } - if (typeof row[name as keyof T] === "string") { + if (typeof row[name] === "string") { row[name as keyof T] = JSON.parse(row[name]) } } From 7cee1509aa3ee47677e3116121beab3b57a6deef Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 1 Oct 2024 16:17:11 +0100 Subject: [PATCH 06/13] Fix sqlAlias.spec.ts --- packages/backend-core/src/sql/sql.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 0f72eea96e..105116e828 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -810,13 +810,18 @@ class InternalBuilder { getTableName(t?: Table | string): string { let table: Table if (typeof t === "string") { - if (!this.query.meta.tables?.[t]) { + if (this.query.table?.name === t) { + table = this.query.table + } else if (this.query.meta.table?.name === t) { + table = this.query.meta.table + } else if (!this.query.meta.tables?.[t]) { // This can legitimately happen in custom queries, where the user is // querying against a table that may not have been imported into // Budibase. return t + } else { + table = this.query.meta.tables[t] } - table = this.query.meta.tables[t] } else if (t) { table = t } else { From 4165c6cab42affee7f40f5493e29e06a90c67ec4 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 1 Oct 2024 16:06:40 +0100 Subject: [PATCH 07/13] Test all aggregation types. --- .../src/api/routes/tests/viewV2.spec.ts | 55 +++++++++++++++++++ packages/server/src/integrations/postgres.ts | 3 +- .../src/utilities/rowProcessor/index.ts | 19 +++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index b03c445b78..1d6c1d50cd 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -2490,6 +2490,61 @@ describe.each([ expect(row["Total Price"]).toEqual(priceByQuantity[row.quantity]) } }) + + it.each([ + CalculationType.COUNT, + CalculationType.SUM, + CalculationType.AVG, + CalculationType.MIN, + CalculationType.MAX, + ])("should be able to calculate $type", async type => { + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + schema: { + aggregate: { + visible: true, + calculationType: type, + field: "price", + }, + }, + }) + + const response = await config.api.viewV2.search(view.id, { + query: {}, + }) + + function calculate( + type: CalculationType, + numbers: number[] + ): number { + switch (type) { + case CalculationType.COUNT: + return numbers.length + case CalculationType.SUM: + return numbers.reduce((a, b) => a + b, 0) + case CalculationType.AVG: + return numbers.reduce((a, b) => a + b, 0) / numbers.length + case CalculationType.MIN: + return Math.min(...numbers) + case CalculationType.MAX: + return Math.max(...numbers) + } + } + + const prices = rows.map(row => row.price) + const expected = calculate(type, prices) + const actual = response.rows[0].aggregate + + if (type === CalculationType.AVG) { + // The average calculation can introduce floating point rounding + // errors, so we need to compare to within a small margin of + // error. + expect(actual).toBeCloseTo(expected) + } else { + expect(actual).toEqual(expected) + } + }) }) }) diff --git a/packages/server/src/integrations/postgres.ts b/packages/server/src/integrations/postgres.ts index 3652864991..ce8b21eede 100644 --- a/packages/server/src/integrations/postgres.ts +++ b/packages/server/src/integrations/postgres.ts @@ -272,7 +272,8 @@ class PostgresIntegration extends Sql implements DatasourcePlus { try { const bindings = query.bindings || [] this.log(query.sql, bindings) - return await client.query(query.sql, bindings) + const result = await client.query(query.sql, bindings) + return result } catch (err: any) { await this.closeConnection() let readableMessage = getReadableErrorMessage( diff --git a/packages/server/src/utilities/rowProcessor/index.ts b/packages/server/src/utilities/rowProcessor/index.ts index bddd590f25..7332f8b244 100644 --- a/packages/server/src/utilities/rowProcessor/index.ts +++ b/packages/server/src/utilities/rowProcessor/index.ts @@ -426,6 +426,25 @@ export async function coreOutputProcessing( } } } + + if (sdk.views.isView(source)) { + const calculationFields = Object.keys( + helpers.views.calculationFields(source) + ) + + // We ensure all calculation fields are returned as numbers. During the + // testing of this feature it was discovered that the COUNT operation + // returns a string for MySQL, MariaDB, and Postgres. But given that all + // calculation fields should be numbers, we blanket make sure of that + // here. + for (const key of calculationFields) { + for (const row of rows) { + if (typeof row[key] === "string") { + row[key] = parseFloat(row[key]) + } + } + } + } } if (!isUserMetadataTable(table._id!)) { From 13248c409f358cb3c0a791bce59b8e05c01247da Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 1 Oct 2024 16:44:16 +0100 Subject: [PATCH 08/13] Respond to PR comment. --- packages/server/src/integrations/postgres.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/server/src/integrations/postgres.ts b/packages/server/src/integrations/postgres.ts index ce8b21eede..3652864991 100644 --- a/packages/server/src/integrations/postgres.ts +++ b/packages/server/src/integrations/postgres.ts @@ -272,8 +272,7 @@ class PostgresIntegration extends Sql implements DatasourcePlus { try { const bindings = query.bindings || [] this.log(query.sql, bindings) - const result = await client.query(query.sql, bindings) - return result + return await client.query(query.sql, bindings) } catch (err: any) { await this.closeConnection() let readableMessage = getReadableErrorMessage( From 08f1c4dadc668db7fe645f01336832341546828e Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 2 Oct 2024 09:35:15 +0100 Subject: [PATCH 09/13] Update packages/backend-core/src/sql/sql.ts Co-authored-by: Adria Navarro --- packages/backend-core/src/sql/sql.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 105116e828..e55524a67e 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -1542,7 +1542,7 @@ class SqlQueryBuilder extends SqlTableQueryBuilder { // SQS uses the table ID rather than the table name name = table._id } - return aliases?.[name] ? aliases[name] : name + return aliases?.[name] || name } convertJsonStringColumns>( From ddd229062c20e37f2860c8a07583688f67c0ed1c Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 2 Oct 2024 09:39:54 +0100 Subject: [PATCH 10/13] Rename total field when doing row counts. --- packages/backend-core/src/sql/sql.ts | 2 +- packages/server/src/sdk/app/rows/utils.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index e55524a67e..701797329f 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -842,7 +842,7 @@ class InternalBuilder { throw new Error("SQL counting requires primary key to be supplied") } return query.countDistinct( - `${this.getTableName()}.${this.table.primary[0]} as total` + `${this.getTableName()}.${this.table.primary[0]} as __bb_total` ) } diff --git a/packages/server/src/sdk/app/rows/utils.ts b/packages/server/src/sdk/app/rows/utils.ts index d5c0560d9b..3d6bf39d3f 100644 --- a/packages/server/src/sdk/app/rows/utils.ts +++ b/packages/server/src/sdk/app/rows/utils.ts @@ -57,8 +57,8 @@ export function getSQLClient(datasource: Datasource): SqlClient { export function processRowCountResponse( response: DatasourcePlusQueryResponse ): number { - if (response && response.length === 1 && "total" in response[0]) { - const total = response[0].total + if (response && response.length === 1 && "__bb_total" in response[0]) { + const total = response[0].__bb_total return typeof total === "number" ? total : parseInt(total) } else { throw new Error("Unable to count rows in query - no count response") From 7b9af81fd510f2aa87c757173abd028c068cebba Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 2 Oct 2024 09:44:20 +0100 Subject: [PATCH 11/13] Clean up params and isSqs --- packages/backend-core/src/sql/sql.ts | 36 +++++++++++++++------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 701797329f..627be039ca 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -87,6 +87,13 @@ function convertBooleans(query: SqlQuery | SqlQuery[]): SqlQuery | SqlQuery[] { return query } +function isSqs(table: Table): boolean { + return ( + table.sourceType === TableSourceType.INTERNAL || + table.sourceId === INTERNAL_TABLE_SOURCE_ID + ) +} + class InternalBuilder { private readonly client: SqlClient private readonly query: QueryJson @@ -799,37 +806,34 @@ class InternalBuilder { return query } - isSqs(t?: Table): boolean { - const table = t || this.table - return ( - table.sourceType === TableSourceType.INTERNAL || - table.sourceId === INTERNAL_TABLE_SOURCE_ID - ) + isSqs(): boolean { + return isSqs(this.table) } - getTableName(t?: Table | string): string { + getTableName(tableOrName?: Table | string): string { let table: Table - if (typeof t === "string") { - if (this.query.table?.name === t) { + if (typeof tableOrName === "string") { + const name = tableOrName + if (this.query.table?.name === name) { table = this.query.table - } else if (this.query.meta.table?.name === t) { + } else if (this.query.meta.table?.name === name) { table = this.query.meta.table - } else if (!this.query.meta.tables?.[t]) { + } else if (!this.query.meta.tables?.[name]) { // This can legitimately happen in custom queries, where the user is // querying against a table that may not have been imported into // Budibase. - return t + return name } else { - table = this.query.meta.tables[t] + table = this.query.meta.tables[name] } - } else if (t) { - table = t + } else if (tableOrName) { + table = tableOrName } else { table = this.table } let name = table.name - if (this.isSqs(table) && table._id) { + if (isSqs(table) && table._id) { // SQS uses the table ID rather than the table name name = table._id } From a4c67f1812a780e60cfc9451dd505aff1acc0cff Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Wed, 2 Oct 2024 11:11:20 +0100 Subject: [PATCH 12/13] Show notification when running row actions in the data section --- .../[application]/data/table/[tableId]/[viewId]/index.svelte | 5 ++--- .../app/[application]/data/table/[tableId]/index.svelte | 3 ++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/builder/src/pages/builder/app/[application]/data/table/[tableId]/[viewId]/index.svelte b/packages/builder/src/pages/builder/app/[application]/data/table/[tableId]/[viewId]/index.svelte index b5b65b3f1d..eb5e7eb95d 100644 --- a/packages/builder/src/pages/builder/app/[application]/data/table/[tableId]/[viewId]/index.svelte +++ b/packages/builder/src/pages/builder/app/[application]/data/table/[tableId]/[viewId]/index.svelte @@ -3,6 +3,7 @@ import { admin, themeStore } from "stores/portal" import { Grid } from "@budibase/frontend-core" import { API } from "api" + import { notifications } from "@budibase/bbui" import GridCreateEditRowModal from "components/backend/DataTable/modals/grid/GridCreateEditRowModal.svelte" import GridFilterButton from "components/backend/DataTable/buttons/grid/GridFilterButton.svelte" import GridManageAccessButton from "components/backend/DataTable/buttons/grid/GridManageAccessButton.svelte" @@ -26,14 +27,12 @@ $: currentTheme = $themeStore?.theme $: darkMode = !currentTheme.includes("light") - $: currentTheme = $themeStore?.theme - $: darkMode = !currentTheme.includes("light") - const makeRowActionButtons = actions => { return (actions || []).map(action => ({ text: action.name, onClick: async row => { await rowActions.trigger(id, action.id, row._id) + notifications.success("Row action triggered successfully") }, })) } diff --git a/packages/builder/src/pages/builder/app/[application]/data/table/[tableId]/index.svelte b/packages/builder/src/pages/builder/app/[application]/data/table/[tableId]/index.svelte index b9b58cbfce..24851c723d 100644 --- a/packages/builder/src/pages/builder/app/[application]/data/table/[tableId]/index.svelte +++ b/packages/builder/src/pages/builder/app/[application]/data/table/[tableId]/index.svelte @@ -1,5 +1,5 @@