From b5672d676fd2aebbe7b2539febfa8ebb424b531d Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 29 Jan 2024 17:38:52 +0000 Subject: [PATCH 1/7] Add a check to stringSplit that gives a nicer error message is a non-string is passed. --- packages/server/src/automations/automationUtils.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/server/src/automations/automationUtils.ts b/packages/server/src/automations/automationUtils.ts index 3e25665a60..8f63b1d0dd 100644 --- a/packages/server/src/automations/automationUtils.ts +++ b/packages/server/src/automations/automationUtils.ts @@ -131,6 +131,9 @@ export function stringSplit(value: string | string[]) { if (value == null || Array.isArray(value)) { return value || [] } + if (typeof value !== "string") { + throw new Error(`Unable to split value of type ${typeof value}: ${value}`) + } if (value.split("\n").length > 1) { value = value.split("\n") } else { From 2bfa4c6f91a4cb5428b3be8a742b5ef2986abc16 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 29 Jan 2024 17:43:08 +0000 Subject: [PATCH 2/7] Mild refactor of stringSplit to make it easier to understand. --- .../server/src/automations/automationUtils.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/server/src/automations/automationUtils.ts b/packages/server/src/automations/automationUtils.ts index 8f63b1d0dd..662cecbb25 100644 --- a/packages/server/src/automations/automationUtils.ts +++ b/packages/server/src/automations/automationUtils.ts @@ -128,18 +128,20 @@ export function substituteLoopStep(hbsString: string, substitute: string) { } export function stringSplit(value: string | string[]) { - if (value == null || Array.isArray(value)) { - return value || [] + if (value == null) { + return [] + } + if (Array.isArray(value)) { + return value } if (typeof value !== "string") { throw new Error(`Unable to split value of type ${typeof value}: ${value}`) } - if (value.split("\n").length > 1) { - value = value.split("\n") - } else { - value = value.split(",") + const splitOnNewLine = value.split("\n") + if (splitOnNewLine.length > 1) { + return splitOnNewLine } - return value + return value.split(",") } export function typecastForLooping(loopStep: LoopStep, input: LoopInput) { From 669b0743ac5a6db0418f7d29333f0db27a558aaf Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 30 Jan 2024 10:00:44 +0000 Subject: [PATCH 3/7] Typing improvements around automation loop tests. --- packages/server/src/automations/automationUtils.ts | 5 +++-- packages/server/src/automations/tests/loop.spec.ts | 12 +++++++----- packages/server/src/definitions/automations.ts | 6 ++---- packages/server/src/tests/utilities/structures.ts | 8 ++++++-- packages/server/src/threads/automation.ts | 9 +++------ 5 files changed, 21 insertions(+), 19 deletions(-) diff --git a/packages/server/src/automations/automationUtils.ts b/packages/server/src/automations/automationUtils.ts index 3e25665a60..256f46f19f 100644 --- a/packages/server/src/automations/automationUtils.ts +++ b/packages/server/src/automations/automationUtils.ts @@ -5,7 +5,7 @@ import { } from "@budibase/string-templates" import sdk from "../sdk" import { Row } from "@budibase/types" -import { LoopStep, LoopStepType, LoopInput } from "../definitions/automations" +import { LoopStep, LoopStepType } from "../definitions/automations" /** * When values are input to the system generally they will be of type string as this is required for template strings. @@ -139,7 +139,8 @@ export function stringSplit(value: string | string[]) { return value } -export function typecastForLooping(loopStep: LoopStep, input: LoopInput) { +export function typecastForLooping(loopStep: LoopStep) { + const input = loopStep.inputs if (!input || !input.binding) { return null } diff --git a/packages/server/src/automations/tests/loop.spec.ts b/packages/server/src/automations/tests/loop.spec.ts index b64f7b16f8..70b771c445 100644 --- a/packages/server/src/automations/tests/loop.spec.ts +++ b/packages/server/src/automations/tests/loop.spec.ts @@ -3,11 +3,13 @@ import * as triggers from "../triggers" import { loopAutomation } from "../../tests/utilities/structures" import { context } from "@budibase/backend-core" import * as setup from "./utilities" +import { Row, Table } from "@budibase/types" +import { LoopInput, LoopStepType } from "../../definitions/automations" describe("Attempt to run a basic loop automation", () => { let config = setup.getConfig(), - table: any, - row: any + table: Table, + row: Row beforeEach(async () => { await automation.init() @@ -18,12 +20,12 @@ describe("Attempt to run a basic loop automation", () => { afterAll(setup.afterAll) - async function runLoop(loopOpts?: any) { + async function runLoop(loopOpts?: LoopInput) { const appId = config.getAppId() return await context.doInAppContext(appId, async () => { const params = { fields: { appId } } return await triggers.externalTrigger( - loopAutomation(table._id, loopOpts), + loopAutomation(table._id!, loopOpts), params, { getResponses: true } ) @@ -37,7 +39,7 @@ describe("Attempt to run a basic loop automation", () => { it("test a loop with a string", async () => { const resp = await runLoop({ - type: "String", + option: LoopStepType.STRING, binding: "a,b,c", }) expect(resp.steps[2].outputs.iterations).toBe(3) diff --git a/packages/server/src/definitions/automations.ts b/packages/server/src/definitions/automations.ts index 7e86608bf3..48a66408ca 100644 --- a/packages/server/src/definitions/automations.ts +++ b/packages/server/src/definitions/automations.ts @@ -6,13 +6,11 @@ export enum LoopStepType { } export interface LoopStep extends AutomationStep { - inputs: { - option: LoopStepType - [key: string]: any - } + inputs: LoopInput } export interface LoopInput { + option: LoopStepType binding: string[] | string } diff --git a/packages/server/src/tests/utilities/structures.ts b/packages/server/src/tests/utilities/structures.ts index b1c2c494a5..fe82311810 100644 --- a/packages/server/src/tests/utilities/structures.ts +++ b/packages/server/src/tests/utilities/structures.ts @@ -23,6 +23,7 @@ import { TableSourceType, Query, } from "@budibase/types" +import { LoopInput, LoopStepType } from "../../definitions/automations" const { BUILTIN_ROLE_IDS } = roles @@ -204,10 +205,13 @@ export function serverLogAutomation(appId?: string): Automation { } } -export function loopAutomation(tableId: string, loopOpts?: any): Automation { +export function loopAutomation( + tableId: string, + loopOpts?: LoopInput +): Automation { if (!loopOpts) { loopOpts = { - option: "Array", + option: LoopStepType.ARRAY, binding: "{{ steps.1.rows }}", } } diff --git a/packages/server/src/threads/automation.ts b/packages/server/src/threads/automation.ts index 4447899f96..1c3f921147 100644 --- a/packages/server/src/threads/automation.ts +++ b/packages/server/src/threads/automation.ts @@ -23,7 +23,6 @@ import { } from "@budibase/types" import { AutomationContext, - LoopInput, LoopStep, TriggerOutput, } from "../definitions/automations" @@ -47,9 +46,8 @@ function getLoopIterations(loopStep: LoopStep) { if (!binding) { return 0 } - const isString = typeof binding === "string" try { - if (isString) { + if (typeof binding === "string") { binding = JSON.parse(binding) } } catch (err) { @@ -58,7 +56,7 @@ function getLoopIterations(loopStep: LoopStep) { if (Array.isArray(binding)) { return binding.length } - if (isString) { + if (typeof binding === "string") { return automationUtils.stringSplit(binding).length } return 0 @@ -331,8 +329,7 @@ class Orchestrator { } try { loopStep.inputs.binding = automationUtils.typecastForLooping( - loopStep as LoopStep, - loopStep.inputs as LoopInput + loopStep as LoopStep ) } catch (err) { this.updateContextAndOutput( From 67a848bb868530ab2d00af4c248d53c0670fe481 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 30 Jan 2024 10:23:11 +0000 Subject: [PATCH 4/7] Fix tests. --- packages/server/scripts/test.sh | 8 +-- .../unitTests/automationUtils.spec.ts | 50 +++++++++---------- .../server/src/definitions/automations.ts | 2 +- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/packages/server/scripts/test.sh b/packages/server/scripts/test.sh index eb1cf67b01..7f871ac337 100644 --- a/packages/server/scripts/test.sh +++ b/packages/server/scripts/test.sh @@ -5,10 +5,10 @@ if [[ -n $CI ]] then # Running in ci, where resources are limited export NODE_OPTIONS="--max-old-space-size=4096" - echo "jest --coverage --maxWorkers=2 --forceExit --workerIdleMemoryLimit=2000MB --bail" - jest --coverage --maxWorkers=2 --forceExit --workerIdleMemoryLimit=2000MB --bail + echo "jest --coverage --maxWorkers=2 --forceExit --workerIdleMemoryLimit=2000MB --bail $@" + jest --coverage --maxWorkers=2 --forceExit --workerIdleMemoryLimit=2000MB --bail $@ else # --maxWorkers performs better in development - echo "jest --coverage --maxWorkers=2 --forceExit" - jest --coverage --maxWorkers=2 --forceExit + echo "jest --coverage --maxWorkers=2 --forceExit $@" + jest --coverage --maxWorkers=2 --forceExit $@ fi \ No newline at end of file diff --git a/packages/server/src/automations/unitTests/automationUtils.spec.ts b/packages/server/src/automations/unitTests/automationUtils.spec.ts index 2291df9bc2..af1dea4070 100644 --- a/packages/server/src/automations/unitTests/automationUtils.spec.ts +++ b/packages/server/src/automations/unitTests/automationUtils.spec.ts @@ -1,10 +1,15 @@ -const automationUtils = require("../automationUtils") +import { LoopStep, LoopStepType } from "../../definitions/automations" +import { + typecastForLooping, + cleanInputValues, + substituteLoopStep, +} from "../automationUtils" describe("automationUtils", () => { describe("substituteLoopStep", () => { it("should allow multiple loop binding substitutes", () => { expect( - automationUtils.substituteLoopStep( + substituteLoopStep( `{{ loop.currentItem._id }} {{ loop.currentItem._id }} {{ loop.currentItem._id }}`, "step.2" ) @@ -15,7 +20,7 @@ describe("automationUtils", () => { it("should handle not subsituting outside of curly braces", () => { expect( - automationUtils.substituteLoopStep( + substituteLoopStep( `loop {{ loop.currentItem._id }}loop loop{{ loop.currentItem._id }}loop`, "step.2" ) @@ -28,37 +33,32 @@ describe("automationUtils", () => { describe("typeCastForLooping", () => { it("should parse to correct type", () => { expect( - automationUtils.typecastForLooping( - { inputs: { option: "Array" } }, - { binding: [1, 2, 3] } - ) + typecastForLooping({ + inputs: { option: LoopStepType.ARRAY, binding: [1, 2, 3] }, + } as LoopStep) ).toEqual([1, 2, 3]) expect( - automationUtils.typecastForLooping( - { inputs: { option: "Array" } }, - { binding: "[1, 2, 3]" } - ) + typecastForLooping({ + inputs: { option: LoopStepType.ARRAY, binding: "[1,2,3]" }, + } as LoopStep) ).toEqual([1, 2, 3]) expect( - automationUtils.typecastForLooping( - { inputs: { option: "String" } }, - { binding: [1, 2, 3] } - ) + typecastForLooping({ + inputs: { option: LoopStepType.STRING, binding: [1, 2, 3] }, + } as LoopStep) ).toEqual("1,2,3") }) it("should handle null values", () => { // expect it to handle where the binding is null expect( - automationUtils.typecastForLooping( - { inputs: { option: "Array" } }, - { binding: null } - ) + typecastForLooping({ + inputs: { option: LoopStepType.ARRAY }, + } as LoopStep) ).toEqual(null) expect(() => - automationUtils.typecastForLooping( - { inputs: { option: "Array" } }, - { binding: "test" } - ) + typecastForLooping({ + inputs: { option: LoopStepType.ARRAY, binding: "test" }, + } as LoopStep) ).toThrow() }) }) @@ -80,7 +80,7 @@ describe("automationUtils", () => { }, } expect( - automationUtils.cleanInputValues( + cleanInputValues( { row: { relationship: `[{"_id": "ro_ta_users_us_3"}]`, @@ -113,7 +113,7 @@ describe("automationUtils", () => { }, } expect( - automationUtils.cleanInputValues( + cleanInputValues( { row: { relationship: `ro_ta_users_us_3`, diff --git a/packages/server/src/definitions/automations.ts b/packages/server/src/definitions/automations.ts index 48a66408ca..b0b25e5cfa 100644 --- a/packages/server/src/definitions/automations.ts +++ b/packages/server/src/definitions/automations.ts @@ -11,7 +11,7 @@ export interface LoopStep extends AutomationStep { export interface LoopInput { option: LoopStepType - binding: string[] | string + binding?: string[] | string | number[] } export interface TriggerOutput { From 456817ee7b582ce399b9970ce054ccaf1f79f396 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 30 Jan 2024 10:37:23 +0000 Subject: [PATCH 5/7] More loop step typing improvements. --- .../server/src/automations/automationUtils.ts | 7 +++--- .../unitTests/automationUtils.spec.ts | 22 +++++-------------- .../server/src/definitions/automations.ts | 2 ++ packages/server/src/threads/automation.ts | 12 +++++----- 4 files changed, 17 insertions(+), 26 deletions(-) diff --git a/packages/server/src/automations/automationUtils.ts b/packages/server/src/automations/automationUtils.ts index 256f46f19f..227122bccc 100644 --- a/packages/server/src/automations/automationUtils.ts +++ b/packages/server/src/automations/automationUtils.ts @@ -5,7 +5,7 @@ import { } from "@budibase/string-templates" import sdk from "../sdk" import { Row } from "@budibase/types" -import { LoopStep, LoopStepType } from "../definitions/automations" +import { LoopInput, LoopStep, LoopStepType } from "../definitions/automations" /** * When values are input to the system generally they will be of type string as this is required for template strings. @@ -139,13 +139,12 @@ export function stringSplit(value: string | string[]) { return value } -export function typecastForLooping(loopStep: LoopStep) { - const input = loopStep.inputs +export function typecastForLooping(input: LoopInput) { if (!input || !input.binding) { return null } try { - switch (loopStep.inputs.option) { + switch (input.option) { case LoopStepType.ARRAY: if (typeof input.binding === "string") { return JSON.parse(input.binding) diff --git a/packages/server/src/automations/unitTests/automationUtils.spec.ts b/packages/server/src/automations/unitTests/automationUtils.spec.ts index af1dea4070..7de4a2e35b 100644 --- a/packages/server/src/automations/unitTests/automationUtils.spec.ts +++ b/packages/server/src/automations/unitTests/automationUtils.spec.ts @@ -33,32 +33,20 @@ describe("automationUtils", () => { describe("typeCastForLooping", () => { it("should parse to correct type", () => { expect( - typecastForLooping({ - inputs: { option: LoopStepType.ARRAY, binding: [1, 2, 3] }, - } as LoopStep) + typecastForLooping({ option: LoopStepType.ARRAY, binding: [1, 2, 3] }) ).toEqual([1, 2, 3]) expect( - typecastForLooping({ - inputs: { option: LoopStepType.ARRAY, binding: "[1,2,3]" }, - } as LoopStep) + typecastForLooping({ option: LoopStepType.ARRAY, binding: "[1,2,3]" }) ).toEqual([1, 2, 3]) expect( - typecastForLooping({ - inputs: { option: LoopStepType.STRING, binding: [1, 2, 3] }, - } as LoopStep) + typecastForLooping({ option: LoopStepType.STRING, binding: [1, 2, 3] }) ).toEqual("1,2,3") }) it("should handle null values", () => { // expect it to handle where the binding is null - expect( - typecastForLooping({ - inputs: { option: LoopStepType.ARRAY }, - } as LoopStep) - ).toEqual(null) + expect(typecastForLooping({ option: LoopStepType.ARRAY })).toEqual(null) expect(() => - typecastForLooping({ - inputs: { option: LoopStepType.ARRAY, binding: "test" }, - } as LoopStep) + typecastForLooping({ option: LoopStepType.ARRAY, binding: "test" }) ).toThrow() }) }) diff --git a/packages/server/src/definitions/automations.ts b/packages/server/src/definitions/automations.ts index b0b25e5cfa..c205149a5b 100644 --- a/packages/server/src/definitions/automations.ts +++ b/packages/server/src/definitions/automations.ts @@ -12,6 +12,8 @@ export interface LoopStep extends AutomationStep { export interface LoopInput { option: LoopStepType binding?: string[] | string | number[] + iterations?: string + failure?: any } export interface TriggerOutput { diff --git a/packages/server/src/threads/automation.ts b/packages/server/src/threads/automation.ts index 1c3f921147..56d9280f31 100644 --- a/packages/server/src/threads/automation.ts +++ b/packages/server/src/threads/automation.ts @@ -23,6 +23,7 @@ import { } from "@budibase/types" import { AutomationContext, + LoopInput, LoopStep, TriggerOutput, } from "../definitions/automations" @@ -254,7 +255,7 @@ class Orchestrator { this._context.env = await sdkUtils.getEnvironmentVariables() let automation = this._automation let stopped = false - let loopStep: AutomationStep | undefined = undefined + let loopStep: LoopStep | undefined = undefined let stepCount = 0 let loopStepNumber: any = undefined @@ -309,7 +310,7 @@ class Orchestrator { stepCount++ if (step.stepId === LOOP_STEP_ID) { - loopStep = step + loopStep = step as LoopStep loopStepNumber = stepCount continue } @@ -329,7 +330,7 @@ class Orchestrator { } try { loopStep.inputs.binding = automationUtils.typecastForLooping( - loopStep as LoopStep + loopStep.inputs as LoopInput ) } catch (err) { this.updateContextAndOutput( @@ -345,7 +346,7 @@ class Orchestrator { loopStep = undefined break } - let item = [] + let item: any[] = [] if ( typeof loopStep.inputs.binding === "string" && loopStep.inputs.option === "String" @@ -396,7 +397,8 @@ class Orchestrator { if ( index === env.AUTOMATION_MAX_ITERATIONS || - index === parseInt(loopStep.inputs.iterations) + (loopStep.inputs.iterations && + index === parseInt(loopStep.inputs.iterations)) ) { this.updateContextAndOutput( loopStepNumber, From 72d63d0c006379096fcaa6a78992715d01978333 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 30 Jan 2024 10:13:45 +0000 Subject: [PATCH 6/7] Rename executeSynchronously to be a bit less confusing, as it does not execute synchronously. --- packages/server/src/automations/triggers.ts | 5 ++--- packages/server/src/threads/automation.ts | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/server/src/automations/triggers.ts b/packages/server/src/automations/triggers.ts index 17a33e4394..08e3199a11 100644 --- a/packages/server/src/automations/triggers.ts +++ b/packages/server/src/automations/triggers.ts @@ -9,7 +9,7 @@ import * as utils from "./utils" import env from "../environment" import { context, db as dbCore } from "@budibase/backend-core" import { Automation, Row, AutomationData, AutomationJob } from "@budibase/types" -import { executeSynchronously } from "../threads/automation" +import { executeInThread } from "../threads/automation" export const TRIGGER_DEFINITIONS = definitions const JOB_OPTS = { @@ -117,8 +117,7 @@ export async function externalTrigger( appId: context.getAppId(), automation, } - const job = { data } as AutomationJob - return executeSynchronously(job) + return executeInThread({ data } as AutomationJob) } else { return automationQueue.add(data, JOB_OPTS) } diff --git a/packages/server/src/threads/automation.ts b/packages/server/src/threads/automation.ts index 56d9280f31..ed0203797d 100644 --- a/packages/server/src/threads/automation.ts +++ b/packages/server/src/threads/automation.ts @@ -614,7 +614,7 @@ export function execute(job: Job, callback: WorkerCallback) { }) } -export function executeSynchronously(job: Job) { +export async function executeInThread(job: Job) { const appId = job.data.event.appId if (!appId) { throw new Error("Unable to execute, event doesn't contain app ID.") @@ -626,10 +626,10 @@ export function executeSynchronously(job: Job) { }, job.data.event.timeout || 12000) }) - return context.doInAppContext(appId, async () => { + return await context.doInAppContext(appId, async () => { const envVars = await sdkUtils.getEnvironmentVariables() // put into automation thread for whole context - return context.doInEnvironmentContext(envVars, async () => { + return await context.doInEnvironmentContext(envVars, async () => { const automationOrchestrator = new Orchestrator(job) return await Promise.race([ automationOrchestrator.execute(), From b3c949b091fbd7a162e08ea75cebdc9ae9f431e8 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 30 Jan 2024 11:06:09 +0000 Subject: [PATCH 7/7] Fix case where if a binding returned an int it would throw an error. --- packages/server/src/automations/tests/loop.spec.ts | 8 ++++++++ packages/server/src/threads/automation.ts | 10 ++++------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/server/src/automations/tests/loop.spec.ts b/packages/server/src/automations/tests/loop.spec.ts index 70b771c445..68ab694c5d 100644 --- a/packages/server/src/automations/tests/loop.spec.ts +++ b/packages/server/src/automations/tests/loop.spec.ts @@ -44,4 +44,12 @@ describe("Attempt to run a basic loop automation", () => { }) expect(resp.steps[2].outputs.iterations).toBe(3) }) + + it("test a loop with a binding that returns an integer", async () => { + const resp = await runLoop({ + option: LoopStepType.ARRAY, + binding: "{{ 1 }}", + }) + expect(resp.steps[2].outputs.iterations).toBe(1) + }) }) diff --git a/packages/server/src/threads/automation.ts b/packages/server/src/threads/automation.ts index ed0203797d..a828af5d19 100644 --- a/packages/server/src/threads/automation.ts +++ b/packages/server/src/threads/automation.ts @@ -43,20 +43,18 @@ const CRON_STEP_ID = triggerDefs.CRON.stepId const STOPPED_STATUS = { success: true, status: AutomationStatus.STOPPED } function getLoopIterations(loopStep: LoopStep) { - let binding = loopStep.inputs.binding + const binding = loopStep.inputs.binding if (!binding) { return 0 } try { - if (typeof binding === "string") { - binding = JSON.parse(binding) + const json = typeof binding === "string" ? JSON.parse(binding) : binding + if (Array.isArray(json)) { + return json.length } } catch (err) { // ignore error - wasn't able to parse } - if (Array.isArray(binding)) { - return binding.length - } if (typeof binding === "string") { return automationUtils.stringSplit(binding).length }