Skip to content
Next Next commit
Add support to pass in a json string for RC server template initializ…
…ation
  • Loading branch information
trekforever committed Apr 2, 2024
commit 43e0f2e21c206342d65bbdd93c89d4388a00651d
22 changes: 13 additions & 9 deletions src/remote-config/remote-config-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -354,29 +354,22 @@ export interface ServerTemplateData {
/**
* Represents optional arguments that can be used when instantiating {@link ServerTemplate}.
*/
export interface GetServerTemplateOptions {
export interface ServerTemplateOptions {

/**
* 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?: ServerConfig,
}

/**
* Represents optional arguments that can be used when instantiating
* {@link ServerTemplate} synchonously.
*/
export interface InitServerTemplateOptions extends GetServerTemplateOptions {

/**
* Enables integrations to use template data loaded independently. For
* example, customers can reduce initialization latency by pre-fetching and
* caching template data and then using this option to initialize the SDK with
* that data.
*/
template?: ServerTemplateData,
template?: ServerTemplateData|string,
}

/**
Expand All @@ -389,6 +382,11 @@ export interface ServerTemplate {
*/
cache: ServerTemplateData;

/**
* A {@link ServerConfig} that contains default Config values.
*/
defaultConfig: ServerConfig;

/**
* Evaluates the current template to produce a {@link ServerConfig}.
*/
Expand All @@ -399,6 +397,12 @@ export interface ServerTemplate {
* project's {@link ServerTemplate}.
*/
load(): Promise<void>;

/**
* Convenient method that returns the JSON string of the cached template data
* @returns A JSON-string of this object.
*/
toJSON(): string;
}

/**
Expand Down
35 changes: 28 additions & 7 deletions src/remote-config/remote-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ import {
RemoteConfigParameterValue,
EvaluationContext,
ServerTemplateData,
ServerTemplateOptions,
NamedCondition,
GetServerTemplateOptions,
InitServerTemplateOptions,
} from './remote-config-api';
import { isString } from 'lodash';

/**
* The Firebase `RemoteConfig` service interface.
Expand Down Expand Up @@ -185,7 +185,7 @@ export class RemoteConfig {
* Instantiates {@link ServerTemplate} and then fetches and caches the latest
* template version of the project.
*/
public async getServerTemplate(options?: GetServerTemplateOptions): Promise<ServerTemplate> {
public async getServerTemplate(options?: ServerTemplateOptions): Promise<ServerTemplate> {
const template = this.initServerTemplate(options);
await template.load();
return template;
Expand All @@ -194,11 +194,24 @@ export class RemoteConfig {
/**
* Synchronously instantiates {@link ServerTemplate}.
*/
public initServerTemplate(options?: InitServerTemplateOptions): ServerTemplate {
public initServerTemplate(options?: ServerTemplateOptions): ServerTemplate {
const template = new ServerTemplateImpl(
this.client, new ConditionEvaluator(), options?.defaultConfig);
if (options?.template) {
template.cache = options?.template;
// Check and instantiates via json string
if (isString(options?.template)) {
try {
template.cache = new ServerTemplateDataImpl(JSON.parse(options?.template));
} catch (e) {
throw new FirebaseRemoteConfigError(
'invalid-argument',
`Failed to parse the JSON string: ${options?.template}. ` + e
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the other errors we throw, we don't include the raw error (e) in the message. We should check w Lahiru if this is an intentional pattern, and if so, follow it here.

Nit: If we do include the error, it might be more readable to differentiate it from the other error message, something like "... Raw error: ${e}".

Copy link
Author

Choose a reason for hiding this comment

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

We don't include it for most errors, but we do have it for the createTemplateFromJSON:
https://github.com/firebase/firebase-admin-node/blob/master/src/remote-config/remote-config.ts#L163-L166
(most of this logic is consistent with that method). But will check with Lahiru when I request his review on this

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. I overlooked that. Makes sense. Shipit 🚢

);
}
} else {
// check and instantiates via ServerTemplateData
template.cache = options?.template;
}
}
return template;
}
Expand Down Expand Up @@ -297,7 +310,7 @@ class ServerTemplateImpl implements ServerTemplate {
constructor(
private readonly apiClient: RemoteConfigApiClient,
private readonly conditionEvaluator: ConditionEvaluator,
private readonly defaultConfig: ServerConfig = {}
public readonly defaultConfig: ServerConfig = {}
) { }

/**
Expand Down Expand Up @@ -387,6 +400,14 @@ class ServerTemplateImpl implements ServerTemplate {
return new Proxy(mergedConfig, proxyHandler);
}

/**
* Convenient method that returns the JSON string of the cached template data
* @returns A JSON-string of this object.
*/
public toJSON(): string {
return JSON.stringify(this.cache);
}

/**
* Private helper method that coerces a parameter value string to the {@link ParameterValueType}.
*/
Expand Down Expand Up @@ -430,7 +451,7 @@ class ServerTemplateDataImpl implements ServerTemplateData {
}

this.etag = template.etag;

if (typeof template.parameters !== 'undefined') {
if (!validator.isNonNullObject(template.parameters)) {
throw new FirebaseRemoteConfigError(
Expand Down
139 changes: 83 additions & 56 deletions test/unit/remote-config/remote-config.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import {
} from '../../../src/remote-config/remote-config-api-client-internal';
import { deepCopy } from '../../../src/utils/deep-copy';
import {
NamedCondition, ServerTemplate, ServerTemplateData
NamedCondition, ServerTemplate, ServerTemplateData, Version
} from '../../../src/remote-config/remote-config-api';

const expect = chai.expect;
Expand Down Expand Up @@ -610,28 +610,20 @@ describe('RemoteConfig', () => {
});

it('should set defaultConfig when passed', () => {
// Defines template with no parameters to demonstrate
// default config will be used instead,
const template = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData;
template.parameters = {};

const stub = sinon
.stub(RemoteConfigApiClient.prototype, operationName)
.resolves(template);
stubs.push(stub);

const defaultConfig = {
holiday_promo_enabled: false,
holiday_promo_discount: 20,
};

const stub = sinon
.stub(RemoteConfigApiClient.prototype, operationName)
.resolves(SERVER_REMOTE_CONFIG_RESPONSE as ServerTemplateData);
stubs.push(stub);

return remoteConfig.getServerTemplate({ defaultConfig })
.then((template) => {
const config = template.evaluate();
expect(config.holiday_promo_enabled).to.equal(
defaultConfig.holiday_promo_enabled);
expect(config.holiday_promo_discount).to.equal(
defaultConfig.holiday_promo_discount);
expect(template.defaultConfig.holiday_promo_enabled).to.equal(false);
expect(template.defaultConfig.holiday_promo_discount).to.equal(20);
});
});
});
Expand All @@ -648,10 +640,61 @@ describe('RemoteConfig', () => {
valueType: 'STRING'
}
};
const initializedTemplate = remoteConfig.initServerTemplate({ template }).cache;
const parsed = JSON.parse(JSON.stringify(initializedTemplate));
const initializedTemplate = remoteConfig.initServerTemplate({ template });
const parsed = JSON.parse(initializedTemplate.toJSON());
expect(parsed).deep.equals(deepCopy(template));
});

it('should set and instantiates template when json string is passed', () => {
const template = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData;
template.parameters = {
dog_type: {
defaultValue: {
value: 'shiba'
},
description: 'Type of dog breed',
valueType: 'STRING'
}
};
const templateJson = JSON.stringify(template);
const initializedTemplate = remoteConfig.initServerTemplate({ template: templateJson });
const parsed = JSON.parse(initializedTemplate.toJSON());
const expectedVersion = deepCopy(VERSION_INFO);
expectedVersion.updateTime = new Date(expectedVersion.updateTime).toUTCString();
template.version = expectedVersion as Version;
expect(parsed).deep.equals(deepCopy(template));
});

describe('should throw error if invalid template JSON is passed', () => {
const INVALID_PARAMETERS: any[] = [null, '', 'abc', 1, true, []];
const INVALID_CONDITIONS: any[] = [null, '', 'abc', 1, true, {}];

let sourceTemplate = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE);
const jsonString = '{invalidJson: null}';
it('should throw if template is an invalid JSON', () => {
expect(() => remoteConfig.initServerTemplate({ template: jsonString }))
.to.throw(/Failed to parse the JSON string: ([\D\w]*)\./);
});

INVALID_PARAMETERS.forEach((invalidParameter) => {
sourceTemplate.parameters = invalidParameter;
const jsonString = JSON.stringify(sourceTemplate);
it(`should throw if the parameters is ${JSON.stringify(invalidParameter)}`, () => {
expect(() => remoteConfig.initServerTemplate({ template: jsonString }))
.to.throw('Remote Config parameters must be a non-null object');
});
});

sourceTemplate = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE);
INVALID_CONDITIONS.forEach((invalidConditions) => {
sourceTemplate.conditions = invalidConditions;
const jsonString = JSON.stringify(sourceTemplate);
it(`should throw if the conditions is ${JSON.stringify(invalidConditions)}`, () => {
expect(() => remoteConfig.initServerTemplate({ template: jsonString }))
.to.throw('Remote Config conditions must be an array');
});
});
});
});

describe('RemoteConfigServerTemplate', () => {
Expand Down Expand Up @@ -1037,66 +1080,50 @@ describe('RemoteConfig', () => {
});

it('uses local default if parameter not in template', () => {
const template = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData;
template.parameters = {};

const stub = sinon
.stub(RemoteConfigApiClient.prototype, 'getServerTemplate')
.resolves(template);
.resolves(SERVER_REMOTE_CONFIG_RESPONSE_2 as ServerTemplateData);
stubs.push(stub);

const defaultConfig = {
dog_coat: 'blue merle',
};

return remoteConfig.getServerTemplate({ defaultConfig })
return remoteConfig.getServerTemplate({
defaultConfig: {
dog_coat: 'blue merle',
}
})
.then((template: ServerTemplate) => {
const config = template.evaluate();
expect(config.dog_coat).to.equal(defaultConfig.dog_coat);
const config = template.evaluate!();
expect(config.dog_coat).to.equal(template.defaultConfig.dog_coat);
});
});

it('uses local default when parameter is in template but default value is undefined', () => {
const template = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData;
template.parameters = {
dog_no_remote_default_value: {}
};

const stub = sinon
.stub(RemoteConfigApiClient.prototype, 'getServerTemplate')
.resolves(template);
.resolves(SERVER_REMOTE_CONFIG_RESPONSE_2 as ServerTemplateData);
stubs.push(stub);

const defaultConfig = {
dog_no_remote_default_value: 'local default'
};

return remoteConfig.getServerTemplate({ defaultConfig })
return remoteConfig.getServerTemplate({
defaultConfig: {
dog_no_remote_default_value: 'local default'
}
})
.then((template: ServerTemplate) => {
const config = template.evaluate!();
expect(config.dog_no_remote_default_value).to.equal(defaultConfig.dog_no_remote_default_value);
expect(config.dog_no_remote_default_value).to.equal(template.defaultConfig.dog_no_remote_default_value);
});
});

it('uses local default when in-app default value specified', () => {
const template = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData;
template.parameters = {
dog_no_remote_default_value: {}
};

const stub = sinon
.stub(RemoteConfigApiClient.prototype, 'getServerTemplate')
.resolves(template);
.resolves(SERVER_REMOTE_CONFIG_RESPONSE_2 as ServerTemplateData);
stubs.push(stub);

const defaultConfig = {
dog_use_inapp_default: '🐕'
};

return remoteConfig.getServerTemplate({ defaultConfig })
return remoteConfig.getServerTemplate({
defaultConfig: {
dog_use_inapp_default: '🐕'
}
})
.then((template: ServerTemplate) => {
const config = template.evaluate!();
expect(config.dog_use_inapp_default).to.equal(defaultConfig.dog_use_inapp_default);
expect(config.dog_use_inapp_default).to.equal(template.defaultConfig.dog_use_inapp_default);
});
});

Expand Down