Merge pull request #44 from flippercloud/fix/actors-vs-percentage-of-actors

Refactor Flipper gate precedence and semantics to match Ruby implementation

Author: jonmagic

packages/flipper/CHANGELOG.md

@@ -1,5 +1,16 @@
 # @flippercloud/flipper
 
+## 1.0.1
+
+### Fixed
+
+- Ruby parity: gate precedence now matches Ruby (boolean → expression → actor → percentage of actors → percentage of time → group).
+- Ruby parity: expressions now evaluate against actor `flipperProperties` (instead of losing properties due to pre-wrapping).
+- Ruby parity: percentage-of-actors bucketing now matches Ruby semantics (CRC32 + scaling factor) and supports up to 3 decimal places.
+- `disableExpression()` now disables only the expression gate (no longer clears unrelated gate values).
+- Memory adapter: disabling numeric gates (percentage-of-actors / percentage-of-time) no longer clears other gate values.
+- Ruby parity: actor/group gates treat missing/invalid actor ids as closed (no more literal `'undefined'` string checks).
+
 ## 1.0.0
 
 Initial release of Flipper feature flags for TypeScript/JavaScript.

packages/flipper/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@flippercloud/flipper",
   "description": "Flipper feature flags for TypeScript/JavaScript",
-  "version": "1.0.0",
+  "version": "1.0.1",
   "author": "Jonathan Hoyt",
   "license": "MIT",
   "main": "./dist/cjs/index.js",

packages/flipper/src/ActorGate.test.ts

@@ -1,4 +1,7 @@
 import ActorGate from './ActorGate'
+import ActorType from './ActorType'
+import FeatureCheckContext from './FeatureCheckContext'
+import GateValues from './GateValues'
 
 const gate = new ActorGate()
 
@@ -8,4 +11,16 @@ describe('ActorGate', () => {
     expect(gate.key).toEqual('actors')
     expect(gate.dataType).toEqual('set')
   })
+
+  test('isOpen returns false when actor flipperId is missing', () => {
+    const values = new GateValues({ actors: ['undefined'] })
+
+    // Note: flipperId is present as a key, but the value is invalid at runtime.
+    // Ruby treats this like "no actor".
+    const actorWithMissingId = { flipperId: undefined as unknown as string }
+    const actorType = ActorType.wrap(actorWithMissingId as unknown)
+
+    const context = new FeatureCheckContext('feature-1', values, actorType)
+    expect(gate.isOpen(context)).toEqual(false)
+  })
 })

packages/flipper/src/ActorGate.ts

@@ -62,17 +62,20 @@ class ActorGate implements IGate {
    * @returns True if the actor is in the enabled set
    */
   public isOpen(context: FeatureCheckContext): boolean {
-    if (context.thing === 'undefined') {
+    if (!this.protectsThing(context.thing)) {
       return false
-    } else {
-      if (this.protectsThing(context.thing)) {
-        const enabledActors = context.actorsValue
-        const actorType = context.thing as ActorType
-        return enabledActors.has(String(actorType.value))
-      } else {
-        return false
-      }
     }
+
+    const enabledActors = context.actorsValue
+    const actorId = context.thing instanceof ActorType ? context.thing.value : (context.thing as any).flipperId
+
+    // Actor ids must be present and string-like (Ruby parity: a missing flipper_id
+    // is treated as no actor).
+    if (typeof actorId !== 'string' || actorId.length === 0) {
+      return false
+    }
+
+    return enabledActors.has(actorId)
   }
 
   /**

packages/flipper/src/Dsl.test.ts

@@ -1,6 +1,7 @@
 import Dsl from './Dsl'
 import { IActor } from './interfaces'
 import MemoryAdapter from './MemoryAdapter'
+import { crc32 } from 'crc'
 import { makeActor } from './testHelpers'
 
 let dsl: Dsl
@@ -30,8 +31,10 @@ describe('Dsl', () => {
   test('enables feature for percentage of actors', async () => {
     await dsl.enablePercentageOfActors('feature-1', 50)
 
-    expect(await dsl.isFeatureEnabled('feature-1', makeActor(5))).toBe(true)
-    expect(await dsl.isFeatureEnabled('feature-1', makeActor(8))).toBe(false)
+    const { enabledActor, disabledActor } = findEnabledAndDisabledActors('feature-1', 50)
+
+    expect(await dsl.isFeatureEnabled('feature-1', enabledActor)).toBe(true)
+    expect(await dsl.isFeatureEnabled('feature-1', disabledActor)).toBe(false)
   })
 
   test('enables feature for percentage of time', async () => {
@@ -61,10 +64,13 @@ describe('Dsl', () => {
 
   test('disables percentage of actors', async () => {
     await dsl.enablePercentageOfActors('feature-1', 50)
-    expect(await dsl.isFeatureEnabled('feature-1', makeActor(5))).toBe(true)
+
+    const { enabledActor } = findEnabledAndDisabledActors('feature-1', 50)
+    expect(await dsl.isFeatureEnabled('feature-1', enabledActor)).toBe(true)
 
     await dsl.disablePercentageOfActors('feature-1')
-    expect(await dsl.isFeatureEnabled('feature-1', makeActor(5))).toBe(false)
+
+    expect(await dsl.isFeatureEnabled('feature-1', enabledActor)).toBe(false)
     expect(await dsl.feature('feature-1').percentageOfActorsValue()).toBe(0)
   })
 
@@ -185,3 +191,39 @@ describe('Dsl', () => {
     })
   })
 })
+
+function rubyPercentageOfActorsOpen(featureName: string, actorId: string, percentage: number): boolean {
+  const scalingFactor = 1000
+  const id = `${featureName}${actorId}`
+  const hash = crc32(id).valueOf()
+  return hash % (100 * scalingFactor) < percentage * scalingFactor
+}
+
+function findEnabledAndDisabledActors(
+  featureName: string,
+  percentage: number
+): { enabledActor: IActor; disabledActor: IActor } {
+  let enabledActor: IActor | null = null
+  let disabledActor: IActor | null = null
+
+  for (let i = 0; i < 100_000; i++) {
+    const candidate = makeActor(i)
+    const open = rubyPercentageOfActorsOpen(featureName, candidate.flipperId, percentage)
+
+    if (!enabledActor && open) {
+      enabledActor = candidate
+    }
+    if (!disabledActor && !open) {
+      disabledActor = candidate
+    }
+    if (enabledActor && disabledActor) {
+      break
+    }
+  }
+
+  if (!enabledActor || !disabledActor) {
+    throw new Error('Failed to find both enabled and disabled actors for percentage-of-actors test')
+  }
+
+  return { enabledActor, disabledActor }
+}

packages/flipper/src/Feature.test.ts

@@ -1,5 +1,6 @@
 import Feature from './Feature'
 import MemoryAdapter from './MemoryAdapter'
+import MemoryInstrumenter from './instrumenters/MemoryInstrumenter'
 import { makeActor } from './testHelpers'
 
 let adapter: MemoryAdapter
@@ -11,6 +12,49 @@ describe('Feature', () => {
     feature = new Feature('feature-1', adapter, {})
   })
 
+  describe('gate precedence (Ruby parity)', () => {
+    test('percentage of time wins over group when both are enabled', async () => {
+      const { default: Dsl } = await import('./Dsl.js')
+      const dsl = new Dsl(adapter)
+      dsl.register('admins', (_actor: unknown) => true)
+
+      const instrumenter = new MemoryInstrumenter()
+      const featureWithGroups = new Feature('precedence-feature', adapter, dsl.groups, {
+        instrumenter,
+      })
+
+      const actor = makeActor(1)
+      await featureWithGroups.enableGroup('admins')
+      await featureWithGroups.enablePercentageOfTime(100)
+
+      instrumenter.reset()
+      expect(await featureWithGroups.isEnabled(actor)).toEqual(true)
+      const event = instrumenter.eventByName('feature_operation.flipper')
+      expect(event?.payload.gate_name).toEqual('percentageOfTime')
+    })
+
+    test('expression evaluates using actor properties', async () => {
+      const { default: Flipper } = await import('./Flipper.js')
+
+      const instrumenter = new MemoryInstrumenter()
+      const featureWithInstrumenter = new Feature('expression-actor-props', adapter, {}, {
+        instrumenter,
+      })
+
+      await featureWithInstrumenter.enableExpression(Flipper.property('admin'))
+
+      const actorWithProps = {
+        flipperId: 'actor:1',
+        flipperProperties: { admin: true },
+      }
+
+      instrumenter.reset()
+      expect(await featureWithInstrumenter.isEnabled(actorWithProps)).toEqual(true)
+      const event = instrumenter.eventByName('feature_operation.flipper')
+      expect(event?.payload.gate_name).toEqual('expression')
+    })
+  })
+
   test('has name', () => {
     expect(feature.name).toEqual('feature-1')
   })
@@ -32,6 +76,33 @@ describe('Feature', () => {
     expect(await feature.isEnabled(actor)).toEqual(false)
   })
 
+  describe('disableExpression', () => {
+    test('only clears the expression gate (does not clear other gate values)', async () => {
+      const { default: Flipper } = await import('./Flipper.js')
+
+      const enabledActor = makeActor(1)
+      const expressionActor = {
+        flipperId: 'actor:2',
+        flipperProperties: { admin: true },
+      }
+
+      await feature.enableActor(enabledActor)
+      await feature.enableExpression(Flipper.property('admin'))
+
+      // Expression is active pre-disable.
+      expect(await feature.isEnabled(expressionActor)).toEqual(true)
+
+      await feature.disableExpression()
+
+      // Actor enablement should remain.
+      expect(await feature.isEnabled(enabledActor)).toEqual(true)
+
+      // Expression should be removed.
+      expect(await feature.isEnabled(expressionActor)).toEqual(false)
+      expect(await feature.enabledGateNames()).toEqual(['actor'])
+    })
+  })
+
   describe('disablePercentageOfActors', () => {
     test('sets percentage to 0', async () => {
       await feature.enablePercentageOfActors(25)
@@ -53,6 +124,18 @@ describe('Feature', () => {
       await feature.disablePercentageOfActors()
       expect(await feature.percentageOfActorsValue()).toEqual(0)
     })
+
+    test('does not clear other gate values', async () => {
+      const actor = makeActor(5)
+
+      await feature.enableActor(actor)
+      await feature.enablePercentageOfActors(50)
+
+      await feature.disablePercentageOfActors()
+
+      // Actor enablement should remain after disabling the percentage gate.
+      expect(await feature.isEnabled(actor)).toEqual(true)
+    })
   })
 
   describe('disablePercentageOfTime', () => {
@@ -75,6 +158,18 @@ describe('Feature', () => {
       await feature.disablePercentageOfTime()
       expect(await feature.percentageOfTimeValue()).toEqual(0)
     })
+
+    test('does not clear other gate values', async () => {
+      const actor = makeActor(5)
+
+      await feature.enableActor(actor)
+      await feature.enablePercentageOfTime(50)
+
+      await feature.disablePercentageOfTime()
+
+      // Actor enablement should remain after disabling the percentage gate.
+      expect(await feature.isEnabled(actor)).toEqual(true)
+    })
   })
 
   describe('state', () => {

packages/flipper/src/Feature.ts

@@ -109,9 +109,9 @@ class Feature {
       new BooleanGate(),
       new ExpressionGate(),
       new ActorGate(),
-      new GroupGate(this.groups),
       new PercentageOfActorsGate(),
       new PercentageOfTimeGate(),
+      new GroupGate(this.groups),
     ]
   }
 
@@ -253,12 +253,18 @@ class Feature {
    * @returns True if successful
    */
   async disableExpression(): Promise<boolean> {
-    const gate = this.gate('expression')
+    const gate = this.gates.find(g => g.name === 'expression')
     if (!gate) {
       throw new Error('Expression gate not found')
     }
+
     await this.adapter.add(this)
-    return this.adapter.clear(this)
+
+    // Adapter APIs require a "thing" value for disable(). For JSON gates (like
+    // expression), adapters should ignore the specific value and simply remove
+    // the stored gate key.
+    const placeholder = ExpressionType.wrap({ Constant: false })
+    return this.adapter.disable(this, gate, placeholder)
   }
 
   /**
@@ -277,10 +283,17 @@ class Feature {
 
       this.gates.some(gate => {
         let thingType: unknown = thing
-        const actorGate = this.gate('actor')
-        if (typeof thingType !== 'undefined' && actorGate) {
-          thingType = actorGate.wrap(thing)
+
+        // Only wrap the thing for gates that require ActorType.
+        // Wrapping everything breaks expression evaluation, which needs access
+        // to actor `flipperProperties`.
+        if (gate.name === 'actor' || gate.name === 'group') {
+          const actorGate = this.gate('actor')
+          if (actorGate && actorGate.protectsThing(thingType)) {
+            thingType = actorGate.wrap(thingType)
+          }
         }
+
         const context = new FeatureCheckContext(this.name, values, thingType)
         const isOpen = gate.isOpen(context)
         if (isOpen) {

packages/flipper/src/GroupGate.test.ts

@@ -1,11 +1,32 @@
+import ActorType from './ActorType'
+import FeatureCheckContext from './FeatureCheckContext'
+import GateValues from './GateValues'
 import GroupGate from './GroupGate'
-
-const gate = new GroupGate({})
+import GroupType from './GroupType'
 
 describe('GroupGate', () => {
+  const gate = new GroupGate({})
+
   test('has name, key, and dataType', () => {
     expect(gate.name).toBe('group')
     expect(gate.key).toBe('groups')
     expect(gate.dataType).toBe('set')
   })
+
+  test('isOpen returns false when actor flipperId is missing (even if group callback matches)', () => {
+    const groups = {
+      admins: new GroupType('admins', () => true),
+    }
+
+    const groupGate = new GroupGate(groups)
+    const values = new GateValues({ groups: ['admins'] })
+
+    // Note: flipperId is present as a key, but the value is invalid at runtime.
+    // Ruby treats this like "no actor".
+    const actorWithMissingId = { flipperId: undefined as unknown as string, role: 'admin' }
+    const actorType = ActorType.wrap(actorWithMissingId as unknown)
+
+    const context = new FeatureCheckContext('feature-1', values, actorType)
+    expect(groupGate.isOpen(context)).toEqual(false)
+  })
 })