From 672469526e95181280543d646fa302c86fc79fee Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 16:33:14 +0100 Subject: [PATCH 1/7] Mark calculation views explicitly instead of figuring it out implicitly. --- .../src/api/controllers/view/viewsV2.ts | 2 + .../src/api/routes/tests/viewV2.spec.ts | 64 ++++++++++++++++++- packages/server/src/sdk/app/views/external.ts | 3 + packages/server/src/sdk/app/views/index.ts | 7 ++ packages/server/src/sdk/app/views/internal.ts | 4 ++ packages/shared-core/src/helpers/views.ts | 5 ++ packages/types/src/documents/app/view.ts | 5 ++ 7 files changed, 88 insertions(+), 2 deletions(-) diff --git a/packages/server/src/api/controllers/view/viewsV2.ts b/packages/server/src/api/controllers/view/viewsV2.ts index c98940ed88..c60ab497cd 100644 --- a/packages/server/src/api/controllers/view/viewsV2.ts +++ b/packages/server/src/api/controllers/view/viewsV2.ts @@ -127,6 +127,7 @@ export async function create(ctx: Ctx) { const parsedView: Omit, "id" | "version"> = { name: view.name, + type: view.type, tableId: view.tableId, query: view.query, queryUI: view.queryUI, @@ -163,6 +164,7 @@ export async function update(ctx: Ctx) { const parsedView: RequiredKeys = { id: view.id, name: view.name, + type: view.type, version: view.version, tableId: view.tableId, query: view.query, diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 5238b9b752..0a8dd2ac70 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -26,6 +26,7 @@ import { BBReferenceFieldSubType, NumericCalculationFieldMetadata, ViewV2Schema, + ViewV2Type, } from "@budibase/types" import { generator, mocks } from "@budibase/backend-core/tests" import { DatabaseName, getDatasource } from "../../../integrations/tests/utils" @@ -155,7 +156,7 @@ describe.each([ }) it("can persist views with all fields", async () => { - const newView: Required> = { + const newView: Required> = { name: generator.name(), tableId: table._id!, primaryDisplay: "id", @@ -549,6 +550,7 @@ describe.each([ let view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { sum: { visible: true, @@ -571,6 +573,29 @@ describe.each([ expect(sum.calculationType).toEqual(CalculationType.SUM) expect(sum.field).toEqual("Price") }) + + it("cannot create a view with calculation fields unless it has the right type", async () => { + await config.api.viewV2.create( + { + tableId: table._id!, + name: generator.guid(), + schema: { + sum: { + visible: true, + calculationType: CalculationType.SUM, + field: "Price", + }, + }, + }, + { + status: 400, + body: { + message: + "Calculation fields are not allowed in non-calculation views", + }, + } + ) + }) }) describe("update", () => { @@ -615,7 +640,9 @@ describe.each([ it("can update all fields", async () => { const tableId = table._id! - const updatedData: Required> = { + const updatedData: Required< + Omit + > = { version: view.version, id: view.id, tableId, @@ -830,6 +857,32 @@ describe.each([ ) }) + it("cannot update view type after creation", async () => { + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + schema: { + id: { visible: true }, + Price: { + visible: true, + }, + }, + }) + + await config.api.viewV2.update( + { + ...view, + type: ViewV2Type.CALCULATION, + }, + { + status: 400, + body: { + message: "Cannot update view type after creation", + }, + } + ) + }) + isInternal && it("updating schema will only validate modified field", async () => { let view = await config.api.viewV2.create({ @@ -902,6 +955,7 @@ describe.each([ view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { country: { visible: true, @@ -2639,6 +2693,7 @@ describe.each([ it("should be able to search by calculations", async () => { const view = await config.api.viewV2.create({ tableId: table._id!, + type: ViewV2Type.CALCULATION, name: generator.guid(), schema: { "Quantity Sum": { @@ -2673,6 +2728,7 @@ describe.each([ const view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { quantity: { visible: true, @@ -2711,6 +2767,7 @@ describe.each([ const view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { aggregate: { visible: true, @@ -2771,6 +2828,7 @@ describe.each([ const view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { count: { visible: true, @@ -2805,6 +2863,7 @@ describe.each([ { tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { count: { visible: true, @@ -2853,6 +2912,7 @@ describe.each([ const view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { sum: { visible: true, diff --git a/packages/server/src/sdk/app/views/external.ts b/packages/server/src/sdk/app/views/external.ts index 3afd7e9bf9..b69ac0b9eb 100644 --- a/packages/server/src/sdk/app/views/external.ts +++ b/packages/server/src/sdk/app/views/external.ts @@ -70,6 +70,9 @@ export async function update(tableId: string, view: ViewV2): Promise { if (!existingView || !existingView.name) { throw new HTTPError(`View ${view.id} not found in table ${tableId}`, 404) } + if (isV2(existingView) && existingView.type !== view.type) { + throw new HTTPError(`Cannot update view type after creation`, 400) + } delete views[existingView.name] views[view.name] = view diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 73785edd98..e217f16400 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -111,6 +111,13 @@ async function guardViewSchema( if (helpers.views.isCalculationView(view)) { await guardCalculationViewSchema(table, view) + } else { + if (helpers.views.hasCalculationFields(view)) { + throw new HTTPError( + "Calculation fields are not allowed in non-calculation views", + 400 + ) + } } await checkReadonlyFields(table, view) diff --git a/packages/server/src/sdk/app/views/internal.ts b/packages/server/src/sdk/app/views/internal.ts index 19a9f6ab14..96b41bffe8 100644 --- a/packages/server/src/sdk/app/views/internal.ts +++ b/packages/server/src/sdk/app/views/internal.ts @@ -59,6 +59,10 @@ export async function update(tableId: string, view: ViewV2): Promise { throw new HTTPError(`View ${view.id} not found in table ${tableId}`, 404) } + if (isV2(existingView) && existingView.type !== view.type) { + throw new HTTPError(`Cannot update view type after creation`, 400) + } + delete table.views[existingView.name] table.views[view.name] = view await db.put(table) diff --git a/packages/shared-core/src/helpers/views.ts b/packages/shared-core/src/helpers/views.ts index f41c66adc8..0113375adf 100644 --- a/packages/shared-core/src/helpers/views.ts +++ b/packages/shared-core/src/helpers/views.ts @@ -3,6 +3,7 @@ import { ViewCalculationFieldMetadata, ViewFieldMetadata, ViewV2, + ViewV2Type, } from "@budibase/types" import { pickBy } from "lodash" @@ -21,6 +22,10 @@ export function isBasicViewField( type UnsavedViewV2 = Omit export function isCalculationView(view: UnsavedViewV2) { + return view.type === ViewV2Type.CALCULATION +} + +export function hasCalculationFields(view: UnsavedViewV2) { return Object.values(view.schema || {}).some(isCalculationField) } diff --git a/packages/types/src/documents/app/view.ts b/packages/types/src/documents/app/view.ts index c58852ecea..02c9b23e38 100644 --- a/packages/types/src/documents/app/view.ts +++ b/packages/types/src/documents/app/view.ts @@ -79,10 +79,15 @@ export enum CalculationType { MAX = "max", } +export enum ViewV2Type { + CALCULATION = "calculation", +} + export interface ViewV2 { version: 2 id: string name: string + type?: ViewV2Type primaryDisplay?: string tableId: string query?: LegacyFilter[] | SearchFilters From f4b430e27c86b68da9dd541c35471d86318bc258 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 16:38:18 +0100 Subject: [PATCH 2/7] Remove uiMetadata from ViewV2, it's not needed now we have the type field. --- packages/server/src/api/controllers/view/viewsV2.ts | 2 -- packages/server/src/api/routes/tests/viewV2.spec.ts | 6 ------ packages/types/src/documents/app/view.ts | 1 - 3 files changed, 9 deletions(-) diff --git a/packages/server/src/api/controllers/view/viewsV2.ts b/packages/server/src/api/controllers/view/viewsV2.ts index c60ab497cd..f864df9e9e 100644 --- a/packages/server/src/api/controllers/view/viewsV2.ts +++ b/packages/server/src/api/controllers/view/viewsV2.ts @@ -134,7 +134,6 @@ export async function create(ctx: Ctx) { sort: view.sort, schema, primaryDisplay: view.primaryDisplay, - uiMetadata: view.uiMetadata, } const result = await sdk.views.create(tableId, parsedView) ctx.status = 201 @@ -172,7 +171,6 @@ export async function update(ctx: Ctx) { sort: view.sort, schema, primaryDisplay: view.primaryDisplay, - uiMetadata: view.uiMetadata, } const result = await sdk.views.update(tableId, parsedView) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 0a8dd2ac70..b2b78853ef 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -178,9 +178,6 @@ describe.each([ visible: true, }, }, - uiMetadata: { - foo: "bar", - }, } const res = await config.api.viewV2.create(newView) @@ -670,9 +667,6 @@ describe.each([ readonly: true, }, }, - uiMetadata: { - foo: "bar", - }, } await config.api.viewV2.update(updatedData) diff --git a/packages/types/src/documents/app/view.ts b/packages/types/src/documents/app/view.ts index 02c9b23e38..1b2372da85 100644 --- a/packages/types/src/documents/app/view.ts +++ b/packages/types/src/documents/app/view.ts @@ -99,7 +99,6 @@ export interface ViewV2 { type?: SortType } schema?: ViewV2Schema - uiMetadata?: Record } export type ViewV2Schema = Record From 11804f6ddd5318c80a80ac06a11368f89ad4d284 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 18:18:04 +0100 Subject: [PATCH 3/7] Create a feature flag helper for tests. --- .../backend-core/src/features/features.ts | 300 ++++++++++++++++++ packages/backend-core/src/features/index.ts | 283 +---------------- .../backend-core/src/features/tests/utils.ts | 64 ++++ .../src/api/routes/tests/application.spec.ts | 11 +- .../server/src/api/routes/tests/row.spec.ts | 30 +- .../src/api/routes/tests/search.spec.ts | 18 +- .../src/api/routes/tests/templates.spec.ts | 73 +++-- .../src/api/routes/tests/viewV2.spec.ts | 24 +- .../tests/20240604153647_initial_sqs.spec.ts | 5 +- .../sdk/app/rows/search/tests/search.spec.ts | 15 +- .../tests/outputProcessing.spec.ts | 8 +- .../api/routes/global/tests/auditLogs.spec.ts | 10 +- 12 files changed, 458 insertions(+), 383 deletions(-) create mode 100644 packages/backend-core/src/features/features.ts create mode 100644 packages/backend-core/src/features/tests/utils.ts diff --git a/packages/backend-core/src/features/features.ts b/packages/backend-core/src/features/features.ts new file mode 100644 index 0000000000..90a395d52a --- /dev/null +++ b/packages/backend-core/src/features/features.ts @@ -0,0 +1,300 @@ +import env from "../environment" +import * as context from "../context" +import { PostHog, PostHogOptions } from "posthog-node" +import { FeatureFlag, IdentityType, UserCtx } from "@budibase/types" +import tracer from "dd-trace" +import { Duration } from "../utils" + +let posthog: PostHog | undefined +export function init(opts?: PostHogOptions) { + if ( + env.POSTHOG_TOKEN && + env.POSTHOG_API_HOST && + !env.SELF_HOSTED && + env.POSTHOG_FEATURE_FLAGS_ENABLED + ) { + console.log("initializing posthog client...") + posthog = new PostHog(env.POSTHOG_TOKEN, { + host: env.POSTHOG_API_HOST, + personalApiKey: env.POSTHOG_PERSONAL_TOKEN, + featureFlagsPollingInterval: Duration.fromMinutes(3).toMs(), + ...opts, + }) + } else { + console.log("posthog disabled") + } +} + +export function shutdown() { + posthog?.shutdown() +} + +export abstract class Flag { + static boolean(defaultValue: boolean): 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 +} + +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") { + return ["true", "t", "1"].includes(value.toLowerCase()) + } + + if (typeof value === "boolean") { + return value + } + + throw new Error(`could not parse value "${value}" as boolean`) + } +} + +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`) + } +} + +export interface EnvFlagEntry { + tenantId: string + key: string + value: boolean +} + +export function parseEnvFlags(flags: string): EnvFlagEntry[] { + const split = flags.split(",").map(x => x.split(":")) + const result: EnvFlagEntry[] = [] + for (const [tenantId, ...features] of split) { + for (let feature of features) { + let value = true + if (feature.startsWith("!")) { + feature = feature.slice(1) + value = false + } + result.push({ tenantId, key: feature, value }) + } + } + return result +} + +export class FlagSet, T extends { [key: string]: V }> { + // This is used to safely cache flags sets in the current request context. + // Because multiple sets could theoretically exist, we don't want the cache of + // one to leak into another. + private readonly setId: string + + constructor(private readonly flagSchema: T) { + this.setId = crypto.randomUUID() + } + + defaults(): FlagValues { + return Object.keys(this.flagSchema).reduce((acc, key) => { + const typedKey = key as keyof T + acc[typedKey] = this.flagSchema[key].defaultValue + return acc + }, {} as FlagValues) + } + + isFlagName(name: string | number | symbol): name is keyof T { + return this.flagSchema[name as keyof T] !== undefined + } + + async get( + key: K, + ctx?: UserCtx + ): Promise[K]> { + const flags = await this.fetch(ctx) + return flags[key] + } + + async isEnabled>( + key: K, + ctx?: UserCtx + ): Promise { + const flags = await this.fetch(ctx) + return flags[key] + } + + async fetch(ctx?: UserCtx): Promise> { + return await tracer.trace("features.fetch", async span => { + const cachedFlags = context.getFeatureFlags>(this.setId) + if (cachedFlags) { + span?.addTags({ fromCache: true }) + return cachedFlags + } + + const tags: Record = {} + const flagValues = this.defaults() + const currentTenantId = context.getTenantId() + const specificallySetFalse = new Set() + + for (const { tenantId, key, value } of parseEnvFlags( + env.TENANT_FEATURE_FLAGS || "" + )) { + if (!tenantId || (tenantId !== "*" && tenantId !== currentTenantId)) { + continue + } + + tags[`readFromEnvironmentVars`] = true + + if (value === false) { + specificallySetFalse.add(key) + } + + // ignore unknown flags + if (!this.isFlagName(key)) { + continue + } + + if (typeof flagValues[key] !== "boolean") { + throw new Error(`Feature: ${key} is not a boolean`) + } + + // @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[key as keyof FlagValues] = value + tags[`flags.${key}.source`] = "environment" + } + + const license = ctx?.user?.license + if (license) { + tags[`readFromLicense`] = true + + for (const feature of license.features) { + if (!this.isFlagName(feature)) { + continue + } + + 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-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" + } + } + + const identity = context.getIdentity() + tags[`identity.type`] = identity?.type + tags[`identity.tenantId`] = identity?.tenantId + tags[`identity._id`] = identity?._id + + if (posthog && identity?.type === IdentityType.USER) { + tags[`readFromPostHog`] = true + + const personProperties: Record = {} + if (identity.tenantId) { + personProperties.tenantId = identity.tenantId + } + + const posthogFlags = await posthog.getAllFlagsAndPayloads( + identity._id, + { + personProperties, + } + ) + + for (const [name, value] of Object.entries(posthogFlags.featureFlags)) { + 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 (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-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 + // log it and continue. + console.warn(`Error parsing posthog flag "${name}": ${value}`, err) + } + } + } + + context.setFeatureFlags(this.setId, flagValues) + for (const [key, value] of Object.entries(flagValues)) { + tags[`flags.${key}.value`] = value + } + span?.addTags(tags) + + return flagValues + }) + } +} + +// 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. +export const flags = new FlagSet({ + DEFAULT_VALUES: Flag.boolean(env.isDev()), + AUTOMATION_BRANCHING: Flag.boolean(env.isDev()), + SQS: Flag.boolean(env.isDev()), + [FeatureFlag.AI_CUSTOM_CONFIGS]: Flag.boolean(env.isDev()), + [FeatureFlag.ENRICHED_RELATIONSHIPS]: Flag.boolean(env.isDev()), +}) + +type UnwrapPromise = T extends Promise ? U : T +export type FeatureFlags = UnwrapPromise> diff --git a/packages/backend-core/src/features/index.ts b/packages/backend-core/src/features/index.ts index 2b915e5689..f77a62fd4d 100644 --- a/packages/backend-core/src/features/index.ts +++ b/packages/backend-core/src/features/index.ts @@ -1,281 +1,2 @@ -import env from "../environment" -import * as context from "../context" -import { PostHog, PostHogOptions } from "posthog-node" -import { FeatureFlag, IdentityType, UserCtx } from "@budibase/types" -import tracer from "dd-trace" -import { Duration } from "../utils" - -let posthog: PostHog | undefined -export function init(opts?: PostHogOptions) { - if ( - env.POSTHOG_TOKEN && - env.POSTHOG_API_HOST && - !env.SELF_HOSTED && - env.POSTHOG_FEATURE_FLAGS_ENABLED - ) { - console.log("initializing posthog client...") - posthog = new PostHog(env.POSTHOG_TOKEN, { - host: env.POSTHOG_API_HOST, - personalApiKey: env.POSTHOG_PERSONAL_TOKEN, - featureFlagsPollingInterval: Duration.fromMinutes(3).toMs(), - ...opts, - }) - } else { - console.log("posthog disabled") - } -} - -export function shutdown() { - posthog?.shutdown() -} - -export abstract class Flag { - static boolean(defaultValue: boolean): 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 -} - -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") { - return ["true", "t", "1"].includes(value.toLowerCase()) - } - - if (typeof value === "boolean") { - return value - } - - throw new Error(`could not parse value "${value}" as boolean`) - } -} - -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`) - } -} - -export class FlagSet, T extends { [key: string]: V }> { - // This is used to safely cache flags sets in the current request context. - // Because multiple sets could theoretically exist, we don't want the cache of - // one to leak into another. - private readonly setId: string - - constructor(private readonly flagSchema: T) { - this.setId = crypto.randomUUID() - } - - defaults(): FlagValues { - return Object.keys(this.flagSchema).reduce((acc, key) => { - const typedKey = key as keyof T - acc[typedKey] = this.flagSchema[key].defaultValue - return acc - }, {} as FlagValues) - } - - isFlagName(name: string | number | symbol): name is keyof T { - return this.flagSchema[name as keyof T] !== undefined - } - - async get( - key: K, - ctx?: UserCtx - ): Promise[K]> { - const flags = await this.fetch(ctx) - return flags[key] - } - - async isEnabled>( - key: K, - ctx?: UserCtx - ): Promise { - const flags = await this.fetch(ctx) - return flags[key] - } - - async fetch(ctx?: UserCtx): Promise> { - return await tracer.trace("features.fetch", async span => { - const cachedFlags = context.getFeatureFlags>(this.setId) - if (cachedFlags) { - span?.addTags({ fromCache: true }) - return cachedFlags - } - - const tags: Record = {} - const flagValues = 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 - } - - tags[`readFromEnvironmentVars`] = true - - for (let feature of features) { - let value = true - if (feature.startsWith("!")) { - feature = feature.slice(1) - value = false - specificallySetFalse.add(feature) - } - - // ignore unknown flags - if (!this.isFlagName(feature)) { - continue - } - - if (typeof flagValues[feature] !== "boolean") { - throw new Error(`Feature: ${feature} is not a boolean`) - } - - // @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 as keyof FlagValues] = value - tags[`flags.${feature}.source`] = "environment" - } - } - - const license = ctx?.user?.license - if (license) { - tags[`readFromLicense`] = true - - for (const feature of license.features) { - if (!this.isFlagName(feature)) { - continue - } - - 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-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" - } - } - - const identity = context.getIdentity() - tags[`identity.type`] = identity?.type - tags[`identity.tenantId`] = identity?.tenantId - tags[`identity._id`] = identity?._id - - if (posthog && identity?.type === IdentityType.USER) { - tags[`readFromPostHog`] = true - - const personProperties: Record = {} - if (identity.tenantId) { - personProperties.tenantId = identity.tenantId - } - - const posthogFlags = await posthog.getAllFlagsAndPayloads( - identity._id, - { - personProperties, - } - ) - - for (const [name, value] of Object.entries(posthogFlags.featureFlags)) { - 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 (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-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 - // log it and continue. - console.warn(`Error parsing posthog flag "${name}": ${value}`, err) - } - } - } - - context.setFeatureFlags(this.setId, flagValues) - for (const [key, value] of Object.entries(flagValues)) { - tags[`flags.${key}.value`] = value - } - span?.addTags(tags) - - return flagValues - }) - } -} - -// 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. -export const flags = new FlagSet({ - DEFAULT_VALUES: Flag.boolean(env.isDev()), - AUTOMATION_BRANCHING: Flag.boolean(env.isDev()), - SQS: Flag.boolean(env.isDev()), - [FeatureFlag.AI_CUSTOM_CONFIGS]: Flag.boolean(env.isDev()), - [FeatureFlag.ENRICHED_RELATIONSHIPS]: Flag.boolean(env.isDev()), -}) +export * from "./features" +export * as testutils from "./tests/utils" diff --git a/packages/backend-core/src/features/tests/utils.ts b/packages/backend-core/src/features/tests/utils.ts new file mode 100644 index 0000000000..cc633c083d --- /dev/null +++ b/packages/backend-core/src/features/tests/utils.ts @@ -0,0 +1,64 @@ +import { FeatureFlags, parseEnvFlags } from ".." +import { setEnv } from "../../environment" + +function getCurrentFlags(): Record> { + const result: Record> = {} + for (const { tenantId, key, value } of parseEnvFlags( + process.env.TENANT_FEATURE_FLAGS || "" + )) { + const tenantFlags = result[tenantId] || {} + // Don't allow overwriting specifically false flags, to match the beheaviour + // of FlagSet. + if (tenantFlags[key] === false) { + continue + } + tenantFlags[key] = value + result[tenantId] = tenantFlags + } + return result +} + +function buildFlagString( + flags: Record> +): string { + const parts: string[] = [] + for (const [tenantId, tenantFlags] of Object.entries(flags)) { + for (const [key, value] of Object.entries(tenantFlags)) { + if (value === false) { + parts.push(`${tenantId}:!${key}`) + } else { + parts.push(`${tenantId}:${key}`) + } + } + } + return parts.join(",") +} + +export function setFeatureFlags( + tenantId: string, + flags: Partial +): () => void { + const current = getCurrentFlags() + for (const [key, value] of Object.entries(flags)) { + const tenantFlags = current[tenantId] || {} + tenantFlags[key] = value + current[tenantId] = tenantFlags + } + const flagString = buildFlagString(current) + return setEnv({ TENANT_FEATURE_FLAGS: flagString }) +} + +export function withFeatureFlags( + tenantId: string, + flags: Partial, + f: () => T +) { + const cleanup = setFeatureFlags(tenantId, flags) + const result = f() + if (result instanceof Promise) { + return result.finally(cleanup) + } else { + cleanup() + return result + } +} diff --git a/packages/server/src/api/routes/tests/application.spec.ts b/packages/server/src/api/routes/tests/application.spec.ts index 40ed828dce..6e41c8b4ec 100644 --- a/packages/server/src/api/routes/tests/application.spec.ts +++ b/packages/server/src/api/routes/tests/application.spec.ts @@ -19,6 +19,7 @@ import { utils, context, withEnv as withCoreEnv, + features, } from "@budibase/backend-core" import env from "../../../environment" import { type App } from "@budibase/types" @@ -358,9 +359,13 @@ describe("/applications", () => { .delete(`/api/global/roles/${prodAppId}`) .reply(200, {}) - await withCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }, async () => { - await config.api.application.delete(app.appId) - }) + await features.testutils.withFeatureFlags( + "*", + { SQS: true }, + async () => { + await config.api.application.delete(app.appId) + } + ) }) }) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 5222069460..3fc140e8e5 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -15,6 +15,7 @@ import { tenancy, withEnv as withCoreEnv, setEnv as setCoreEnv, + features, } from "@budibase/backend-core" import { quotas } from "@budibase/pro" import { @@ -98,12 +99,12 @@ describe.each([ let envCleanup: (() => void) | undefined beforeAll(async () => { - await withCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }, () => config.init()) - if (isLucene) { - envCleanup = setCoreEnv({ TENANT_FEATURE_FLAGS: "*:!SQS" }) - } else if (isSqs) { - envCleanup = setCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }) - } + await features.testutils.withFeatureFlags("*", { SQS: true }, () => + config.init() + ) + envCleanup = features.testutils.setFeatureFlags("*", { + SQS: isSqs, + }) if (dsProvider) { const rawDatasource = await dsProvider @@ -2517,15 +2518,9 @@ describe.each([ let flagCleanup: (() => void) | undefined beforeAll(async () => { - const env = { - TENANT_FEATURE_FLAGS: `*:${FeatureFlag.ENRICHED_RELATIONSHIPS}`, - } - if (isSqs) { - env.TENANT_FEATURE_FLAGS = `${env.TENANT_FEATURE_FLAGS},*:SQS` - } else { - env.TENANT_FEATURE_FLAGS = `${env.TENANT_FEATURE_FLAGS},*:!SQS` - } - flagCleanup = setCoreEnv(env) + flagCleanup = features.testutils.setFeatureFlags("*", { + ENRICHED_RELATIONSHIPS: true, + }) const aux2Table = await config.api.table.save(saveTableRequest()) const aux2Data = await config.api.row.save(aux2Table._id!, {}) @@ -2752,9 +2747,10 @@ describe.each([ it.each(testScenarios)( "does not enrich relationships when not enabled (via %s)", async (__, retrieveDelegate) => { - await withCoreEnv( + await features.testutils.withFeatureFlags( + "*", { - TENANT_FEATURE_FLAGS: `*:!${FeatureFlag.ENRICHED_RELATIONSHIPS}`, + ENRICHED_RELATIONSHIPS: false, }, async () => { const otherRows = _.sampleSize(auxData, 5) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 110899e292..51965b5574 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -7,9 +7,9 @@ import { import { context, db as dbCore, + features, MAX_VALID_DATE, MIN_VALID_DATE, - setEnv as setCoreEnv, SQLITE_DESIGN_DOC_ID, utils, withEnv as withCoreEnv, @@ -94,16 +94,12 @@ describe.each([ } beforeAll(async () => { - await withCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }, () => config.init()) - if (isLucene) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:!SQS", - }) - } else if (isSqs) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:SQS", - }) - } + await features.testutils.withFeatureFlags("*", { SQS: true }, () => + config.init() + ) + envCleanup = features.testutils.setFeatureFlags("*", { + SQS: isSqs, + }) if (config.app?.appId) { config.app = await config.api.application.update(config.app?.appId, { diff --git a/packages/server/src/api/routes/tests/templates.spec.ts b/packages/server/src/api/routes/tests/templates.spec.ts index 6f4d468a68..a4f9318720 100644 --- a/packages/server/src/api/routes/tests/templates.spec.ts +++ b/packages/server/src/api/routes/tests/templates.spec.ts @@ -2,7 +2,11 @@ import * as setup from "./utilities" import path from "path" import nock from "nock" import { generator } from "@budibase/backend-core/tests" -import { withEnv as withCoreEnv, env as coreEnv } from "@budibase/backend-core" +import { + withEnv as withCoreEnv, + env as coreEnv, + features, +} from "@budibase/backend-core" interface App { background: string @@ -85,41 +89,44 @@ describe("/templates", () => { it.each(["sqs", "lucene"])( `should be able to create an app from a template (%s)`, async source => { - const env: Partial = { - TENANT_FEATURE_FLAGS: source === "sqs" ? "*:SQS" : "", - } + await features.testutils.withFeatureFlags( + "*", + { SQS: source === "sqs" }, + async () => { + const name = generator.guid().replaceAll("-", "") + const url = `/${name}` - await withCoreEnv(env, async () => { - const name = generator.guid().replaceAll("-", "") - const url = `/${name}` - - const app = await config.api.application.create({ - name, - url, - useTemplate: "true", - templateName: "Agency Client Portal", - templateKey: "app/agency-client-portal", - }) - expect(app.name).toBe(name) - expect(app.url).toBe(url) - - await config.withApp(app, async () => { - const tables = await config.api.table.fetch() - expect(tables).toHaveLength(2) - - tables.sort((a, b) => a.name.localeCompare(b.name)) - const [agencyProjects, users] = tables - expect(agencyProjects.name).toBe("Agency Projects") - expect(users.name).toBe("Users") - - const { rows } = await config.api.row.search(agencyProjects._id!, { - tableId: agencyProjects._id!, - query: {}, + const app = await config.api.application.create({ + name, + url, + useTemplate: "true", + templateName: "Agency Client Portal", + templateKey: "app/agency-client-portal", }) + expect(app.name).toBe(name) + expect(app.url).toBe(url) - expect(rows).toHaveLength(3) - }) - }) + await config.withApp(app, async () => { + const tables = await config.api.table.fetch() + expect(tables).toHaveLength(2) + + tables.sort((a, b) => a.name.localeCompare(b.name)) + const [agencyProjects, users] = tables + expect(agencyProjects.name).toBe("Agency Projects") + expect(users.name).toBe("Users") + + const { rows } = await config.api.row.search( + agencyProjects._id!, + { + tableId: agencyProjects._id!, + query: {}, + } + ) + + expect(rows).toHaveLength(3) + }) + } + ) } ) }) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 04e51fc212..ca4805864b 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -37,6 +37,7 @@ import { withEnv as withCoreEnv, setEnv as setCoreEnv, env, + features, } from "@budibase/backend-core" describe.each([ @@ -102,18 +103,13 @@ describe.each([ } beforeAll(async () => { - await withCoreEnv({ TENANT_FEATURE_FLAGS: isSqs ? "*:SQS" : "" }, () => + await features.testutils.withFeatureFlags("*", { SQS: isSqs }, () => config.init() ) - if (isLucene) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:!SQS", - }) - } else if (isSqs) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:SQS", - }) - } + + envCleanup = features.testutils.setFeatureFlags("*", { + SQS: isSqs, + }) if (dsProvider) { datasource = await config.createDatasource({ @@ -2490,12 +2486,8 @@ describe.each([ describe("foreign relationship columns", () => { let envCleanup: () => void beforeAll(() => { - const flags = [`*:${FeatureFlag.ENRICHED_RELATIONSHIPS}`] - if (env.TENANT_FEATURE_FLAGS) { - flags.push(...env.TENANT_FEATURE_FLAGS.split(",")) - } - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: flags.join(","), + envCleanup = features.testutils.setFeatureFlags("*", { + ENRICHED_RELATIONSHIPS: true, }) }) 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 b4f708edb5..759665f1c2 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 @@ -2,6 +2,7 @@ import * as setup from "../../../api/routes/tests/utilities" import { basicTable } from "../../../tests/utilities/structures" import { db as dbCore, + features, SQLITE_DESIGN_DOC_ID, withEnv as withCoreEnv, } from "@budibase/backend-core" @@ -71,11 +72,11 @@ function oldLinkDocument(): Omit { } async function sqsDisabled(cb: () => Promise) { - await withCoreEnv({ TENANT_FEATURE_FLAGS: "*:!SQS" }, cb) + await features.testutils.withFeatureFlags("*", { SQS: false }, cb) } async function sqsEnabled(cb: () => Promise) { - await withCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }, cb) + await features.testutils.withFeatureFlags("*", { SQS: true }, cb) } describe("SQS migration", () => { 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 e7fd095865..fce1e14094 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 @@ -13,6 +13,7 @@ import { generator } from "@budibase/backend-core/tests" import { withEnv as withCoreEnv, setEnv as setCoreEnv, + features, } from "@budibase/backend-core" import { DatabaseName, @@ -41,19 +42,13 @@ describe.each([ let table: Table beforeAll(async () => { - await withCoreEnv({ TENANT_FEATURE_FLAGS: isSqs ? "*:SQS" : "" }, () => + await features.testutils.withFeatureFlags("*", { SQS: isSqs }, () => config.init() ) - if (isLucene) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:!SQS", - }) - } else if (isSqs) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:SQS", - }) - } + envCleanup = features.testutils.setFeatureFlags("*", { + SQS: isSqs, + }) if (dsProvider) { datasource = await config.createDatasource({ diff --git a/packages/server/src/utilities/rowProcessor/tests/outputProcessing.spec.ts b/packages/server/src/utilities/rowProcessor/tests/outputProcessing.spec.ts index b6c2cdb11c..8cbe585d90 100644 --- a/packages/server/src/utilities/rowProcessor/tests/outputProcessing.spec.ts +++ b/packages/server/src/utilities/rowProcessor/tests/outputProcessing.spec.ts @@ -8,7 +8,7 @@ import { } from "@budibase/types" import { outputProcessing } from ".." import { generator, structures } from "@budibase/backend-core/tests" -import { setEnv as setCoreEnv } from "@budibase/backend-core" +import { features } from "@budibase/backend-core" import * as bbReferenceProcessor from "../bbReferenceProcessor" import TestConfiguration from "../../../tests/utilities/TestConfiguration" @@ -21,7 +21,7 @@ jest.mock("../bbReferenceProcessor", (): typeof bbReferenceProcessor => ({ describe("rowProcessor - outputProcessing", () => { const config = new TestConfiguration() - let cleanupEnv: () => void = () => {} + let cleanupFlags: () => void = () => {} beforeAll(async () => { await config.init() @@ -33,11 +33,11 @@ describe("rowProcessor - outputProcessing", () => { beforeEach(() => { jest.resetAllMocks() - cleanupEnv = setCoreEnv({ TENANT_FEATURE_FLAGS: "*SQS" }) + cleanupFlags = features.testutils.setFeatureFlags("*", { SQS: true }) }) afterEach(() => { - cleanupEnv() + cleanupFlags() }) const processOutputBBReferenceMock = diff --git a/packages/worker/src/api/routes/global/tests/auditLogs.spec.ts b/packages/worker/src/api/routes/global/tests/auditLogs.spec.ts index b09c27762d..b540836583 100644 --- a/packages/worker/src/api/routes/global/tests/auditLogs.spec.ts +++ b/packages/worker/src/api/routes/global/tests/auditLogs.spec.ts @@ -1,5 +1,5 @@ import { mocks, structures } from "@budibase/backend-core/tests" -import { context, events, setEnv as setCoreEnv } from "@budibase/backend-core" +import { context, events, features } from "@budibase/backend-core" import { Event, IdentityType } from "@budibase/types" import { TestConfiguration } from "../../../../tests" @@ -17,11 +17,9 @@ describe.each(["lucene", "sql"])("/api/global/auditlogs (%s)", method => { let envCleanup: (() => void) | undefined beforeAll(async () => { - if (method === "lucene") { - envCleanup = setCoreEnv({ TENANT_FEATURE_FLAGS: "*:!SQS" }) - } else if (method === "sql") { - envCleanup = setCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }) - } + envCleanup = features.testutils.setFeatureFlags("*", { + SQS: method === "sql", + }) await config.beforeAll() }) From d0c57c82ad978ac0f1d5d15aa41d8442e6287939 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 8 Oct 2024 11:15:53 +0100 Subject: [PATCH 4/7] Fix lint. --- .../server/src/api/routes/tests/application.spec.ts | 8 +------- packages/server/src/api/routes/tests/row.spec.ts | 3 --- packages/server/src/api/routes/tests/templates.spec.ts | 6 +----- packages/server/src/api/routes/tests/viewV2.spec.ts | 10 +--------- .../tests/20240604153647_initial_sqs.spec.ts | 1 - .../src/sdk/app/rows/search/tests/search.spec.ts | 6 +----- 6 files changed, 4 insertions(+), 30 deletions(-) diff --git a/packages/server/src/api/routes/tests/application.spec.ts b/packages/server/src/api/routes/tests/application.spec.ts index 6e41c8b4ec..fe8250bde5 100644 --- a/packages/server/src/api/routes/tests/application.spec.ts +++ b/packages/server/src/api/routes/tests/application.spec.ts @@ -14,13 +14,7 @@ jest.mock("../../../utilities/redis", () => ({ import { checkBuilderEndpoint } from "./utilities/TestFunctions" import * as setup from "./utilities" import { AppStatus } from "../../../db/utils" -import { - events, - utils, - context, - withEnv as withCoreEnv, - features, -} from "@budibase/backend-core" +import { events, utils, context, features } from "@budibase/backend-core" import env from "../../../environment" import { type App } from "@budibase/types" import tk from "timekeeper" diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 3fc140e8e5..207420bf9f 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -13,8 +13,6 @@ import { context, InternalTable, tenancy, - withEnv as withCoreEnv, - setEnv as setCoreEnv, features, } from "@budibase/backend-core" import { quotas } from "@budibase/pro" @@ -41,7 +39,6 @@ import { TableSchema, JsonFieldSubType, RowExportFormat, - FeatureFlag, RelationSchemaField, } from "@budibase/types" import { generator, mocks } from "@budibase/backend-core/tests" diff --git a/packages/server/src/api/routes/tests/templates.spec.ts b/packages/server/src/api/routes/tests/templates.spec.ts index a4f9318720..d5483c54b4 100644 --- a/packages/server/src/api/routes/tests/templates.spec.ts +++ b/packages/server/src/api/routes/tests/templates.spec.ts @@ -2,11 +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, - env as coreEnv, - features, -} from "@budibase/backend-core" +import { features } from "@budibase/backend-core" interface App { background: string diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index fe926170b9..dce5a660b5 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -22,7 +22,6 @@ import { RelationshipType, TableSchema, RenameColumn, - FeatureFlag, BBReferenceFieldSubType, NumericCalculationFieldMetadata, ViewV2Schema, @@ -31,14 +30,7 @@ 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, - withEnv as withCoreEnv, - setEnv as setCoreEnv, - env, - features, -} from "@budibase/backend-core" +import { db, roles, features } from "@budibase/backend-core" describe.each([ ["lucene", undefined], 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 759665f1c2..fe44b7b901 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 @@ -4,7 +4,6 @@ import { db as dbCore, features, SQLITE_DESIGN_DOC_ID, - withEnv as withCoreEnv, } from "@budibase/backend-core" import { LinkDocument, 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 fce1e14094..4d8a6b6d69 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 @@ -10,11 +10,7 @@ import { 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, - features, -} from "@budibase/backend-core" +import { features } from "@budibase/backend-core" import { DatabaseName, getDatasource, From 2d19644bd805f1fae19e277dc0508640d5611746 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 8 Oct 2024 11:19:43 +0100 Subject: [PATCH 5/7] Fix tests. --- packages/server/src/api/routes/tests/viewV2.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index e22f028caa..93346eb710 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -599,6 +599,7 @@ describe.each([ { tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { sum: { visible: true, From 7818546ade84b612f2d52d286a3fe16c9f0eecb3 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 8 Oct 2024 12:31:14 +0100 Subject: [PATCH 6/7] Fix tests. --- packages/server/src/api/routes/tests/viewV2.spec.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 51be1e7bf8..5d565dbdbc 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -647,6 +647,7 @@ describe.each([ { tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { sum: { visible: true, @@ -675,6 +676,7 @@ describe.each([ { tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { count: { visible: true, @@ -701,6 +703,7 @@ describe.each([ { tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { count: { visible: true, @@ -730,6 +733,7 @@ describe.each([ await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { count: { visible: true, From e3c6b60211cf521a1bf3b7cc01b4809d8911d914 Mon Sep 17 00:00:00 2001 From: melohagan <101575380+melohagan@users.noreply.github.com> Date: Tue, 8 Oct 2024 13:56:48 +0100 Subject: [PATCH 7/7] Remove unused properties (#14732) * Remove unused properties * lint --- .../src/middleware/passport/sso/sso.ts | 24 ------------------- .../core/utilities/structures/accounts.ts | 4 ---- .../tests/core/utilities/structures/users.ts | 5 ---- packages/types/src/api/account/accounts.ts | 1 - .../types/src/documents/account/account.ts | 2 -- packages/types/src/documents/global/user.ts | 2 -- 6 files changed, 38 deletions(-) diff --git a/packages/backend-core/src/middleware/passport/sso/sso.ts b/packages/backend-core/src/middleware/passport/sso/sso.ts index ee84f03dae..8901fcc56f 100644 --- a/packages/backend-core/src/middleware/passport/sso/sso.ts +++ b/packages/backend-core/src/middleware/passport/sso/sso.ts @@ -2,7 +2,6 @@ import { generateGlobalUserID } from "../../../db" import { authError } from "../utils" import * as users from "../../../users" import * as context from "../../../context" -import fetch from "node-fetch" import { SaveSSOUserFunction, SSOAuthDetails, @@ -97,28 +96,13 @@ export async function authenticate( return done(null, ssoUser) } -async function getProfilePictureUrl(user: User, details: SSOAuthDetails) { - const pictureUrl = details.profile?._json.picture - if (pictureUrl) { - const response = await fetch(pictureUrl) - if (response.status === 200) { - const type = response.headers.get("content-type") as string - if (type.startsWith("image/")) { - return pictureUrl - } - } - } -} - /** * @returns a user that has been sync'd with third party information */ async function syncUser(user: User, details: SSOAuthDetails): Promise { let firstName let lastName - let pictureUrl let oauth2 - let thirdPartyProfile if (details.profile) { const profile = details.profile @@ -134,12 +118,6 @@ async function syncUser(user: User, details: SSOAuthDetails): Promise { lastName = name.familyName } } - - pictureUrl = await getProfilePictureUrl(user, details) - - thirdPartyProfile = { - ...profile._json, - } } // oauth tokens for future use @@ -155,8 +133,6 @@ async function syncUser(user: User, details: SSOAuthDetails): Promise { providerType: details.providerType, firstName, lastName, - thirdPartyProfile, - pictureUrl, oauth2, } } diff --git a/packages/backend-core/tests/core/utilities/structures/accounts.ts b/packages/backend-core/tests/core/utilities/structures/accounts.ts index daf4965c81..7910f3c423 100644 --- a/packages/backend-core/tests/core/utilities/structures/accounts.ts +++ b/packages/backend-core/tests/core/utilities/structures/accounts.ts @@ -59,10 +59,8 @@ export function ssoAccount(account: Account = cloudAccount()): SSOAccount { accessToken: generator.string(), refreshToken: generator.string(), }, - pictureUrl: generator.url(), provider: provider(), providerType: providerType(), - thirdPartyProfile: {}, } } @@ -76,9 +74,7 @@ export function verifiableSsoAccount( accessToken: generator.string(), refreshToken: generator.string(), }, - pictureUrl: generator.url(), provider: AccountSSOProvider.MICROSOFT, providerType: AccountSSOProviderType.MICROSOFT, - thirdPartyProfile: { id: "abc123" }, } } diff --git a/packages/backend-core/tests/core/utilities/structures/users.ts b/packages/backend-core/tests/core/utilities/structures/users.ts index 0171353e23..ffddae663b 100644 --- a/packages/backend-core/tests/core/utilities/structures/users.ts +++ b/packages/backend-core/tests/core/utilities/structures/users.ts @@ -25,7 +25,6 @@ export const user = (userProps?: Partial>): User => { roles: { app_test: "admin" }, firstName: generator.first(), lastName: generator.last(), - pictureUrl: "http://example.com", tenantId: tenant.id(), ...userProps, } @@ -86,9 +85,5 @@ export function ssoUser( oauth2: opts.details?.oauth2, provider: opts.details?.provider!, providerType: opts.details?.providerType!, - thirdPartyProfile: { - email: base.email, - picture: base.pictureUrl, - }, } } diff --git a/packages/types/src/api/account/accounts.ts b/packages/types/src/api/account/accounts.ts index 1be506e14e..e05c1d0bf3 100644 --- a/packages/types/src/api/account/accounts.ts +++ b/packages/types/src/api/account/accounts.ts @@ -12,7 +12,6 @@ export interface CreateAccountRequest { name?: string password: string provider?: AccountSSOProvider - thirdPartyProfile: object } export interface SearchAccountsRequest { diff --git a/packages/types/src/documents/account/account.ts b/packages/types/src/documents/account/account.ts index c219229889..aac5bf2d20 100644 --- a/packages/types/src/documents/account/account.ts +++ b/packages/types/src/documents/account/account.ts @@ -98,8 +98,6 @@ export interface AccountSSO { provider: AccountSSOProvider providerType: AccountSSOProviderType oauth2?: OAuthTokens - pictureUrl?: string - thirdPartyProfile: any // TODO: define what the google profile looks like } export type SSOAccount = (Account | CloudAccount) & AccountSSO diff --git a/packages/types/src/documents/global/user.ts b/packages/types/src/documents/global/user.ts index 7605f013d1..a1c5b2506f 100644 --- a/packages/types/src/documents/global/user.ts +++ b/packages/types/src/documents/global/user.ts @@ -21,7 +21,6 @@ export interface UserSSO { provider: string // the individual provider e.g. Okta, Auth0, Google providerType: SSOProviderType oauth2?: OAuth2 - thirdPartyProfile?: SSOProfileJson profile?: { displayName?: string name?: { @@ -45,7 +44,6 @@ export interface User extends Document { userId?: string firstName?: string lastName?: string - pictureUrl?: string forceResetPassword?: boolean roles: UserRoles builder?: {