From 36044430aa2044a773025fd9d0a618551e592ad7 Mon Sep 17 00:00:00 2001 From: Erik Eldridge Date: Tue, 16 Jul 2024 14:12:54 -0700 Subject: [PATCH] Remove Remote Config dependency on Long JS (#2614) The conditional logic used by the Remote Config config fetch endpoint hashes randomization IDs into 64 bit numbers (via an algorithm called Farmhash). JS numbers are limited to 53 bits, so we used the long.js package to emulate the fetch endpoint logic in the Admin SDK. PR 2603 migrated the Admin SDK to use a Farmhash library that produces BigInt, which supports 64 bit numbers natively, so we can remove the long.js dependency. Additionally, the SDK currently converts the BigInt to a string before converting the string to a longjs object, which is clunky; and Long JS broke the SDK's ES6 compatibility. --------- Co-authored-by: Lahiru Maramba --- package-lock.json | 3 +- package.json | 1 - .../condition-evaluator-internal.ts | 37 ++++++---- .../remote-config/condition-evaluator.spec.ts | 73 +++++++++++++++++++ 4 files changed, 97 insertions(+), 17 deletions(-) diff --git a/package-lock.json b/package-lock.json index 3f55635f72..604a7fb1b1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5651,7 +5651,8 @@ "long": { "version": "5.2.3", "resolved": "https://registry.npmjs.org/long/-/long-5.2.3.tgz", - "integrity": "sha512-lcHwpNoggQTObv5apGNCTdJrO69eHOZMi4BNC+rTLER8iHAqGrUVeLh/irVIM7zTw2bOXA8T6uNPeujwOLg/2Q==" + "integrity": "sha512-lcHwpNoggQTObv5apGNCTdJrO69eHOZMi4BNC+rTLER8iHAqGrUVeLh/irVIM7zTw2bOXA8T6uNPeujwOLg/2Q==", + "optional": true }, "loupe": { "version": "2.3.7", diff --git a/package.json b/package.json index 6a68c36302..a31cc77ae3 100644 --- a/package.json +++ b/package.json @@ -203,7 +203,6 @@ "farmhash-modern": "^1.1.0", "jsonwebtoken": "^9.0.0", "jwks-rsa": "^3.1.0", - "long": "^5.2.3", "node-forge": "^1.3.1", "uuid": "^10.0.0" }, diff --git a/src/remote-config/condition-evaluator-internal.ts b/src/remote-config/condition-evaluator-internal.ts index 46803f8402..2be55ec08d 100644 --- a/src/remote-config/condition-evaluator-internal.ts +++ b/src/remote-config/condition-evaluator-internal.ts @@ -26,7 +26,6 @@ import { PercentConditionOperator } from './remote-config-api'; import * as farmhash from 'farmhash-modern'; -import long = require('long'); /** * Encapsulates condition evaluation logic to simplify organization and @@ -147,26 +146,18 @@ export class ConditionEvaluator { const seedPrefix = seed && seed.length > 0 ? `${seed}.` : ''; const stringToHash = `${seedPrefix}${context.randomizationId}`; + const hash64 = ConditionEvaluator.hashSeededRandomizationId(stringToHash) - // Using a 64-bit long for consistency with the Remote Config fetch endpoint. - let hash64 = long.fromString(farmhash.fingerprint64(stringToHash).toString()); + const instanceMicroPercentile = hash64 % BigInt(100 * 1_000_000); - // Negate the hash if its value is less than 0. We handle this manually because the - // Long library doesn't provided an absolute value method. - if (hash64.lt(0)) { - hash64 = hash64.negate(); - } - - const instanceMicroPercentile = hash64.mod(100 * 1_000_000); - switch (percentOperator) { case PercentConditionOperator.LESS_OR_EQUAL: - return instanceMicroPercentile.lte(normalizedMicroPercent); + return instanceMicroPercentile <= normalizedMicroPercent; case PercentConditionOperator.GREATER_THAN: - return instanceMicroPercentile.gt(normalizedMicroPercent); + return instanceMicroPercentile > normalizedMicroPercent; case PercentConditionOperator.BETWEEN: - return instanceMicroPercentile.gt(normalizedMicroPercentLowerBound) - && instanceMicroPercentile.lte(normalizedMicroPercentUpperBound); + return instanceMicroPercentile > normalizedMicroPercentLowerBound + && instanceMicroPercentile <= normalizedMicroPercentUpperBound; case PercentConditionOperator.UNKNOWN: default: break; @@ -175,4 +166,20 @@ export class ConditionEvaluator { // TODO: add logging once we have a wrapped logger. return false; } + + // Visible for testing + 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; + } } diff --git a/test/unit/remote-config/condition-evaluator.spec.ts b/test/unit/remote-config/condition-evaluator.spec.ts index 5bb0f89133..17e524f6d8 100644 --- a/test/unit/remote-config/condition-evaluator.spec.ts +++ b/test/unit/remote-config/condition-evaluator.spec.ts @@ -858,4 +858,77 @@ 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')) + }); + }); });