Skip to content

Define type-specific getters for SSRC #2519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Apr 4, 2024
Merged
27 changes: 23 additions & 4 deletions etc/firebase-admin.remote-config.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ export interface AndCondition {
conditions?: Array<OneOfCondition>;
}

// @public
export type DefaultConfig = {
[key: string]: string | number | boolean;
};

// @public
export type EvaluationContext = {
randomizationId?: string;
Expand All @@ -30,7 +35,7 @@ export function getRemoteConfig(app?: App): RemoteConfig;

// @public
export interface GetServerTemplateOptions {
defaultConfig?: ServerConfig;
defaultConfig?: DefaultConfig;
}

// @public
Expand Down Expand Up @@ -169,9 +174,12 @@ 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 {
Expand All @@ -193,6 +201,17 @@ export interface 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;
Expand Down
3 changes: 3 additions & 0 deletions src/remote-config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { RemoteConfig } from './remote-config';

export {
AndCondition,
DefaultConfig,
EvaluationContext,
ExplicitParameterValue,
GetServerTemplateOptions,
Expand All @@ -50,6 +51,8 @@ export {
ServerTemplate,
ServerTemplateData,
TagColor,
Value,
ValueSource,
Version,
} from './remote-config-api';
export { RemoteConfig } from './remote-config';
Expand Down
61 changes: 61 additions & 0 deletions src/remote-config/internal/value-impl.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*!
* 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 '../remote-config-api';

/**
* Implements type-safe getters for parameter values.
*
* Visible for testing.
*
* @internal
*/
export class ValueImpl implements Value {
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) { }
asString(): string {
return this.value;
}
asBoolean(): boolean {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have a 'DEFAULT_VALUE_FOR_BOOLEAN' for the static behavior? Similar to https://github.com/firebase/firebase-js-sdk/blob/master/packages/remote-config/src/value.ts#L36-L39?

Copy link
Contributor Author

@erikeldridge erikeldridge Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's inconsistent, but given the default value is an empty string when the source is static, BOOLEAN_TRUTHY_VALUES.indexOf(this.value.toLowerCase()) will return false, so I'm not seeing a technical need ... That said, I can imagine how explicitly defining the default value for boolean would make our system easier to understand, so I'll make the change.

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;
}
getSource(): ValueSource {
return this.source;
}
}
98 changes: 96 additions & 2 deletions src/remote-config/remote-config-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ export interface GetServerTemplateOptions {
* 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?: DefaultConfig;
}

/**
Expand Down Expand Up @@ -539,4 +539,98 @@ 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 <code>serverConfig.getValue(key).asBoolean()</code>.
*
* @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 <code>serverConfig.getValue(key).asNumber()</code>.
*
* @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 <code>serverConfig.getValue(key).asString()</code>.
*
* @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.
*
* 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.
*/
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 <code>Number(value) || 0</code>.
*/
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.
*
* <ul>
* <li>"static" indicates the value was defined by a static constant.</li>
* <li>"default" indicates the value was defined by default config.</li>
* <li>"remote" indicates the value was defined by config produced by
* evaluating a template.</li>
* </ul>
*/
export type ValueSource = 'static' | 'default' | 'remote';

/**
* Defines the format for in-app default parameter values.
*/
export type DefaultConfig = { [key: string]: string | number | boolean };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this also contain object or Value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add object. I think it'd pair well with a getObject<T>, so those could be the same review. I think it'd be safe to add these after the initial launch.

I don't think Value would make sense because it's a tuple of the string value and an indication of where it came from (the "source"). If the customer is setting it, then the its source will always be "default". Setting a string value in defaultConfig accomplishes the same thing in a more user-friendly way.

82 changes: 40 additions & 42 deletions src/remote-config/remote-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -30,12 +31,13 @@ import {
Version,
ExplicitParameterValue,
InAppDefaultValue,
ParameterValueType,
ServerConfig,
RemoteConfigParameterValue,
EvaluationContext,
ServerTemplateData,
NamedCondition,
Value,
DefaultConfig,
GetServerTemplateOptions,
InitServerTemplateOptions,
} from './remote-config-api';
Expand Down Expand Up @@ -293,12 +295,20 @@ class RemoteConfigTemplateImpl implements RemoteConfigTemplate {
*/
class ServerTemplateImpl implements ServerTemplate {
public cache: ServerTemplateData;
private stringifiedDefaultConfig: {[key: string]: string} = {};

constructor(
private readonly apiClient: RemoteConfigApiClient,
private readonly conditionEvaluator: ConditionEvaluator,
private readonly defaultConfig: ServerConfig = {}
) { }
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
// the external declaration to an internal string representation.
for (const key in defaultConfig) {
this.stringifiedDefaultConfig[key] = String(defaultConfig[key]);
}
}

/**
* Fetches and caches the current active version of the project's {@link ServerTemplate}.
Expand Down Expand Up @@ -327,10 +337,16 @@ class ServerTemplateImpl implements ServerTemplate {
const evaluatedConditions = this.conditionEvaluator.evaluateConditions(
this.cache.conditions, context);

const evaluatedConfig: ServerConfig = {};
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, valueType } = parameter;
const { conditionalValues, defaultValue } = parameter;

// Supports parameters with no conditional values.
const normalizedConditionalValues = conditionalValues || {};
Expand All @@ -353,7 +369,7 @@ class ServerTemplateImpl implements ServerTemplate {

if (parameterValueWrapper) {
const parameterValue = (parameterValueWrapper as ExplicitParameterValue).value;
evaluatedConfig[key] = this.parseRemoteConfigParameterValue(valueType, parameterValue);
configValues[key] = new ValueImpl('remote', parameterValue);
continue;
}

Expand All @@ -368,46 +384,28 @@ class ServerTemplateImpl implements ServerTemplate {
}

const parameterDefaultValue = (defaultValue as ExplicitParameterValue).value;
evaluatedConfig[key] = this.parseRemoteConfigParameterValue(valueType, parameterDefaultValue);
configValues[key] = new ValueImpl('remote', 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(configValues);
}
}

/**
* 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;
} else {
// Treat everything else as string
return parameterValue || DEFAULT_VALUE_FOR_STRING;
}
class ServerConfigImpl implements ServerConfig {
constructor(
private readonly configValues: { [key: string]: Value },
){}
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 {
return this.configValues[key] || new ValueImpl('static');
}
}

Expand Down
1 change: 1 addition & 0 deletions test/unit/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
Loading