Skip to content

Commit

Permalink
Fixes incorrect use of Object.assign when backfilling SSRC config w…
Browse files Browse the repository at this point in the history
…ith defaults. (#2503)

In the logic where we backfill config with defaults, the first argument to Object.assign should be an object to assign to, but the code passed the object containing the defaults.
  • Loading branch information
erikeldridge authored Mar 21, 2024
1 parent f89632a commit 7246526
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 6 deletions.
6 changes: 4 additions & 2 deletions src/remote-config/remote-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -334,8 +334,10 @@ class ServerTemplateImpl implements ServerTemplate {
evaluatedConfig[key] = this.parseRemoteConfigParameterValue(valueType, parameterDefaultValue);
}

// Merges rendered config over default config.
const mergedConfig = Object.assign(this.defaultConfig, evaluatedConfig);
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.
Expand Down
66 changes: 62 additions & 4 deletions test/unit/remote-config/remote-config.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -948,19 +948,77 @@ describe('RemoteConfig', () => {
});
});

it('overrides local default when value exists', () => {
it('uses local default when in-app default value specified after loading remote values', async () => {
// We had a bug caused by forgetting the first argument to
// Object.assign. This resulted in defaultConfig being overwritten
// by the remote values. So this test asserts we can use in-app
// default after loading remote values.
const template = remoteConfig.initServerTemplate({
defaultConfig: {
dog_type: 'corgi'
}
});

const response = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE);

response.parameters = {
dog_type: {
defaultValue: {
value: 'pug'
},
valueType: 'STRING'
},
}

template.cache = response as ServerTemplateData;

let config = template.evaluate();

expect(config.dog_type).to.equal('pug');

response.parameters = {
dog_type: {
defaultValue: {
useInAppDefault: true
},
valueType: 'STRING'
},
}

template.cache = response as ServerTemplateData;

config = template.evaluate();

expect(config.dog_type).to.equal('corgi');
});

it('overrides local default when remote value exists', () => {
const response = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE);
response.parameters = {
dog_type_enabled: {
defaultValue: {
// Defines remote value
value: 'true'
},
valueType: 'BOOLEAN'
},
}

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

return remoteConfig.getServerTemplate({
defaultConfig: {
// Defines local default
dog_type_enabled: false
}
})
.then((template: ServerTemplate) => {
const config = template.evaluate!();
expect(config.dog_type_enabled).to.equal(template.defaultConfig.dog_type_enabled);
const config = template.evaluate();
// Asserts remote value overrides local default.
expect(config.dog_type_enabled).to.be.true;
});
});
});
Expand Down

0 comments on commit 7246526

Please sign in to comment.