Skip to content
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

Adds support for exporting a serialized config response from the RC server side SDK. #2829

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
19 changes: 19 additions & 0 deletions etc/firebase-admin.remote-config.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,15 @@ export interface ExplicitParameterValue {
value: string;
}

// @public
export interface FetchResponseData {
config?: {
[key: string]: string;
};
eTag: string;
status: number;
}

// Warning: (ae-forgotten-export) The symbol "App" needs to be exported by the entry point index.d.ts
//
// @public
Expand Down Expand Up @@ -162,6 +171,13 @@ export interface RemoteConfigCondition {
tagColor?: TagColor;
}

// @public
export class RemoteConfigFetchResponse {
constructor(app: App, serverConfig: ServerConfig, requestEtag?: string);
// (undocumented)
toJSON(): FetchResponseData;
}

// @public
export interface RemoteConfigParameter {
conditionalValues?: {
Expand Down Expand Up @@ -205,6 +221,9 @@ export interface RemoteConfigUser {

// @public
export interface ServerConfig {
getAll(): {
[key: string]: Value;
};
getBoolean(key: string): boolean;
getNumber(key: string): number;
getString(key: string): string;
Expand Down
3 changes: 2 additions & 1 deletion src/remote-config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export {
DefaultConfig,
EvaluationContext,
ExplicitParameterValue,
FetchResponseData,
GetServerTemplateOptions,
InAppDefaultValue,
InitServerTemplateOptions,
Expand Down Expand Up @@ -60,7 +61,7 @@ export {
ValueSource,
Version,
} from './remote-config-api';
export { RemoteConfig } from './remote-config';
export { RemoteConfig, RemoteConfigFetchResponse } from './remote-config';

/**
* Gets the {@link RemoteConfig} service for the default app or a given app.
Expand Down
38 changes: 36 additions & 2 deletions src/remote-config/remote-config-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ export enum CustomSignalOperator {
/**
* Matches a numeric value less than or equal to the target value.
*/
NUMERIC_LESS_EQUAL ='NUMERIC_LESS_EQUAL',
NUMERIC_LESS_EQUAL = 'NUMERIC_LESS_EQUAL',

/**
* Matches a numeric value equal to the target value.
Expand Down Expand Up @@ -537,7 +537,7 @@ export interface ServerTemplate {
/**
* Generic map of developer-defined signals used as evaluation input signals.
*/
export type UserProvidedSignals = {[key: string]: string|number};
export type UserProvidedSignals = { [key: string]: string | number };

/**
* Predefined template evaluation input signals.
Expand Down Expand Up @@ -727,6 +727,40 @@ export interface ServerConfig {
* @returns The value for the given key.
*/
getValue(key: string): Value;

/**
* Returns all config values.
*
* @returns A map of all config keys to their values.
*/
getAll(): { [key: string]: Value }
}

/**
* JSON-serializable representation of evaluated config values. This can be consumed by
* Remote Config web client SDKs.
*/
export interface FetchResponseData {
/**
* The HTTP status, which is useful for differentiating success responses with data from
* those without.
*
* This use of 200 and 304 response codes is consistent with Remote Config's server
* implementation.
*/
status: number;

/**
* Defines the ETag response header value.
*
* This is consistent with Remote Config's server eTag implementation.
*/
eTag: string;

/**
* Defines the map of parameters returned as "entries" in the fetch response body.
*/
config?: { [key: string]: string };
}

/**
Expand Down
64 changes: 62 additions & 2 deletions src/remote-config/remote-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

import { App } from '../app';
import * as utils from '../utils/index';
import * as validator from '../utils/validator';
import { FirebaseRemoteConfigError, RemoteConfigApiClient } from './remote-config-api-client-internal';
import { ConditionEvaluator } from './condition-evaluator-internal';
Expand All @@ -41,6 +42,7 @@ import {
GetServerTemplateOptions,
InitServerTemplateOptions,
ServerTemplateDataType,
FetchResponseData,
} from './remote-config-api';

/**
Expand Down Expand Up @@ -298,7 +300,7 @@ class RemoteConfigTemplateImpl implements RemoteConfigTemplate {
*/
class ServerTemplateImpl implements ServerTemplate {
private cache: ServerTemplateData;
private stringifiedDefaultConfig: {[key: string]: string} = {};
private stringifiedDefaultConfig: { [key: string]: string } = {};

constructor(
private readonly apiClient: RemoteConfigApiClient,
Expand Down Expand Up @@ -425,7 +427,7 @@ class ServerTemplateImpl implements ServerTemplate {
class ServerConfigImpl implements ServerConfig {
constructor(
private readonly configValues: { [key: string]: Value },
){}
) { }
getBoolean(key: string): boolean {
return this.getValue(key).asBoolean();
}
Expand All @@ -438,6 +440,9 @@ class ServerConfigImpl implements ServerConfig {
getValue(key: string): Value {
return this.configValues[key] || new ValueImpl('static');
}
getAll(): { [key: string]: Value } {
return { ...this.configValues };
}
}

/**
Expand Down Expand Up @@ -613,3 +618,58 @@ class VersionImpl implements Version {
return validator.isNonEmptyString(timestamp) && (new Date(timestamp)).getTime() > 0;
}
}

/**
* Represents a fetch response that can be used to interact with RC's client SDK.
*/
export class RemoteConfigFetchResponse {
private response: FetchResponseData;

/**
* @param app - The app for this RemoteConfig service.
* @param serverConfig - The server config for which to generate a fetch response.
* @param requestEtag - A request eTag with which to compare the current response.

Choose a reason for hiding this comment

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

What's the request in this context? Do we expect the client to pass an etag to the customer server which can be used to avoid a config pushdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct - this would come in from a client request. it should be the etag of the most recent config that the client has. Since our internal servers do this, it makes sense to allow customer servers to do it as well

*/
constructor(app: App, serverConfig: ServerConfig, requestEtag?: string) {
const config: { [key: string]: string } = {};
for (const [param, value] of Object.entries(serverConfig.getAll())) {
config[param] = value.asString();
}

const currentEtag = this.processEtag(config, app);

if (currentEtag === requestEtag) {
this.response = {
status: 304,

Choose a reason for hiding this comment

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

Optional: replace the number with a readable constant?

eTag: currentEtag,
};
} else {
this.response = {
status: 200,
eTag: currentEtag,
config,
}
}
}

/**
* @returns JSON representation of the fetch response that can be consumed
* by the RC client SDK.
*/
Comment on lines +658 to +661

Choose a reason for hiding this comment

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

Probably needs to be annotated with @public to show up in the API docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't actually know - none of the other public methods are annotated with an @public...

public toJSON(): FetchResponseData {
return this.response;
}

private processEtag(config: { [key: string]: string }, app: App): string {
const configJson = JSON.stringify(config);
let hash = 0;
for (let i = 0; i < configJson.length; i++) {
const char = configJson.charCodeAt(i);
hash = (hash << 5) - hash + char;

Choose a reason for hiding this comment

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

This is quite cool. Should we add a comment describing the equation and also that we're trying to replicate the String.hashcode() impl here for context? I see that the java hashcode implementation mentions it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added

hash |= 0;

Choose a reason for hiding this comment

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

What is the significance of this operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment per below comment

}
const projectId = utils.getExplicitProjectId(app);
const parts = ['etag', projectId, 'firebase-server', 'fetch', hash];
return parts.filter(a => !!a).join('-');
}
}
97 changes: 95 additions & 2 deletions test/unit/remote-config/remote-config.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
RemoteConfigCondition,
TagColor,
ListVersionsResult,
RemoteConfigFetchResponse,
} from '../../../src/remote-config/index';
import { FirebaseApp } from '../../../src/app/firebase-app';
import * as mocks from '../../resources/mocks';
Expand Down Expand Up @@ -1292,13 +1293,46 @@ 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('getAll', () => {
it('should return all values', () => {
const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData;
templateData.parameters = {
dog_type: {
defaultValue: {
value: 'pug'
}
},
dog_type_enabled: {
defaultValue: {
value: 'true'
}
},
dog_age: {
defaultValue: {
value: '22'
}
},
dog_use_inapp_default: {
defaultValue: {
useInAppDefault: true
}
},
};
const template = remoteConfig.initServerTemplate({ template: templateData });
const config = template.evaluate().getAll();
expect(Object.keys(config)).deep.equal(['dog_type', 'dog_type_enabled', 'dog_age']);
expect(config['dog_type'].asString()).to.equal('pug');
expect(config['dog_type_enabled'].asBoolean()).to.equal(true);
expect(config['dog_age'].asNumber()).to.equal(22);
});
});

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');
Expand Down Expand Up @@ -1391,6 +1425,65 @@ describe('RemoteConfig', () => {
});
});

describe('RemoteConfigFetchResponse', () => {
it('should return a 200 response with no etag', () => {

Choose a reason for hiding this comment

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

I was thrown off by the description, can we rephrase it as - "should return a 200 response when supplied with no etag"? Similarly for the next test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing!

const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData;
// Defines remote parameter values.
templateData.parameters = {
dog_type: {
defaultValue: {
value: 'beagle'
}
}
};
const template = remoteConfig.initServerTemplate({ template: templateData });
const fetchResponse = new RemoteConfigFetchResponse(mockApp, template.evaluate());
expect(fetchResponse.toJSON()).deep.equals({
status: 200,
eTag: 'etag-project_id-firebase-server-fetch--2039110429',
config: { 'dog_type': 'beagle' }
});
});

it('should return a 200 response with a stale etag', () => {
const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData;
// Defines remote parameter values.
templateData.parameters = {
dog_type: {
defaultValue: {
value: 'beagle'
}
}
};
const template = remoteConfig.initServerTemplate({ template: templateData });
const fetchResponse = new RemoteConfigFetchResponse(mockApp, template.evaluate(), 'fake-etag');
expect(fetchResponse.toJSON()).deep.equals({
status: 200,
eTag: 'etag-project_id-firebase-server-fetch--2039110429',
config: { 'dog_type': 'beagle' }
});
});

it('should return a 304 repsonse with matching etag', () => {
const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData;
// Defines remote parameter values.
templateData.parameters = {
dog_type: {
defaultValue: {
value: 'beagle'
}
}
};
const template = remoteConfig.initServerTemplate({ template: templateData });
const fetchResponse = new RemoteConfigFetchResponse(
mockApp, template.evaluate(), 'etag-project_id-firebase-server-fetch--2039110429');
expect(fetchResponse.toJSON()).deep.equals({
status: 304,
eTag: 'etag-project_id-firebase-server-fetch--2039110429'
});
});
});

function runInvalidResponseTests(rcOperation: () => Promise<RemoteConfigTemplate>,
operationName: any): void {
it('should propagate API errors', () => {
Expand Down
Loading