-
Notifications
You must be signed in to change notification settings - Fork 373
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
base: master
Are you sure you want to change the base?
Changes from 7 commits
13c1b1d
62bf436
812f374
eaf78e2
e1dff12
805dec8
e67c529
8d66a91
6b60fac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -41,6 +42,7 @@ import { | |
GetServerTemplateOptions, | ||
InitServerTemplateOptions, | ||
ServerTemplateDataType, | ||
FetchResponseData, | ||
} from './remote-config-api'; | ||
|
||
/** | ||
|
@@ -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, | ||
|
@@ -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(); | ||
} | ||
|
@@ -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 }; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -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. | ||
*/ | ||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably needs to be annotated with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment added |
||
hash |= 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the significance of this operation? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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('-'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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'); | ||
|
@@ -1391,6 +1425,65 @@ describe('RemoteConfig', () => { | |
}); | ||
}); | ||
|
||
describe('RemoteConfigFetchResponse', () => { | ||
it('should return a 200 response with no etag', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', () => { | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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