Improve testing of feature flags by not polluting production flags with test ones.

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,

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<T> {
+export abstract class Flag<T> {
   static boolean(defaultValue: boolean): Flag<boolean> {
     return new BooleanFlag(defaultValue)
   }
@@ -33,6 +32,16 @@ abstract class Flag<T> {
   abstract parse(value: any): T
 }
 
+type UnwrapFlag<F> = F extends Flag<infer U> ? U : never
+
+export type FlagValues<T> = {
+  [K in keyof T]: UnwrapFlag<T[K]>
+}
+
+type KeysOfType<T, U> = {
+  [K in keyof T]: T[K] extends Flag<U> ? K : never
+}[keyof T]
+
 class BooleanFlag extends Flag<boolean> {
   parse(value: any) {
     if (typeof value === "string") {
@@ -73,149 +82,123 @@ class NumberFlag extends Flag<number> {
   }
 }
 
+export class FlagSet<V extends Flag<any>, T extends { [key: string]: V }> {
+  private readonly flags: T
+
+  constructor(flags: T) {
+    this.flags = flags
+  }
+
+  defaults(): FlagValues<T> {
+    return Object.keys(this.flags).reduce((acc, key) => {
+      const typedKey = key as keyof T
+      acc[typedKey] = this.flags[key].defaultValue
+      return acc
+    }, {} as FlagValues<T>)
+  }
+
+  isFlagName(name: string | number | symbol): name is keyof T {
+    return this.flags[name as keyof T] !== undefined
+  }
+
+  async get<K extends keyof T>(key: K): Promise<FlagValues<T>[K]> {
+    const flags = await this.fetch()
+    return flags[key]
+  }
+
+  async isEnabled<K extends KeysOfType<T, boolean>>(key: K): Promise<boolean> {
+    const flags = await this.fetch()
+    return flags[key]
+  }
+
+  async fetch(): Promise<FlagValues<T>> {
+    return await tracer.trace("features.fetch", async span => {
+      const tags: Record<string, any> = {}
+
+      const flags = this.defaults()
+
+      const currentTenantId = context.getTenantId()
+      const specificallySetFalse = new Set<string>()
+
+      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> = F extends Flag<infer U> ? 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<Flags> {
-  return await tracer.trace("features.fetch", async span => {
-    const tags: Record<string, any> = {}
-
-    const flags = defaultFlags()
-
-    const currentTenantId = context.getTenantId()
-    const specificallySetFalse = new Set<string>()
-
-    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<K extends keyof Flags>(name: K): Promise<Flags[K]> {
-  const flags = await fetch()
-  return flags[name]
-}
-
-type BooleanFlags = {
-  [K in keyof typeof FLAGS]: (typeof FLAGS)[K] extends Flag<boolean> ? K : never
-}[keyof typeof FLAGS]
-
-// Convenience function for boolean flag values. This makes callsites more
-// readable for boolean flags.
-export async function isEnabled<K extends BooleanFlags>(
-  name: K
-): Promise<boolean> {
-  const flags = await fetch()
-  return flags[name]
-}

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<Flags>
+    expected: Partial<FlagValues<typeof schema>>
   }
 
   it.each<TestCase>([
     {
       tenant: "tenant1",
-      flags: "tenant1:_TEST_BOOLEAN",
+      flags: "tenant1:TEST_BOOLEAN",
-      expected: { _TEST_BOOLEAN: true },
+      expected: { TEST_BOOLEAN: true },
     },
     {
       tenant: "tenant1",
-      flags: "tenant1:!_TEST_BOOLEAN",
+      flags: "tenant1:!TEST_BOOLEAN",
-      expected: { _TEST_BOOLEAN: false },
+      expected: { TEST_BOOLEAN: false },
     },
     {
       tenant: "tenant1",
-      flags: "*:_TEST_BOOLEAN",
+      flags: "*:TEST_BOOLEAN",
-      expected: { _TEST_BOOLEAN: true },
+      expected: { TEST_BOOLEAN: true },
     },
     {
       tenant: "tenant1",
-      flags: "tenant2:_TEST_BOOLEAN",
+      flags: "tenant2:TEST_BOOLEAN",
-      expected: { _TEST_BOOLEAN: false },
+      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 () => {
+        withEnv({ TENANT_FEATURE_FLAGS: envFlags }, async () => {
-          const flags = await fetch()
+          const values = await flags.fetch()
-          expect(flags).toMatchObject(expected)
+          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<FailedTestCase>([
     {
       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 }, () =>
+        withEnv({ TENANT_FEATURE_FLAGS: envFlags }, () =>
-          expect(fetch).rejects.toThrow(expected)
+          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()
+        const values = await flags.fetch()
-        expect(flags._TEST_BOOLEAN).toBe(true)
+        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()
+        const values = await flags.fetch()
-        expect(flags._TEST_STRING).toBe("test payload")
+        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()
+        const values = await flags.fetch()
-        expect(flags._TEST_NUMBER).toBe(123)
+        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()
+            const values = await flags.fetch()
-            expect(flags._TEST_BOOLEAN).toBe(true)
+            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()
+            const values = await flags.fetch()
-            expect(flags._TEST_BOOLEAN).toBe(false)
+            expect(values.TEST_BOOLEAN).toBe(false)
           })
         }
       )

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)