diff --git a/src/remote-config/condition-evaluator-internal.ts b/src/remote-config/condition-evaluator-internal.ts index 2e5c68371f..c0fc0484a8 100644 --- a/src/remote-config/condition-evaluator-internal.ts +++ b/src/remote-config/condition-evaluator-internal.ts @@ -27,7 +27,8 @@ import { CustomSignalCondition, CustomSignalOperator, } from './remote-config-api'; -import * as farmhash from 'farmhash-modern'; +import { createHash } from 'crypto'; + /** * Encapsulates condition evaluation logic to simplify organization and @@ -151,9 +152,8 @@ export class ConditionEvaluator { const seedPrefix = seed && seed.length > 0 ? `${seed}.` : ''; const stringToHash = `${seedPrefix}${context.randomizationId}`; - const hash64 = ConditionEvaluator.hashSeededRandomizationId(stringToHash) - - const instanceMicroPercentile = hash64 % BigInt(100 * 1_000_000); + const hash = ConditionEvaluator.hashSeededRandomizationId(stringToHash) + const instanceMicroPercentile = hash % BigInt(100 * 1_000_000); switch (percentOperator) { case PercentConditionOperator.LESS_OR_EQUAL: @@ -173,18 +173,8 @@ export class ConditionEvaluator { } static hashSeededRandomizationId(seededRandomizationId: string): bigint { - // For consistency with the Remote Config fetch endpoint's percent condition behavior - // we use Farmhash's fingerprint64 algorithm and interpret the resulting unsigned value - // as a signed value. - let hash64 = BigInt.asIntN(64, farmhash.fingerprint64(seededRandomizationId)); - - // Manually negate the hash if its value is less than 0, since Math.abs doesn't - // support BigInt. - if (hash64 < 0) { - hash64 = -hash64; - } - - return hash64; + const hex = createHash('sha256').update(seededRandomizationId).digest('hex'); + return BigInt(`0x${hex}`); } private evaluateCustomSignalCondition( diff --git a/test/unit/remote-config/condition-evaluator.spec.ts b/test/unit/remote-config/condition-evaluator.spec.ts index c1159238b4..8bfa168df6 100644 --- a/test/unit/remote-config/condition-evaluator.spec.ts +++ b/test/unit/remote-config/condition-evaluator.spec.ts @@ -28,7 +28,7 @@ import { } from '../../../src/remote-config/remote-config-api'; import { v4 as uuidv4 } from 'uuid'; import { clone } from 'lodash'; -import * as farmhash from 'farmhash-modern'; +import * as crypto from 'crypto'; const expect = chai.expect; @@ -251,9 +251,8 @@ describe('ConditionEvaluator', () => { it('should use zero for undefined microPercent', () => { // Stubs ID hasher to return a number larger than zero. - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(1n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns('1'); stubs.push(stub); const condition = { @@ -284,9 +283,8 @@ describe('ConditionEvaluator', () => { it('should use zeros for undefined microPercentRange', () => { // Stubs ID hasher to return a number in range. - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(1n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns('1'); stubs.push(stub); const condition = { @@ -317,9 +315,8 @@ describe('ConditionEvaluator', () => { it('should use zero for undefined microPercentUpperBound', () => { // Stubs ID hasher to return a number outside range. - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(1n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns('1'); stubs.push(stub); const condition = { @@ -353,9 +350,8 @@ describe('ConditionEvaluator', () => { it('should use zero for undefined microPercentLowerBound', () => { // Stubs ID hasher to return a number in range. - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(1n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns('1'); stubs.push(stub); const condition = { @@ -388,9 +384,9 @@ describe('ConditionEvaluator', () => { }); it('should evaluate 9 as less or equal to 10', () => { - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(9n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns('9'); + stubs.push(stub); stubs.push(stub); const condition = { @@ -419,9 +415,9 @@ describe('ConditionEvaluator', () => { }); it('should evaluate 10 as less or equal to 10', () => { - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(10n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns((10).toString(16)); + stubs.push(stub); stubs.push(stub); const condition = { @@ -450,9 +446,9 @@ describe('ConditionEvaluator', () => { }); it('should evaluate 11 as not less or equal to 10', () => { - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(11n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns((11).toString(16)); + stubs.push(stub); stubs.push(stub); const condition = { @@ -481,9 +477,9 @@ describe('ConditionEvaluator', () => { }); it('should negate -11 to 11 and evaluate as not less or equal to 10', () => { - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(-11n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns((11).toString(16)); + stubs.push(stub); stubs.push(stub); const condition = { @@ -540,9 +536,9 @@ describe('ConditionEvaluator', () => { }); it('should evaluate 11M as greater than 10M', () => { - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(11n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns((11).toString(16)); + stubs.push(stub); stubs.push(stub); const condition = { @@ -571,9 +567,8 @@ describe('ConditionEvaluator', () => { }); it('should evaluate 9 as not greater than 10', () => { - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(9n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns('9'); stubs.push(stub); const condition = { @@ -661,9 +656,8 @@ describe('ConditionEvaluator', () => { }); it('should evaluate 10 as between 9 and 11', () => { - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(10n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns((10).toString(16)); stubs.push(stub); const condition = { @@ -726,9 +720,8 @@ describe('ConditionEvaluator', () => { }); it('should evaluate 12 as not between 9 and 11', () => { - const stub = sinon - .stub(farmhash, 'fingerprint64') - .returns(12n); + const stub = sinon.stub(crypto.Hash.prototype, 'digest'); + stub.withArgs('hex').returns((12).toString(16)); stubs.push(stub); const condition = { @@ -759,6 +752,51 @@ describe('ConditionEvaluator', () => { expect(actual).to.be.false; }); + describe('known percent condition values', () => { + // This test is useful for ensuring consistency across all places we + // evaluate percent conditions. It creates a set of 10 conditions targeting 50% + // with randomizationIds 0-9 and a constant `seed` value. + const conditionEvaluator = new ConditionEvaluator(); + + const percentCondition = { + percentOperator: PercentConditionOperator.BETWEEN, + microPercentRange: { + microPercentLowerBound: 0, + microPercentUpperBound: 50_000_000 // 50% + }, + }; + + const testCases = [ + { seed: '1', randomizationId: 'one', result: false }, + { seed: '2', randomizationId: 'two', result: false }, + { seed: '3', randomizationId: 'three', result: true }, + { seed: '4', randomizationId: 'four', result: false }, + { seed: '5', randomizationId: 'five', result: true }, + { seed: '', randomizationId: '😊', result: true }, + { seed: '', randomizationId: '😀', result: false }, + { seed: 'hêl£o', randomizationId: 'wørlÐ', result: false }, + { seed: 'řemøťe', randomizationId: 'çōnfįġ', result: true }, + { seed: 'long', randomizationId: '.'.repeat(100), result: true }, + { seed: 'very-long', randomizationId: '.'.repeat(1000), result: false }, + ]; + + testCases.map(({ randomizationId, seed, result }) => { + + const idSummary = randomizationId.length > 25 + ? `a ${randomizationId.length} character randomizationID` + : `"${randomizationId}"`; + + it(`should evaluate ${idSummary} with seed "${seed}" to ${result}`, () => { + const context = { randomizationId }; + const evalResult = conditionEvaluator.evaluateConditions([{ + name: 'is_enabled', + condition: { percent: { ...percentCondition, seed } } + }], context); + expect(evalResult.get('is_enabled')).to.equal(result); + }); + }); + }); + // The following tests are probabilistic. They use tolerances based on // standard deviations to balance accuracy and flakiness. Random IDs will // hash to the target range + 3 standard deviations 99.7% of the time, @@ -958,7 +996,7 @@ describe('ConditionEvaluator', () => { describe('STRING_CONTAINS', () => { const testCases: CustomSignalTestCase[] = [ { targets: ['foo', 'biz'], actual: 'foobar', outcome: true }, - { targets: ['foo', 'biz'],actual: 'bar',outcome: false }, + { targets: ['foo', 'biz'], actual: 'bar', outcome: false }, ]; testCases.forEach(runCustomSignalTestCase(CustomSignalOperator.STRING_CONTAINS)); @@ -966,7 +1004,7 @@ describe('ConditionEvaluator', () => { describe('STRING_DOES_NOT_CONTAIN', () => { const testCases: CustomSignalTestCase[] = [ - { targets: ['foo', 'biz'],actual: 'bar',outcome: true }, + { targets: ['foo', 'biz'], actual: 'bar', outcome: true }, { targets: ['foo', 'biz'], actual: 'foobar', outcome: false }, ]; @@ -1135,77 +1173,4 @@ describe('ConditionEvaluator', () => { }); }); }); - - describe('hashSeededRandomizationId', () => { - // The Farmhash algorithm produces a 64 bit unsigned integer, - // which we convert to a signed integer for legacy compatibility. - // This has caused confusion in the past, so we explicitly - // test here. - it('should leave numbers <= 2^63-1 (max signed long) as is', function () { - if (nodeVersion.startsWith('14')) { - this.skip(); - } - - const stub = sinon - .stub(farmhash, 'fingerprint64') - // 2^63-1 = 9223372036854775807. - .returns(BigInt('9223372036854775807')); - stubs.push(stub); - - const actual = ConditionEvaluator.hashSeededRandomizationId('anything'); - - expect(actual).to.equal(BigInt('9223372036854775807')) - }); - - it('should convert 2^63 to negative (min signed long) and then find the absolute value', function () { - if (nodeVersion.startsWith('14')) { - this.skip(); - } - - const stub = sinon - .stub(farmhash, 'fingerprint64') - // 2^63 = 9223372036854775808. - .returns(BigInt('9223372036854775808')); - stubs.push(stub); - - const actual = ConditionEvaluator.hashSeededRandomizationId('anything'); - - // 2^63 is the negation of 2^63-1 - expect(actual).to.equal(BigInt('9223372036854775808')) - }); - - it('should convert 2^63+1 to negative and then find the absolute value', function () { - if (nodeVersion.startsWith('14')) { - this.skip(); - } - - const stub = sinon - .stub(farmhash, 'fingerprint64') - // 2^63+1 9223372036854775809. - .returns(BigInt('9223372036854775809')); - stubs.push(stub); - - const actual = ConditionEvaluator.hashSeededRandomizationId('anything'); - - // 2^63+1 is larger than 2^63, so the absolute value is smaller - expect(actual).to.equal(BigInt('9223372036854775807')) - }); - - it('should handle the value that initially caused confusion', function () { - if (nodeVersion.startsWith('14')) { - this.skip(); - } - - const stub = sinon - .stub(farmhash, 'fingerprint64') - // We were initially confused about the nature of this value ... - .returns(BigInt('16081085603393958147')); - stubs.push(stub); - - const actual = ConditionEvaluator.hashSeededRandomizationId('anything'); - - // ... Now we know it's the unsigned equivalent of this absolute value. - expect(actual).to.equal(BigInt('2365658470315593469')) - }); - }); });