From edb3b685b5f9905311f1255f449e88c44cdaa229 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 7 Aug 2024 15:26:04 +0100 Subject: [PATCH 01/18] Remove node-fetch mock from backend-core and replace with nock. --- packages/backend-core/package.json | 2 +- packages/backend-core/src/environment.ts | 27 +++ .../processors/posthog/PosthogProcessor.ts | 2 +- .../posthog/tests/PosthogProcessor.spec.ts | 29 ++- packages/backend-core/src/features/index.ts | 70 +++++- .../src/features/tests/features.spec.ts | 69 ++++-- .../passport/sso/tests/oidc.spec.ts | 16 +- .../middleware/passport/sso/tests/sso.spec.ts | 20 +- .../src/plugin/tests/validation.spec.ts | 226 ++++++++++++++---- packages/backend-core/src/redis/redis.ts | 4 + .../src/redis/tests/redis.spec.ts | 7 +- .../tests/core/utilities/mocks/fetch.ts | 17 -- .../tests/core/utilities/mocks/index.ts | 2 - .../tests/core/utilities/mocks/posthog.ts | 7 - packages/backend-core/tests/jestSetup.ts | 13 +- packages/server/src/startup/index.ts | 4 + packages/worker/src/index.ts | 2 + yarn.lock | 17 +- 18 files changed, 386 insertions(+), 148 deletions(-) delete mode 100644 packages/backend-core/tests/core/utilities/mocks/fetch.ts delete mode 100644 packages/backend-core/tests/core/utilities/mocks/posthog.ts diff --git a/packages/backend-core/package.json b/packages/backend-core/package.json index bf5215a724..b68cba5fd9 100644 --- a/packages/backend-core/package.json +++ b/packages/backend-core/package.json @@ -45,7 +45,7 @@ "passport-oauth2-refresh": "^2.1.0", "pino": "8.11.0", "pino-http": "8.3.3", - "posthog-node": "1.3.0", + "posthog-node": "4.0.1", "pouchdb": "7.3.0", "pouchdb-find": "7.2.2", "redlock": "4.2.0", diff --git a/packages/backend-core/src/environment.ts b/packages/backend-core/src/environment.ts index 64e3187956..5091a4971a 100644 --- a/packages/backend-core/src/environment.ts +++ b/packages/backend-core/src/environment.ts @@ -1,5 +1,6 @@ import { existsSync, readFileSync } from "fs" import { ServiceType } from "@budibase/types" +import { cloneDeep } from "lodash" function isTest() { return isJest() @@ -208,6 +209,32 @@ const environment = { OPENAI_API_KEY: process.env.OPENAI_API_KEY, } +export function setEnv(newEnvVars: Partial): () => void { + const oldEnv = cloneDeep(environment) + + let key: keyof typeof newEnvVars + for (key in newEnvVars) { + environment._set(key, newEnvVars[key]) + } + + return () => { + for (const [key, value] of Object.entries(oldEnv)) { + environment._set(key, value) + } + } +} + +export function withEnv(envVars: Partial, f: () => T) { + const cleanup = setEnv(envVars) + const result = f() + if (result instanceof Promise) { + return result.finally(cleanup) + } else { + cleanup() + return result + } +} + type EnvironmentKey = keyof typeof environment export const SECRETS: EnvironmentKey[] = [ "API_ENCRYPTION_KEY", diff --git a/packages/backend-core/src/events/processors/posthog/PosthogProcessor.ts b/packages/backend-core/src/events/processors/posthog/PosthogProcessor.ts index d37b85a9b8..12d2bb7e2c 100644 --- a/packages/backend-core/src/events/processors/posthog/PosthogProcessor.ts +++ b/packages/backend-core/src/events/processors/posthog/PosthogProcessor.ts @@ -1,4 +1,4 @@ -import PostHog from "posthog-node" +import { PostHog } from "posthog-node" import { Event, Identity, Group, BaseEvent } from "@budibase/types" import { EventProcessor } from "../types" import env from "../../../environment" diff --git a/packages/backend-core/src/events/processors/posthog/tests/PosthogProcessor.spec.ts b/packages/backend-core/src/events/processors/posthog/tests/PosthogProcessor.spec.ts index d9a5504073..1d9e341ffe 100644 --- a/packages/backend-core/src/events/processors/posthog/tests/PosthogProcessor.spec.ts +++ b/packages/backend-core/src/events/processors/posthog/tests/PosthogProcessor.spec.ts @@ -1,9 +1,7 @@ import { testEnv } from "../../../../../tests/extra" import PosthogProcessor from "../PosthogProcessor" import { Event, IdentityType, Hosting } from "@budibase/types" - -const tk = require("timekeeper") - +import tk from "timekeeper" import * as cache from "../../../../cache/generic" import { CacheKey } from "../../../../cache/generic" import * as context from "../../../../context" @@ -32,27 +30,30 @@ describe("PosthogProcessor", () => { describe("processEvent", () => { it("processes event", async () => { const processor = new PosthogProcessor("test") + const spy = jest.spyOn(processor.posthog, "capture") const identity = newIdentity() const properties = {} await processor.processEvent(Event.APP_CREATED, identity, properties) - expect(processor.posthog.capture).toHaveBeenCalledTimes(1) + expect(spy).toHaveBeenCalledTimes(1) }) it("honours exclusions", async () => { const processor = new PosthogProcessor("test") + const spy = jest.spyOn(processor.posthog, "capture") const identity = newIdentity() const properties = {} await processor.processEvent(Event.AUTH_SSO_UPDATED, identity, properties) - expect(processor.posthog.capture).toHaveBeenCalledTimes(0) + expect(spy).toHaveBeenCalledTimes(0) }) it("removes audited information", async () => { const processor = new PosthogProcessor("test") + const spy = jest.spyOn(processor.posthog, "capture") const identity = newIdentity() const properties = { @@ -63,7 +64,7 @@ describe("PosthogProcessor", () => { } await processor.processEvent(Event.USER_CREATED, identity, properties) - expect(processor.posthog.capture).toHaveBeenCalled() + expect(spy).toHaveBeenCalled() // @ts-ignore const call = processor.posthog.capture.mock.calls[0][0] expect(call.properties.audited).toBeUndefined() @@ -73,6 +74,8 @@ describe("PosthogProcessor", () => { describe("rate limiting", () => { it("sends daily event once in same day", async () => { const processor = new PosthogProcessor("test") + const spy = jest.spyOn(processor.posthog, "capture") + const identity = newIdentity() const properties = {} @@ -82,11 +85,12 @@ describe("PosthogProcessor", () => { tk.freeze(new Date(2022, 0, 1, 15, 0)) await processor.processEvent(Event.SERVED_BUILDER, identity, properties) - expect(processor.posthog.capture).toHaveBeenCalledTimes(1) + expect(spy).toHaveBeenCalledTimes(1) }) it("sends daily event once per unique day", async () => { const processor = new PosthogProcessor("test") + const spy = jest.spyOn(processor.posthog, "capture") const identity = newIdentity() const properties = {} @@ -102,11 +106,13 @@ describe("PosthogProcessor", () => { tk.freeze(new Date(2022, 0, 3, 6, 0)) await processor.processEvent(Event.SERVED_BUILDER, identity, properties) - expect(processor.posthog.capture).toHaveBeenCalledTimes(3) + expect(spy).toHaveBeenCalledTimes(3) }) it("sends event again after cache expires", async () => { const processor = new PosthogProcessor("test") + const spy = jest.spyOn(processor.posthog, "capture") + const identity = newIdentity() const properties = {} @@ -120,11 +126,12 @@ describe("PosthogProcessor", () => { tk.freeze(new Date(2022, 0, 1, 14, 0)) await processor.processEvent(Event.SERVED_BUILDER, identity, properties) - expect(processor.posthog.capture).toHaveBeenCalledTimes(2) + expect(spy).toHaveBeenCalledTimes(2) }) it("sends per app events once per day per app", async () => { const processor = new PosthogProcessor("test") + const spy = jest.spyOn(processor.posthog, "capture") const identity = newIdentity() const properties = {} @@ -160,10 +167,10 @@ describe("PosthogProcessor", () => { } await runAppEvents("app_1") - expect(processor.posthog.capture).toHaveBeenCalledTimes(4) + expect(spy).toHaveBeenCalledTimes(4) await runAppEvents("app_2") - expect(processor.posthog.capture).toHaveBeenCalledTimes(8) + expect(spy).toHaveBeenCalledTimes(8) }) }) }) diff --git a/packages/backend-core/src/features/index.ts b/packages/backend-core/src/features/index.ts index d7f7c76436..7a77eb8b56 100644 --- a/packages/backend-core/src/features/index.ts +++ b/packages/backend-core/src/features/index.ts @@ -1,13 +1,40 @@ import env from "../environment" import * as context from "../context" import { cloneDeep } from "lodash" +import { PostHog } from "posthog-node" +import { IdentityType } from "@budibase/types" -class Flag { - static withDefault(value: T) { - return new Flag(value) +let posthog: PostHog | undefined +export function init() { + if (env.POSTHOG_TOKEN) { + posthog = new PostHog(env.POSTHOG_TOKEN, { + host: "https://us.i.posthog.com", + }) + } +} + +abstract class Flag { + static boolean(defaultValue: boolean): Flag { + return new BooleanFlag(defaultValue) } - private constructor(public defaultValue: T) {} + protected constructor(public defaultValue: T) {} + + abstract parse(value: any): T +} + +class BooleanFlag extends Flag { + parse(value: any) { + if (typeof value === "string") { + return ["true", "t", "1"].includes(value.toLowerCase()) + } + + if (typeof value === "boolean") { + return value + } + + throw new Error(`could not parse value "${value}" as boolean`) + } } // This is the primary source of truth for feature flags. If you want to add a @@ -15,10 +42,10 @@ class Flag { // All of the machinery in this file is to make sure that flags have their // default values set correctly and their types flow through the system. const FLAGS = { - LICENSING: Flag.withDefault(false), - GOOGLE_SHEETS: Flag.withDefault(false), - USER_GROUPS: Flag.withDefault(false), - ONBOARDING_TOUR: Flag.withDefault(false), + LICENSING: Flag.boolean(false), + GOOGLE_SHEETS: Flag.boolean(false), + USER_GROUPS: Flag.boolean(false), + ONBOARDING_TOUR: Flag.boolean(false), } const DEFAULTS = Object.keys(FLAGS).reduce((acc, key) => { @@ -53,9 +80,10 @@ function isFlagName(name: string): name is keyof Flags { * they will be accessed through this function as well. */ export async function fetch(): Promise { - const currentTenantId = context.getTenantId() const flags = defaultFlags() + const currentTenantId = context.getTenantId() + const split = (env.TENANT_FEATURE_FLAGS || "") .split(",") .map(x => x.split(":")) @@ -79,11 +107,33 @@ export async function fetch(): Promise { throw new Error(`Feature: ${feature} is not a boolean`) } - // @ts-ignore flags[feature] = value } } + const identity = context.getIdentity() + if (posthog && identity?.type === IdentityType.USER) { + const posthogFlags = await posthog.getAllFlagsAndPayloads(identity._id) + for (const [name, value] of Object.entries(posthogFlags)) { + const key = name as keyof typeof FLAGS + const flag = FLAGS[key] + if (!flag) { + // We don't want an unexpected PostHog flag to break the app, so we + // just log it and continue. + console.warn(`Unexpected posthog flag "${name}": ${value}`) + continue + } + + try { + flags[key] = flag.parse(value) + } catch (err) { + // We don't want an invalid PostHog flag to break the app, so we just + // log it and continue. + console.warn(`Error parsing posthog flag "${name}": ${value}`, err) + } + } + } + return flags } diff --git a/packages/backend-core/src/features/tests/features.spec.ts b/packages/backend-core/src/features/tests/features.spec.ts index 83a89940b8..9022d568ee 100644 --- a/packages/backend-core/src/features/tests/features.spec.ts +++ b/packages/backend-core/src/features/tests/features.spec.ts @@ -1,16 +1,8 @@ -import { defaultFlags, fetch, get, Flags } from "../" +import { IdentityContext, IdentityType } from "@budibase/types" +import { defaultFlags, fetch, get, Flags, init } from "../" import { context } from "../.." -import env from "../../environment" - -async function withFlags(flags: string, f: () => T): Promise { - const oldFlags = env.TENANT_FEATURE_FLAGS - env._set("TENANT_FEATURE_FLAGS", flags) - try { - return await f() - } finally { - env._set("TENANT_FEATURE_FLAGS", oldFlags) - } -} +import { setEnv, withEnv } from "../../environment" +import nock from "nock" describe("feature flags", () => { interface TestCase { @@ -48,8 +40,8 @@ describe("feature flags", () => { ])( 'should find flags $expected for $tenant with string "$flags"', ({ tenant, flags, expected }) => - context.doInTenant(tenant, () => - withFlags(flags, async () => { + context.doInTenant(tenant, async () => + withEnv({ TENANT_FEATURE_FLAGS: flags }, async () => { const flags = await fetch() expect(flags).toMatchObject(expected) @@ -75,12 +67,51 @@ describe("feature flags", () => { }, ])( "should fail with message \"$expected\" for $tenant with string '$flags'", - async ({ tenant, flags, expected }) => { + ({ tenant, flags, expected }) => context.doInTenant(tenant, () => - withFlags(flags, async () => { - await expect(fetch()).rejects.toThrow(expected) - }) + withEnv({ TENANT_FEATURE_FLAGS: flags }, () => + expect(fetch()).rejects.toThrow(expected) + ) ) - } ) + + // describe("posthog", () => { + // const identity: IdentityContext = { + // _id: "us_1234", + // tenantId: "budibase", + // type: IdentityType.USER, + // email: "test@example.com", + // firstName: "Test", + // lastName: "User", + // } + + // let cleanup: () => void + + // beforeAll(() => { + // cleanup = setEnv({ POSTHOG_TOKEN: "test" }) + // init() + // }) + + // afterAll(() => { + // cleanup() + // }) + + // beforeEach(() => { + // nock.cleanAll() + // }) + + // it("should be able to read flags from posthog", () => + // context.doInIdentityContext(identity, async () => { + // nock("https://app.posthog.com") + // .get("/api/feature_flags/tenant/budibase") + // .reply(200, { + // flags: { + // "budibase:onboardingTour": true, + // }, + // }) + + // const flags = await fetch() + // expect(flags.ONBOARDING_TOUR).toBe(true) + // })) + // }) }) diff --git a/packages/backend-core/src/middleware/passport/sso/tests/oidc.spec.ts b/packages/backend-core/src/middleware/passport/sso/tests/oidc.spec.ts index a705739bd6..594e197204 100644 --- a/packages/backend-core/src/middleware/passport/sso/tests/oidc.spec.ts +++ b/packages/backend-core/src/middleware/passport/sso/tests/oidc.spec.ts @@ -1,4 +1,4 @@ -import { generator, mocks, structures } from "../../../../../tests" +import { generator, structures } from "../../../../../tests" import { JwtClaims, OIDCInnerConfig, @@ -7,6 +7,7 @@ import { } from "@budibase/types" import * as _sso from "../sso" import * as oidc from "../oidc" +import nock from "nock" jest.mock("@techpass/passport-openidconnect") const mockStrategy = require("@techpass/passport-openidconnect").Strategy @@ -22,16 +23,9 @@ describe("oidc", () => { const oidcConfig: OIDCInnerConfig = structures.sso.oidcConfig() const wellKnownConfig = structures.sso.oidcWellKnownConfig() - function mockRetrieveWellKnownConfig() { - // mock the request to retrieve the oidc configuration - mocks.fetch.mockReturnValue({ - ok: true, - json: () => wellKnownConfig, - }) - } - beforeEach(() => { - mockRetrieveWellKnownConfig() + nock.cleanAll() + nock(oidcConfig.configUrl).get("/").reply(200, wellKnownConfig) }) describe("strategyFactory", () => { @@ -42,8 +36,6 @@ describe("oidc", () => { ) await oidc.strategyFactory(strategyConfiguration, mockSaveUser) - expect(mocks.fetch).toHaveBeenCalledWith(oidcConfig.configUrl) - const expectedOptions = { issuer: wellKnownConfig.issuer, authorizationURL: wellKnownConfig.authorization_endpoint, diff --git a/packages/backend-core/src/middleware/passport/sso/tests/sso.spec.ts b/packages/backend-core/src/middleware/passport/sso/tests/sso.spec.ts index ea9584c284..9fa82b6594 100644 --- a/packages/backend-core/src/middleware/passport/sso/tests/sso.spec.ts +++ b/packages/backend-core/src/middleware/passport/sso/tests/sso.spec.ts @@ -5,6 +5,7 @@ import { SSOAuthDetails, User } from "@budibase/types" import { HTTPError } from "../../../../errors" import * as sso from "../sso" import * as context from "../../../../context" +import nock from "nock" const mockDone = jest.fn() const mockSaveUser = jest.fn() @@ -23,6 +24,7 @@ describe("sso", () => { beforeEach(() => { jest.clearAllMocks() testEnv.singleTenant() + nock.cleanAll() }) describe("validation", () => { @@ -51,15 +53,6 @@ describe("sso", () => { }) }) - function mockGetProfilePicture() { - mocks.fetch.mockReturnValueOnce( - Promise.resolve({ - status: 200, - headers: { get: () => "image/" }, - }) - ) - } - describe("when the user doesn't exist", () => { let user: User let details: SSOAuthDetails @@ -68,7 +61,10 @@ describe("sso", () => { users.getById.mockImplementationOnce(() => { throw new HTTPError("", 404) }) - mockGetProfilePicture() + + nock("http://example.com").get("/").reply(200, undefined, { + "Content-Type": "image/png", + }) user = structures.users.user() delete user._rev @@ -131,7 +127,9 @@ describe("sso", () => { existingUser = structures.users.user() existingUser._id = structures.uuid() details = structures.sso.authDetails(existingUser) - mockGetProfilePicture() + nock("http://example.com").get("/").reply(200, undefined, { + "Content-Type": "image/png", + }) }) describe("exists by email", () => { diff --git a/packages/backend-core/src/plugin/tests/validation.spec.ts b/packages/backend-core/src/plugin/tests/validation.spec.ts index 0fea009645..6f1a3e300b 100644 --- a/packages/backend-core/src/plugin/tests/validation.spec.ts +++ b/packages/backend-core/src/plugin/tests/validation.spec.ts @@ -1,12 +1,129 @@ import { validate } from "../utils" import fetch from "node-fetch" import { PluginType } from "@budibase/types" +import nock from "nock" -const repoUrl = - "https://raw.githubusercontent.com/Budibase/budibase-skeleton/master" -const automationLink = `${repoUrl}/automation/schema.json.hbs` -const componentLink = `${repoUrl}/component/schema.json.hbs` -const datasourceLink = `${repoUrl}/datasource/schema.json.hbs` +const automationLink = `http://example.com/automation/schema.json` +const componentLink = `http://example.com/component/schema.json` +const datasourceLink = `http://example.com/datasource/schema.json` + +function mockDatasourceSchema() { + nock("http://example.com") + .get("/datasource/schema.json") + .reply(200, { + type: "datasource", + metadata: {}, + schema: { + docs: "https://docs.budibase.com", + friendlyName: "Basic HTTP", + type: "API", + description: "Performs a basic HTTP calls to a URL", + datasource: { + url: { + type: "string", + required: true, + }, + cookie: { + type: "string", + required: false, + }, + }, + query: { + create: { + type: "json", + }, + read: { + type: "fields", + fields: { + queryString: { + display: "Query string", + type: "string", + required: false, + }, + }, + }, + update: { + type: "json", + }, + delete: { + type: "fields", + fields: { + id: { + type: "string", + required: true, + }, + }, + }, + }, + }, + }) +} + +function mockAutomationSchema() { + nock("http://example.com") + .get("/automation/schema.json") + .reply(200, { + type: "automation", + metadata: {}, + schema: { + name: "{{ name }}", + tagline: "{{ description }}", + icon: "Actions", + description: "{{ description }}", + type: "action", + stepId: "{{ name }}", + inputs: { + text: "", + }, + schema: { + inputs: { + properties: { + text: { + type: "string", + title: "Log", + }, + }, + required: ["text"], + }, + outputs: { + properties: { + success: { + type: "boolean", + description: "Whether the action was successful", + }, + message: { + type: "string", + description: "What was output", + }, + }, + required: ["success", "message"], + }, + }, + }, + }) +} + +function mockComponentSchema() { + nock("http://example.com") + .get("/component/schema.json") + .reply(200, { + type: "component", + metadata: {}, + schema: { + name: "{{ name }}", + friendlyName: "{{ name }}", + description: "{{ description }}", + icon: "Text", + settings: [ + { + type: "text", + key: "text", + label: "Text", + }, + ], + }, + }) +} async function getSchema(link: string) { const response = await fetch(link) @@ -31,53 +148,62 @@ async function runTest(opts: { link?: string; schema?: any }) { return error } -describe("it should be able to validate an automation schema", () => { - it("should return automation skeleton schema is valid", async () => { - const error = await runTest({ link: automationLink }) - expect(error).toBeUndefined() +describe("plugin validation", () => { + beforeEach(() => { + nock.cleanAll() + mockAutomationSchema() + mockComponentSchema() + mockDatasourceSchema() }) - it("should fail given invalid automation schema", async () => { - const error = await runTest({ - schema: { - type: PluginType.AUTOMATION, - schema: {}, - }, + describe("it should be able to validate an automation schema", () => { + it("should return automation skeleton schema is valid", async () => { + const error = await runTest({ link: automationLink }) + expect(error).toBeUndefined() + }) + + it("should fail given invalid automation schema", async () => { + const error = await runTest({ + schema: { + type: PluginType.AUTOMATION, + schema: {}, + }, + }) + expect(error).toBeDefined() + }) + }) + + describe("it should be able to validate a component schema", () => { + it("should return component skeleton schema is valid", async () => { + const error = await runTest({ link: componentLink }) + expect(error).toBeUndefined() + }) + + it("should fail given invalid component schema", async () => { + const error = await runTest({ + schema: { + type: PluginType.COMPONENT, + schema: {}, + }, + }) + expect(error).toBeDefined() + }) + }) + + describe("it should be able to validate a datasource schema", () => { + it("should return datasource skeleton schema is valid", async () => { + const error = await runTest({ link: datasourceLink }) + expect(error).toBeUndefined() + }) + + it("should fail given invalid datasource schema", async () => { + const error = await runTest({ + schema: { + type: PluginType.DATASOURCE, + schema: {}, + }, + }) + expect(error).toBeDefined() }) - expect(error).toBeDefined() - }) -}) - -describe("it should be able to validate a component schema", () => { - it("should return component skeleton schema is valid", async () => { - const error = await runTest({ link: componentLink }) - expect(error).toBeUndefined() - }) - - it("should fail given invalid component schema", async () => { - const error = await runTest({ - schema: { - type: PluginType.COMPONENT, - schema: {}, - }, - }) - expect(error).toBeDefined() - }) -}) - -describe("it should be able to validate a datasource schema", () => { - it("should return datasource skeleton schema is valid", async () => { - const error = await runTest({ link: datasourceLink }) - expect(error).toBeUndefined() - }) - - it("should fail given invalid datasource schema", async () => { - const error = await runTest({ - schema: { - type: PluginType.DATASOURCE, - schema: {}, - }, - }) - expect(error).toBeDefined() }) }) diff --git a/packages/backend-core/src/redis/redis.ts b/packages/backend-core/src/redis/redis.ts index 79f75421d3..1271086fc5 100644 --- a/packages/backend-core/src/redis/redis.ts +++ b/packages/backend-core/src/redis/redis.ts @@ -111,6 +111,10 @@ function init(selectDb = DEFAULT_SELECT_DB) { CLIENTS[selectDb] = client } +export function closeAll() { + Object.values(CLIENTS).forEach(client => client.disconnect()) +} + function waitForConnection(selectDb: number = DEFAULT_SELECT_DB) { return new Promise(resolve => { if (pickClient(selectDb) == null) { diff --git a/packages/backend-core/src/redis/tests/redis.spec.ts b/packages/backend-core/src/redis/tests/redis.spec.ts index 9160c6a6dd..376afbfab7 100644 --- a/packages/backend-core/src/redis/tests/redis.spec.ts +++ b/packages/backend-core/src/redis/tests/redis.spec.ts @@ -1,6 +1,6 @@ import { GenericContainer, StartedTestContainer } from "testcontainers" import { generator, structures } from "../../../tests" -import RedisWrapper from "../redis" +import RedisWrapper, { closeAll } from "../redis" import { env } from "../.." import { randomUUID } from "crypto" @@ -23,7 +23,10 @@ describe("redis", () => { env._set("REDIS_PASSWORD", 0) }) - afterAll(() => container?.stop()) + afterAll(() => { + container?.stop() + closeAll() + }) beforeEach(async () => { redis = new RedisWrapper(structures.db.id()) diff --git a/packages/backend-core/tests/core/utilities/mocks/fetch.ts b/packages/backend-core/tests/core/utilities/mocks/fetch.ts deleted file mode 100644 index f7447d2c47..0000000000 --- a/packages/backend-core/tests/core/utilities/mocks/fetch.ts +++ /dev/null @@ -1,17 +0,0 @@ -const mockFetch = jest.fn((url: any, opts: any) => { - const fetch = jest.requireActual("node-fetch") - const env = jest.requireActual("../../../../src/environment").default - if (url.includes(env.COUCH_DB_URL) || url.includes("raw.github")) { - return fetch(url, opts) - } - return undefined -}) - -const enable = () => { - jest.mock("node-fetch", () => mockFetch) -} - -export default { - ...mockFetch, - enable, -} diff --git a/packages/backend-core/tests/core/utilities/mocks/index.ts b/packages/backend-core/tests/core/utilities/mocks/index.ts index 8705e563cb..ef7304d64e 100644 --- a/packages/backend-core/tests/core/utilities/mocks/index.ts +++ b/packages/backend-core/tests/core/utilities/mocks/index.ts @@ -5,7 +5,5 @@ export const accounts = jest.mocked(_accounts) export * as date from "./date" export * as licenses from "./licenses" -export { default as fetch } from "./fetch" export * from "./alerts" import "./events" -import "./posthog" diff --git a/packages/backend-core/tests/core/utilities/mocks/posthog.ts b/packages/backend-core/tests/core/utilities/mocks/posthog.ts deleted file mode 100644 index e9cc653ccc..0000000000 --- a/packages/backend-core/tests/core/utilities/mocks/posthog.ts +++ /dev/null @@ -1,7 +0,0 @@ -jest.mock("posthog-node", () => { - return jest.fn().mockImplementation(() => { - return { - capture: jest.fn(), - } - }) -}) diff --git a/packages/backend-core/tests/jestSetup.ts b/packages/backend-core/tests/jestSetup.ts index e5d144290b..e7f2a6cc98 100644 --- a/packages/backend-core/tests/jestSetup.ts +++ b/packages/backend-core/tests/jestSetup.ts @@ -2,14 +2,21 @@ import "./core/logging" import env from "../src/environment" import { cleanup } from "../src/timers" import { mocks, testContainerUtils } from "./core/utilities" - -// must explicitly enable fetch mock -mocks.fetch.enable() +import nock from "nock" // mock all dates to 2020-01-01T00:00:00.000Z // use tk.reset() to use real dates in individual tests import tk from "timekeeper" +nock.disableNetConnect() +nock.enableNetConnect(host => { + return ( + host.includes("localhost") || + host.includes("127.0.0.1") || + host.includes("::1") + ) +}) + tk.freeze(mocks.date.MOCK_DATE) if (!process.env.DEBUG) { diff --git a/packages/server/src/startup/index.ts b/packages/server/src/startup/index.ts index 5bb1f9aa0f..53c4f884cc 100644 --- a/packages/server/src/startup/index.ts +++ b/packages/server/src/startup/index.ts @@ -9,6 +9,7 @@ import { users, cache, env as coreEnv, + features, } from "@budibase/backend-core" import { watch } from "../watch" import * as automations from "../automations" @@ -96,6 +97,9 @@ export async function startup( console.log("Initialising events") eventInit() + console.log("Initialising feature flags") + features.init() + if (app && server) { console.log("Initialising websockets") initialiseWebsockets(app, server) diff --git a/packages/worker/src/index.ts b/packages/worker/src/index.ts index 85e5d6ad2e..d59d8d96ef 100644 --- a/packages/worker/src/index.ts +++ b/packages/worker/src/index.ts @@ -18,6 +18,7 @@ import { timers, redis, cache, + features, } from "@budibase/backend-core" db.init() @@ -99,6 +100,7 @@ export default server.listen(parseInt(env.PORT || "4002"), async () => { // configure events to use the pro audit log write // can't integrate directly into backend-core due to cyclic issues await events.processors.init(proSdk.auditLogs.write) + features.init() }) process.on("uncaughtException", err => { diff --git a/yarn.lock b/yarn.lock index 0195f19a2a..123eec3dd9 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2073,7 +2073,7 @@ passport-oauth2-refresh "^2.1.0" pino "8.11.0" pino-http "8.3.3" - posthog-node "1.3.0" + posthog-node "4.0.1" pouchdb "7.3.0" pouchdb-find "7.2.2" redlock "4.2.0" @@ -7343,7 +7343,7 @@ axios-retry@^3.1.9: "@babel/runtime" "^7.15.4" is-retry-allowed "^2.2.0" -axios@0.24.0, axios@1.1.3, axios@1.6.3, axios@^0.21.1, axios@^1.0.0, axios@^1.1.3, axios@^1.4.0, axios@^1.5.0: +axios@0.24.0, axios@1.1.3, axios@1.6.3, axios@^0.21.1, axios@^1.0.0, axios@^1.1.3, axios@^1.4.0, axios@^1.5.0, axios@^1.6.2: version "1.6.3" resolved "https://registry.yarnpkg.com/axios/-/axios-1.6.3.tgz#7f50f23b3aa246eff43c54834272346c396613f4" integrity sha512-fWyNdeawGam70jXSVlKl+SUNVcL6j6W79CuSIPfi6HnDUmSCH6gyUys/HrqHeA/wU0Az41rRgean494d0Jb+ww== @@ -18110,6 +18110,14 @@ posthog-node@1.3.0: remove-trailing-slash "^0.1.1" uuid "^8.3.2" +posthog-node@4.0.1: + version "4.0.1" + resolved "https://registry.yarnpkg.com/posthog-node/-/posthog-node-4.0.1.tgz#eb8b6cdf68c3fdd0dc2b75e8aab2e0ec3727fb2a" + integrity sha512-rtqm2h22QxLGBrW2bLYzbRhliIrqgZ0k+gF0LkQ1SNdeD06YE5eilV0MxZppFSxC8TfH0+B0cWCuebEnreIDgQ== + dependencies: + axios "^1.6.2" + rusha "^0.8.14" + pouch-stream@^0.4.0: version "0.4.1" resolved "https://registry.yarnpkg.com/pouch-stream/-/pouch-stream-0.4.1.tgz#0c6d8475c9307677627991a2f079b301c3b89bdd" @@ -19574,6 +19582,11 @@ run-parallel@^1.1.9: dependencies: queue-microtask "^1.2.2" +rusha@^0.8.14: + version "0.8.14" + resolved "https://registry.yarnpkg.com/rusha/-/rusha-0.8.14.tgz#a977d0de9428406138b7bb90d3de5dcd024e2f68" + integrity sha512-cLgakCUf6PedEu15t8kbsjnwIFFR2D4RfL+W3iWFJ4iac7z4B0ZI8fxy4R3J956kAI68HclCFGL8MPoUVC3qVA== + rxjs@^6.6.6: version "6.6.7" resolved "https://registry.yarnpkg.com/rxjs/-/rxjs-6.6.7.tgz#90ac018acabf491bf65044235d5863c4dab804c9" From a9b4d0017f7ab418646c57a3039cad835ba1668d Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 7 Aug 2024 16:59:33 +0100 Subject: [PATCH 02/18] add tests for posthog feature flags --- packages/backend-core/src/features/index.ts | 51 ++++++- .../src/features/tests/features.spec.ts | 130 ++++++++++++------ 2 files changed, 135 insertions(+), 46 deletions(-) diff --git a/packages/backend-core/src/features/index.ts b/packages/backend-core/src/features/index.ts index 7a77eb8b56..4a29dc6daf 100644 --- a/packages/backend-core/src/features/index.ts +++ b/packages/backend-core/src/features/index.ts @@ -1,14 +1,15 @@ import env from "../environment" import * as context from "../context" import { cloneDeep } from "lodash" -import { PostHog } from "posthog-node" +import { PostHog, PostHogOptions } from "posthog-node" import { IdentityType } from "@budibase/types" let posthog: PostHog | undefined -export function init() { +export function init(opts?: PostHogOptions) { if (env.POSTHOG_TOKEN) { posthog = new PostHog(env.POSTHOG_TOKEN, { host: "https://us.i.posthog.com", + ...opts, }) } } @@ -18,6 +19,14 @@ abstract class Flag { return new BooleanFlag(defaultValue) } + static string(defaultValue: string): Flag { + return new StringFlag(defaultValue) + } + + static number(defaultValue: number): Flag { + return new NumberFlag(defaultValue) + } + protected constructor(public defaultValue: T) {} abstract parse(value: any): T @@ -37,6 +46,32 @@ class BooleanFlag extends Flag { } } +class StringFlag extends Flag { + parse(value: any) { + if (typeof value === "string") { + return value + } + throw new Error(`could not parse value "${value}" as string`) + } +} + +class NumberFlag extends Flag { + parse(value: any) { + if (typeof value === "number") { + return value + } + + if (typeof value === "string") { + const parsed = parseFloat(value) + if (!isNaN(parsed)) { + return parsed + } + } + + throw new Error(`could not parse value "${value}" as number`) + } +} + // This is the primary source of truth for feature flags. If you want to add a // new flag, add it here and use the `fetch` and `get` functions to access it. // All of the machinery in this file is to make sure that flags have their @@ -46,6 +81,10 @@ const FLAGS = { GOOGLE_SHEETS: Flag.boolean(false), USER_GROUPS: Flag.boolean(false), ONBOARDING_TOUR: Flag.boolean(false), + + _TEST_BOOLEAN: Flag.boolean(false), + _TEST_STRING: Flag.string("default value"), + _TEST_NUMBER: Flag.number(0), } const DEFAULTS = Object.keys(FLAGS).reduce((acc, key) => { @@ -107,6 +146,7 @@ export async function fetch(): Promise { throw new Error(`Feature: ${feature} is not a boolean`) } + // @ts-ignore flags[feature] = value } } @@ -114,7 +154,7 @@ export async function fetch(): Promise { const identity = context.getIdentity() if (posthog && identity?.type === IdentityType.USER) { const posthogFlags = await posthog.getAllFlagsAndPayloads(identity._id) - for (const [name, value] of Object.entries(posthogFlags)) { + for (const [name, value] of Object.entries(posthogFlags.featureFlags)) { const key = name as keyof typeof FLAGS const flag = FLAGS[key] if (!flag) { @@ -124,8 +164,11 @@ export async function fetch(): Promise { continue } + const payload = posthogFlags.featureFlagPayloads?.[name] + try { - flags[key] = flag.parse(value) + // @ts-ignore + flags[key] = flag.parse(payload || value) } catch (err) { // We don't want an invalid PostHog flag to break the app, so we just // log it and continue. diff --git a/packages/backend-core/src/features/tests/features.spec.ts b/packages/backend-core/src/features/tests/features.spec.ts index 9022d568ee..de40e24b3c 100644 --- a/packages/backend-core/src/features/tests/features.spec.ts +++ b/packages/backend-core/src/features/tests/features.spec.ts @@ -2,6 +2,7 @@ import { IdentityContext, IdentityType } from "@budibase/types" import { defaultFlags, fetch, get, Flags, init } from "../" import { context } from "../.." import { setEnv, withEnv } from "../../environment" +import nodeFetch from "node-fetch" import nock from "nock" describe("feature flags", () => { @@ -14,23 +15,23 @@ describe("feature flags", () => { it.each([ { tenant: "tenant1", - flags: "tenant1:ONBOARDING_TOUR", - expected: { ONBOARDING_TOUR: true }, + flags: "tenant1:_TEST_BOOLEAN", + expected: { _TEST_BOOLEAN: true }, }, { tenant: "tenant1", - flags: "tenant1:!ONBOARDING_TOUR", - expected: { ONBOARDING_TOUR: false }, + flags: "tenant1:!_TEST_BOOLEAN", + expected: { _TEST_BOOLEAN: false }, }, { tenant: "tenant1", - flags: "*:ONBOARDING_TOUR", - expected: { ONBOARDING_TOUR: true }, + flags: "*:_TEST_BOOLEAN", + expected: { _TEST_BOOLEAN: true }, }, { tenant: "tenant1", - flags: "tenant2:ONBOARDING_TOUR", - expected: { ONBOARDING_TOUR: false }, + flags: "tenant2:_TEST_BOOLEAN", + expected: { _TEST_BOOLEAN: false }, }, { tenant: "tenant1", @@ -62,7 +63,7 @@ describe("feature flags", () => { it.each([ { tenant: "tenant1", - flags: "tenant1:ONBOARDING_TOUR,tenant1:FOO", + flags: "tenant1:_TEST_BOOLEAN,tenant1:FOO", expected: "Feature: FOO is not an allowed option", }, ])( @@ -75,43 +76,88 @@ describe("feature flags", () => { ) ) - // describe("posthog", () => { - // const identity: IdentityContext = { - // _id: "us_1234", - // tenantId: "budibase", - // type: IdentityType.USER, - // email: "test@example.com", - // firstName: "Test", - // lastName: "User", - // } + describe("posthog", () => { + const identity: IdentityContext = { + _id: "us_1234", + tenantId: "budibase", + type: IdentityType.USER, + email: "test@example.com", + firstName: "Test", + lastName: "User", + } - // let cleanup: () => void + let cleanup: () => void - // beforeAll(() => { - // cleanup = setEnv({ POSTHOG_TOKEN: "test" }) - // init() - // }) + beforeAll(() => { + cleanup = setEnv({ POSTHOG_TOKEN: "test" }) + }) - // afterAll(() => { - // cleanup() - // }) + afterAll(() => { + cleanup() + }) - // beforeEach(() => { - // nock.cleanAll() - // }) + beforeEach(() => { + nock.cleanAll() - // it("should be able to read flags from posthog", () => - // context.doInIdentityContext(identity, async () => { - // nock("https://app.posthog.com") - // .get("/api/feature_flags/tenant/budibase") - // .reply(200, { - // flags: { - // "budibase:onboardingTour": true, - // }, - // }) + // We need to pass in node-fetch here otherwise nock won't get used + // because posthog-node uses axios under the hood. + init({ fetch: nodeFetch }) + }) - // const flags = await fetch() - // expect(flags.ONBOARDING_TOUR).toBe(true) - // })) - // }) + function mockFlags(flags: { + featureFlags?: Record + featureFlagPayloads?: Record + }) { + nock("https://us.i.posthog.com") + .post("/decide/?v=3", body => { + return body.token === "test" && body.distinct_id === "us_1234" + }) + .reply(200, flags) + } + + it("should be able to read flags from posthog", async () => { + mockFlags({ + featureFlags: { + _TEST_BOOLEAN: true, + }, + }) + + await context.doInIdentityContext(identity, async () => { + const flags = await fetch() + expect(flags._TEST_BOOLEAN).toBe(true) + }) + }) + + it("should be able to read flags from posthog with payloads", async () => { + mockFlags({ + featureFlags: { + _TEST_STRING: true, + }, + featureFlagPayloads: { + _TEST_STRING: "test payload", + }, + }) + + await context.doInIdentityContext(identity, async () => { + const flags = await fetch() + expect(flags._TEST_STRING).toBe("test payload") + }) + }) + + it("should be able to read flags from posthog with numbers", async () => { + mockFlags({ + featureFlags: { + _TEST_NUMBER: true, + }, + featureFlagPayloads: { + _TEST_NUMBER: 123, + }, + }) + + await context.doInIdentityContext(identity, async () => { + const flags = await fetch() + expect(flags._TEST_NUMBER).toBe(123) + }) + }) + }) }) From d7e07bb44aaca857383e913e156a3254948c5978 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 7 Aug 2024 17:33:47 +0100 Subject: [PATCH 03/18] Fix worker tests. --- .../src/features/tests/features.spec.ts | 14 ++++++- .../middleware/passport/sso/tests/sso.spec.ts | 2 +- .../src/api/routes/global/tests/auth.spec.ts | 29 +++++++------ .../api/routes/global/tests/realEmail.spec.ts | 42 +++++++++++++------ packages/worker/src/tests/api/configs.ts | 2 +- packages/worker/src/tests/jestSetup.ts | 16 +++++-- 6 files changed, 70 insertions(+), 35 deletions(-) diff --git a/packages/backend-core/src/features/tests/features.spec.ts b/packages/backend-core/src/features/tests/features.spec.ts index de40e24b3c..25e9765287 100644 --- a/packages/backend-core/src/features/tests/features.spec.ts +++ b/packages/backend-core/src/features/tests/features.spec.ts @@ -71,7 +71,7 @@ describe("feature flags", () => { ({ tenant, flags, expected }) => context.doInTenant(tenant, () => withEnv({ TENANT_FEATURE_FLAGS: flags }, () => - expect(fetch()).rejects.toThrow(expected) + expect(fetch).rejects.toThrow(expected) ) ) ) @@ -159,5 +159,17 @@ describe("feature flags", () => { expect(flags._TEST_NUMBER).toBe(123) }) }) + + it("should not fail when a flag is not known", async () => { + mockFlags({ + featureFlags: { + _SOME_RANDOM_FLAG: true, + }, + }) + + await context.doInIdentityContext(identity, async () => { + await expect(fetch()).resolves.not.toThrow() + }) + }) }) }) diff --git a/packages/backend-core/src/middleware/passport/sso/tests/sso.spec.ts b/packages/backend-core/src/middleware/passport/sso/tests/sso.spec.ts index 9fa82b6594..62b71055d9 100644 --- a/packages/backend-core/src/middleware/passport/sso/tests/sso.spec.ts +++ b/packages/backend-core/src/middleware/passport/sso/tests/sso.spec.ts @@ -1,4 +1,4 @@ -import { structures, mocks } from "../../../../../tests" +import { structures } from "../../../../../tests" import { testEnv } from "../../../../../tests/extra" import { SSOAuthDetails, User } from "@budibase/types" diff --git a/packages/worker/src/api/routes/global/tests/auth.spec.ts b/packages/worker/src/api/routes/global/tests/auth.spec.ts index 3f2b3045de..9a004497c7 100644 --- a/packages/worker/src/api/routes/global/tests/auth.spec.ts +++ b/packages/worker/src/api/routes/global/tests/auth.spec.ts @@ -292,9 +292,9 @@ describe("/api/global/auth", () => { it("redirects to auth provider", async () => { nock("http://someconfigurl").get("/").times(1).reply(200, { issuer: "test", - authorization_endpoint: "http://localhost/auth", - token_endpoint: "http://localhost/token", - userinfo_endpoint: "http://localhost/userinfo", + authorization_endpoint: "http://example.com/auth", + token_endpoint: "http://example.com/token", + userinfo_endpoint: "http://example.com/userinfo", }) const configId = await generateOidcConfig() @@ -305,7 +305,7 @@ describe("/api/global/auth", () => { const location: string = res.get("location") expect( location.startsWith( - `http://localhost/auth?response_type=code&client_id=clientId&redirect_uri=http%3A%2F%2Flocalhost%3A10000%2Fapi%2Fglobal%2Fauth%2F${config.tenantId}%2Foidc%2Fcallback&scope=openid%20profile%20email%20offline_access` + `http://example.com/auth?response_type=code&client_id=clientId&redirect_uri=http%3A%2F%2Flocalhost%3A10000%2Fapi%2Fglobal%2Fauth%2F${config.tenantId}%2Foidc%2Fcallback&scope=openid%20profile%20email%20offline_access` ) ).toBe(true) }) @@ -313,11 +313,13 @@ describe("/api/global/auth", () => { describe("GET /api/global/auth/:tenantId/oidc/callback", () => { it("logs in", async () => { + const email = `${generator.guid()}@example.com` + nock("http://someconfigurl").get("/").times(2).reply(200, { issuer: "test", - authorization_endpoint: "http://localhost/auth", - token_endpoint: "http://localhost/token", - userinfo_endpoint: "http://localhost/userinfo", + authorization_endpoint: "http://example.com/auth", + token_endpoint: "http://example.com/token", + userinfo_endpoint: "http://example.com/userinfo", }) const token = jwt.sign( @@ -326,20 +328,20 @@ describe("/api/global/auth", () => { sub: "sub", aud: "clientId", exp: Math.floor(Date.now() / 1000) + 60 * 60, - email: "oauth@example.com", + email, }, "secret" ) - nock("http://localhost").post("/token").reply(200, { + nock("http://example.com").post("/token").reply(200, { access_token: "access", refresh_token: "refresh", id_token: token, }) - nock("http://localhost").get("/userinfo?schema=openid").reply(200, { + nock("http://example.com").get("/userinfo?schema=openid").reply(200, { sub: "sub", - email: "oauth@example.com", + email, }) const configId = await generateOidcConfig() @@ -351,10 +353,7 @@ describe("/api/global/auth", () => { ) } - expect(events.auth.login).toHaveBeenCalledWith( - "oidc", - "oauth@example.com" - ) + expect(events.auth.login).toHaveBeenCalledWith("oidc", email) expect(events.auth.login).toHaveBeenCalledTimes(1) expect(res.status).toBe(302) const location: string = res.get("location") diff --git a/packages/worker/src/api/routes/global/tests/realEmail.spec.ts b/packages/worker/src/api/routes/global/tests/realEmail.spec.ts index bea627d5b6..a18d8ee247 100644 --- a/packages/worker/src/api/routes/global/tests/realEmail.spec.ts +++ b/packages/worker/src/api/routes/global/tests/realEmail.spec.ts @@ -12,6 +12,33 @@ const nodemailer = require("nodemailer") // for the real email tests give them a long time to try complete/fail jest.setTimeout(30000) +function cancelableTimeout(timeout: number): [Promise, () => void] { + let timeoutId: NodeJS.Timeout + return [ + new Promise((resolve, reject) => { + timeoutId = setTimeout(() => { + reject({ + status: 301, + errno: "ETIME", + }) + }, timeout) + }), + () => { + clearTimeout(timeoutId) + }, + ] +} + +async function withTimeout( + timeout: number, + promise: Promise +): Promise { + const [timeoutPromise, cancel] = cancelableTimeout(timeout) + const result = (await Promise.race([promise, timeoutPromise])) as T + cancel() + return result +} + describe("/api/global/email", () => { const config = new TestConfiguration() @@ -30,19 +57,8 @@ describe("/api/global/email", () => { ) { let response, text try { - const timeout = () => - new Promise((resolve, reject) => - setTimeout( - () => - reject({ - status: 301, - errno: "ETIME", - }), - 20000 - ) - ) - await Promise.race([config.saveEtherealSmtpConfig(), timeout()]) - await Promise.race([config.saveSettingsConfig(), timeout()]) + await withTimeout(20000, config.saveEtherealSmtpConfig()) + await withTimeout(20000, config.saveSettingsConfig()) let res if (attachments) { res = await config.api.emails diff --git a/packages/worker/src/tests/api/configs.ts b/packages/worker/src/tests/api/configs.ts index 74cef2bf8b..86fe0830ca 100644 --- a/packages/worker/src/tests/api/configs.ts +++ b/packages/worker/src/tests/api/configs.ts @@ -40,7 +40,7 @@ export class ConfigAPI extends TestAPI { const sessionContent = JSON.parse( Buffer.from(koaSession, "base64").toString("utf-8") ) - const handle = sessionContent["openidconnect:localhost"].state.handle + const handle = sessionContent["openidconnect:example.com"].state.handle return this.request .get(`/api/global/auth/${this.config.getTenantId()}/oidc/callback`) .query({ code: "test", state: handle }) diff --git a/packages/worker/src/tests/jestSetup.ts b/packages/worker/src/tests/jestSetup.ts index 0f933bd562..6a98031d34 100644 --- a/packages/worker/src/tests/jestSetup.ts +++ b/packages/worker/src/tests/jestSetup.ts @@ -1,13 +1,21 @@ import { mocks, testContainerUtils } from "@budibase/backend-core/tests" import env from "../environment" import { env as coreEnv, timers } from "@budibase/backend-core" - -// must explicitly enable fetch mock -mocks.fetch.enable() +import nock from "nock" // mock all dates to 2020-01-01T00:00:00.000Z // use tk.reset() to use real dates in individual tests -const tk = require("timekeeper") +import tk from "timekeeper" + +nock.disableNetConnect() +nock.enableNetConnect(host => { + return ( + host.includes("localhost") || + host.includes("127.0.0.1") || + host.includes("::1") || + host.includes("ethereal.email") // used in realEmail.spec.ts + ) +}) tk.freeze(mocks.date.MOCK_DATE) From 88031094968764be853f5d13617d9a5cd3966faf Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 7 Aug 2024 17:50:39 +0100 Subject: [PATCH 04/18] Add DataDog tracing to feature flags. --- packages/backend-core/src/features/index.ts | 116 ++++++++++-------- .../src/features/tests/features.spec.ts | 38 +++++- 2 files changed, 105 insertions(+), 49 deletions(-) diff --git a/packages/backend-core/src/features/index.ts b/packages/backend-core/src/features/index.ts index 4a29dc6daf..2ffa35974f 100644 --- a/packages/backend-core/src/features/index.ts +++ b/packages/backend-core/src/features/index.ts @@ -3,6 +3,7 @@ import * as context from "../context" import { cloneDeep } from "lodash" import { PostHog, PostHogOptions } from "posthog-node" import { IdentityType } from "@budibase/types" +import tracer from "dd-trace" let posthog: PostHog | undefined export function init(opts?: PostHogOptions) { @@ -119,65 +120,84 @@ function isFlagName(name: string): name is keyof Flags { * they will be accessed through this function as well. */ export async function fetch(): Promise { - const flags = defaultFlags() + return await tracer.trace("features.fetch", async span => { + const tags: Record = {} - const currentTenantId = context.getTenantId() + const flags = defaultFlags() - const split = (env.TENANT_FEATURE_FLAGS || "") - .split(",") - .map(x => x.split(":")) - for (const [tenantId, ...features] of split) { - if (!tenantId || (tenantId !== "*" && tenantId !== currentTenantId)) { - continue - } + const currentTenantId = context.getTenantId() + const specificallySetFalse = new Set() - for (let feature of features) { - let value = true - if (feature.startsWith("!")) { - feature = feature.slice(1) - value = false - } - - if (!isFlagName(feature)) { - throw new Error(`Feature: ${feature} is not an allowed option`) - } - - if (typeof flags[feature] !== "boolean") { - throw new Error(`Feature: ${feature} is not a boolean`) - } - - // @ts-ignore - flags[feature] = value - } - } - - const identity = context.getIdentity() - if (posthog && identity?.type === IdentityType.USER) { - const posthogFlags = await posthog.getAllFlagsAndPayloads(identity._id) - for (const [name, value] of Object.entries(posthogFlags.featureFlags)) { - const key = name as keyof typeof FLAGS - const flag = FLAGS[key] - if (!flag) { - // We don't want an unexpected PostHog flag to break the app, so we - // just log it and continue. - console.warn(`Unexpected posthog flag "${name}": ${value}`) + const split = (env.TENANT_FEATURE_FLAGS || "") + .split(",") + .map(x => x.split(":")) + for (const [tenantId, ...features] of split) { + if (!tenantId || (tenantId !== "*" && tenantId !== currentTenantId)) { continue } - const payload = posthogFlags.featureFlagPayloads?.[name] + for (let feature of features) { + let value = true + if (feature.startsWith("!")) { + feature = feature.slice(1) + value = false + specificallySetFalse.add(feature) + } + + if (!isFlagName(feature)) { + throw new Error(`Feature: ${feature} is not an allowed option`) + } + + if (typeof flags[feature] !== "boolean") { + throw new Error(`Feature: ${feature} is not a boolean`) + } - try { // @ts-ignore - flags[key] = flag.parse(payload || value) - } catch (err) { - // We don't want an invalid PostHog flag to break the app, so we just - // log it and continue. - console.warn(`Error parsing posthog flag "${name}": ${value}`, err) + flags[feature] = value + tags[`flags.${feature}.source`] = "environment" } } - } - return flags + const identity = context.getIdentity() + if (posthog && identity?.type === IdentityType.USER) { + const posthogFlags = await posthog.getAllFlagsAndPayloads(identity._id) + for (const [name, value] of Object.entries(posthogFlags.featureFlags)) { + const key = name as keyof typeof FLAGS + const flag = FLAGS[key] + if (!flag) { + // We don't want an unexpected PostHog flag to break the app, so we + // just log it and continue. + console.warn(`Unexpected posthog flag "${name}": ${value}`) + continue + } + + if (flags[key] === true || specificallySetFalse.has(key)) { + // If the flag is already set to through environment variables, we + // don't want to override it back to false here. + continue + } + + const payload = posthogFlags.featureFlagPayloads?.[name] + + try { + // @ts-ignore + flags[key] = flag.parse(payload || value) + tags[`flags.${key}.source`] = "posthog" + } catch (err) { + // We don't want an invalid PostHog flag to break the app, so we just + // log it and continue. + console.warn(`Error parsing posthog flag "${name}": ${value}`, err) + } + } + } + + for (const [key, value] of Object.entries(flags)) { + tags[`flags.${key}.value`] = value + } + span?.addTags(tags) + + return flags + }) } // Gets a single feature flag value. This is a convenience function for diff --git a/packages/backend-core/src/features/tests/features.spec.ts b/packages/backend-core/src/features/tests/features.spec.ts index 25e9765287..1b85a74024 100644 --- a/packages/backend-core/src/features/tests/features.spec.ts +++ b/packages/backend-core/src/features/tests/features.spec.ts @@ -79,7 +79,7 @@ describe("feature flags", () => { describe("posthog", () => { const identity: IdentityContext = { _id: "us_1234", - tenantId: "budibase", + tenantId: "tenant1", type: IdentityType.USER, email: "test@example.com", firstName: "Test", @@ -171,5 +171,41 @@ describe("feature flags", () => { await expect(fetch()).resolves.not.toThrow() }) }) + + it("should not override flags set in the environment", async () => { + mockFlags({ + featureFlags: { + _TEST_BOOLEAN: false, + }, + }) + + await withEnv( + { TENANT_FEATURE_FLAGS: `${identity.tenantId}:_TEST_BOOLEAN` }, + async () => { + await context.doInIdentityContext(identity, async () => { + const flags = await fetch() + expect(flags._TEST_BOOLEAN).toBe(true) + }) + } + ) + }) + + it("should not override flags set in the environment with a ! prefix", async () => { + mockFlags({ + featureFlags: { + _TEST_BOOLEAN: true, + }, + }) + + await withEnv( + { TENANT_FEATURE_FLAGS: `${identity.tenantId}:!_TEST_BOOLEAN` }, + async () => { + await context.doInIdentityContext(identity, async () => { + const flags = await fetch() + expect(flags._TEST_BOOLEAN).toBe(false) + }) + } + ) + }) }) }) From 9733ba5f95eb49c55be6dbbc36020b74d6d0e505 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 7 Aug 2024 18:04:07 +0100 Subject: [PATCH 05/18] Allowing incorrectly setup column schemas to still function as part of search - requires further investigation as to how this happens, but search should still work. --- .../src/api/controllers/row/utils/basic.ts | 3 +- .../src/api/routes/tests/search.spec.ts | 29 +++++++++++++++++++ .../server/src/integrations/utils/utils.ts | 7 +++-- .../src/sdk/app/rows/search/internal/sqs.ts | 5 ++-- 4 files changed, 38 insertions(+), 6 deletions(-) diff --git a/packages/server/src/api/controllers/row/utils/basic.ts b/packages/server/src/api/controllers/row/utils/basic.ts index 883ba5a806..f28f650422 100644 --- a/packages/server/src/api/controllers/row/utils/basic.ts +++ b/packages/server/src/api/controllers/row/utils/basic.ts @@ -71,8 +71,7 @@ export function basicProcessing({ }): Row { const thisRow: Row = {} // filter the row down to what is actually the row (not joined) - for (let field of Object.values(table.schema)) { - const fieldName = field.name + for (let fieldName of Object.keys(table.schema)) { let value = extractFieldValue({ row, tableName: table.name, diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index d8c2a4a257..6967f7bd6e 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -2353,6 +2353,35 @@ describe.each([ }) }) + describe("Invalid column definitions", () => { + beforeAll(async () => { + // need to create an invalid table - means ignoring typescript + table = await createTable({ + // @ts-ignore + invalid: { + type: FieldType.STRING, + }, + name: { + name: "name", + type: FieldType.STRING, + }, + }) + await createRows([ + { name: "foo", invalid: "id1" }, + { name: "bar", invalid: "id2" }, + ]) + }) + + it("can get rows with all table data", async () => { + await expectSearch({ + query: {}, + }).toContain([ + { name: "foo", invalid: "id1" }, + { name: "bar", invalid: "id2" }, + ]) + }) + }) + describe.each(["data_name_test", "name_data_test", "name_test_data_"])( "special (%s) case", column => { diff --git a/packages/server/src/integrations/utils/utils.ts b/packages/server/src/integrations/utils/utils.ts index 84742626c1..6d0e4b838f 100644 --- a/packages/server/src/integrations/utils/utils.ts +++ b/packages/server/src/integrations/utils/utils.ts @@ -15,6 +15,7 @@ import { helpers, utils } from "@budibase/shared-core" import { pipeline } from "stream/promises" import tmp from "tmp" import fs from "fs" +import { merge } from "lodash" type PrimitiveTypes = | FieldType.STRING @@ -291,10 +292,12 @@ function copyExistingPropsOver( const fetchedColumnDefinition: FieldSchema | undefined = table.schema[key] table.schema[key] = { - ...existingTableSchema[key], + // merge the properties - anything missing will be filled in, old definition preferred + ...merge(fetchedColumnDefinition, existingTableSchema[key]), + // always take externalType and autocolumn from the fetched definition externalType: existingTableSchema[key].externalType || - table.schema[key]?.externalType, + fetchedColumnDefinition?.externalType, autocolumn: fetchedColumnDefinition?.autocolumn, } as FieldSchema // check constraints which can be fetched from the DB (they could be updated) 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 66ec905c61..4bfa8f8fa5 100644 --- a/packages/server/src/sdk/app/rows/search/internal/sqs.ts +++ b/packages/server/src/sdk/app/rows/search/internal/sqs.ts @@ -73,13 +73,14 @@ function buildInternalFieldList( fieldList = fieldList.concat( PROTECTED_INTERNAL_COLUMNS.map(col => `${table._id}.${col}`) ) - for (let col of Object.values(table.schema)) { + for (let key of Object.keys(table.schema)) { + const col = table.schema[key] const isRelationship = col.type === FieldType.LINK if (!opts?.relationships && isRelationship) { continue } if (!isRelationship) { - fieldList.push(`${table._id}.${mapToUserColumn(col.name)}`) + fieldList.push(`${table._id}.${mapToUserColumn(key)}`) } else { const linkCol = col as RelationshipFieldMetadata const relatedTable = tables.find(table => table._id === linkCol.tableId) From efafb3e3c22b59e4127d49f2befe6ef278857ebe Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 8 Aug 2024 18:55:41 +0100 Subject: [PATCH 06/18] Getting composite keys working, fixing p2 issue and adding test case for it. --- packages/backend-core/src/sql/sqlTable.ts | 21 ++++++++--- .../api/controllers/row/ExternalRequest.ts | 4 +- .../src/api/routes/tests/search.spec.ts | 37 +++++++++++++++++++ packages/server/src/utilities/schema.ts | 3 +- 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/packages/backend-core/src/sql/sqlTable.ts b/packages/backend-core/src/sql/sqlTable.ts index 02acc8af85..55a6bfcaeb 100644 --- a/packages/backend-core/src/sql/sqlTable.ts +++ b/packages/backend-core/src/sql/sqlTable.ts @@ -28,16 +28,25 @@ function generateSchema( oldTable: null | Table = null, renamed?: RenameColumn ) { - let primaryKey = table && table.primary ? table.primary[0] : null + let primaryKeys = table && table.primary ? table.primary : [] const columns = Object.values(table.schema) // all columns in a junction table will be meta let metaCols = columns.filter(col => (col as NumberFieldMetadata).meta) let isJunction = metaCols.length === columns.length + let columnTypeSet: string[] = [] + // can't change primary once its set for now - if (primaryKey && !oldTable && !isJunction) { - schema.increments(primaryKey).primary() - } else if (!oldTable && isJunction) { - schema.primary(metaCols.map(col => col.name)) + if (!oldTable) { + // junction tables are special - we have an expected format + if (isJunction) { + schema.primary(metaCols.map(col => col.name)) + } else if (primaryKeys.length === 1) { + schema.increments(primaryKeys[0]).primary() + // note that we've set its type + columnTypeSet.push(primaryKeys[0]) + } else { + schema.primary(primaryKeys) + } } // check if any columns need added @@ -49,7 +58,7 @@ function generateSchema( const oldColumn = oldTable ? oldTable.schema[key] : null if ( (oldColumn && oldColumn.type) || - (primaryKey === key && !isJunction) || + columnTypeSet.includes(key) || renamed?.updated === key ) { continue diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 6538e7347a..14dd909edd 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -1,5 +1,6 @@ import dayjs from "dayjs" import { + ArrayOperator, AutoFieldSubType, AutoReason, Datasource, @@ -196,11 +197,12 @@ export class ExternalRequest { // need to map over the filters and make sure the _id field isn't present let prefix = 1 for (const operator of Object.values(filters)) { + const isArrayOperator = Object.values(ArrayOperator).includes(operator) for (const field of Object.keys(operator || {})) { if (dbCore.removeKeyNumbering(field) === "_id") { if (primary) { const parts = breakRowIdField(operator[field]) - if (primary.length > 1) { + if (primary.length > 1 && isArrayOperator) { operator[InternalSearchFilterOperator.COMPLEX_ID_OPERATOR] = { id: primary, values: parts[0], diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 6967f7bd6e..d0cc8848ae 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -39,6 +39,7 @@ import { dataFilters } from "@budibase/shared-core" import { Knex } from "knex" import { structures } from "@budibase/backend-core/tests" import { DEFAULT_EMPLOYEE_TABLE_SCHEMA } from "../../../db/defaultData/datasource_bb_default" +import { generateRowIdField } from "../../../integrations/utils" describe.each([ ["in-memory", undefined], @@ -2648,6 +2649,42 @@ describe.each([ }) }) + !isInternal && + describe("search by composite key", () => { + beforeAll(async () => { + table = await config.api.table.save( + tableForDatasource(datasource, { + schema: { + idColumn1: { + name: "idColumn1", + type: FieldType.NUMBER, + }, + idColumn2: { + name: "idColumn2", + type: FieldType.NUMBER, + }, + }, + primary: ["idColumn1", "idColumn2"], + }) + ) + await createRows([{ idColumn1: 1, idColumn2: 2 }]) + }) + + it("can filter by the row ID with limit 1", async () => { + await expectSearch({ + query: { + equal: { _id: generateRowIdField([1, 2]) }, + }, + limit: 1, + }).toContain([ + { + idColumn1: 1, + idColumn2: 2, + }, + ]) + }) + }) + isSql && describe("pagination edge case with relationships", () => { let mainRows: Row[] = [] diff --git a/packages/server/src/utilities/schema.ts b/packages/server/src/utilities/schema.ts index b398285710..b1fbd7577a 100644 --- a/packages/server/src/utilities/schema.ts +++ b/packages/server/src/utilities/schema.ts @@ -146,7 +146,8 @@ export function parse(rows: Rows, table: Table): Rows { return rows.map(row => { const parsedRow: Row = {} - Object.entries(row).forEach(([columnName, columnData]) => { + Object.keys(row).forEach(columnName => { + const columnData = row[columnName] const schema = table.schema if (!(columnName in schema)) { // Objects can be present in the row data but not in the schema, so make sure we don't proceed in such a case From 3d590e879e7abcc8c6eaf72ad044db00260ffe8b Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 9 Aug 2024 10:30:27 +0100 Subject: [PATCH 07/18] Rely on withEnv and setEnv functions in respective environment.ts files. --- packages/backend-core/src/index.ts | 2 +- .../{apikeys.spec.js => apikeys.spec.ts} | 15 ++--- .../src/api/routes/tests/application.spec.ts | 9 ++- .../src/api/routes/tests/attachment.spec.ts | 5 +- .../server/src/api/routes/tests/row.spec.ts | 7 ++- .../src/api/routes/tests/search.spec.ts | 8 ++- .../tests/{static.spec.js => static.spec.ts} | 25 ++++---- .../src/api/routes/tests/templates.spec.ts | 3 +- .../src/api/routes/tests/viewV2.spec.ts | 14 +++-- .../src/api/routes/tests/webhook.spec.ts | 3 +- .../tests/20240604153647_initial_sqs.spec.ts | 10 +++- .../src/automations/tests/openai.spec.ts | 5 +- packages/server/src/environment.ts | 27 +++++++++ .../integrations/tests/googlesheets.spec.ts | 3 +- .../sdk/app/rows/search/tests/search.spec.ts | 11 ++-- .../server/src/startup/tests/startup.spec.ts | 12 +++- .../src/tests/utilities/TestConfiguration.ts | 59 ------------------- 17 files changed, 109 insertions(+), 109 deletions(-) rename packages/server/src/api/routes/tests/{apikeys.spec.js => apikeys.spec.ts} (75%) rename packages/server/src/api/routes/tests/{static.spec.js => static.spec.ts} (88%) diff --git a/packages/backend-core/src/index.ts b/packages/backend-core/src/index.ts index a14a344655..dbdce51c50 100644 --- a/packages/backend-core/src/index.ts +++ b/packages/backend-core/src/index.ts @@ -27,7 +27,7 @@ export * as locks from "./redis/redlockImpl" export * as utils from "./utils" export * as errors from "./errors" export * as timers from "./timers" -export { default as env } from "./environment" +export { default as env, withEnv, setEnv } from "./environment" export * as blacklist from "./blacklist" export * as docUpdates from "./docUpdates" export * from "./utils/Duration" diff --git a/packages/server/src/api/routes/tests/apikeys.spec.js b/packages/server/src/api/routes/tests/apikeys.spec.ts similarity index 75% rename from packages/server/src/api/routes/tests/apikeys.spec.js rename to packages/server/src/api/routes/tests/apikeys.spec.ts index 678da38f28..30f87fcfec 100644 --- a/packages/server/src/api/routes/tests/apikeys.spec.js +++ b/packages/server/src/api/routes/tests/apikeys.spec.ts @@ -1,11 +1,12 @@ -const setup = require("./utilities") -const { checkBuilderEndpoint } = require("./utilities/TestFunctions") +import { withEnv } from "../../../environment" +import { getRequest, getConfig, afterAll as _afterAll } from "./utilities" +import { checkBuilderEndpoint } from "./utilities/TestFunctions" describe("/api/keys", () => { - let request = setup.getRequest() - let config = setup.getConfig() + let request = getRequest() + let config = getConfig() - afterAll(setup.afterAll) + afterAll(_afterAll) beforeAll(async () => { await config.init() @@ -13,7 +14,7 @@ describe("/api/keys", () => { describe("fetch", () => { it("should allow fetching", async () => { - await config.withEnv({ SELF_HOSTED: "true" }, async () => { + await withEnv({ SELF_HOSTED: "true" }, async () => { const res = await request .get(`/api/keys`) .set(config.defaultHeaders()) @@ -34,7 +35,7 @@ describe("/api/keys", () => { describe("update", () => { it("should allow updating a value", async () => { - await config.withEnv({ SELF_HOSTED: "true" }, async () => { + await withEnv({ SELF_HOSTED: "true" }, async () => { const res = await request .put(`/api/keys/TEST`) .send({ diff --git a/packages/server/src/api/routes/tests/application.spec.ts b/packages/server/src/api/routes/tests/application.spec.ts index 6ae598ee84..a8e060a2b5 100644 --- a/packages/server/src/api/routes/tests/application.spec.ts +++ b/packages/server/src/api/routes/tests/application.spec.ts @@ -14,7 +14,12 @@ jest.mock("../../../utilities/redis", () => ({ import { checkBuilderEndpoint } from "./utilities/TestFunctions" import * as setup from "./utilities" import { AppStatus } from "../../../db/utils" -import { events, utils, context } from "@budibase/backend-core" +import { + events, + utils, + context, + withEnv as withCoreEnv, +} from "@budibase/backend-core" import env from "../../../environment" import { type App } from "@budibase/types" import tk from "timekeeper" @@ -353,7 +358,7 @@ describe("/applications", () => { .delete(`/api/global/roles/${prodAppId}`) .reply(200, {}) - await config.withCoreEnv({ SQS_SEARCH_ENABLE: "true" }, async () => { + await withCoreEnv({ SQS_SEARCH_ENABLE: "true" }, async () => { await config.api.application.delete(app.appId) }) }) diff --git a/packages/server/src/api/routes/tests/attachment.spec.ts b/packages/server/src/api/routes/tests/attachment.spec.ts index 7ff4e67bcc..53e0a1b15d 100644 --- a/packages/server/src/api/routes/tests/attachment.spec.ts +++ b/packages/server/src/api/routes/tests/attachment.spec.ts @@ -1,3 +1,4 @@ +import { withEnv } from "../../../environment" import * as setup from "./utilities" import { APIError } from "@budibase/types" @@ -28,7 +29,7 @@ describe("/api/applications/:appId/sync", () => { }) it("should reject an upload with a malicious file extension", async () => { - await config.withEnv({ SELF_HOSTED: undefined }, async () => { + await withEnv({ SELF_HOSTED: undefined }, async () => { let resp = (await config.api.attachment.process( "ohno.exe", Buffer.from([0]), @@ -39,7 +40,7 @@ describe("/api/applications/:appId/sync", () => { }) it("should reject an upload with a malicious uppercase file extension", async () => { - await config.withEnv({ SELF_HOSTED: undefined }, async () => { + await withEnv({ SELF_HOSTED: undefined }, async () => { let resp = (await config.api.attachment.process( "OHNO.EXE", Buffer.from([0]), diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index cf94eb9f13..9351976d66 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -40,6 +40,7 @@ import _, { merge } from "lodash" import * as uuid from "uuid" import { Knex } from "knex" import { InternalTables } from "../../../db/utils" +import { withEnv } from "../../../environment" const timestamp = new Date("2023-01-26T11:48:57.597Z").toISOString() tk.freeze(timestamp) @@ -1688,7 +1689,7 @@ describe.each([ } const row = await config.api.row.save(testTable._id!, draftRow) - await config.withEnv({ SELF_HOSTED: "true" }, async () => { + await withEnv({ SELF_HOSTED: "true" }, async () => { return context.doInAppContext(config.getAppId(), async () => { const enriched: Row[] = await outputProcessing(table, [row]) const [targetRow] = enriched @@ -2456,7 +2457,7 @@ describe.each([ describe("Formula JS protection", () => { it("should time out JS execution if a single cell takes too long", async () => { - await config.withEnv({ JS_PER_INVOCATION_TIMEOUT_MS: 40 }, async () => { + await withEnv({ JS_PER_INVOCATION_TIMEOUT_MS: 40 }, async () => { const js = Buffer.from( ` let i = 0; @@ -2494,7 +2495,7 @@ describe.each([ }) it("should time out JS execution if a multiple cells take too long", async () => { - await config.withEnv( + await withEnv( { JS_PER_INVOCATION_TIMEOUT_MS: 40, JS_PER_REQUEST_TIMEOUT_MS: 80, diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index d8c2a4a257..5c0a04d64f 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -11,6 +11,8 @@ import { MIN_VALID_DATE, SQLITE_DESIGN_DOC_ID, utils, + withEnv as withCoreEnv, + setEnv as setCoreEnv, } from "@budibase/backend-core" import * as setup from "./utilities" @@ -64,9 +66,9 @@ describe.each([ let rows: Row[] beforeAll(async () => { - await config.withCoreEnv({ SQS_SEARCH_ENABLE: "true" }, () => config.init()) + await withCoreEnv({ SQS_SEARCH_ENABLE: "true" }, () => config.init()) if (isSqs) { - envCleanup = config.setCoreEnv({ + envCleanup = setCoreEnv({ SQS_SEARCH_ENABLE: "true", SQS_SEARCH_ENABLE_TENANTS: [config.getTenantId()], }) @@ -2668,7 +2670,7 @@ describe.each([ }) it("can still page when the hard limit is hit", async () => { - await config.withCoreEnv( + await withCoreEnv( { SQL_MAX_ROWS: "6", }, diff --git a/packages/server/src/api/routes/tests/static.spec.js b/packages/server/src/api/routes/tests/static.spec.ts similarity index 88% rename from packages/server/src/api/routes/tests/static.spec.js rename to packages/server/src/api/routes/tests/static.spec.ts index 1358b5418a..62b72b2b8f 100644 --- a/packages/server/src/api/routes/tests/static.spec.js +++ b/packages/server/src/api/routes/tests/static.spec.ts @@ -8,23 +8,24 @@ jest.mock("aws-sdk", () => ({ })), })) -const setup = require("./utilities") -const { constants } = require("@budibase/backend-core") +import { Datasource, SourceName } from "@budibase/types" +import { setEnv } from "../../../environment" +import { getRequest, getConfig, afterAll as _afterAll } from "./utilities" +import { constants } from "@budibase/backend-core" describe("/static", () => { - let request = setup.getRequest() - let config = setup.getConfig() - let app - let cleanupEnv + let request = getRequest() + let config = getConfig() + let cleanupEnv: () => void afterAll(() => { - setup.afterAll() + _afterAll() cleanupEnv() }) beforeAll(async () => { - cleanupEnv = config.setEnv({ SELF_HOSTED: "true" }) - app = await config.init() + cleanupEnv = setEnv({ SELF_HOSTED: "true" }) + await config.init() }) describe("/app", () => { @@ -49,7 +50,7 @@ describe("/static", () => { delete headers[constants.Header.APP_ID] const res = await request - .get(`/app${config.prodApp.url}`) + .get(`/app${config.getProdApp().url}`) .set(headers) .expect(200) @@ -68,14 +69,14 @@ describe("/static", () => { describe("/attachments", () => { describe("generateSignedUrls", () => { - let datasource + let datasource: Datasource beforeEach(async () => { datasource = await config.createDatasource({ datasource: { type: "datasource", name: "Test", - source: "S3", + source: SourceName.S3, config: {}, }, }) diff --git a/packages/server/src/api/routes/tests/templates.spec.ts b/packages/server/src/api/routes/tests/templates.spec.ts index 81c615487d..98d375dec6 100644 --- a/packages/server/src/api/routes/tests/templates.spec.ts +++ b/packages/server/src/api/routes/tests/templates.spec.ts @@ -2,6 +2,7 @@ import * as setup from "./utilities" import path from "path" import nock from "nock" import { generator } from "@budibase/backend-core/tests" +import { withEnv as withCoreEnv } from "@budibase/backend-core" interface App { background: string @@ -89,7 +90,7 @@ describe("/templates", () => { SQS_SEARCH_ENABLE_TENANTS: [config.getTenantId()], } - await config.withCoreEnv(env, async () => { + await withCoreEnv(env, async () => { const name = generator.guid().replaceAll("-", "") const url = `/${name}` diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index ea6aedbe3c..e18cffeaa8 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -24,7 +24,12 @@ import { generator, mocks } from "@budibase/backend-core/tests" import { DatabaseName, getDatasource } from "../../../integrations/tests/utils" import merge from "lodash/merge" import { quotas } from "@budibase/pro" -import { db, roles } from "@budibase/backend-core" +import { + db, + roles, + withEnv as withCoreEnv, + setEnv as setCoreEnv, +} from "@budibase/backend-core" describe.each([ ["lucene", undefined], @@ -89,12 +94,11 @@ describe.each([ } beforeAll(async () => { - await config.withCoreEnv( - { SQS_SEARCH_ENABLE: isSqs ? "true" : "false" }, - () => config.init() + await withCoreEnv({ SQS_SEARCH_ENABLE: isSqs ? "true" : "false" }, () => + config.init() ) if (isSqs) { - envCleanup = config.setCoreEnv({ + envCleanup = setCoreEnv({ SQS_SEARCH_ENABLE: "true", SQS_SEARCH_ENABLE_TENANTS: [config.getTenantId()], }) diff --git a/packages/server/src/api/routes/tests/webhook.spec.ts b/packages/server/src/api/routes/tests/webhook.spec.ts index b741ce0820..9c40a14f98 100644 --- a/packages/server/src/api/routes/tests/webhook.spec.ts +++ b/packages/server/src/api/routes/tests/webhook.spec.ts @@ -2,6 +2,7 @@ import { Webhook } from "@budibase/types" import * as setup from "./utilities" import { checkBuilderEndpoint } from "./utilities/TestFunctions" import { mocks } from "@budibase/backend-core/tests" +import { setEnv } from "../../../environment" const { basicWebhook, basicAutomation, collectAutomation } = setup.structures @@ -17,7 +18,7 @@ describe("/webhooks", () => { }) const setupTest = async () => { - cleanupEnv = config.setEnv({ SELF_HOSTED: "true" }) + cleanupEnv = setEnv({ SELF_HOSTED: "true" }) await config.init() const autoConfig = basicAutomation() autoConfig.definition.trigger.schema = { diff --git a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts index f337958a5b..5f036bb87d 100644 --- a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts +++ b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts @@ -1,6 +1,10 @@ import * as setup from "../../../api/routes/tests/utilities" import { basicTable } from "../../../tests/utilities/structures" -import { db as dbCore, SQLITE_DESIGN_DOC_ID } from "@budibase/backend-core" +import { + db as dbCore, + SQLITE_DESIGN_DOC_ID, + withEnv as withCoreEnv, +} from "@budibase/backend-core" import { LinkDocument, DocumentType, @@ -69,11 +73,11 @@ function oldLinkDocument(): Omit { type SQSEnvVar = "SQS_MIGRATION_ENABLE" | "SQS_SEARCH_ENABLE" async function sqsDisabled(envVar: SQSEnvVar, cb: () => Promise) { - await config.withCoreEnv({ [envVar]: "", SQS_SEARCH_ENABLE_TENANTS: [] }, cb) + await withCoreEnv({ [envVar]: "", SQS_SEARCH_ENABLE_TENANTS: [] }, cb) } async function sqsEnabled(envVar: SQSEnvVar, cb: () => Promise) { - await config.withCoreEnv( + await withCoreEnv( { [envVar]: "1", SQS_SEARCH_ENABLE_TENANTS: [config.getTenantId()] }, cb ) diff --git a/packages/server/src/automations/tests/openai.spec.ts b/packages/server/src/automations/tests/openai.spec.ts index 6bebe16a49..41456f116d 100644 --- a/packages/server/src/automations/tests/openai.spec.ts +++ b/packages/server/src/automations/tests/openai.spec.ts @@ -1,5 +1,6 @@ import { getConfig, runStep, afterAll as _afterAll } from "./utilities" import { OpenAI } from "openai" +import { withEnv as withCoreEnv, setEnv as setCoreEnv } from "@budibase/backend-core" jest.mock("openai", () => ({ OpenAI: jest.fn().mockImplementation(() => ({ @@ -32,7 +33,7 @@ describe("test the openai action", () => { }) beforeEach(() => { - resetEnv = config.setCoreEnv({ OPENAI_API_KEY: "abc123" }) + resetEnv = setCoreEnv({ OPENAI_API_KEY: "abc123" }) }) afterEach(() => { @@ -42,7 +43,7 @@ describe("test the openai action", () => { afterAll(_afterAll) it("should present the correct error message when the OPENAI_API_KEY variable isn't set", async () => { - await config.withCoreEnv({ OPENAI_API_KEY: "" }, async () => { + await withCoreEnv({ OPENAI_API_KEY: "" }, async () => { let res = await runStep("OPENAI", { prompt: OPENAI_PROMPT }) expect(res.response).toEqual( "OpenAI API Key not configured - please add the OPENAI_API_KEY environment variable." diff --git a/packages/server/src/environment.ts b/packages/server/src/environment.ts index e1525ebd1b..05ebbe3151 100644 --- a/packages/server/src/environment.ts +++ b/packages/server/src/environment.ts @@ -1,5 +1,6 @@ import { env as coreEnv } from "@budibase/backend-core" import { ServiceType } from "@budibase/types" +import cloneDeep from "lodash/cloneDeep" coreEnv._set("SERVICE_TYPE", ServiceType.APPS) import { join } from "path" @@ -133,6 +134,32 @@ const environment = { }, } +export function setEnv(newEnvVars: Partial): () => void { + const oldEnv = cloneDeep(environment) + + let key: keyof typeof newEnvVars + for (key in newEnvVars) { + environment._set(key, newEnvVars[key]) + } + + return () => { + for (const [key, value] of Object.entries(oldEnv)) { + environment._set(key, value) + } + } +} + +export function withEnv(envVars: Partial, f: () => T) { + const cleanup = setEnv(envVars) + const result = f() + if (result instanceof Promise) { + return result.finally(cleanup) + } else { + cleanup() + return result + } +} + function cleanVariables() { // clean up any environment variable edge cases for (let [key, value] of Object.entries(environment)) { diff --git a/packages/server/src/integrations/tests/googlesheets.spec.ts b/packages/server/src/integrations/tests/googlesheets.spec.ts index 9b1ea815f5..e9d5290b2c 100644 --- a/packages/server/src/integrations/tests/googlesheets.spec.ts +++ b/packages/server/src/integrations/tests/googlesheets.spec.ts @@ -1,3 +1,4 @@ +import { setEnv as setCoreEnv } from "@budibase/backend-core" import type { GoogleSpreadsheetWorksheet } from "google-spreadsheet" import nock from "nock" @@ -40,7 +41,7 @@ describe("Google Sheets Integration", () => { let cleanupEnv: () => void beforeAll(() => { - cleanupEnv = config.setCoreEnv({ + cleanupEnv = setCoreEnv({ GOOGLE_CLIENT_ID: "test", GOOGLE_CLIENT_SECRET: "test", }) diff --git a/packages/server/src/sdk/app/rows/search/tests/search.spec.ts b/packages/server/src/sdk/app/rows/search/tests/search.spec.ts index 47124d6667..252b9a6556 100644 --- a/packages/server/src/sdk/app/rows/search/tests/search.spec.ts +++ b/packages/server/src/sdk/app/rows/search/tests/search.spec.ts @@ -3,6 +3,10 @@ import { Datasource, FieldType, Row, Table } from "@budibase/types" import TestConfiguration from "../../../../../tests/utilities/TestConfiguration" import { search } from "../../../../../sdk/app/rows/search" import { generator } from "@budibase/backend-core/tests" +import { + withEnv as withCoreEnv, + setEnv as setCoreEnv, +} from "@budibase/backend-core" import { DatabaseName, getDatasource, @@ -31,13 +35,12 @@ describe.each([ let rows: Row[] beforeAll(async () => { - await config.withCoreEnv( - { SQS_SEARCH_ENABLE: isSqs ? "true" : "false" }, - () => config.init() + await withCoreEnv({ SQS_SEARCH_ENABLE: isSqs ? "true" : "false" }, () => + config.init() ) if (isSqs) { - envCleanup = config.setCoreEnv({ + envCleanup = setCoreEnv({ SQS_SEARCH_ENABLE: "true", SQS_SEARCH_ENABLE_TENANTS: [config.getTenantId()], }) diff --git a/packages/server/src/startup/tests/startup.spec.ts b/packages/server/src/startup/tests/startup.spec.ts index fd2e0df61a..7209033db8 100644 --- a/packages/server/src/startup/tests/startup.spec.ts +++ b/packages/server/src/startup/tests/startup.spec.ts @@ -1,6 +1,12 @@ +import { withEnv } from "../../environment" import TestConfiguration from "../../tests/utilities/TestConfiguration" import { startup } from "../index" -import { users, utils, tenancy } from "@budibase/backend-core" +import { + users, + utils, + tenancy, + withEnv as withCoreEnv, +} from "@budibase/backend-core" import nock from "nock" describe("check BB_ADMIN environment variables", () => { @@ -23,13 +29,13 @@ describe("check BB_ADMIN environment variables", () => { const EMAIL = "budibase@budibase.com", PASSWORD = "budibase" await tenancy.doInTenant(tenancy.DEFAULT_TENANT_ID, async () => { - await config.withEnv( + await withEnv( { MULTI_TENANCY: "0", SELF_HOSTED: "1", }, () => - config.withCoreEnv( + withCoreEnv( { BB_ADMIN_USER_EMAIL: EMAIL, BB_ADMIN_USER_PASSWORD: PASSWORD, diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts index 3d53149385..443d3c6bd9 100644 --- a/packages/server/src/tests/utilities/TestConfiguration.ts +++ b/packages/server/src/tests/utilities/TestConfiguration.ts @@ -247,65 +247,6 @@ export default class TestConfiguration { } } - async withEnv(newEnvVars: Partial, f: () => Promise) { - let cleanup = this.setEnv(newEnvVars) - try { - return await f() - } finally { - cleanup() - } - } - - /* - * Sets the environment variables to the given values and returns a function - * that can be called to reset the environment variables to their original values. - */ - setEnv(newEnvVars: Partial): () => void { - const oldEnv = cloneDeep(env) - - let key: keyof typeof newEnvVars - for (key in newEnvVars) { - env._set(key, newEnvVars[key]) - } - - return () => { - for (const [key, value] of Object.entries(oldEnv)) { - env._set(key, value) - } - } - } - - async withCoreEnv( - newEnvVars: Partial, - f: () => Promise - ) { - let cleanup = this.setCoreEnv(newEnvVars) - try { - return await f() - } finally { - cleanup() - } - } - - /* - * Sets the environment variables to the given values and returns a function - * that can be called to reset the environment variables to their original values. - */ - setCoreEnv(newEnvVars: Partial): () => void { - const oldEnv = cloneDeep(coreEnv) - - let key: keyof typeof newEnvVars - for (key in newEnvVars) { - coreEnv._set(key, newEnvVars[key]) - } - - return () => { - for (const [key, value] of Object.entries(oldEnv)) { - coreEnv._set(key, value) - } - } - } - async withUser(user: User, f: () => Promise) { const oldUser = this.user this.user = user From 4887ca261ebd2c2d87e0117349d3ba1ba66dfe2f Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 9 Aug 2024 11:27:43 +0100 Subject: [PATCH 08/18] Improve testing of feature flags by not polluting production flags with test ones. --- packages/backend-core/src/environment.ts | 1 + packages/backend-core/src/features/index.ts | 265 ++++++++---------- .../src/features/tests/features.spec.ts | 87 +++--- .../worker/src/api/controllers/global/self.ts | 2 +- 4 files changed, 173 insertions(+), 182 deletions(-) diff --git a/packages/backend-core/src/environment.ts b/packages/backend-core/src/environment.ts index 5091a4971a..e064398153 100644 --- a/packages/backend-core/src/environment.ts +++ b/packages/backend-core/src/environment.ts @@ -145,6 +145,7 @@ const environment = { COOKIE_DOMAIN: process.env.COOKIE_DOMAIN, PLATFORM_URL: process.env.PLATFORM_URL || "", POSTHOG_TOKEN: process.env.POSTHOG_TOKEN, + POSTHOG_API_HOST: process.env.POSTHOG_API_HOST || "https://us.i.posthog.com", ENABLE_ANALYTICS: process.env.ENABLE_ANALYTICS, TENANT_FEATURE_FLAGS: process.env.TENANT_FEATURE_FLAGS, CLOUDFRONT_CDN: process.env.CLOUDFRONT_CDN, diff --git a/packages/backend-core/src/features/index.ts b/packages/backend-core/src/features/index.ts index 2ffa35974f..3f0a6cf6b0 100644 --- a/packages/backend-core/src/features/index.ts +++ b/packages/backend-core/src/features/index.ts @@ -1,6 +1,5 @@ import env from "../environment" import * as context from "../context" -import { cloneDeep } from "lodash" import { PostHog, PostHogOptions } from "posthog-node" import { IdentityType } from "@budibase/types" import tracer from "dd-trace" @@ -9,13 +8,13 @@ let posthog: PostHog | undefined export function init(opts?: PostHogOptions) { if (env.POSTHOG_TOKEN) { posthog = new PostHog(env.POSTHOG_TOKEN, { - host: "https://us.i.posthog.com", + host: env.POSTHOG_API_HOST, ...opts, }) } } -abstract class Flag { +export abstract class Flag { static boolean(defaultValue: boolean): Flag { return new BooleanFlag(defaultValue) } @@ -33,6 +32,16 @@ abstract class Flag { abstract parse(value: any): T } +type UnwrapFlag = F extends Flag ? U : never + +export type FlagValues = { + [K in keyof T]: UnwrapFlag +} + +type KeysOfType = { + [K in keyof T]: T[K] extends Flag ? K : never +}[keyof T] + class BooleanFlag extends Flag { parse(value: any) { if (typeof value === "string") { @@ -73,149 +82,123 @@ class NumberFlag extends Flag { } } +export class FlagSet, T extends { [key: string]: V }> { + private readonly flags: T + + constructor(flags: T) { + this.flags = flags + } + + defaults(): FlagValues { + return Object.keys(this.flags).reduce((acc, key) => { + const typedKey = key as keyof T + acc[typedKey] = this.flags[key].defaultValue + return acc + }, {} as FlagValues) + } + + isFlagName(name: string | number | symbol): name is keyof T { + return this.flags[name as keyof T] !== undefined + } + + async get(key: K): Promise[K]> { + const flags = await this.fetch() + return flags[key] + } + + async isEnabled>(key: K): Promise { + const flags = await this.fetch() + return flags[key] + } + + async fetch(): Promise> { + return await tracer.trace("features.fetch", async span => { + const tags: Record = {} + + const flags = this.defaults() + + const currentTenantId = context.getTenantId() + const specificallySetFalse = new Set() + + const split = (env.TENANT_FEATURE_FLAGS || "") + .split(",") + .map(x => x.split(":")) + for (const [tenantId, ...features] of split) { + if (!tenantId || (tenantId !== "*" && tenantId !== currentTenantId)) { + continue + } + + for (let feature of features) { + let value = true + if (feature.startsWith("!")) { + feature = feature.slice(1) + value = false + specificallySetFalse.add(feature) + } + + if (!this.isFlagName(feature)) { + throw new Error(`Feature: ${feature} is not an allowed option`) + } + + if (typeof flags[feature] !== "boolean") { + throw new Error(`Feature: ${feature} is not a boolean`) + } + + // @ts-ignore + flags[feature] = value + tags[`flags.${feature}.source`] = "environment" + } + } + + const identity = context.getIdentity() + if (posthog && identity?.type === IdentityType.USER) { + const posthogFlags = await posthog.getAllFlagsAndPayloads(identity._id) + for (const [name, value] of Object.entries(posthogFlags.featureFlags)) { + const flag = this.flags[name] + if (!flag) { + // We don't want an unexpected PostHog flag to break the app, so we + // just log it and continue. + console.warn(`Unexpected posthog flag "${name}": ${value}`) + continue + } + + if (flags[name] === true || specificallySetFalse.has(name)) { + // If the flag is already set to through environment variables, we + // don't want to override it back to false here. + continue + } + + const payload = posthogFlags.featureFlagPayloads?.[name] + + try { + // @ts-ignore + flags[name] = flag.parse(payload || value) + tags[`flags.${name}.source`] = "posthog" + } catch (err) { + // We don't want an invalid PostHog flag to break the app, so we just + // log it and continue. + console.warn(`Error parsing posthog flag "${name}": ${value}`, err) + } + } + } + + for (const [key, value] of Object.entries(flags)) { + tags[`flags.${key}.value`] = value + } + span?.addTags(tags) + + return flags + }) + } +} + // This is the primary source of truth for feature flags. If you want to add a // new flag, add it here and use the `fetch` and `get` functions to access it. // All of the machinery in this file is to make sure that flags have their // default values set correctly and their types flow through the system. -const FLAGS = { +export const flags = new FlagSet({ LICENSING: Flag.boolean(false), GOOGLE_SHEETS: Flag.boolean(false), USER_GROUPS: Flag.boolean(false), ONBOARDING_TOUR: Flag.boolean(false), - - _TEST_BOOLEAN: Flag.boolean(false), - _TEST_STRING: Flag.string("default value"), - _TEST_NUMBER: Flag.number(0), -} - -const DEFAULTS = Object.keys(FLAGS).reduce((acc, key) => { - const typedKey = key as keyof typeof FLAGS - // @ts-ignore - acc[typedKey] = FLAGS[typedKey].defaultValue - return acc -}, {} as Flags) - -type UnwrapFlag = F extends Flag ? U : never -export type Flags = { - [K in keyof typeof FLAGS]: UnwrapFlag<(typeof FLAGS)[K]> -} - -// Exported for use in tests, should not be used outside of this file. -export function defaultFlags(): Flags { - return cloneDeep(DEFAULTS) -} - -function isFlagName(name: string): name is keyof Flags { - return FLAGS[name as keyof typeof FLAGS] !== undefined -} - -/** - * Reads the TENANT_FEATURE_FLAGS environment variable and returns a Flags object - * populated with the flags for the current tenant, filling in the default values - * if the flag is not set. - * - * Check the tests for examples of how TENANT_FEATURE_FLAGS should be formatted. - * - * In future we plan to add more ways of setting feature flags, e.g. PostHog, and - * they will be accessed through this function as well. - */ -export async function fetch(): Promise { - return await tracer.trace("features.fetch", async span => { - const tags: Record = {} - - const flags = defaultFlags() - - const currentTenantId = context.getTenantId() - const specificallySetFalse = new Set() - - const split = (env.TENANT_FEATURE_FLAGS || "") - .split(",") - .map(x => x.split(":")) - for (const [tenantId, ...features] of split) { - if (!tenantId || (tenantId !== "*" && tenantId !== currentTenantId)) { - continue - } - - for (let feature of features) { - let value = true - if (feature.startsWith("!")) { - feature = feature.slice(1) - value = false - specificallySetFalse.add(feature) - } - - if (!isFlagName(feature)) { - throw new Error(`Feature: ${feature} is not an allowed option`) - } - - if (typeof flags[feature] !== "boolean") { - throw new Error(`Feature: ${feature} is not a boolean`) - } - - // @ts-ignore - flags[feature] = value - tags[`flags.${feature}.source`] = "environment" - } - } - - const identity = context.getIdentity() - if (posthog && identity?.type === IdentityType.USER) { - const posthogFlags = await posthog.getAllFlagsAndPayloads(identity._id) - for (const [name, value] of Object.entries(posthogFlags.featureFlags)) { - const key = name as keyof typeof FLAGS - const flag = FLAGS[key] - if (!flag) { - // We don't want an unexpected PostHog flag to break the app, so we - // just log it and continue. - console.warn(`Unexpected posthog flag "${name}": ${value}`) - continue - } - - if (flags[key] === true || specificallySetFalse.has(key)) { - // If the flag is already set to through environment variables, we - // don't want to override it back to false here. - continue - } - - const payload = posthogFlags.featureFlagPayloads?.[name] - - try { - // @ts-ignore - flags[key] = flag.parse(payload || value) - tags[`flags.${key}.source`] = "posthog" - } catch (err) { - // We don't want an invalid PostHog flag to break the app, so we just - // log it and continue. - console.warn(`Error parsing posthog flag "${name}": ${value}`, err) - } - } - } - - for (const [key, value] of Object.entries(flags)) { - tags[`flags.${key}.value`] = value - } - span?.addTags(tags) - - return flags - }) -} - -// Gets a single feature flag value. This is a convenience function for -// `fetch().then(flags => flags[name])`. -export async function get(name: K): Promise { - const flags = await fetch() - return flags[name] -} - -type BooleanFlags = { - [K in keyof typeof FLAGS]: (typeof FLAGS)[K] extends Flag ? K : never -}[keyof typeof FLAGS] - -// Convenience function for boolean flag values. This makes callsites more -// readable for boolean flags. -export async function isEnabled( - name: K -): Promise { - const flags = await fetch() - return flags[name] -} +}) diff --git a/packages/backend-core/src/features/tests/features.spec.ts b/packages/backend-core/src/features/tests/features.spec.ts index 1b85a74024..95819831b2 100644 --- a/packages/backend-core/src/features/tests/features.spec.ts +++ b/packages/backend-core/src/features/tests/features.spec.ts @@ -1,53 +1,60 @@ import { IdentityContext, IdentityType } from "@budibase/types" -import { defaultFlags, fetch, get, Flags, init } from "../" +import { Flag, FlagSet, FlagValues, init } from "../" import { context } from "../.." import { setEnv, withEnv } from "../../environment" import nodeFetch from "node-fetch" import nock from "nock" +const schema = { + TEST_BOOLEAN: Flag.boolean(false), + TEST_STRING: Flag.string("default value"), + TEST_NUMBER: Flag.number(0), +} +const flags = new FlagSet(schema) + describe("feature flags", () => { interface TestCase { tenant: string flags: string - expected: Partial + expected: Partial> } it.each([ { tenant: "tenant1", - flags: "tenant1:_TEST_BOOLEAN", - expected: { _TEST_BOOLEAN: true }, + flags: "tenant1:TEST_BOOLEAN", + expected: { TEST_BOOLEAN: true }, }, { tenant: "tenant1", - flags: "tenant1:!_TEST_BOOLEAN", - expected: { _TEST_BOOLEAN: false }, + flags: "tenant1:!TEST_BOOLEAN", + expected: { TEST_BOOLEAN: false }, }, { tenant: "tenant1", - flags: "*:_TEST_BOOLEAN", - expected: { _TEST_BOOLEAN: true }, + flags: "*:TEST_BOOLEAN", + expected: { TEST_BOOLEAN: true }, }, { tenant: "tenant1", - flags: "tenant2:_TEST_BOOLEAN", - expected: { _TEST_BOOLEAN: false }, + flags: "tenant2:TEST_BOOLEAN", + expected: { TEST_BOOLEAN: false }, }, { tenant: "tenant1", flags: "", - expected: defaultFlags(), + expected: flags.defaults(), }, ])( 'should find flags $expected for $tenant with string "$flags"', - ({ tenant, flags, expected }) => + ({ tenant, flags: envFlags, expected }) => context.doInTenant(tenant, async () => - withEnv({ TENANT_FEATURE_FLAGS: flags }, async () => { - const flags = await fetch() - expect(flags).toMatchObject(expected) + withEnv({ TENANT_FEATURE_FLAGS: envFlags }, async () => { + const values = await flags.fetch() + expect(values).toMatchObject(expected) for (const [key, expectedValue] of Object.entries(expected)) { - const value = await get(key as keyof Flags) + const value = await flags.get(key as keyof typeof schema) expect(value).toBe(expectedValue) } }) @@ -63,15 +70,15 @@ describe("feature flags", () => { it.each([ { tenant: "tenant1", - flags: "tenant1:_TEST_BOOLEAN,tenant1:FOO", + flags: "tenant1:TEST_BOOLEAN,tenant1:FOO", expected: "Feature: FOO is not an allowed option", }, ])( "should fail with message \"$expected\" for $tenant with string '$flags'", - ({ tenant, flags, expected }) => + ({ tenant, flags: envFlags, expected }) => context.doInTenant(tenant, () => - withEnv({ TENANT_FEATURE_FLAGS: flags }, () => - expect(fetch).rejects.toThrow(expected) + withEnv({ TENANT_FEATURE_FLAGS: envFlags }, () => + expect(flags.fetch()).rejects.toThrow(expected) ) ) ) @@ -118,45 +125,45 @@ describe("feature flags", () => { it("should be able to read flags from posthog", async () => { mockFlags({ featureFlags: { - _TEST_BOOLEAN: true, + TEST_BOOLEAN: true, }, }) await context.doInIdentityContext(identity, async () => { - const flags = await fetch() - expect(flags._TEST_BOOLEAN).toBe(true) + const values = await flags.fetch() + expect(values.TEST_BOOLEAN).toBe(true) }) }) it("should be able to read flags from posthog with payloads", async () => { mockFlags({ featureFlags: { - _TEST_STRING: true, + TEST_STRING: true, }, featureFlagPayloads: { - _TEST_STRING: "test payload", + TEST_STRING: "test payload", }, }) await context.doInIdentityContext(identity, async () => { - const flags = await fetch() - expect(flags._TEST_STRING).toBe("test payload") + const values = await flags.fetch() + expect(values.TEST_STRING).toBe("test payload") }) }) it("should be able to read flags from posthog with numbers", async () => { mockFlags({ featureFlags: { - _TEST_NUMBER: true, + TEST_NUMBER: true, }, featureFlagPayloads: { - _TEST_NUMBER: 123, + TEST_NUMBER: 123, }, }) await context.doInIdentityContext(identity, async () => { - const flags = await fetch() - expect(flags._TEST_NUMBER).toBe(123) + const values = await flags.fetch() + expect(values.TEST_NUMBER).toBe(123) }) }) @@ -168,23 +175,23 @@ describe("feature flags", () => { }) await context.doInIdentityContext(identity, async () => { - await expect(fetch()).resolves.not.toThrow() + await expect(flags.fetch()).resolves.not.toThrow() }) }) it("should not override flags set in the environment", async () => { mockFlags({ featureFlags: { - _TEST_BOOLEAN: false, + TEST_BOOLEAN: false, }, }) await withEnv( - { TENANT_FEATURE_FLAGS: `${identity.tenantId}:_TEST_BOOLEAN` }, + { TENANT_FEATURE_FLAGS: `${identity.tenantId}:TEST_BOOLEAN` }, async () => { await context.doInIdentityContext(identity, async () => { - const flags = await fetch() - expect(flags._TEST_BOOLEAN).toBe(true) + const values = await flags.fetch() + expect(values.TEST_BOOLEAN).toBe(true) }) } ) @@ -193,16 +200,16 @@ describe("feature flags", () => { it("should not override flags set in the environment with a ! prefix", async () => { mockFlags({ featureFlags: { - _TEST_BOOLEAN: true, + TEST_BOOLEAN: true, }, }) await withEnv( - { TENANT_FEATURE_FLAGS: `${identity.tenantId}:!_TEST_BOOLEAN` }, + { TENANT_FEATURE_FLAGS: `${identity.tenantId}:!TEST_BOOLEAN` }, async () => { await context.doInIdentityContext(identity, async () => { - const flags = await fetch() - expect(flags._TEST_BOOLEAN).toBe(false) + const values = await flags.fetch() + expect(values.TEST_BOOLEAN).toBe(false) }) } ) diff --git a/packages/worker/src/api/controllers/global/self.ts b/packages/worker/src/api/controllers/global/self.ts index ec154adf7f..14a2ce8c73 100644 --- a/packages/worker/src/api/controllers/global/self.ts +++ b/packages/worker/src/api/controllers/global/self.ts @@ -104,7 +104,7 @@ export async function getSelf(ctx: any) { ctx.body = await groups.enrichUserRolesFromGroups(user) // add the feature flags for this tenant - const flags = await features.fetch() + const flags = await features.flags.fetch() ctx.body.flags = flags addSessionAttributesToUser(ctx) From 7505d60888946b5bbaa36020e83016bcb806cffa Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 9 Aug 2024 11:31:05 +0100 Subject: [PATCH 09/18] Extract out processor and spy variables to reduce repetition. --- .../posthog/tests/PosthogProcessor.spec.ts | 27 +++++-------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/packages/backend-core/src/events/processors/posthog/tests/PosthogProcessor.spec.ts b/packages/backend-core/src/events/processors/posthog/tests/PosthogProcessor.spec.ts index 1d9e341ffe..1486753e4f 100644 --- a/packages/backend-core/src/events/processors/posthog/tests/PosthogProcessor.spec.ts +++ b/packages/backend-core/src/events/processors/posthog/tests/PosthogProcessor.spec.ts @@ -16,6 +16,9 @@ const newIdentity = () => { } describe("PosthogProcessor", () => { + let processor: PosthogProcessor + let spy: jest.SpyInstance + beforeAll(() => { testEnv.singleTenant() }) @@ -25,25 +28,21 @@ describe("PosthogProcessor", () => { await cache.bustCache( `${CacheKey.EVENTS_RATE_LIMIT}:${Event.SERVED_BUILDER}` ) + + processor = new PosthogProcessor("test") + spy = jest.spyOn(processor.posthog, "capture") }) describe("processEvent", () => { it("processes event", async () => { - const processor = new PosthogProcessor("test") - const spy = jest.spyOn(processor.posthog, "capture") - const identity = newIdentity() const properties = {} await processor.processEvent(Event.APP_CREATED, identity, properties) - expect(spy).toHaveBeenCalledTimes(1) }) it("honours exclusions", async () => { - const processor = new PosthogProcessor("test") - const spy = jest.spyOn(processor.posthog, "capture") - const identity = newIdentity() const properties = {} @@ -52,9 +51,6 @@ describe("PosthogProcessor", () => { }) it("removes audited information", async () => { - const processor = new PosthogProcessor("test") - const spy = jest.spyOn(processor.posthog, "capture") - const identity = newIdentity() const properties = { email: "test", @@ -65,6 +61,7 @@ describe("PosthogProcessor", () => { await processor.processEvent(Event.USER_CREATED, identity, properties) expect(spy).toHaveBeenCalled() + // @ts-ignore const call = processor.posthog.capture.mock.calls[0][0] expect(call.properties.audited).toBeUndefined() @@ -73,9 +70,6 @@ describe("PosthogProcessor", () => { describe("rate limiting", () => { it("sends daily event once in same day", async () => { - const processor = new PosthogProcessor("test") - const spy = jest.spyOn(processor.posthog, "capture") - const identity = newIdentity() const properties = {} @@ -89,8 +83,6 @@ describe("PosthogProcessor", () => { }) it("sends daily event once per unique day", async () => { - const processor = new PosthogProcessor("test") - const spy = jest.spyOn(processor.posthog, "capture") const identity = newIdentity() const properties = {} @@ -110,9 +102,6 @@ describe("PosthogProcessor", () => { }) it("sends event again after cache expires", async () => { - const processor = new PosthogProcessor("test") - const spy = jest.spyOn(processor.posthog, "capture") - const identity = newIdentity() const properties = {} @@ -130,8 +119,6 @@ describe("PosthogProcessor", () => { }) it("sends per app events once per day per app", async () => { - const processor = new PosthogProcessor("test") - const spy = jest.spyOn(processor.posthog, "capture") const identity = newIdentity() const properties = {} From d716d4b4d0d6b434104ddeae407010681b735a3a Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 9 Aug 2024 11:45:55 +0100 Subject: [PATCH 10/18] Fix test failures and lint. --- packages/pro | 2 +- packages/server/src/tests/utilities/TestConfiguration.ts | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/pro b/packages/pro index 62ef0e2d6e..7fe713e51a 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit 62ef0e2d6e83522b6732fb3c61338de303f06ff0 +Subproject commit 7fe713e51afea77c8024579582a4e1a4ec1b55b3 diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts index 443d3c6bd9..d5ed2ce906 100644 --- a/packages/server/src/tests/utilities/TestConfiguration.ts +++ b/packages/server/src/tests/utilities/TestConfiguration.ts @@ -70,7 +70,6 @@ import { } from "@budibase/types" import API from "./api" -import { cloneDeep } from "lodash" import jwt, { Secret } from "jsonwebtoken" import { Server } from "http" From 083b595d50caf1b90d9a8f72e28485fa66a43088 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 9 Aug 2024 11:58:57 +0100 Subject: [PATCH 11/18] Fix formatting. --- packages/server/src/automations/tests/openai.spec.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/server/src/automations/tests/openai.spec.ts b/packages/server/src/automations/tests/openai.spec.ts index 41456f116d..3a5a57475a 100644 --- a/packages/server/src/automations/tests/openai.spec.ts +++ b/packages/server/src/automations/tests/openai.spec.ts @@ -1,6 +1,9 @@ import { getConfig, runStep, afterAll as _afterAll } from "./utilities" import { OpenAI } from "openai" -import { withEnv as withCoreEnv, setEnv as setCoreEnv } from "@budibase/backend-core" +import { + withEnv as withCoreEnv, + setEnv as setCoreEnv, +} from "@budibase/backend-core" jest.mock("openai", () => ({ OpenAI: jest.fn().mockImplementation(() => ({ From 904f0dc9fd506c41db49e22359c88cdd1b972cea Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 9 Aug 2024 13:36:22 +0100 Subject: [PATCH 12/18] Adding jest-extended to allow use of oneOf expect, fixing for updated fetching. --- packages/server/package.json | 1 + .../src/api/routes/tests/datasource.spec.ts | 17 +- packages/server/src/tests/jestSetup.ts | 2 + yarn.lock | 236 ++++++++++-------- 4 files changed, 148 insertions(+), 108 deletions(-) diff --git a/packages/server/package.json b/packages/server/package.json index b835477489..df0ece7bb6 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -144,6 +144,7 @@ "copyfiles": "2.4.1", "docker-compose": "0.23.17", "jest": "29.7.0", + "jest-extended": "^4.0.2", "jest-openapi": "0.14.2", "nock": "13.5.4", "nodemon": "2.0.15", diff --git a/packages/server/src/api/routes/tests/datasource.spec.ts b/packages/server/src/api/routes/tests/datasource.spec.ts index 237133e639..3d98acdf0c 100644 --- a/packages/server/src/api/routes/tests/datasource.spec.ts +++ b/packages/server/src/api/routes/tests/datasource.spec.ts @@ -26,6 +26,14 @@ import { tableForDatasource } from "../../../tests/utilities/structures" import nock from "nock" import { Knex } from "knex" +function fetchedSchemaProps() { + return { + externalType: expect.toBeOneOf([expect.any(String), undefined, null]), + constraints: expect.toBeOneOf([expect.anything(), undefined, null]), + autocolumn: expect.toBeOneOf([expect.anything(), undefined, null]), + } +} + describe("/datasources", () => { const config = setup.getConfig() let datasource: Datasource @@ -380,21 +388,22 @@ describe("/datasources", () => { persisted?.entities && Object.entries(persisted.entities).reduce>( (acc, [tableName, table]) => { - acc[tableName] = { + acc[tableName] = expect.objectContaining({ ...table, primaryDisplay: expect.not.stringMatching( new RegExp(`^${table.primaryDisplay || ""}$`) ), schema: Object.entries(table.schema).reduce( (acc, [fieldName, field]) => { - acc[fieldName] = expect.objectContaining({ + acc[fieldName] = { ...field, - }) + ...fetchedSchemaProps(), + } return acc }, {} ), - } + }) return acc }, {} diff --git a/packages/server/src/tests/jestSetup.ts b/packages/server/src/tests/jestSetup.ts index bc6384e4cd..60cf96cb51 100644 --- a/packages/server/src/tests/jestSetup.ts +++ b/packages/server/src/tests/jestSetup.ts @@ -1,8 +1,10 @@ import env from "../environment" +import * as matchers from "jest-extended" import { env as coreEnv, timers } from "@budibase/backend-core" import { testContainerUtils } from "@budibase/backend-core/tests" import nock from "nock" +expect.extend(matchers) if (!process.env.CI) { // set a longer timeout in dev for debugging 100 seconds jest.setTimeout(100 * 1000) diff --git a/yarn.lock b/yarn.lock index 0195f19a2a..d672fb7643 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2045,44 +2045,6 @@ resolved "https://registry.yarnpkg.com/@bcoe/v8-coverage/-/v8-coverage-0.2.3.tgz#75a2e8b51cb758a7553d6804a5932d7aace75c39" integrity sha512-0hYQ8SB4Db5zvZB4axdMHGwEaQjkZzFjQiN9LVYvIFB2nSUHW9tYpxWriPrWDASIxiaXax83REcLxuSdnGPZtw== -"@budibase/backend-core@2.29.24": - version "0.0.0" - dependencies: - "@budibase/nano" "10.1.5" - "@budibase/pouchdb-replication-stream" "1.2.11" - "@budibase/shared-core" "0.0.0" - "@budibase/types" "0.0.0" - aws-cloudfront-sign "3.0.2" - aws-sdk "2.1030.0" - bcrypt "5.1.0" - bcryptjs "2.4.3" - bull "4.10.1" - correlation-id "4.0.0" - dd-trace "5.2.0" - dotenv "16.0.1" - ioredis "5.3.2" - joi "17.6.0" - jsonwebtoken "9.0.2" - knex "2.4.2" - koa-passport "^6.0.0" - koa-pino-logger "4.0.0" - lodash "4.17.21" - node-fetch "2.6.7" - passport-google-oauth "2.0.0" - passport-local "1.0.0" - passport-oauth2-refresh "^2.1.0" - pino "8.11.0" - pino-http "8.3.3" - posthog-node "1.3.0" - pouchdb "7.3.0" - pouchdb-find "7.2.2" - redlock "4.2.0" - rotating-file-stream "3.1.0" - sanitize-s3-objectkey "0.0.1" - semver "^7.5.4" - tar-fs "2.1.1" - uuid "^8.3.2" - "@budibase/handlebars-helpers@^0.13.2": version "0.13.2" resolved "https://registry.yarnpkg.com/@budibase/handlebars-helpers/-/handlebars-helpers-0.13.2.tgz#73ab51c464e91fd955b429017648e0257060db77" @@ -2125,44 +2087,6 @@ pouchdb-promise "^6.0.4" through2 "^2.0.0" -"@budibase/pro@npm:@budibase/pro@latest": - version "2.29.24" - resolved "https://registry.yarnpkg.com/@budibase/pro/-/pro-2.29.24.tgz#2dbd4c6c0f757aab7e17c413c6d6e4520086f9ac" - integrity sha512-m1v24UD6O21Vbrfsuo5kC5oeg7FzjWO2w8TQMw1VvPKmdIqqclaKDPTPytxwllTMkapMDRNzM5cQzqnQ3yHf6A== - dependencies: - "@budibase/backend-core" "2.29.24" - "@budibase/shared-core" "2.29.24" - "@budibase/string-templates" "2.29.24" - "@budibase/types" "2.29.24" - "@koa/router" "8.0.8" - bull "4.10.1" - joi "17.6.0" - jsonwebtoken "9.0.2" - lru-cache "^7.14.1" - memorystream "^0.3.1" - node-fetch "2.6.7" - scim-patch "^0.8.1" - scim2-parse-filter "^0.2.8" - -"@budibase/shared-core@2.29.24": - version "0.0.0" - dependencies: - "@budibase/types" "0.0.0" - cron-validate "1.4.5" - -"@budibase/string-templates@2.29.24": - version "0.0.0" - dependencies: - "@budibase/handlebars-helpers" "^0.13.2" - dayjs "^1.10.8" - handlebars "^4.7.8" - lodash.clonedeep "^4.5.0" - -"@budibase/types@2.29.24": - version "0.0.0" - dependencies: - scim-patch "^0.8.1" - "@bull-board/api@5.10.2": version "5.10.2" resolved "https://registry.yarnpkg.com/@bull-board/api/-/api-5.10.2.tgz#ae8ff6918b23897bf879a6ead3683f964374c4b3" @@ -7343,7 +7267,30 @@ axios-retry@^3.1.9: "@babel/runtime" "^7.15.4" is-retry-allowed "^2.2.0" -axios@0.24.0, axios@1.1.3, axios@1.6.3, axios@^0.21.1, axios@^1.0.0, axios@^1.1.3, axios@^1.4.0, axios@^1.5.0: +axios@0.24.0: + version "0.24.0" + resolved "https://registry.yarnpkg.com/axios/-/axios-0.24.0.tgz#804e6fa1e4b9c5288501dd9dff56a7a0940d20d6" + integrity sha512-Q6cWsys88HoPgAaFAVUb0WpPk0O8iTeisR9IMqy9G8AbO4NlpVknrnQS03zzF9PGAWgO3cgletO3VjV/P7VztA== + dependencies: + follow-redirects "^1.14.4" + +axios@1.1.3: + version "1.1.3" + resolved "https://registry.yarnpkg.com/axios/-/axios-1.1.3.tgz#8274250dada2edf53814ed7db644b9c2866c1e35" + integrity sha512-00tXVRwKx/FZr/IDVFt4C+f9FYairX517WoGCL6dpOntqLkZofjhu43F/Xl44UOpqa+9sLFDrG/XAnFsUYgkDA== + dependencies: + follow-redirects "^1.15.0" + form-data "^4.0.0" + proxy-from-env "^1.1.0" + +axios@^0.21.1: + version "0.21.4" + resolved "https://registry.yarnpkg.com/axios/-/axios-0.21.4.tgz#c67b90dc0568e5c1cf2b0b858c43ba28e2eda575" + integrity sha512-ut5vewkiu8jjGBdqpM44XxjuCjq9LAKeHVmoVfHVzy8eHgxxq8SbAVQNovDA8mVi05kP0Ea/n/UzcSHcTJQfNg== + dependencies: + follow-redirects "^1.14.0" + +axios@^1.0.0, axios@^1.1.3, axios@^1.4.0, axios@^1.5.0: version "1.6.3" resolved "https://registry.yarnpkg.com/axios/-/axios-1.6.3.tgz#7f50f23b3aa246eff43c54834272346c396613f4" integrity sha512-fWyNdeawGam70jXSVlKl+SUNVcL6j6W79CuSIPfi6HnDUmSCH6gyUys/HrqHeA/wU0Az41rRgean494d0Jb+ww== @@ -11344,7 +11291,7 @@ fn.name@1.x.x: resolved "https://registry.yarnpkg.com/fn.name/-/fn.name-1.1.0.tgz#26cad8017967aea8731bc42961d04a3d5988accc" integrity sha512-GRnmB5gPyJpAhTQdSZTSp9uaPSvl09KoYcMQtsB9rQoOmzs9dH6ffeccH+Z+cv6P68Hu5bC6JjRh4Ah/mHSNRw== -follow-redirects@^1.15.0: +follow-redirects@^1.14.0, follow-redirects@^1.14.4, follow-redirects@^1.15.0: version "1.15.6" resolved "https://registry.yarnpkg.com/follow-redirects/-/follow-redirects-1.15.6.tgz#7f815c0cda4249c74ff09e95ef97c23b5fd0399b" integrity sha512-wWN62YITEaOpSK584EZXJafH1AGpO8RVgElfkuXbTOrPX4fIfOyEpW/CsiNd8JdYrAoOvafRTOEnvsO++qCqFA== @@ -12432,7 +12379,12 @@ http-assert@^1.3.0: deep-equal "~1.0.1" http-errors "~1.8.0" -http-cache-semantics@3.8.1, http-cache-semantics@4.1.1, http-cache-semantics@^4.0.0, http-cache-semantics@^4.1.0, http-cache-semantics@^4.1.1: +http-cache-semantics@3.8.1: + version "3.8.1" + resolved "https://registry.yarnpkg.com/http-cache-semantics/-/http-cache-semantics-3.8.1.tgz#39b0e16add9b605bf0a9ef3d9daaf4843b4cacd2" + integrity sha512-5ai2iksyV8ZXmnZhHH4rWPoxxistEexSi5936zIQ1bnNTW5VnA85B6P/VpXiRM017IgRvb2kKo1a//y+0wSp3w== + +http-cache-semantics@^4.0.0, http-cache-semantics@^4.1.0, http-cache-semantics@^4.1.1: version "4.1.1" resolved "https://registry.yarnpkg.com/http-cache-semantics/-/http-cache-semantics-4.1.1.tgz#abe02fcb2985460bf0323be664436ec3476a6d5a" integrity sha512-er295DKPVsV82j5kw1Gjt+ADA/XYHsajl82cGNQG2eyoPkvgUhX+nDIyelzhIWbbsXP39EHcI6l5tYs2FYqYXQ== @@ -13429,11 +13381,6 @@ isobject@^3.0.1: resolved "https://registry.yarnpkg.com/isobject/-/isobject-3.0.1.tgz#4e431e92b11a9731636aa1f9c8d1ccbcfdab78df" integrity sha512-WhB9zCku7EGTj/HQQRz5aUQEUeoQZH2bWcltRErOpymJ4boYE6wL9Tbr23krRPSZ+C5zqNSrSw+Cc7sZZ4b7vg== -isobject@^4.0.0: - version "4.0.0" - resolved "https://registry.yarnpkg.com/isobject/-/isobject-4.0.0.tgz#3f1c9155e73b192022a80819bacd0343711697b0" - integrity sha512-S/2fF5wH8SJA/kmwr6HYhK/RI/OkhD84k8ntalo0iJjZikgq1XFvR5M8NPT1x5F7fBwCG3qHfnzeP/Vh/ZxCUA== - isolated-vm@^4.7.2: version "4.7.2" resolved "https://registry.yarnpkg.com/isolated-vm/-/isolated-vm-4.7.2.tgz#5670d5cce1d92004f9b825bec5b0b11fc7501b65" @@ -13614,7 +13561,7 @@ jest-config@^29.7.0: slash "^3.0.0" strip-json-comments "^3.1.1" -"jest-diff@>=29.4.3 < 30", jest-diff@^29.4.1, jest-diff@^29.7.0: +"jest-diff@>=29.4.3 < 30", jest-diff@^29.0.0, jest-diff@^29.4.1, jest-diff@^29.7.0: version "29.7.0" resolved "https://registry.yarnpkg.com/jest-diff/-/jest-diff-29.7.0.tgz#017934a66ebb7ecf6f205e84699be10afd70458a" integrity sha512-LMIgiIrhigmPrs03JHpxUh2yISK3vLFPkAodPeo0+BuF7wA2FoQbkEg1u8gBYBThncu7e1oEDUfIXVuTqLRUjw== @@ -13673,12 +13620,20 @@ jest-environment-node@^29.7.0: jest-mock "^29.7.0" jest-util "^29.7.0" +jest-extended@^4.0.2: + version "4.0.2" + resolved "https://registry.yarnpkg.com/jest-extended/-/jest-extended-4.0.2.tgz#d23b52e687cedf66694e6b2d77f65e211e99e021" + integrity sha512-FH7aaPgtGYHc9mRjriS0ZEHYM5/W69tLrFTIdzm+yJgeoCmmrSB/luSfMSqWP9O29QWHPEmJ4qmU6EwsZideog== + dependencies: + jest-diff "^29.0.0" + jest-get-type "^29.0.0" + jest-get-type@^26.3.0: version "26.3.0" resolved "https://registry.yarnpkg.com/jest-get-type/-/jest-get-type-26.3.0.tgz#e97dc3c3f53c2b406ca7afaed4493b1d099199e0" integrity sha512-TpfaviN1R2pQWkIihlfEanwOXK0zcxrKEE4MlU6Tn7keoXdN6/3gK/xl0yEh8DOunn5pOVGKf8hB4R9gVh04ig== -jest-get-type@^29.6.3: +jest-get-type@^29.0.0, jest-get-type@^29.6.3: version "29.6.3" resolved "https://registry.yarnpkg.com/jest-get-type/-/jest-get-type-29.6.3.tgz#36f499fdcea197c1045a127319c0481723908fd1" integrity sha512-zrteXnqYxfQh7l5FHyL38jL39di8H8rHoecLH3JNxH3BwOrBsNeabdap5e0I23lD4HHI8W5VFBZqG4Eaq5LNcw== @@ -15984,7 +15939,7 @@ msgpackr-extract@^3.0.2: "@msgpackr-extract/msgpackr-extract-linux-x64" "3.0.2" "@msgpackr-extract/msgpackr-extract-win32-x64" "3.0.2" -msgpackr@1.10.1, msgpackr@^1.5.2: +msgpackr@^1.5.2: version "1.10.1" resolved "https://registry.yarnpkg.com/msgpackr/-/msgpackr-1.10.1.tgz#51953bb4ce4f3494f0c4af3f484f01cfbb306555" integrity sha512-r5VRLv9qouXuLiIBrLpl2d5ZvPt8svdQTl5/vMvE4nzDMyEX4sgW5yWhuBBj5UmgwOTWj8CIdSXn5sAfsHAWIQ== @@ -16178,13 +16133,25 @@ node-domexception@1.0.0: resolved "https://registry.yarnpkg.com/node-domexception/-/node-domexception-1.0.0.tgz#6888db46a1f71c0b76b3f7555016b63fe64766e5" integrity sha512-/jKZoMpw0F8GRwl4/eLROPA3cfcXtLApP0QzLmUT/HuPCZWyB7IY9ZrMeKw2O/nFIqPQB3PVM9aYm0F312AXDQ== -node-fetch@2.6.0, node-fetch@2.6.7, node-fetch@^2.6.0, node-fetch@^2.6.1, node-fetch@^2.6.7, node-fetch@^2.6.9, node-fetch@^2.7.0: +node-fetch@2.6.0: + version "2.6.0" + resolved "https://registry.yarnpkg.com/node-fetch/-/node-fetch-2.6.0.tgz#e633456386d4aa55863f676a7ab0daa8fdecb0fd" + integrity sha512-8dG4H5ujfvFiqDmVu9fQ5bOHUC15JMjMY/Zumv26oOvvVJjM67KF8koCWIabKQ1GJIa9r2mMZscBq/TbdOcmNA== + +node-fetch@2.6.7, node-fetch@^2.6.0, node-fetch@^2.6.1, node-fetch@^2.6.7: version "2.6.7" resolved "https://registry.yarnpkg.com/node-fetch/-/node-fetch-2.6.7.tgz#24de9fba827e3b4ae44dc8b20256a379160052ad" integrity sha512-ZjMPFEfVx5j+y2yF35Kzx5sF7kDzxuDj6ziH4FFbOp87zKDZNx8yExJIb05OGF4Nlt9IHFIMBkRl41VdvcNdbQ== dependencies: whatwg-url "^5.0.0" +node-fetch@^2.6.9, node-fetch@^2.7.0: + version "2.7.0" + resolved "https://registry.yarnpkg.com/node-fetch/-/node-fetch-2.7.0.tgz#d0f0fa6e3e2dc1d27efcd8ad99d550bda94d187d" + integrity sha512-c4FRfUm/dbcWZ7U+1Wq0AwCyFL+3nt2bEw05wfxSz+DWpWsitgmSgYmy2dQdWyKC1694ELPqMs/YzUSNozLt8A== + dependencies: + whatwg-url "^5.0.0" + node-forge@^1.2.1, node-forge@^1.3.1: version "1.3.1" resolved "https://registry.yarnpkg.com/node-forge/-/node-forge-1.3.1.tgz#be8da2af243b2417d5f646a770663a92b7e9ded3" @@ -17337,7 +17304,15 @@ passport-strategy@1.x.x, passport-strategy@^1.0.0: resolved "https://registry.yarnpkg.com/passport-strategy/-/passport-strategy-1.0.0.tgz#b5539aa8fc225a3d1ad179476ddf236b440f52e4" integrity sha512-CB97UUvDKJde2V0KDWWB3lyf6PC3FaZP7YxZ2G8OAtn9p4HI9j9JLP9qjOGZFvyl8uwNT8qM+hGnz/n16NI7oA== -passport@0.6.0, passport@^0.4.0, passport@^0.6.0: +passport@^0.4.0: + version "0.4.1" + resolved "https://registry.yarnpkg.com/passport/-/passport-0.4.1.tgz#941446a21cb92fc688d97a0861c38ce9f738f270" + integrity sha512-IxXgZZs8d7uFSt3eqNjM9NQ3g3uQCW5avD8mRNoXV99Yig50vjuaez6dQK2qC0kVWPRTujxY0dWgGfT09adjYg== + dependencies: + passport-strategy "1.x.x" + pause "0.0.1" + +passport@^0.6.0: version "0.6.0" resolved "https://registry.yarnpkg.com/passport/-/passport-0.6.0.tgz#e869579fab465b5c0b291e841e6cc95c005fac9d" integrity sha512-0fe+p3ZnrWRW74fe8+SvCyf4a3Pb2/h7gFkQ8yTJpAO50gDzlfjZUZTO1k5Eg9kUct22OxHLqDZoKUWRHOh9ug== @@ -18606,7 +18581,7 @@ pseudomap@^1.0.2: resolved "https://registry.yarnpkg.com/pseudomap/-/pseudomap-1.0.2.tgz#f052a28da70e618917ef0a8ac34c1ae5a68286b3" integrity sha512-b/YwNhb8lk1Zz2+bXXpS/LK9OisiZZ1SNsSLxN1x2OXVEhW2Ckr/7mWE5vrC1ZTiJlD9g19jWszTmJsB+oEpFQ== -psl@^1.1.33: +psl@^1.1.28, psl@^1.1.33: version "1.9.0" resolved "https://registry.yarnpkg.com/psl/-/psl-1.9.0.tgz#d0df2a137f00794565fcaf3b2c00cd09f8d5a5a7" integrity sha512-E/ZsdU4HLs/68gYzgGTkMicWTLPdAftJLfJFlLUAAKZGkStNU72sZjT66SnMDVOfOWY/YAoiD7Jxa9iHvngcag== @@ -19668,6 +19643,11 @@ sax@1.2.1: resolved "https://registry.yarnpkg.com/sax/-/sax-1.2.1.tgz#7b8e656190b228e81a66aea748480d828cd2d37a" integrity sha512-8I2a3LovHTOpm7NV5yOyO8IHqgVsfK4+UuySrXU8YXkSRX7k6hCV9b3HrkKCr3nMpgj+0bmocaJJWpvp1oc7ZA== +sax@>=0.1.1: + version "1.4.1" + resolved "https://registry.yarnpkg.com/sax/-/sax-1.4.1.tgz#44cc8988377f126304d3b3fc1010c733b929ef0f" + integrity sha512-+aWOz7yVScEGoKNd4PA10LZ8sk0A/z5+nXQG5giUO5rprX9jgYsTdov9qCchZiPIZezbZH+jRut8nPodFAX4Jg== + sax@>=0.6.0: version "1.2.4" resolved "https://registry.yarnpkg.com/sax/-/sax-1.2.4.tgz#2816234e2378bddc4e5354fab5caa895df7100d9" @@ -19740,13 +19720,33 @@ semver-diff@^3.1.1: dependencies: semver "^6.3.0" -"semver@2 || 3 || 4 || 5", semver@7.5.3, semver@^5.5.0, semver@^5.6.0, semver@^5.7.1, semver@^6.0.0, semver@^6.1.1, semver@^6.1.2, semver@^6.2.0, semver@^6.3.0, semver@^6.3.1, semver@^7.0.0, semver@^7.1.1, semver@^7.3.2, semver@^7.3.4, semver@^7.3.5, semver@^7.3.7, semver@^7.3.8, semver@^7.5.3, semver@^7.5.4, semver@^7.6.3, semver@~2.3.1: +"semver@2 || 3 || 4 || 5", semver@^5.5.0, semver@^5.6.0, semver@^5.7.1: + version "5.7.2" + resolved "https://registry.yarnpkg.com/semver/-/semver-5.7.2.tgz#48d55db737c3287cd4835e17fa13feace1c41ef8" + integrity sha512-cBznnQ9KjJqU67B52RMC65CMarK2600WFnbkcaiwWq3xy/5haFJlshgnpjovMVJ+Hff49d8GEn0b87C5pDQ10g== + +semver@7.5.3, semver@^7.0.0, semver@^7.1.1, semver@^7.3.2, semver@^7.3.4, semver@^7.3.5, semver@^7.3.7, semver@^7.3.8, semver@^7.5.3: version "7.5.3" resolved "https://registry.yarnpkg.com/semver/-/semver-7.5.3.tgz#161ce8c2c6b4b3bdca6caadc9fa3317a4c4fe88e" integrity sha512-QBlUtyVk/5EeHbi7X0fw6liDZc7BBmEaSYn01fMU1OUYbf6GPsbTtd8WmnqbI20SeycoHSeiybkE/q1Q+qlThQ== dependencies: lru-cache "^6.0.0" +semver@^6.0.0, semver@^6.1.1, semver@^6.1.2, semver@^6.2.0, semver@^6.3.0, semver@^6.3.1: + version "6.3.1" + resolved "https://registry.yarnpkg.com/semver/-/semver-6.3.1.tgz#556d2ef8689146e46dcea4bfdd095f3434dffcb4" + integrity sha512-BR7VvDCVHO+q2xBEWskxS6DJE1qRnb7DxzUrogb71CWoSficBxYsiAGd+Kl0mmq/MprG9yArRkyrQxTO6XjMzA== + +semver@^7.5.4, semver@^7.6.3: + version "7.6.3" + resolved "https://registry.yarnpkg.com/semver/-/semver-7.6.3.tgz#980f7b5550bc175fb4dc09403085627f9eb33143" + integrity sha512-oVekP1cKtI+CTDvHWYFUcMtsK/00wmAEfyqKfNdARm8u1wNVhSgaX7A8d4UuIlUI5e84iEwOhs7ZPYRmzU9U6A== + +semver@~2.3.1: + version "2.3.2" + resolved "https://registry.yarnpkg.com/semver/-/semver-2.3.2.tgz#b9848f25d6cf36333073ec9ef8856d42f1233e52" + integrity sha512-abLdIKCosKfpnmhS52NCTjO4RiLspDfsn37prjzGrp9im5DPJOgh82Os92vtwGh6XdQryKI/7SREZnV+aqiXrA== + seq-queue@^0.0.5: version "0.0.5" resolved "https://registry.yarnpkg.com/seq-queue/-/seq-queue-0.0.5.tgz#d56812e1c017a6e4e7c3e3a37a1da6d78dd3c93e" @@ -21311,7 +21311,7 @@ touch@^3.1.0: dependencies: nopt "~1.0.10" -tough-cookie@4.1.3, "tough-cookie@^2.3.3 || ^3.0.1 || ^4.0.0", tough-cookie@^4.0.0, tough-cookie@^4.1.2, tough-cookie@~2.5.0: +"tough-cookie@^2.3.3 || ^3.0.1 || ^4.0.0", tough-cookie@^4.0.0, tough-cookie@^4.1.2: version "4.1.3" resolved "https://registry.yarnpkg.com/tough-cookie/-/tough-cookie-4.1.3.tgz#97b9adb0728b42280aa3d814b6b999b2ff0318bf" integrity sha512-aX/y5pVRkfRnfmuX+OdbSdXvPe6ieKX/G2s7e98f4poJHnqH3281gDPm/metm6E/WRamfx7WC4HUqkWHfQHprw== @@ -21321,6 +21321,14 @@ tough-cookie@4.1.3, "tough-cookie@^2.3.3 || ^3.0.1 || ^4.0.0", tough-cookie@^4.0 universalify "^0.2.0" url-parse "^1.5.3" +tough-cookie@~2.5.0: + version "2.5.0" + resolved "https://registry.yarnpkg.com/tough-cookie/-/tough-cookie-2.5.0.tgz#cd9fb2a0aa1d5a12b473bd9fb96fa3dcff65ade2" + integrity sha512-nlLsUzgm1kfLXSXfRZMc1KLAugd4hqJHDTvc2hDIwS3mZAfMEuMbc03SujMF+GEcpaX/qboeycw6iO8JwVv2+g== + dependencies: + psl "^1.1.28" + punycode "^2.1.1" + tr46@^2.1.0: version "2.1.0" resolved "https://registry.yarnpkg.com/tr46/-/tr46-2.1.0.tgz#fa87aa81ca5d5941da8cbf1f9b749dc969a4e240" @@ -21804,14 +21812,6 @@ unpipe@1.0.0: resolved "https://registry.yarnpkg.com/unpipe/-/unpipe-1.0.0.tgz#b2bf4ee8514aae6165b4817829d21b2ef49904ec" integrity sha512-pjy2bYhSsufwWlKwPc+l3cN7+wuJlK6uz0YdJEOlQDbl6jo/YlPi4mb8agUkVC8BF7V8NuzeyPNqRksA3hztKQ== -unset-value@2.0.1: - version "2.0.1" - resolved "https://registry.yarnpkg.com/unset-value/-/unset-value-2.0.1.tgz#57bed0c22d26f28d69acde5df9a11b77c74d2df3" - integrity sha512-2hvrBfjUE00PkqN+q0XP6yRAOGrR06uSiUoIQGZkc7GxvQ9H7v8quUPNtZjMg4uux69i8HWpIjLPUKwCuRGyNg== - dependencies: - has-value "^2.0.2" - isobject "^4.0.0" - untildify@^4.0.0: version "4.0.0" resolved "https://registry.yarnpkg.com/untildify/-/untildify-4.0.0.tgz#2bc947b953652487e4600949fb091e3ae8cd919b" @@ -22578,10 +22578,33 @@ xml-parse-from-string@^1.0.0: resolved "https://registry.yarnpkg.com/xml-parse-from-string/-/xml-parse-from-string-1.0.1.tgz#a9029e929d3dbcded169f3c6e28238d95a5d5a28" integrity sha512-ErcKwJTF54uRzzNMXq2X5sMIy88zJvfN2DmdoQvy7PAFJ+tPRU6ydWuOKNMyfmOjdyBQTFREi60s0Y0SyI0G0g== -xml2js@0.1.x, xml2js@0.4.19, xml2js@0.5.0, xml2js@0.6.2, xml2js@^0.4.19, xml2js@^0.4.5: - version "0.6.2" - resolved "https://registry.yarnpkg.com/xml2js/-/xml2js-0.6.2.tgz#dd0b630083aa09c161e25a4d0901e2b2a929b499" - integrity sha512-T4rieHaC1EXcES0Kxxj4JWgaUQHDk+qwHcYOCFHfiwKz7tOVPLq7Hjq9dM1WCMhylqMEfP7hMcOIChvotiZegA== +xml2js@0.1.x: + version "0.1.14" + resolved "https://registry.yarnpkg.com/xml2js/-/xml2js-0.1.14.tgz#5274e67f5a64c5f92974cd85139e0332adc6b90c" + integrity sha512-pbdws4PPPNc1HPluSUKamY4GWMk592K7qwcj6BExbVOhhubub8+pMda/ql68b6L3luZs/OGjGSB5goV7SnmgnA== + dependencies: + sax ">=0.1.1" + +xml2js@0.4.19: + version "0.4.19" + resolved "https://registry.yarnpkg.com/xml2js/-/xml2js-0.4.19.tgz#686c20f213209e94abf0d1bcf1efaa291c7827a7" + integrity sha512-esZnJZJOiJR9wWKMyuvSE1y6Dq5LCuJanqhxslH2bxM6duahNZ+HMpCLhBQGZkbX6xRf8x1Y2eJlgt2q3qo49Q== + dependencies: + sax ">=0.6.0" + xmlbuilder "~9.0.1" + +xml2js@0.5.0: + version "0.5.0" + resolved "https://registry.yarnpkg.com/xml2js/-/xml2js-0.5.0.tgz#d9440631fbb2ed800203fad106f2724f62c493b7" + integrity sha512-drPFnkQJik/O+uPKpqSgr22mpuFHqKdbS835iAQrUC73L2F5WkboIRd63ai/2Yg6I1jzifPFKH2NTK+cfglkIA== + dependencies: + sax ">=0.6.0" + xmlbuilder "~11.0.0" + +xml2js@^0.4.19, xml2js@^0.4.5: + version "0.4.23" + resolved "https://registry.yarnpkg.com/xml2js/-/xml2js-0.4.23.tgz#a0c69516752421eb2ac758ee4d4ccf58843eac66" + integrity sha512-ySPiMjM0+pLDftHgXY4By0uswI3SPKLDw/i3UXbnO8M/p28zqexCUoPmQFrYD+/1BzhGJSs2i1ERWKJAtiLrug== dependencies: sax ">=0.6.0" xmlbuilder "~11.0.0" @@ -22591,6 +22614,11 @@ xmlbuilder@~11.0.0: resolved "https://registry.yarnpkg.com/xmlbuilder/-/xmlbuilder-11.0.1.tgz#be9bae1c8a046e76b31127726347d0ad7002beb3" integrity sha512-fDlsI/kFEx7gLvbecc0/ohLG50fugQp8ryHzMTuW9vSa1GJ0XYWKnhsUx7oie3G98+r56aTQIUB4kht42R3JvA== +xmlbuilder@~9.0.1: + version "9.0.7" + resolved "https://registry.yarnpkg.com/xmlbuilder/-/xmlbuilder-9.0.7.tgz#132ee63d2ec5565c557e20f4c22df9aca686b10d" + integrity sha512-7YXTQc3P2l9+0rjaUbLwMKRhtmwg1M1eDf6nag7urC7pIPYLD9W/jmzQ4ptRSUbodw5S0jfoGTflLemQibSpeQ== + xmlchars@^2.2.0: version "2.2.0" resolved "https://registry.yarnpkg.com/xmlchars/-/xmlchars-2.2.0.tgz#060fe1bcb7f9c76fe2a17db86a9bc3ab894210cb" From a9acc7f87b50c7b3a2ca3b7d890fba595c86ba7e Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 9 Aug 2024 13:39:47 +0100 Subject: [PATCH 13/18] Adding expect function to allow undefined. --- .../server/src/api/routes/tests/datasource.spec.ts | 14 ++++---------- .../api/routes/tests/utilities/TestFunctions.ts | 4 ++++ 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/server/src/api/routes/tests/datasource.spec.ts b/packages/server/src/api/routes/tests/datasource.spec.ts index 3d98acdf0c..7545253181 100644 --- a/packages/server/src/api/routes/tests/datasource.spec.ts +++ b/packages/server/src/api/routes/tests/datasource.spec.ts @@ -1,5 +1,5 @@ import * as setup from "./utilities" -import { checkBuilderEndpoint } from "./utilities/TestFunctions" +import { checkBuilderEndpoint, allowUndefined } from "./utilities/TestFunctions" import { getCachedVariable } from "../../../threads/utils" import { context, events } from "@budibase/backend-core" import sdk from "../../../sdk" @@ -26,14 +26,6 @@ import { tableForDatasource } from "../../../tests/utilities/structures" import nock from "nock" import { Knex } from "knex" -function fetchedSchemaProps() { - return { - externalType: expect.toBeOneOf([expect.any(String), undefined, null]), - constraints: expect.toBeOneOf([expect.anything(), undefined, null]), - autocolumn: expect.toBeOneOf([expect.anything(), undefined, null]), - } -} - describe("/datasources", () => { const config = setup.getConfig() let datasource: Datasource @@ -397,7 +389,9 @@ describe("/datasources", () => { (acc, [fieldName, field]) => { acc[fieldName] = { ...field, - ...fetchedSchemaProps(), + externalType: allowUndefined(expect.any(String)), + constraints: allowUndefined(expect.any(Object)), + autocolumn: allowUndefined(expect.any(Boolean)), } return acc }, diff --git a/packages/server/src/api/routes/tests/utilities/TestFunctions.ts b/packages/server/src/api/routes/tests/utilities/TestFunctions.ts index a9a8e7051b..6fa9e054b9 100644 --- a/packages/server/src/api/routes/tests/utilities/TestFunctions.ts +++ b/packages/server/src/api/routes/tests/utilities/TestFunctions.ts @@ -184,3 +184,7 @@ export const runInProd = async (func: any) => { env._set("NODE_ENV", nodeEnv) env._set("JEST_WORKER_ID", workerId) } + +export function allowUndefined(expectation: jest.Expect) { + return expect.toBeOneOf([expectation, undefined, null]) +} From 3e1a0a60b4b5b5b66eb458e6b41896b2e0b859e1 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 9 Aug 2024 14:35:13 +0100 Subject: [PATCH 14/18] Checking the correct operation - also typeguarding the check. --- packages/backend-core/src/sql/sqlTable.ts | 7 ++++++- packages/server/src/api/controllers/row/ExternalRequest.ts | 7 +++---- packages/server/src/sdk/app/rows/utils.ts | 6 ++++++ 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/backend-core/src/sql/sqlTable.ts b/packages/backend-core/src/sql/sqlTable.ts index 55a6bfcaeb..35d7978449 100644 --- a/packages/backend-core/src/sql/sqlTable.ts +++ b/packages/backend-core/src/sql/sqlTable.ts @@ -70,7 +70,12 @@ function generateSchema( case FieldType.LONGFORM: case FieldType.BARCODEQR: case FieldType.BB_REFERENCE_SINGLE: - schema.text(key) + // primary key strings have to have a length in some DBs + if (primaryKeys.includes(key)) { + schema.string(key, 255) + } else { + schema.text(key) + } break case FieldType.NUMBER: // if meta is specified then this is a junction table entry diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 14dd909edd..5ee2d0fe2b 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -1,6 +1,5 @@ import dayjs from "dayjs" import { - ArrayOperator, AutoFieldSubType, AutoReason, Datasource, @@ -196,13 +195,13 @@ export class ExternalRequest { if (filters) { // need to map over the filters and make sure the _id field isn't present let prefix = 1 - for (const operator of Object.values(filters)) { - const isArrayOperator = Object.values(ArrayOperator).includes(operator) + for (const [operatorType, operator] of Object.entries(filters)) { + const isArrayOp = sdk.rows.utils.isArrayFilter(operatorType) for (const field of Object.keys(operator || {})) { if (dbCore.removeKeyNumbering(field) === "_id") { if (primary) { const parts = breakRowIdField(operator[field]) - if (primary.length > 1 && isArrayOperator) { + if (primary.length > 1 && isArrayOp) { operator[InternalSearchFilterOperator.COMPLEX_ID_OPERATOR] = { id: primary, values: parts[0], diff --git a/packages/server/src/sdk/app/rows/utils.ts b/packages/server/src/sdk/app/rows/utils.ts index e463397ad9..0cae39f5a9 100644 --- a/packages/server/src/sdk/app/rows/utils.ts +++ b/packages/server/src/sdk/app/rows/utils.ts @@ -12,6 +12,7 @@ import { Table, TableSchema, SqlClient, + ArrayOperator, } from "@budibase/types" import { makeExternalQuery } from "../../../integrations/base/query" import { Format } from "../../../api/controllers/view/exporters" @@ -311,3 +312,8 @@ function validateTimeOnlyField( return res } + +// type-guard check +export function isArrayFilter(operator: any): operator is ArrayOperator { + return Object.values(ArrayOperator).includes(operator) +} From 28a10bf908f7bf20c81b8f93d14c6fd337ea1e18 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 9 Aug 2024 14:54:47 +0100 Subject: [PATCH 15/18] Another test fix (yay for this one) --- packages/server/src/integrations/utils/utils.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/server/src/integrations/utils/utils.ts b/packages/server/src/integrations/utils/utils.ts index 6d0e4b838f..43302de36f 100644 --- a/packages/server/src/integrations/utils/utils.ts +++ b/packages/server/src/integrations/utils/utils.ts @@ -15,7 +15,7 @@ import { helpers, utils } from "@budibase/shared-core" import { pipeline } from "stream/promises" import tmp from "tmp" import fs from "fs" -import { merge } from "lodash" +import { merge, cloneDeep } from "lodash" type PrimitiveTypes = | FieldType.STRING @@ -293,7 +293,11 @@ function copyExistingPropsOver( table.schema[key] table.schema[key] = { // merge the properties - anything missing will be filled in, old definition preferred - ...merge(fetchedColumnDefinition, existingTableSchema[key]), + // have to clone due to the way merge works + ...merge( + cloneDeep(fetchedColumnDefinition), + existingTableSchema[key] + ), // always take externalType and autocolumn from the fetched definition externalType: existingTableSchema[key].externalType || From 0eaf6b7d425c7895b2b9b713d7dc3ab94f5201a1 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 9 Aug 2024 15:59:04 +0100 Subject: [PATCH 16/18] Make it possible to get flag values from the license. --- packages/backend-core/src/features/index.ts | 40 +- .../src/features/tests/features.spec.ts | 347 +++++++++--------- packages/pro | 2 +- 3 files changed, 211 insertions(+), 178 deletions(-) diff --git a/packages/backend-core/src/features/index.ts b/packages/backend-core/src/features/index.ts index 3f0a6cf6b0..cce407bfb0 100644 --- a/packages/backend-core/src/features/index.ts +++ b/packages/backend-core/src/features/index.ts @@ -1,12 +1,12 @@ import env from "../environment" import * as context from "../context" import { PostHog, PostHogOptions } from "posthog-node" -import { IdentityType } from "@budibase/types" +import { IdentityType, UserCtx } from "@budibase/types" import tracer from "dd-trace" let posthog: PostHog | undefined export function init(opts?: PostHogOptions) { - if (env.POSTHOG_TOKEN) { + if (env.POSTHOG_TOKEN && env.POSTHOG_API_HOST) { posthog = new PostHog(env.POSTHOG_TOKEN, { host: env.POSTHOG_API_HOST, ...opts, @@ -101,17 +101,23 @@ export class FlagSet, T extends { [key: string]: V }> { return this.flags[name as keyof T] !== undefined } - async get(key: K): Promise[K]> { - const flags = await this.fetch() + async get( + key: K, + ctx?: UserCtx + ): Promise[K]> { + const flags = await this.fetch(ctx) return flags[key] } - async isEnabled>(key: K): Promise { - const flags = await this.fetch() + async isEnabled>( + key: K, + ctx?: UserCtx + ): Promise { + const flags = await this.fetch(ctx) return flags[key] } - async fetch(): Promise> { + async fetch(ctx?: UserCtx): Promise> { return await tracer.trace("features.fetch", async span => { const tags: Record = {} @@ -150,6 +156,26 @@ export class FlagSet, T extends { [key: string]: V }> { } } + const license = ctx?.user?.license + if (license) { + for (const feature of license.features) { + const flag = this.flags[feature] + if (!flag) { + continue + } + + if (flags[feature] === true || specificallySetFalse.has(feature)) { + // If the flag is already set to through environment variables, we + // don't want to override it back to false here. + continue + } + + // @ts-ignore + flags[feature] = true + tags[`flags.${feature}.source`] = "license" + } + } + const identity = context.getIdentity() if (posthog && identity?.type === IdentityType.USER) { const posthogFlags = await posthog.getAllFlagsAndPayloads(identity._id) diff --git a/packages/backend-core/src/features/tests/features.spec.ts b/packages/backend-core/src/features/tests/features.spec.ts index 95819831b2..9c928f5545 100644 --- a/packages/backend-core/src/features/tests/features.spec.ts +++ b/packages/backend-core/src/features/tests/features.spec.ts @@ -1,7 +1,7 @@ -import { IdentityContext, IdentityType } from "@budibase/types" +import { IdentityContext, IdentityType, UserCtx } from "@budibase/types" import { Flag, FlagSet, FlagValues, init } from "../" import { context } from "../.." -import { setEnv, withEnv } from "../../environment" +import environment, { withEnv } from "../../environment" import nodeFetch from "node-fetch" import nock from "nock" @@ -12,207 +12,214 @@ const schema = { } const flags = new FlagSet(schema) +interface TestCase { + it: string + identity?: Partial + environmentFlags?: string + posthogFlags?: PostHogFlags + licenseFlags?: Array + expected?: Partial> + errorMessage?: string | RegExp +} + +interface PostHogFlags { + featureFlags?: Record + featureFlagPayloads?: Record +} + +function mockPosthogFlags(flags: PostHogFlags) { + nock("https://us.i.posthog.com") + .post("/decide/?v=3", body => { + return body.token === "test" && body.distinct_id === "us_1234" + }) + .reply(200, flags) + .persist() +} + describe("feature flags", () => { - interface TestCase { - tenant: string - flags: string - expected: Partial> - } + beforeEach(() => { + nock.cleanAll() + }) it.each([ { - tenant: "tenant1", - flags: "tenant1:TEST_BOOLEAN", + it: "should should find a simple boolean flag in the environment", + environmentFlags: "default:TEST_BOOLEAN", expected: { TEST_BOOLEAN: true }, }, { - tenant: "tenant1", - flags: "tenant1:!TEST_BOOLEAN", + it: "should should find a simple netgative boolean flag in the environment", + environmentFlags: "default:!TEST_BOOLEAN", expected: { TEST_BOOLEAN: false }, }, { - tenant: "tenant1", - flags: "*:TEST_BOOLEAN", + it: "should should match stars in the environment", + environmentFlags: "*:TEST_BOOLEAN", expected: { TEST_BOOLEAN: true }, }, { - tenant: "tenant1", - flags: "tenant2:TEST_BOOLEAN", + it: "should not match a different tenant's flags", + environmentFlags: "otherTenant:TEST_BOOLEAN", expected: { TEST_BOOLEAN: false }, }, { - tenant: "tenant1", - flags: "", + it: "should return the defaults when no flags are set", + expected: flags.defaults(), + }, + { + it: "should fail when an environment flag is not recognised", + environmentFlags: "default:TEST_BOOLEAN,default:FOO", + errorMessage: "Feature: FOO is not an allowed option", + }, + { + it: "should be able to read boolean flags from PostHog", + posthogFlags: { + featureFlags: { TEST_BOOLEAN: true }, + }, + expected: { TEST_BOOLEAN: true }, + }, + { + it: "should be able to read string flags from PostHog", + posthogFlags: { + featureFlags: { TEST_STRING: true }, + featureFlagPayloads: { TEST_STRING: "test" }, + }, + expected: { TEST_STRING: "test" }, + }, + { + it: "should be able to read numeric flags from PostHog", + posthogFlags: { + featureFlags: { TEST_NUMBER: true }, + featureFlagPayloads: { TEST_NUMBER: "123" }, + }, + expected: { TEST_NUMBER: 123 }, + }, + { + it: "should not be able to override a negative environment flag from PostHog", + environmentFlags: "default:!TEST_BOOLEAN", + posthogFlags: { + featureFlags: { TEST_BOOLEAN: true }, + }, + expected: { TEST_BOOLEAN: false }, + }, + { + it: "should not be able to override a positive environment flag from PostHog", + environmentFlags: "default:TEST_BOOLEAN", + posthogFlags: { + featureFlags: { + TEST_BOOLEAN: false, + }, + }, + expected: { TEST_BOOLEAN: true }, + }, + { + it: "should be able to set boolean flags through the license", + licenseFlags: ["TEST_BOOLEAN"], + expected: { TEST_BOOLEAN: true }, + }, + { + it: "should not be able to override a negative environment flag from license", + environmentFlags: "default:!TEST_BOOLEAN", + licenseFlags: ["TEST_BOOLEAN"], + expected: { TEST_BOOLEAN: false }, + }, + { + it: "should not error on unrecognised PostHog flag", + posthogFlags: { + featureFlags: { UNDEFINED: true }, + }, + expected: flags.defaults(), + }, + { + it: "should not error on unrecognised license flag", + licenseFlags: ["UNDEFINED"], expected: flags.defaults(), }, ])( - 'should find flags $expected for $tenant with string "$flags"', - ({ tenant, flags: envFlags, expected }) => - context.doInTenant(tenant, async () => - withEnv({ TENANT_FEATURE_FLAGS: envFlags }, async () => { - const values = await flags.fetch() - expect(values).toMatchObject(expected) + "$it", + async ({ + identity, + environmentFlags, + posthogFlags, + licenseFlags, + expected, + errorMessage, + }) => { + const env: Partial = { + TENANT_FEATURE_FLAGS: environmentFlags, + } - for (const [key, expectedValue] of Object.entries(expected)) { - const value = await flags.get(key as keyof typeof schema) - expect(value).toBe(expectedValue) + if (posthogFlags) { + mockPosthogFlags(posthogFlags) + env.POSTHOG_TOKEN = "test" + env.POSTHOG_API_HOST = "https://us.i.posthog.com" + } + + const ctx = { user: { license: { features: licenseFlags || [] } } } + + await withEnv(env, async () => { + // We need to pass in node-fetch here otherwise nock won't get used + // because posthog-node uses axios under the hood. + init({ fetch: nodeFetch }) + + const fullIdentity: IdentityContext = { + _id: "us_1234", + tenantId: "default", + type: IdentityType.USER, + email: "test@example.com", + firstName: "Test", + lastName: "User", + ...identity, + } + + await context.doInIdentityContext(fullIdentity, async () => { + if (errorMessage) { + await expect(flags.fetch(ctx as UserCtx)).rejects.toThrow( + errorMessage + ) + } else if (expected) { + const values = await flags.fetch(ctx as UserCtx) + expect(values).toMatchObject(expected) + + for (const [key, expectedValue] of Object.entries(expected)) { + const value = await flags.get( + key as keyof typeof schema, + ctx as UserCtx + ) + expect(value).toBe(expectedValue) + } + } else { + throw new Error("No expected value") } }) - ) + }) + } ) - interface FailedTestCase { - tenant: string - flags: string - expected: string | RegExp - } - - it.each([ - { - tenant: "tenant1", - flags: "tenant1:TEST_BOOLEAN,tenant1:FOO", - expected: "Feature: FOO is not an allowed option", - }, - ])( - "should fail with message \"$expected\" for $tenant with string '$flags'", - ({ tenant, flags: envFlags, expected }) => - context.doInTenant(tenant, () => - withEnv({ TENANT_FEATURE_FLAGS: envFlags }, () => - expect(flags.fetch()).rejects.toThrow(expected) - ) - ) - ) - - describe("posthog", () => { + it("should not error if PostHog is down", async () => { const identity: IdentityContext = { _id: "us_1234", - tenantId: "tenant1", + tenantId: "default", type: IdentityType.USER, email: "test@example.com", firstName: "Test", lastName: "User", } - let cleanup: () => void + nock("https://us.i.posthog.com") + .post("/decide/?v=3", body => { + return body.token === "test" && body.distinct_id === "us_1234" + }) + .reply(503) + .persist() - beforeAll(() => { - cleanup = setEnv({ POSTHOG_TOKEN: "test" }) - }) - - afterAll(() => { - cleanup() - }) - - beforeEach(() => { - nock.cleanAll() - - // We need to pass in node-fetch here otherwise nock won't get used - // because posthog-node uses axios under the hood. - init({ fetch: nodeFetch }) - }) - - function mockFlags(flags: { - featureFlags?: Record - featureFlagPayloads?: Record - }) { - nock("https://us.i.posthog.com") - .post("/decide/?v=3", body => { - return body.token === "test" && body.distinct_id === "us_1234" + await withEnv( + { POSTHOG_TOKEN: "test", POSTHOG_API_HOST: "https://us.i.posthog.com" }, + async () => { + await context.doInIdentityContext(identity, async () => { + await flags.fetch() }) - .reply(200, flags) - } - - it("should be able to read flags from posthog", async () => { - mockFlags({ - featureFlags: { - TEST_BOOLEAN: true, - }, - }) - - await context.doInIdentityContext(identity, async () => { - const values = await flags.fetch() - expect(values.TEST_BOOLEAN).toBe(true) - }) - }) - - it("should be able to read flags from posthog with payloads", async () => { - mockFlags({ - featureFlags: { - TEST_STRING: true, - }, - featureFlagPayloads: { - TEST_STRING: "test payload", - }, - }) - - await context.doInIdentityContext(identity, async () => { - const values = await flags.fetch() - expect(values.TEST_STRING).toBe("test payload") - }) - }) - - it("should be able to read flags from posthog with numbers", async () => { - mockFlags({ - featureFlags: { - TEST_NUMBER: true, - }, - featureFlagPayloads: { - TEST_NUMBER: 123, - }, - }) - - await context.doInIdentityContext(identity, async () => { - const values = await flags.fetch() - expect(values.TEST_NUMBER).toBe(123) - }) - }) - - it("should not fail when a flag is not known", async () => { - mockFlags({ - featureFlags: { - _SOME_RANDOM_FLAG: true, - }, - }) - - await context.doInIdentityContext(identity, async () => { - await expect(flags.fetch()).resolves.not.toThrow() - }) - }) - - it("should not override flags set in the environment", async () => { - mockFlags({ - featureFlags: { - TEST_BOOLEAN: false, - }, - }) - - await withEnv( - { TENANT_FEATURE_FLAGS: `${identity.tenantId}:TEST_BOOLEAN` }, - async () => { - await context.doInIdentityContext(identity, async () => { - const values = await flags.fetch() - expect(values.TEST_BOOLEAN).toBe(true) - }) - } - ) - }) - - it("should not override flags set in the environment with a ! prefix", async () => { - mockFlags({ - featureFlags: { - TEST_BOOLEAN: true, - }, - }) - - await withEnv( - { TENANT_FEATURE_FLAGS: `${identity.tenantId}:!TEST_BOOLEAN` }, - async () => { - await context.doInIdentityContext(identity, async () => { - const values = await flags.fetch() - expect(values.TEST_BOOLEAN).toBe(false) - }) - } - ) - }) + } + ) }) }) diff --git a/packages/pro b/packages/pro index 7fe713e51a..b91a944c1f 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit 7fe713e51afea77c8024579582a4e1a4ec1b55b3 +Subproject commit b91a944c1f02a3ee43ad22890ad53840eeced8fe From c14f108d4d2c2718e9df6d1a89d4c6e51fce5524 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 12 Aug 2024 09:58:44 +0100 Subject: [PATCH 17/18] Respond to PR comments. --- packages/backend-core/src/features/index.ts | 53 ++++++++++----------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/packages/backend-core/src/features/index.ts b/packages/backend-core/src/features/index.ts index cce407bfb0..25c9b260d8 100644 --- a/packages/backend-core/src/features/index.ts +++ b/packages/backend-core/src/features/index.ts @@ -83,22 +83,18 @@ class NumberFlag extends Flag { } export class FlagSet, T extends { [key: string]: V }> { - private readonly flags: T - - constructor(flags: T) { - this.flags = flags - } + constructor(private readonly flagSchema: T) {} defaults(): FlagValues { - return Object.keys(this.flags).reduce((acc, key) => { + return Object.keys(this.flagSchema).reduce((acc, key) => { const typedKey = key as keyof T - acc[typedKey] = this.flags[key].defaultValue + acc[typedKey] = this.flagSchema[key].defaultValue return acc }, {} as FlagValues) } isFlagName(name: string | number | symbol): name is keyof T { - return this.flags[name as keyof T] !== undefined + return this.flagSchema[name as keyof T] !== undefined } async get( @@ -120,9 +116,7 @@ export class FlagSet, T extends { [key: string]: V }> { async fetch(ctx?: UserCtx): Promise> { return await tracer.trace("features.fetch", async span => { const tags: Record = {} - - const flags = this.defaults() - + const flagValues = this.defaults() const currentTenantId = context.getTenantId() const specificallySetFalse = new Set() @@ -146,12 +140,13 @@ export class FlagSet, T extends { [key: string]: V }> { throw new Error(`Feature: ${feature} is not an allowed option`) } - if (typeof flags[feature] !== "boolean") { + if (typeof flagValues[feature] !== "boolean") { throw new Error(`Feature: ${feature} is not a boolean`) } - // @ts-ignore - flags[feature] = value + // @ts-expect-error - TS does not like you writing into a generic type, + // but we know that it's okay in this case because it's just an object. + flagValues[feature] = value tags[`flags.${feature}.source`] = "environment" } } @@ -159,19 +154,22 @@ export class FlagSet, T extends { [key: string]: V }> { const license = ctx?.user?.license if (license) { for (const feature of license.features) { - const flag = this.flags[feature] - if (!flag) { + if (!this.isFlagName(feature)) { continue } - if (flags[feature] === true || specificallySetFalse.has(feature)) { + if ( + flagValues[feature] === true || + specificallySetFalse.has(feature) + ) { // If the flag is already set to through environment variables, we // don't want to override it back to false here. continue } - // @ts-ignore - flags[feature] = true + // @ts-expect-error - TS does not like you writing into a generic type, + // but we know that it's okay in this case because it's just an object. + flagValues[feature] = true tags[`flags.${feature}.source`] = "license" } } @@ -180,25 +178,26 @@ export class FlagSet, T extends { [key: string]: V }> { if (posthog && identity?.type === IdentityType.USER) { const posthogFlags = await posthog.getAllFlagsAndPayloads(identity._id) for (const [name, value] of Object.entries(posthogFlags.featureFlags)) { - const flag = this.flags[name] - if (!flag) { + if (!this.isFlagName(name)) { // We don't want an unexpected PostHog flag to break the app, so we // just log it and continue. console.warn(`Unexpected posthog flag "${name}": ${value}`) continue } - if (flags[name] === true || specificallySetFalse.has(name)) { + if (flagValues[name] === true || specificallySetFalse.has(name)) { // If the flag is already set to through environment variables, we // don't want to override it back to false here. continue } const payload = posthogFlags.featureFlagPayloads?.[name] - + const flag = this.flagSchema[name] try { - // @ts-ignore - flags[name] = flag.parse(payload || value) + // @ts-expect-error - TS does not like you writing into a generic + // type, but we know that it's okay in this case because it's just + // an object. + flagValues[name] = flag.parse(payload || value) tags[`flags.${name}.source`] = "posthog" } catch (err) { // We don't want an invalid PostHog flag to break the app, so we just @@ -208,12 +207,12 @@ export class FlagSet, T extends { [key: string]: V }> { } } - for (const [key, value] of Object.entries(flags)) { + for (const [key, value] of Object.entries(flagValues)) { tags[`flags.${key}.value`] = value } span?.addTags(tags) - return flags + return flagValues }) } } From f4a445667920d13d526e382e6a6d75958f2d7ffb Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 12 Aug 2024 10:10:33 +0100 Subject: [PATCH 18/18] Update packages/pro refernce to latest master. --- packages/pro | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pro b/packages/pro index b91a944c1f..94747fd5bb 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit b91a944c1f02a3ee43ad22890ad53840eeced8fe +Subproject commit 94747fd5bb67c218244bb60b9540f3a6f1c3f6f1