Skip to content

Add functionality to accept a JSON template string when initializing a RC server template #2520

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.

Sign up for GitHub

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 9 commits into from
Apr 4, 2024
Merged

Add functionality to accept a JSON template string when initializing a RC server template #2520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
43e0f2e
Add support to pass in a json string for RC server template initializ…
trekforever Apr 2, 2024
dac5893
Add support to pass in a json string for RC server template initializ…
trekforever Apr 2, 2024
de597ee
Merge ssrc changes
trekforever Apr 2, 2024
0608aaf
Apply lint changes
trekforever Apr 2, 2024
e926fcd
Update comments
trekforever Apr 2, 2024
48fe8a2
Run api-extractor:local
trekforever Apr 2, 2024
295636b
Update comments and address feedback
trekforever Apr 3, 2024
9ae7d96
Update inline comment and lint fixes
trekforever Apr 3, 2024
c7d4c6c
Revert toJSON functionality for ServerTemplate
trekforever Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion etc/firebase-admin.remote-config.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export interface InAppDefaultValue {

// @public
export interface InitServerTemplateOptions extends GetServerTemplateOptions {
template?: ServerTemplateData;
template?: ServerTemplateData | string;
}

// @public
Expand Down
4 changes: 3 additions & 1 deletion src/remote-config/remote-config-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,8 +375,10 @@ export interface InitServerTemplateOptions extends GetServerTemplateOptions {
* 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.
* The template can be initialized with either a {@link ServerTemplateData}
* object or a JSON string.
*/
template?: ServerTemplateData,
template?: ServerTemplateData|string,
}

/**
Expand Down
16 changes: 14 additions & 2 deletions src/remote-config/remote-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
GetServerTemplateOptions,
InitServerTemplateOptions,
} from './remote-config-api';
import { isString } from 'lodash';

/**
* The Firebase `RemoteConfig` service interface.
Expand Down Expand Up @@ -198,7 +199,19 @@ export class RemoteConfig {
const template = new ServerTemplateImpl(
this.client, new ConditionEvaluator(), options?.defaultConfig);
if (options?.template) {
template.cache = options?.template;
// Check and instantiates the template via a 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 {
template.cache = options?.template;
}
}
return template;
}
Expand Down Expand Up @@ -430,7 +443,6 @@ class ServerTemplateDataImpl implements ServerTemplateData {
}

this.etag = template.etag;

if (typeof template.parameters !== 'undefined') {
if (!validator.isNonNullObject(template.parameters)) {
throw new FirebaseRemoteConfigError(
Expand Down
67 changes: 59 additions & 8 deletions test/unit/remote-config/remote-config.spec.ts
View file
Open in desktop
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 @@ -648,10 +648,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(JSON.stringify(initializedTemplate.cache));
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(JSON.stringify(initializedTemplate.cache));
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 @@ -1039,12 +1090,12 @@ 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);
stubs.push(stub);

const defaultConfig = {
dog_coat: 'blue merle',
};
Expand All @@ -1061,12 +1112,12 @@ describe('RemoteConfig', () => {
template.parameters = {
dog_no_remote_default_value: {}
};

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

const defaultConfig = {
dog_no_remote_default_value: 'local default'
};
Expand All @@ -1083,7 +1134,7 @@ describe('RemoteConfig', () => {
template.parameters = {
dog_no_remote_default_value: {}
};

const stub = sinon
.stub(RemoteConfigApiClient.prototype, 'getServerTemplate')
.resolves(template);
Expand Down