From fe0efc7539620686b5f9c11ed4cb73b46786c331 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Fri, 1 Dec 2023 09:36:24 +0100 Subject: [PATCH 1/7] Remove unused test context code --- .../backend-core/src/context/mainContext.ts | 39 +++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/packages/backend-core/src/context/mainContext.ts b/packages/backend-core/src/context/mainContext.ts index d2259cfcab..0dba2456a9 100644 --- a/packages/backend-core/src/context/mainContext.ts +++ b/packages/backend-core/src/context/mainContext.ts @@ -145,23 +145,19 @@ export async function doInTenant( } export async function doInAppContext( - appId: string | null, + appId: string, task: () => T ): Promise { - if (!appId && !env.isTest()) { + if (!appId) { throw new Error("appId is required") } - let updates: ContextMap - if (!appId) { - updates = { appId: "" } - } else { - const tenantId = getTenantIDFromAppID(appId) - updates = { appId } - if (tenantId) { - updates.tenantId = tenantId - } + const tenantId = getTenantIDFromAppID(appId) + const updates: ContextMap = { appId } + if (tenantId) { + updates.tenantId = tenantId } + return newContext(updates, task) } @@ -182,6 +178,27 @@ export async function doInIdentityContext( return newContext(context, task) } +export async function doInAppMigrationContext( + appId: string, + task: () => T +): Promise { + if (!appId && !env.isTest()) { + throw new Error("appId is required") + } + + let updates: ContextMap + if (!appId) { + updates = { appId: "" } + } else { + const tenantId = getTenantIDFromAppID(appId) + updates = { appId } + if (tenantId) { + updates.tenantId = tenantId + } + } + return newContext(updates, task) +} + export function getIdentity(): IdentityContext | undefined { try { const context = Context.get() From f62dd56dd6f304c54082b96cbba33c2299e33947 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Fri, 1 Dec 2023 10:19:28 +0100 Subject: [PATCH 2/7] Add doInAppMigrationContext --- .../backend-core/src/context/mainContext.ts | 32 +++++++++-------- .../src/context/tests/index.spec.ts | 36 +++++++++++++++++++ packages/backend-core/src/context/types.ts | 1 + 3 files changed, 54 insertions(+), 15 deletions(-) diff --git a/packages/backend-core/src/context/mainContext.ts b/packages/backend-core/src/context/mainContext.ts index 0dba2456a9..1cbf1de1c3 100644 --- a/packages/backend-core/src/context/mainContext.ts +++ b/packages/backend-core/src/context/mainContext.ts @@ -147,13 +147,21 @@ export async function doInTenant( export async function doInAppContext( appId: string, task: () => T +): Promise { + return _doInAppContext(appId, task) +} + +async function _doInAppContext( + appId: string, + task: () => T, + extraContextSettings?: ContextMap ): Promise { if (!appId) { throw new Error("appId is required") } const tenantId = getTenantIDFromAppID(appId) - const updates: ContextMap = { appId } + const updates: ContextMap = { appId, ...extraContextSettings } if (tenantId) { updates.tenantId = tenantId } @@ -182,21 +190,15 @@ export async function doInAppMigrationContext( appId: string, task: () => T ): Promise { - if (!appId && !env.isTest()) { - throw new Error("appId is required") + try { + return _doInAppContext(appId, task, { + isMigrating: true, + }) + } finally { + updateContext({ + isMigrating: undefined, + }) } - - let updates: ContextMap - if (!appId) { - updates = { appId: "" } - } else { - const tenantId = getTenantIDFromAppID(appId) - updates = { appId } - if (tenantId) { - updates.tenantId = tenantId - } - } - return newContext(updates, task) } export function getIdentity(): IdentityContext | undefined { diff --git a/packages/backend-core/src/context/tests/index.spec.ts b/packages/backend-core/src/context/tests/index.spec.ts index d8bb598af1..ef7cf17e0c 100644 --- a/packages/backend-core/src/context/tests/index.spec.ts +++ b/packages/backend-core/src/context/tests/index.spec.ts @@ -1,6 +1,10 @@ import { testEnv } from "../../../tests/extra" import * as context from "../" import { DEFAULT_TENANT_ID } from "../../constants" +import { structures } from "../../../tests" +import { db } from "../.." +import Context from "../Context" +import { ContextMap } from "../types" describe("context", () => { describe("doInTenant", () => { @@ -144,4 +148,36 @@ describe("context", () => { expect(isScim).toBe(false) }) }) + + describe("doInAppMigrationContext", () => { + it("the context is set correctly", async () => { + const appId = db.generateAppID() + + await context.doInAppMigrationContext(appId, () => { + const context = Context.get() + + const expected: ContextMap = { + appId, + isMigrating: true, + } + expect(context).toEqual(expected) + }) + }) + + it("the context is set correctly when running in a tenant id", async () => { + const tenantId = structures.tenant.id() + const appId = db.generateAppID(tenantId) + + await context.doInAppMigrationContext(appId, () => { + const context = Context.get() + + const expected: ContextMap = { + appId, + isMigrating: true, + tenantId, + } + expect(context).toEqual(expected) + }) + }) + }) }) diff --git a/packages/backend-core/src/context/types.ts b/packages/backend-core/src/context/types.ts index d687a93594..a1606a17b9 100644 --- a/packages/backend-core/src/context/types.ts +++ b/packages/backend-core/src/context/types.ts @@ -8,4 +8,5 @@ export type ContextMap = { environmentVariables?: Record isScim?: boolean automationId?: string + isMigrating?: boolean } From 14fc91d58a5a8880be025c570fa0170a8f336162 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Fri, 1 Dec 2023 10:23:47 +0100 Subject: [PATCH 3/7] Add tests --- .../backend-core/src/context/mainContext.ts | 6 ------ .../src/context/tests/index.spec.ts | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/packages/backend-core/src/context/mainContext.ts b/packages/backend-core/src/context/mainContext.ts index 1cbf1de1c3..1a4d6db454 100644 --- a/packages/backend-core/src/context/mainContext.ts +++ b/packages/backend-core/src/context/mainContext.ts @@ -190,15 +190,9 @@ export async function doInAppMigrationContext( appId: string, task: () => T ): Promise { - try { return _doInAppContext(appId, task, { isMigrating: true, }) - } finally { - updateContext({ - isMigrating: undefined, - }) - } } export function getIdentity(): IdentityContext | undefined { diff --git a/packages/backend-core/src/context/tests/index.spec.ts b/packages/backend-core/src/context/tests/index.spec.ts index ef7cf17e0c..4bfbea74ce 100644 --- a/packages/backend-core/src/context/tests/index.spec.ts +++ b/packages/backend-core/src/context/tests/index.spec.ts @@ -179,5 +179,23 @@ describe("context", () => { expect(context).toEqual(expected) }) }) + + it("the context is not modified outside the delegate", async () => { + const appId = db.generateAppID() + + expect(Context.get()).toBeUndefined() + + await context.doInAppMigrationContext(appId, () => { + const context = Context.get() + + const expected: ContextMap = { + appId, + isMigrating: true, + } + expect(context).toEqual(expected) + }) + + expect(Context.get()).toBeUndefined() + }) }) }) From 7f52a1e28ce622b3c2259213f26397641dbe7ed3 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Fri, 1 Dec 2023 10:39:44 +0100 Subject: [PATCH 4/7] Guard migration --- .../backend-core/src/context/mainContext.ts | 17 ++++-- .../src/context/tests/index.spec.ts | 54 +++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/packages/backend-core/src/context/mainContext.ts b/packages/backend-core/src/context/mainContext.ts index 1a4d6db454..d2fdd040df 100644 --- a/packages/backend-core/src/context/mainContext.ts +++ b/packages/backend-core/src/context/mainContext.ts @@ -99,6 +99,8 @@ function updateContext(updates: ContextMap): ContextMap { } async function newContext(updates: ContextMap, task: () => T) { + guardMigration() + // see if there already is a context setup let context: ContextMap = updateContext(updates) return Context.run(context, task) @@ -186,13 +188,22 @@ export async function doInIdentityContext( return newContext(context, task) } +function guardMigration() { + const context = Context.get() + if (context?.isMigrating) { + throw new Error( + "The context cannot be change, a migration is currently running" + ) + } +} + export async function doInAppMigrationContext( appId: string, task: () => T ): Promise { - return _doInAppContext(appId, task, { - isMigrating: true, - }) + return _doInAppContext(appId, task, { + isMigrating: true, + }) } export function getIdentity(): IdentityContext | undefined { diff --git a/packages/backend-core/src/context/tests/index.spec.ts b/packages/backend-core/src/context/tests/index.spec.ts index 4bfbea74ce..183e09a509 100644 --- a/packages/backend-core/src/context/tests/index.spec.ts +++ b/packages/backend-core/src/context/tests/index.spec.ts @@ -5,6 +5,7 @@ import { structures } from "../../../tests" import { db } from "../.." import Context from "../Context" import { ContextMap } from "../types" +import { IdentityType } from "@budibase/types" describe("context", () => { describe("doInTenant", () => { @@ -197,5 +198,58 @@ describe("context", () => { expect(Context.get()).toBeUndefined() }) + + it.each([ + [ + "doInAppMigrationContext", + () => context.doInAppMigrationContext(db.generateAppID(), () => {}), + ], + [ + "doInAppContext", + () => context.doInAppContext(db.generateAppID(), () => {}), + ], + [ + "doInAutomationContext", + () => + context.doInAutomationContext({ + appId: db.generateAppID(), + automationId: structures.generator.guid(), + task: () => {}, + }), + ], + ["doInContext", () => context.doInContext(db.generateAppID(), () => {})], + [ + "doInEnvironmentContext", + () => context.doInEnvironmentContext({}, () => {}), + ], + [ + "doInIdentityContext", + () => + context.doInIdentityContext( + { + account: undefined, + type: IdentityType.USER, + _id: structures.users.user()._id!, + }, + () => {} + ), + ], + ["doInScimContext", () => context.doInScimContext(() => {})], + [ + "doInTenant", + () => context.doInTenant(structures.tenant.id(), () => {}), + ], + ])( + "a nested context.%s function cannot run", + async (_, otherContextCall: () => Promise) => { + await expect( + context.doInAppMigrationContext(db.generateAppID(), async () => { + await otherContextCall() + }) + ).rejects.toThrowError( + "The context cannot be change, a migration is currently running" + ) + } + ) }) }) From fa7693f6df62532446e15eb8f30af2525316ef02 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 4 Dec 2023 09:09:42 +0100 Subject: [PATCH 5/7] Typo Co-authored-by: Sam Rose --- packages/backend-core/src/context/mainContext.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-core/src/context/mainContext.ts b/packages/backend-core/src/context/mainContext.ts index d2fdd040df..983a4d20e1 100644 --- a/packages/backend-core/src/context/mainContext.ts +++ b/packages/backend-core/src/context/mainContext.ts @@ -192,7 +192,7 @@ function guardMigration() { const context = Context.get() if (context?.isMigrating) { throw new Error( - "The context cannot be change, a migration is currently running" + "The context cannot be changed, a migration is currently running" ) } } From 188d5d09a2a55a6c746c6768be07ec45c87ef02c Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 4 Dec 2023 09:09:59 +0100 Subject: [PATCH 6/7] Typo Co-authored-by: Sam Rose --- packages/backend-core/src/context/tests/index.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-core/src/context/tests/index.spec.ts b/packages/backend-core/src/context/tests/index.spec.ts index 183e09a509..cfc820e169 100644 --- a/packages/backend-core/src/context/tests/index.spec.ts +++ b/packages/backend-core/src/context/tests/index.spec.ts @@ -247,7 +247,7 @@ describe("context", () => { await otherContextCall() }) ).rejects.toThrowError( - "The context cannot be change, a migration is currently running" + "The context cannot be changed, a migration is currently running" ) } ) From 083ff0b7c7c223f5a0381e3cc56c207f1ac47f0e Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 4 Dec 2023 09:23:01 +0100 Subject: [PATCH 7/7] Fix tests typing --- .../server/src/tests/utilities/TestConfiguration.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts index 3a14a87d2a..b7886ccea4 100644 --- a/packages/server/src/tests/utilities/TestConfiguration.ts +++ b/packages/server/src/tests/utilities/TestConfiguration.ts @@ -137,6 +137,10 @@ class TestConfiguration { } getAppId() { + if (!this.appId) { + throw "appId has not been initialised properly" + } + return this.appId } @@ -510,7 +514,7 @@ class TestConfiguration { // create dev app // clear any old app this.appId = null - this.app = await context.doInAppContext(null, async () => { + this.app = await context.doInTenant(this.tenantId!, async () => { const app = await this._req( { name: appName }, null, @@ -519,7 +523,7 @@ class TestConfiguration { this.appId = app.appId! return app }) - return await context.doInAppContext(this.appId, async () => { + return await context.doInAppContext(this.getAppId(), async () => { // create production app this.prodApp = await this.publish() @@ -817,7 +821,7 @@ class TestConfiguration { } async getAutomationLogs() { - return context.doInAppContext(this.appId, async () => { + return context.doInAppContext(this.getAppId(), async () => { const now = new Date() return await pro.sdk.automations.logs.logSearch({ startDate: new Date(now.getTime() - 100000).toISOString(),