From 5732ea9a4df1d5e66e85ade0fc5d2e78b790c48d Mon Sep 17 00:00:00 2001 From: Erik Eldridge Date: Fri, 29 Mar 2024 18:38:57 -0700 Subject: [PATCH 01/10] Update SDK to use type-specific getters --- src/remote-config/remote-config-api.ts | 91 +++++++++++++++++- src/remote-config/remote-config.ts | 94 +++++++++++-------- test/unit/remote-config/remote-config.spec.ts | 25 ++--- 3 files changed, 156 insertions(+), 54 deletions(-) diff --git a/src/remote-config/remote-config-api.ts b/src/remote-config/remote-config-api.ts index e102094cde..740ac3fa97 100644 --- a/src/remote-config/remote-config-api.ts +++ b/src/remote-config/remote-config-api.ts @@ -361,7 +361,7 @@ export interface ServerTemplateOptions { * intended before it connects to the Remote Config backend, and so that * default values are available if none are set on the backend. */ - defaultConfig?: ServerConfig, + defaultConfig?: { [key: string]: string | number | boolean }; /** * Enables integrations to use template data loaded independently. For @@ -385,7 +385,7 @@ export interface ServerTemplate { /** * A {@link ServerConfig} that contains default Config values. */ - defaultConfig: ServerConfig; + defaultConfig: { [key: string]: string | number | boolean }; /** * Evaluates the current template to produce a {@link ServerConfig}. @@ -537,4 +537,89 @@ export interface ListVersionsOptions { /** * Represents the configuration produced by evaluating a server template. */ -export type ServerConfig = { [key: string]: string | boolean | number } +export interface ServerConfig { + + /** + * Gets the value for the given key as a boolean. + * + * Convenience method for calling serverConfig.getValue(key).asBoolean(). + * + * @param key - The name of the parameter. + * + * @returns The value for the given key as a boolean. + */ + getBoolean(key: string): boolean; + + /** + * Gets the value for the given key as a number. + * + * Convenience method for calling serverConfig.getValue(key).asNumber(). + * + * @param key - The name of the parameter. + * + * @returns The value for the given key as a number. + */ + getNumber(key: string): number; + + /** + * Gets the value for the given key as a string. + * Convenience method for calling serverConfig.getValue(key).asString(). + * + * @param key - The name of the parameter. + * + * @returns The value for the given key as a string. + */ + getString(key: string): string; + + /** + * Gets the {@link Value} for the given key. + * + * @param key - The name of the parameter. + * + * @returns The value for the given key. + */ + getValue(key: string): Value; +} + +/** + * Wraps a parameter value with metadata and type-safe getters. + * + * Type-safe getters insulate application logic from remote + * changes to parameter names and types. + */ +export interface Value { + + /** + * Gets the value as a boolean. + * + * The following values (case insensitive) are interpreted as true: + * "1", "true", "t", "yes", "y", "on". Other values are interpreted as false. + */ + asBoolean(): boolean; + + /** + * Gets the value as a number. Comparable to calling Number(value) || 0. + */ + asNumber(): number; + + /** + * Gets the value as a string. + */ + asString(): string; + + /** + * Gets the {@link ValueSource} for the given key. + */ + getSource(): ValueSource; +} + +/** + * Indicates the source of a value. + * + * + */ +export type ValueSource = 'static' | 'default' | 'remote'; diff --git a/src/remote-config/remote-config.ts b/src/remote-config/remote-config.ts index 1a720f9220..bc1b74d334 100644 --- a/src/remote-config/remote-config.ts +++ b/src/remote-config/remote-config.ts @@ -30,13 +30,14 @@ import { Version, ExplicitParameterValue, InAppDefaultValue, - ParameterValueType, ServerConfig, RemoteConfigParameterValue, EvaluationContext, ServerTemplateData, ServerTemplateOptions, NamedCondition, + Value, + ValueSource, } from './remote-config-api'; /** @@ -296,7 +297,7 @@ class ServerTemplateImpl implements ServerTemplate { constructor( private readonly apiClient: RemoteConfigApiClient, private readonly conditionEvaluator: ConditionEvaluator, - public readonly defaultConfig: ServerConfig = {} + public readonly defaultConfig: { [key: string]: string | number | boolean } = {} ) { } /** @@ -326,10 +327,10 @@ class ServerTemplateImpl implements ServerTemplate { const evaluatedConditions = this.conditionEvaluator.evaluateConditions( this.cache.conditions, context); - const evaluatedConfig: ServerConfig = {}; + const evaluatedConfig: { [key: string]: string } = {}; for (const [key, parameter] of Object.entries(this.cache.parameters)) { - const { conditionalValues, defaultValue, valueType } = parameter; + const { conditionalValues, defaultValue } = parameter; // Supports parameters with no conditional values. const normalizedConditionalValues = conditionalValues || {}; @@ -352,7 +353,7 @@ class ServerTemplateImpl implements ServerTemplate { if (parameterValueWrapper) { const parameterValue = (parameterValueWrapper as ExplicitParameterValue).value; - evaluatedConfig[key] = this.parseRemoteConfigParameterValue(valueType, parameterValue); + evaluatedConfig[key] = parameterValue; continue; } @@ -367,47 +368,62 @@ class ServerTemplateImpl implements ServerTemplate { } const parameterDefaultValue = (defaultValue as ExplicitParameterValue).value; - evaluatedConfig[key] = this.parseRemoteConfigParameterValue(valueType, parameterDefaultValue); + evaluatedConfig[key] = parameterDefaultValue; } - const mergedConfig = {}; - - // Merges default config and rendered config, prioritizing the latter. - Object.assign(mergedConfig, this.defaultConfig, evaluatedConfig); - - // Enables config to be a convenient object, but with the ability to perform additional - // functionality when a value is retrieved. - const proxyHandler = { - get(target: ServerConfig, prop: string) { - return target[prop]; - } - }; - - return new Proxy(mergedConfig, proxyHandler); + return new ServerConfigImpl(evaluatedConfig, this.defaultConfig); } +} - /** - * Private helper method that coerces a parameter value string to the {@link ParameterValueType}. - */ - private parseRemoteConfigParameterValue(parameterType: ParameterValueType | undefined, - parameterValue: string): string | number | boolean { - const BOOLEAN_TRUTHY_VALUES = ['1', 'true', 't', 'yes', 'y', 'on']; - const DEFAULT_VALUE_FOR_NUMBER = 0; - const DEFAULT_VALUE_FOR_STRING = ''; - - if (parameterType === 'BOOLEAN') { - return BOOLEAN_TRUTHY_VALUES.indexOf(parameterValue) >= 0; - } else if (parameterType === 'NUMBER') { - const num = Number(parameterValue); - if (isNaN(num)) { - return DEFAULT_VALUE_FOR_NUMBER; - } - return num; +class ServerConfigImpl implements ServerConfig { + constructor( + private readonly evaluatedConfig: { [key: string]: string }, + private readonly defaultConfig: { [key: string]: string | number | boolean } + ){} + getBoolean(key: string): boolean { + return this.getValue(key).asBoolean(); + } + getNumber(key: string): number { + return this.getValue(key).asNumber(); + } + getString(key: string): string { + return this.getValue(key).asString(); + } + getValue(key: string): Value { + if (key in this.evaluatedConfig) { + return new ValueImpl('remote', this.evaluatedConfig[key]); + } else if (key in this.defaultConfig) { + return new ValueImpl('default', String(this.defaultConfig[key])); } else { - // Treat everything else as string - return parameterValue || DEFAULT_VALUE_FOR_STRING; + return new ValueImpl('static'); + } + } +} + +class ValueImpl implements Value { + static BOOLEAN_TRUTHY_VALUES = ['1', 'true', 't', 'yes', 'y', 'on']; + static DEFAULT_VALUE_FOR_NUMBER = 0; + static DEFAULT_VALUE_FOR_STRING = ''; + constructor( + private readonly source: ValueSource, + private readonly value = ValueImpl.DEFAULT_VALUE_FOR_STRING){} + asBoolean(): boolean { + return ValueImpl.BOOLEAN_TRUTHY_VALUES.indexOf(this.value) >= 0; + } + asNumber(): number { + const num = Number(this.value); + if (isNaN(num)) { + return ValueImpl.DEFAULT_VALUE_FOR_NUMBER; } + return num; + } + asString(): string { + return this.value; } + getSource(): ValueSource { + return this.source; + } + } /** diff --git a/test/unit/remote-config/remote-config.spec.ts b/test/unit/remote-config/remote-config.spec.ts index 81e34fa4b7..be6de50a13 100644 --- a/test/unit/remote-config/remote-config.spec.ts +++ b/test/unit/remote-config/remote-config.spec.ts @@ -921,10 +921,9 @@ describe('RemoteConfig', () => { return remoteConfig.getServerTemplate() .then((template: ServerTemplate) => { const config = template.evaluate!(); - expect(config.dog_type).to.equal('corgi'); - expect(config.dog_type_enabled).to.equal(true); - expect(config.dog_age).to.equal(22); - expect(config.dog_jsonified).to.equal('{"name":"Taro","breed":"Corgi","age":1,"fluffiness":100}'); + expect(config.getString('dog_type')).to.equal('corgi'); + expect(config.getBoolean('dog_type_enabled')).to.equal(true); + expect(config.getNumber('dog_age')).to.equal(22); }); }); @@ -963,7 +962,7 @@ describe('RemoteConfig', () => { } }); const config = template.evaluate(); - expect(config.is_enabled).to.be.true; + expect(config.getBoolean('is_enabled')).to.be.true; }); it('honors condition order', () => { @@ -1025,7 +1024,7 @@ describe('RemoteConfig', () => { } }); const config = template.evaluate(); - expect(config.dog_type).to.eq('corgi'); + expect(config.getString('dog_type')).to.eq('corgi'); }); it('uses local default if parameter not in template', () => { @@ -1040,7 +1039,7 @@ describe('RemoteConfig', () => { }) .then((template: ServerTemplate) => { const config = template.evaluate!(); - expect(config.dog_coat).to.equal(template.defaultConfig.dog_coat); + expect(config.getString('dog_coat')).to.equal(template.defaultConfig.dog_coat); }); }); @@ -1056,7 +1055,8 @@ describe('RemoteConfig', () => { }) .then((template: ServerTemplate) => { const config = template.evaluate!(); - expect(config.dog_no_remote_default_value).to.equal(template.defaultConfig.dog_no_remote_default_value); + expect(config.getString('dog_no_remote_default_value')).to.equal( + template.defaultConfig.dog_no_remote_default_value); }); }); @@ -1072,7 +1072,8 @@ describe('RemoteConfig', () => { }) .then((template: ServerTemplate) => { const config = template.evaluate!(); - expect(config.dog_use_inapp_default).to.equal(template.defaultConfig.dog_use_inapp_default); + expect(config.getString('dog_use_inapp_default')).to.equal( + template.defaultConfig.dog_use_inapp_default); }); }); @@ -1102,7 +1103,7 @@ describe('RemoteConfig', () => { let config = template.evaluate(); - expect(config.dog_type).to.equal('pug'); + expect(config.getString('dog_type')).to.equal('pug'); response.parameters = { dog_type: { @@ -1117,7 +1118,7 @@ describe('RemoteConfig', () => { config = template.evaluate(); - expect(config.dog_type).to.equal('corgi'); + expect(config.getString('dog_type')).to.equal('corgi'); }); it('overrides local default when remote value exists', () => { @@ -1146,7 +1147,7 @@ describe('RemoteConfig', () => { .then((template: ServerTemplate) => { const config = template.evaluate(); // Asserts remote value overrides local default. - expect(config.dog_type_enabled).to.be.true; + expect(config.getBoolean('dog_type_enabled')).to.be.true; }); }); }); From a59da2f7bca93ad2489f493910e2b04e624f6da7 Mon Sep 17 00:00:00 2001 From: Erik Eldridge Date: Fri, 29 Mar 2024 18:48:56 -0700 Subject: [PATCH 02/10] Stringify default config on construction --- src/remote-config/remote-config.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/remote-config/remote-config.ts b/src/remote-config/remote-config.ts index bc1b74d334..7186683944 100644 --- a/src/remote-config/remote-config.ts +++ b/src/remote-config/remote-config.ts @@ -293,12 +293,20 @@ class RemoteConfigTemplateImpl implements RemoteConfigTemplate { */ class ServerTemplateImpl implements ServerTemplate { public cache: ServerTemplateData; + private defaultConfigAsString: {[key: string]: string} = {}; constructor( private readonly apiClient: RemoteConfigApiClient, private readonly conditionEvaluator: ConditionEvaluator, public readonly defaultConfig: { [key: string]: string | number | boolean } = {} - ) { } + ) { + // RC stores all remote values as string, but it's more intuitive + // to declare default values with specific types, so this converts + // the external declaration to an internal string representation. + for (const key in defaultConfig) { + this.defaultConfigAsString[key] = String(defaultConfig[key]); + } + } /** * Fetches and caches the current active version of the project's {@link ServerTemplate}. @@ -371,14 +379,14 @@ class ServerTemplateImpl implements ServerTemplate { evaluatedConfig[key] = parameterDefaultValue; } - return new ServerConfigImpl(evaluatedConfig, this.defaultConfig); + return new ServerConfigImpl(evaluatedConfig, this.defaultConfigAsString); } } class ServerConfigImpl implements ServerConfig { constructor( private readonly evaluatedConfig: { [key: string]: string }, - private readonly defaultConfig: { [key: string]: string | number | boolean } + private readonly defaultConfig: { [key: string]: string } ){} getBoolean(key: string): boolean { return this.getValue(key).asBoolean(); @@ -393,7 +401,7 @@ class ServerConfigImpl implements ServerConfig { if (key in this.evaluatedConfig) { return new ValueImpl('remote', this.evaluatedConfig[key]); } else if (key in this.defaultConfig) { - return new ValueImpl('default', String(this.defaultConfig[key])); + return new ValueImpl('default', this.defaultConfig[key]); } else { return new ValueImpl('static'); } From 9bb2303a65fac7207ce4dcec3cd8f9d0c4609874 Mon Sep 17 00:00:00 2001 From: Erik Eldridge Date: Fri, 29 Mar 2024 19:37:17 -0700 Subject: [PATCH 03/10] Move Value instantiation to evaluation stage --- src/remote-config/remote-config-api.ts | 8 +++++-- src/remote-config/remote-config.ts | 30 ++++++++++++-------------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/remote-config/remote-config-api.ts b/src/remote-config/remote-config-api.ts index 740ac3fa97..d66d6a44cb 100644 --- a/src/remote-config/remote-config-api.ts +++ b/src/remote-config/remote-config-api.ts @@ -574,6 +574,9 @@ export interface ServerConfig { /** * Gets the {@link Value} for the given key. * + * Ensures application logic will always have a type-safe reference, + * even if the parameter is removed remotely. + * * @param key - The name of the parameter. * * @returns The value for the given key. @@ -583,7 +586,7 @@ export interface ServerConfig { /** * Wraps a parameter value with metadata and type-safe getters. - * + * * Type-safe getters insulate application logic from remote * changes to parameter names and types. */ @@ -619,7 +622,8 @@ export interface Value { *
    *
  • "static" indicates the value was defined by a static constant.
    • *
  • "default" indicates the value was defined by default config.
    • - *
  • "remote" indicates the value was defined by fetched config.
    • + *
  • "remote" indicates the value was defined by config produced by + * evaluating a template.
    • *
*/ export type ValueSource = 'static' | 'default' | 'remote'; diff --git a/src/remote-config/remote-config.ts b/src/remote-config/remote-config.ts index 7186683944..5b55922108 100644 --- a/src/remote-config/remote-config.ts +++ b/src/remote-config/remote-config.ts @@ -293,7 +293,7 @@ class RemoteConfigTemplateImpl implements RemoteConfigTemplate { */ class ServerTemplateImpl implements ServerTemplate { public cache: ServerTemplateData; - private defaultConfigAsString: {[key: string]: string} = {}; + private stringifiedDefaultConfig: {[key: string]: string} = {}; constructor( private readonly apiClient: RemoteConfigApiClient, @@ -304,7 +304,7 @@ class ServerTemplateImpl implements ServerTemplate { // to declare default values with specific types, so this converts // the external declaration to an internal string representation. for (const key in defaultConfig) { - this.defaultConfigAsString[key] = String(defaultConfig[key]); + this.stringifiedDefaultConfig[key] = String(defaultConfig[key]); } } @@ -335,8 +335,14 @@ class ServerTemplateImpl implements ServerTemplate { const evaluatedConditions = this.conditionEvaluator.evaluateConditions( this.cache.conditions, context); - const evaluatedConfig: { [key: string]: string } = {}; + const configValues: { [key: string]: Value } = {}; + // Initializes config Value objects with default values. + for (const key in this.stringifiedDefaultConfig) { + configValues[key] = new ValueImpl('default', this.stringifiedDefaultConfig[key]); + } + + // Overlays config Value objects derived by evaluating the template. for (const [key, parameter] of Object.entries(this.cache.parameters)) { const { conditionalValues, defaultValue } = parameter; @@ -361,7 +367,7 @@ class ServerTemplateImpl implements ServerTemplate { if (parameterValueWrapper) { const parameterValue = (parameterValueWrapper as ExplicitParameterValue).value; - evaluatedConfig[key] = parameterValue; + configValues[key] = new ValueImpl('remote', parameterValue); continue; } @@ -376,17 +382,16 @@ class ServerTemplateImpl implements ServerTemplate { } const parameterDefaultValue = (defaultValue as ExplicitParameterValue).value; - evaluatedConfig[key] = parameterDefaultValue; + configValues[key] = new ValueImpl('remote', parameterDefaultValue); } - return new ServerConfigImpl(evaluatedConfig, this.defaultConfigAsString); + return new ServerConfigImpl(configValues); } } class ServerConfigImpl implements ServerConfig { constructor( - private readonly evaluatedConfig: { [key: string]: string }, - private readonly defaultConfig: { [key: string]: string } + private readonly configValues: { [key: string]: Value }, ){} getBoolean(key: string): boolean { return this.getValue(key).asBoolean(); @@ -398,13 +403,7 @@ class ServerConfigImpl implements ServerConfig { return this.getValue(key).asString(); } getValue(key: string): Value { - if (key in this.evaluatedConfig) { - return new ValueImpl('remote', this.evaluatedConfig[key]); - } else if (key in this.defaultConfig) { - return new ValueImpl('default', this.defaultConfig[key]); - } else { - return new ValueImpl('static'); - } + return this.configValues[key] || new ValueImpl('static'); } } @@ -431,7 +430,6 @@ class ValueImpl implements Value { getSource(): ValueSource { return this.source; } - } /** From fbecfe7281395e7a471663d4279a680364dfcd20 Mon Sep 17 00:00:00 2001 From: Erik Eldridge Date: Tue, 2 Apr 2024 08:52:11 -0700 Subject: [PATCH 04/10] Remove references to valueType and description from tests --- test/unit/remote-config/remote-config.spec.ts | 49 +++++-------------- 1 file changed, 12 insertions(+), 37 deletions(-) diff --git a/test/unit/remote-config/remote-config.spec.ts b/test/unit/remote-config/remote-config.spec.ts index be6de50a13..5d7878d049 100644 --- a/test/unit/remote-config/remote-config.spec.ts +++ b/test/unit/remote-config/remote-config.spec.ts @@ -132,9 +132,7 @@ describe('RemoteConfig', () => { parameters: { holiday_promo_enabled: { defaultValue: { value: 'true' }, - conditionalValues: { ios: { useInAppDefault: true } }, - description: 'this is a promo', - valueType: 'BOOLEAN', + conditionalValues: { ios: { useInAppDefault: true } } }, }, etag: 'etag-123456789012-5', @@ -592,8 +590,6 @@ describe('RemoteConfig', () => { const p1 = template.cache.parameters[key]; expect(p1.defaultValue).deep.equals({ value: 'true' }); expect(p1.conditionalValues).deep.equals({ ios: { useInAppDefault: true } }); - expect(p1.description).equals('this is a promo'); - expect(p1.valueType).equals('BOOLEAN'); const c = template.cache.conditions.find((c) => c.name === 'ios'); expect(c).to.be.not.undefined; @@ -635,9 +631,7 @@ describe('RemoteConfig', () => { dog_type: { defaultValue: { value: 'shiba' - }, - description: 'Type of dog breed', - valueType: 'STRING' + } } }; const initializedTemplate = remoteConfig.initServerTemplate({ template }).cache; @@ -652,41 +646,29 @@ describe('RemoteConfig', () => { dog_type: { defaultValue: { value: 'corgi' - }, - description: 'Type of dog breed', - valueType: 'STRING' + } }, dog_type_enabled: { defaultValue: { value: 'true' - }, - description: 'It\'s true or false', - valueType: 'BOOLEAN' + } }, dog_age: { defaultValue: { value: '22' - }, - description: 'Age', - valueType: 'NUMBER' + } }, dog_jsonified: { defaultValue: { value: '{"name":"Taro","breed":"Corgi","age":1,"fluffiness":100}' - }, - description: 'Dog Json Response', - valueType: 'JSON' + } }, dog_use_inapp_default: { defaultValue: { useInAppDefault: true - }, - description: 'Use in-app default dog', - valueType: 'STRING' + } }, dog_no_remote_default_value: { - description: 'TIL: default values are optional!', - valueType: 'STRING' } }; @@ -801,8 +783,6 @@ describe('RemoteConfig', () => { const p1 = template.cache.parameters[key]; expect(p1.defaultValue).deep.equals({ value: 'true' }); expect(p1.conditionalValues).deep.equals({ ios: { useInAppDefault: true } }); - expect(p1.description).equals('this is a promo'); - expect(p1.valueType).equals('BOOLEAN'); const c = template.cache.conditions.find((c) => c.name === 'ios'); expect(c).to.be.not.undefined; @@ -954,8 +934,7 @@ describe('RemoteConfig', () => { parameters: { is_enabled: { defaultValue: { value: 'false' }, - conditionalValues: { is_true: { value: 'true' } }, - valueType: 'BOOLEAN', + conditionalValues: { is_true: { value: 'true' } } }, }, etag: '123' @@ -1016,8 +995,7 @@ describe('RemoteConfig', () => { // value is selected. is_true_too: { value: 'dachshund' }, is_true: { value: 'corgi' } - }, - valueType: 'STRING', + } }, }, etag: '123' @@ -1094,8 +1072,7 @@ describe('RemoteConfig', () => { dog_type: { defaultValue: { value: 'pug' - }, - valueType: 'STRING' + } }, } @@ -1109,8 +1086,7 @@ describe('RemoteConfig', () => { dog_type: { defaultValue: { useInAppDefault: true - }, - valueType: 'STRING' + } }, } @@ -1128,8 +1104,7 @@ describe('RemoteConfig', () => { defaultValue: { // Defines remote value value: 'true' - }, - valueType: 'BOOLEAN' + } }, } From a4b78f3494d6d861f3ac83fb698b36fc3817ae58 Mon Sep 17 00:00:00 2001 From: Erik Eldridge Date: Tue, 2 Apr 2024 09:46:16 -0700 Subject: [PATCH 05/10] Define unit tests for Value --- src/remote-config/internal/value-impl.ts | 36 +++++++ src/remote-config/remote-config.ts | 29 +---- test/unit/index.spec.ts | 1 + .../remote-config/internal/value-impl.spec.ts | 70 ++++++++++++ test/unit/remote-config/remote-config.spec.ts | 102 ++++++++++++++++++ 5 files changed, 211 insertions(+), 27 deletions(-) create mode 100644 src/remote-config/internal/value-impl.ts create mode 100644 test/unit/remote-config/internal/value-impl.spec.ts diff --git a/src/remote-config/internal/value-impl.ts b/src/remote-config/internal/value-impl.ts new file mode 100644 index 0000000000..302b0612ce --- /dev/null +++ b/src/remote-config/internal/value-impl.ts @@ -0,0 +1,36 @@ +import { + Value, + ValueSource, +} from '../remote-config-api'; + +/** + * Implements type-safe getters for parameter values. + * + * Visible for testing. + * + * @internal + */ +export class ValueImpl implements Value { + public static BOOLEAN_TRUTHY_VALUES = ['1', 'true', 't', 'yes', 'y', 'on']; + public static DEFAULT_VALUE_FOR_NUMBER = 0; + public static DEFAULT_VALUE_FOR_STRING = ''; + constructor( + private readonly source: ValueSource, + private readonly value = ValueImpl.DEFAULT_VALUE_FOR_STRING) { } + asBoolean(): boolean { + return ValueImpl.BOOLEAN_TRUTHY_VALUES.indexOf(this.value) >= 0; + } + asNumber(): number { + const num = Number(this.value); + if (isNaN(num)) { + return ValueImpl.DEFAULT_VALUE_FOR_NUMBER; + } + return num; + } + asString(): string { + return this.value; + } + getSource(): ValueSource { + return this.source; + } +} \ No newline at end of file diff --git a/src/remote-config/remote-config.ts b/src/remote-config/remote-config.ts index 5b55922108..a3e8f9fbdb 100644 --- a/src/remote-config/remote-config.ts +++ b/src/remote-config/remote-config.ts @@ -18,6 +18,7 @@ import { App } from '../app'; import * as validator from '../utils/validator'; import { FirebaseRemoteConfigError, RemoteConfigApiClient } from './remote-config-api-client-internal'; import { ConditionEvaluator } from './condition-evaluator-internal'; +import { ValueImpl } from './internal/value-impl'; import { ListVersionsOptions, ListVersionsResult, @@ -36,8 +37,7 @@ import { ServerTemplateData, ServerTemplateOptions, NamedCondition, - Value, - ValueSource, + Value } from './remote-config-api'; /** @@ -407,31 +407,6 @@ class ServerConfigImpl implements ServerConfig { } } -class ValueImpl implements Value { - static BOOLEAN_TRUTHY_VALUES = ['1', 'true', 't', 'yes', 'y', 'on']; - static DEFAULT_VALUE_FOR_NUMBER = 0; - static DEFAULT_VALUE_FOR_STRING = ''; - constructor( - private readonly source: ValueSource, - private readonly value = ValueImpl.DEFAULT_VALUE_FOR_STRING){} - asBoolean(): boolean { - return ValueImpl.BOOLEAN_TRUTHY_VALUES.indexOf(this.value) >= 0; - } - asNumber(): number { - const num = Number(this.value); - if (isNaN(num)) { - return ValueImpl.DEFAULT_VALUE_FOR_NUMBER; - } - return num; - } - asString(): string { - return this.value; - } - getSource(): ValueSource { - return this.source; - } -} - /** * Remote Config dataplane template data implementation. */ diff --git a/test/unit/index.spec.ts b/test/unit/index.spec.ts index 29516d7a82..31efeaf979 100644 --- a/test/unit/index.spec.ts +++ b/test/unit/index.spec.ts @@ -98,6 +98,7 @@ import './remote-config/index.spec'; import './remote-config/remote-config.spec'; import './remote-config/remote-config-api-client.spec'; import './remote-config/condition-evaluator.spec'; +import './remote-config/internal/value-impl.spec'; // AppCheck import './app-check/app-check.spec'; diff --git a/test/unit/remote-config/internal/value-impl.spec.ts b/test/unit/remote-config/internal/value-impl.spec.ts new file mode 100644 index 0000000000..ad0cf9b138 --- /dev/null +++ b/test/unit/remote-config/internal/value-impl.spec.ts @@ -0,0 +1,70 @@ +/*! + * Copyright 2024 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +import * as chai from 'chai'; +import { ValueImpl } from '../../../../src/remote-config/internal/value-impl'; + +const expect = chai.expect; + +describe('ValueImpl', () => { + describe('getSource', () => { + it('returns the source string', () => { + const value = new ValueImpl('static'); + expect(value.getSource()).to.equal('static'); + }); + }); + + describe('asString', () => { + it('returns string value as a string', () => { + const value = new ValueImpl('default', 'shiba'); + expect(value.asString()).to.equal('shiba'); + }); + + it('defaults to empty string', () => { + const value = new ValueImpl('static'); + expect(value.asString()).to.equal(ValueImpl.DEFAULT_VALUE_FOR_STRING); + }); + }); + + describe('asNumber', () => { + it('returns numeric value as a number', () => { + const value = new ValueImpl('default', '123'); + expect(value.asNumber()).to.equal(123); + }); + + it('defaults to zero for non-numeric value', () => { + const value = new ValueImpl('default', 'Hi, NaN!'); + expect(value.asNumber()).to.equal(ValueImpl.DEFAULT_VALUE_FOR_NUMBER); + }); + }); + + describe('asBoolean', () => { + it('returns true for truthy values', () => { + for (const truthyValue of ValueImpl.BOOLEAN_TRUTHY_VALUES) { + const value = new ValueImpl('default', truthyValue); + expect(value.asBoolean()).to.be.true; + } + }); + + it('returns false for falsy values', () => { + const value = new ValueImpl('default', "I'm falsy"); + expect(value.asBoolean()).to.be.false; + }); + }); +}); + diff --git a/test/unit/remote-config/remote-config.spec.ts b/test/unit/remote-config/remote-config.spec.ts index 5d7878d049..a3d2394d63 100644 --- a/test/unit/remote-config/remote-config.spec.ts +++ b/test/unit/remote-config/remote-config.spec.ts @@ -1128,6 +1128,108 @@ describe('RemoteConfig', () => { }); }); + // Note the static source is set in the getValue() method, but the other sources + // are set in the evaluate() method, so these tests span a couple layers. + describe('ServerConfig', () => { + describe('getValue', () => { + it('should return static when default and remote are not defined', () => { + const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData; + // Omits remote parameter values. + templateData.parameters = { + }; + // Omits in-app default values. + const template = remoteConfig.initServerTemplate({ template: templateData }); + const config = template.evaluate(); + const value = config.getValue('dog_type'); + expect(value.asString()).to.equal(''); + expect(value.getSource()).to.equal('static'); + }); + + it('should return default value when it is defined', () => { + const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData; + // Omits remote parameter values. + templateData.parameters = { + }; + const template = remoteConfig.initServerTemplate({ + template: templateData, + // Defines in-app default values. + defaultConfig: { + dog_type: 'shiba' + } + }); + const config = template.evaluate(); + const value = config.getValue('dog_type'); + expect(value.asString()).to.equal('shiba'); + expect(value.getSource()).to.equal('default'); + }); + + it('should return remote value when it is defined', () => { + const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData; + // Defines remote parameter values. + templateData.parameters = { + dog_type: { + defaultValue: { + value: 'pug' + } + } + }; + const template = remoteConfig.initServerTemplate({ + template: templateData, + // Defines in-app default values. + defaultConfig: { + dog_type: 'shiba' + } + }); + const config = template.evaluate(); + const value = config.getValue('dog_type'); + expect(value.asString()).to.equal('pug'); + expect(value.getSource()).to.equal('remote'); + }); + }); + + describe('getString', () => { + it('returns a string value', () => { + const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData; + const template = remoteConfig.initServerTemplate({ + template: templateData, + defaultConfig: { + dog_type: 'shiba' + } + }); + const config = template.evaluate(); + expect(config.getString('dog_type')).to.equal('shiba'); + }); + }); + + describe('getNumber', () => { + it('returns a numeric value', () => { + const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData; + const template = remoteConfig.initServerTemplate({ + template: templateData, + defaultConfig: { + dog_age: 12 + } + }); + const config = template.evaluate(); + expect(config.getNumber('dog_age')).to.equal(12); + }); + }); + + describe('getBoolean', () => { + it('returns a boolean value', () => { + const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData; + const template = remoteConfig.initServerTemplate({ + template: templateData, + defaultConfig: { + dog_is_cute: true + } + }); + const config = template.evaluate(); + expect(config.getBoolean('dog_is_cute')).to.be.true; + }); + }); + }); + function runInvalidResponseTests(rcOperation: () => Promise, operationName: any): void { it('should propagate API errors', () => { From b6bbe9dbe4edff65f97e845eb87ba1241cdad47f Mon Sep 17 00:00:00 2001 From: Erik Eldridge Date: Tue, 2 Apr 2024 12:04:44 -0700 Subject: [PATCH 06/10] Define type for default config --- src/remote-config/remote-config-api.ts | 13 ++++++++++--- src/remote-config/remote-config.ts | 5 +++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/remote-config/remote-config-api.ts b/src/remote-config/remote-config-api.ts index d66d6a44cb..0eee4b6aa6 100644 --- a/src/remote-config/remote-config-api.ts +++ b/src/remote-config/remote-config-api.ts @@ -361,7 +361,7 @@ export interface ServerTemplateOptions { * intended before it connects to the Remote Config backend, and so that * default values are available if none are set on the backend. */ - defaultConfig?: { [key: string]: string | number | boolean }; + defaultConfig?: DefaultConfig; /** * Enables integrations to use template data loaded independently. For @@ -383,9 +383,11 @@ export interface ServerTemplate { cache: ServerTemplateData; /** - * A {@link ServerConfig} that contains default Config values. + * Defines in-app default parameter values, so that your app behaves as + * intended before it connects to the Remote Config backend, and so that + * default values are available if none are set on the backend. */ - defaultConfig: { [key: string]: string | number | boolean }; + defaultConfig: DefaultConfig; /** * Evaluates the current template to produce a {@link ServerConfig}. @@ -627,3 +629,8 @@ export interface Value { * */ export type ValueSource = 'static' | 'default' | 'remote'; + +/** + * Defines the format for in-app default parameter values. + */ +export type DefaultConfig = { [key: string]: string | number | boolean }; \ No newline at end of file diff --git a/src/remote-config/remote-config.ts b/src/remote-config/remote-config.ts index a3e8f9fbdb..c04bf850c3 100644 --- a/src/remote-config/remote-config.ts +++ b/src/remote-config/remote-config.ts @@ -37,7 +37,8 @@ import { ServerTemplateData, ServerTemplateOptions, NamedCondition, - Value + Value, + DefaultConfig } from './remote-config-api'; /** @@ -298,7 +299,7 @@ class ServerTemplateImpl implements ServerTemplate { constructor( private readonly apiClient: RemoteConfigApiClient, private readonly conditionEvaluator: ConditionEvaluator, - public readonly defaultConfig: { [key: string]: string | number | boolean } = {} + public readonly defaultConfig: DefaultConfig = {} ) { // RC stores all remote values as string, but it's more intuitive // to declare default values with specific types, so this converts From f2509b45efe62349686832d0243dbb6ab02c80eb Mon Sep 17 00:00:00 2001 From: Erik Eldridge Date: Tue, 2 Apr 2024 12:07:17 -0700 Subject: [PATCH 07/10] Extract API --- etc/firebase-admin.remote-config.api.md | 29 ++++++++++++++++++++----- src/remote-config/index.ts | 3 +++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/etc/firebase-admin.remote-config.api.md b/etc/firebase-admin.remote-config.api.md index 6712175a60..60a83f3ff0 100644 --- a/etc/firebase-admin.remote-config.api.md +++ b/etc/firebase-admin.remote-config.api.md @@ -13,6 +13,11 @@ export interface AndCondition { conditions?: Array; } +// @public +export type DefaultConfig = { + [key: string]: string | number | boolean; +}; + // @public export type EvaluationContext = { randomizationId?: string; @@ -159,14 +164,17 @@ export interface RemoteConfigUser { } // @public -export type ServerConfig = { - [key: string]: string | boolean | number; -}; +export interface ServerConfig { + getBoolean(key: string): boolean; + getNumber(key: string): number; + getString(key: string): string; + getValue(key: string): Value; +} // @public export interface ServerTemplate { cache: ServerTemplateData; - defaultConfig: ServerConfig; + defaultConfig: DefaultConfig; evaluate(context?: EvaluationContext): ServerConfig; load(): Promise; } @@ -183,13 +191,24 @@ export interface ServerTemplateData { // @public export interface ServerTemplateOptions { - defaultConfig?: ServerConfig; + defaultConfig?: DefaultConfig; template?: ServerTemplateData; } // @public export type TagColor = 'BLUE' | 'BROWN' | 'CYAN' | 'DEEP_ORANGE' | 'GREEN' | 'INDIGO' | 'LIME' | 'ORANGE' | 'PINK' | 'PURPLE' | 'TEAL'; +// @public +export interface Value { + asBoolean(): boolean; + asNumber(): number; + asString(): string; + getSource(): ValueSource; +} + +// @public +export type ValueSource = 'static' | 'default' | 'remote'; + // @public export interface Version { description?: string; diff --git a/src/remote-config/index.ts b/src/remote-config/index.ts index 103ec462f3..ae7c46f66f 100644 --- a/src/remote-config/index.ts +++ b/src/remote-config/index.ts @@ -26,6 +26,7 @@ import { RemoteConfig } from './remote-config'; export { AndCondition, + DefaultConfig, EvaluationContext, ExplicitParameterValue, InAppDefaultValue, @@ -49,6 +50,8 @@ export { ServerTemplateData, ServerTemplateOptions, TagColor, + Value, + ValueSource, Version, } from './remote-config-api'; export { RemoteConfig } from './remote-config'; From f0d4e5382da0caeb1dcb39412aa556ae6ac676b6 Mon Sep 17 00:00:00 2001 From: Erik Eldridge Date: Tue, 2 Apr 2024 12:10:32 -0700 Subject: [PATCH 08/10] Add copyright header to new file --- src/remote-config/internal/value-impl.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/remote-config/internal/value-impl.ts b/src/remote-config/internal/value-impl.ts index 302b0612ce..13c617d681 100644 --- a/src/remote-config/internal/value-impl.ts +++ b/src/remote-config/internal/value-impl.ts @@ -1,3 +1,21 @@ +/*! + * Copyright 2024 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + import { Value, ValueSource, From 9e0cb44ea80fbbf3a01e68effda5f95e8cbc0b56 Mon Sep 17 00:00:00 2001 From: Erik Eldridge Date: Tue, 2 Apr 2024 13:01:26 -0700 Subject: [PATCH 09/10] Make boolean getter case-insensitive --- src/remote-config/internal/value-impl.ts | 4 ++-- src/remote-config/remote-config-api.ts | 2 +- test/unit/remote-config/internal/value-impl.spec.ts | 9 +++++++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/remote-config/internal/value-impl.ts b/src/remote-config/internal/value-impl.ts index 13c617d681..1fba9009d1 100644 --- a/src/remote-config/internal/value-impl.ts +++ b/src/remote-config/internal/value-impl.ts @@ -36,7 +36,7 @@ export class ValueImpl implements Value { private readonly source: ValueSource, private readonly value = ValueImpl.DEFAULT_VALUE_FOR_STRING) { } asBoolean(): boolean { - return ValueImpl.BOOLEAN_TRUTHY_VALUES.indexOf(this.value) >= 0; + return ValueImpl.BOOLEAN_TRUTHY_VALUES.indexOf(this.value.toLowerCase()) >= 0; } asNumber(): number { const num = Number(this.value); @@ -51,4 +51,4 @@ export class ValueImpl implements Value { getSource(): ValueSource { return this.source; } -} \ No newline at end of file +} diff --git a/src/remote-config/remote-config-api.ts b/src/remote-config/remote-config-api.ts index 0eee4b6aa6..b4bd1db46b 100644 --- a/src/remote-config/remote-config-api.ts +++ b/src/remote-config/remote-config-api.ts @@ -633,4 +633,4 @@ export type ValueSource = 'static' | 'default' | 'remote'; /** * Defines the format for in-app default parameter values. */ -export type DefaultConfig = { [key: string]: string | number | boolean }; \ No newline at end of file +export type DefaultConfig = { [key: string]: string | number | boolean }; diff --git a/test/unit/remote-config/internal/value-impl.spec.ts b/test/unit/remote-config/internal/value-impl.spec.ts index ad0cf9b138..b344d0c9d1 100644 --- a/test/unit/remote-config/internal/value-impl.spec.ts +++ b/test/unit/remote-config/internal/value-impl.spec.ts @@ -54,14 +54,19 @@ describe('ValueImpl', () => { }); describe('asBoolean', () => { - it('returns true for truthy values', () => { + it("returns true for any value in RC's list of truthy values", () => { for (const truthyValue of ValueImpl.BOOLEAN_TRUTHY_VALUES) { const value = new ValueImpl('default', truthyValue); expect(value.asBoolean()).to.be.true; } }); - it('returns false for falsy values', () => { + it('is case-insensitive', () => { + const value = new ValueImpl('default', 'TRUE'); + expect(value.asBoolean()).to.be.true; + }); + + it("returns false for any value not in RC's list of truthy values", () => { const value = new ValueImpl('default', "I'm falsy"); expect(value.asBoolean()).to.be.false; }); From a28252c3468e5667be04c8b8fe2915fa927bf830 Mon Sep 17 00:00:00 2001 From: Erik Eldridge Date: Thu, 4 Apr 2024 14:48:32 -0700 Subject: [PATCH 10/10] Make Value constants readonly and define default boolean value. Per discussion on the PR, explicitly defining the default boolean value is more readable. --- src/remote-config/internal/value-impl.ts | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/remote-config/internal/value-impl.ts b/src/remote-config/internal/value-impl.ts index 1fba9009d1..6d71476538 100644 --- a/src/remote-config/internal/value-impl.ts +++ b/src/remote-config/internal/value-impl.ts @@ -29,25 +29,32 @@ import { * @internal */ export class ValueImpl implements Value { - public static BOOLEAN_TRUTHY_VALUES = ['1', 'true', 't', 'yes', 'y', 'on']; - public static DEFAULT_VALUE_FOR_NUMBER = 0; - public static DEFAULT_VALUE_FOR_STRING = ''; + public static readonly DEFAULT_VALUE_FOR_BOOLEAN = false; + public static readonly DEFAULT_VALUE_FOR_STRING = ''; + public static readonly DEFAULT_VALUE_FOR_NUMBER = 0; + public static readonly BOOLEAN_TRUTHY_VALUES = ['1', 'true', 't', 'yes', 'y', 'on']; constructor( - private readonly source: ValueSource, - private readonly value = ValueImpl.DEFAULT_VALUE_FOR_STRING) { } + private readonly source: ValueSource, + private readonly value = ValueImpl.DEFAULT_VALUE_FOR_STRING) { } + asString(): string { + return this.value; + } asBoolean(): boolean { + if (this.source === 'static') { + return ValueImpl.DEFAULT_VALUE_FOR_BOOLEAN; + } return ValueImpl.BOOLEAN_TRUTHY_VALUES.indexOf(this.value.toLowerCase()) >= 0; } asNumber(): number { + if (this.source === 'static') { + return ValueImpl.DEFAULT_VALUE_FOR_NUMBER; + } const num = Number(this.value); if (isNaN(num)) { return ValueImpl.DEFAULT_VALUE_FOR_NUMBER; } return num; } - asString(): string { - return this.value; - } getSource(): ValueSource { return this.source; }