From 65a95b26edb357daa209f78e4fe5b7fe03f9b4a0 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Tue, 21 Apr 2020 14:03:03 -0700 Subject: [PATCH 1/3] Don't mutate datafile object --- .../lib/core/project_config/index.js | 40 ++++++++++++++++--- .../lib/core/project_config/index.tests.js | 16 +++++++- .../lib/optimizely/index.tests.js | 22 ++++++++++ 3 files changed, 70 insertions(+), 8 deletions(-) diff --git a/packages/optimizely-sdk/lib/core/project_config/index.js b/packages/optimizely-sdk/lib/core/project_config/index.js index 7624a266b..a120775d6 100644 --- a/packages/optimizely-sdk/lib/core/project_config/index.js +++ b/packages/optimizely-sdk/lib/core/project_config/index.js @@ -36,20 +36,36 @@ var MODULE_NAME = 'PROJECT_CONFIG'; export var createProjectConfig = function(datafile) { var projectConfig = fns.assign({}, datafile); - /* - * Conditions of audiences in projectConfig.typedAudiences are not - * expected to be string-encoded as they are here in projectConfig.audiences. - */ - (projectConfig.audiences || []).forEach(function(audience) { - audience.conditions = JSON.parse(audience.conditions); + // Shallow copy audiences to avoid mutating datafile object (below we will overwrite + // the conditions string with the parsed conditions) + projectConfig.audiences = (datafile.audiences || []).map(function(audience) { + var audienceCopy = fns.assign({}, audience); + /* + * Note: Conditions of audiences in projectConfig.typedAudiences are not + * expected to be string-encoded as they are here in projectConfig.audiences. + */ + audienceCopy.conditions = JSON.parse(audienceCopy.conditions); + return audienceCopy; }); projectConfig.audiencesById = fns.keyBy(projectConfig.audiences, 'id'); fns.assign(projectConfig.audiencesById, fns.keyBy(projectConfig.typedAudiences, 'id')); projectConfig.attributeKeyMap = fns.keyBy(projectConfig.attributes, 'key'); projectConfig.eventKeyMap = fns.keyBy(projectConfig.events, 'key'); + + projectConfig.groups = (datafile.groups || []).map(function(group) { + var groupCopy = fns.assign({}, group); + groupCopy.experiments = (group.experiments || []).map(function(experiment) { + return fns.assign({}, experiment); + }); + return groupCopy; + }); projectConfig.groupIdMap = fns.keyBy(projectConfig.groups, 'id'); + projectConfig.experiments = (datafile.experiments || []).map(function(experiment) { + return fns.assign({}, experiment); + }); + var experiments; Object.keys(projectConfig.groupIdMap || {}).forEach(function(Id) { experiments = projectConfig.groupIdMap[Id].experiments; @@ -58,6 +74,14 @@ export var createProjectConfig = function(datafile) { }); }); + projectConfig.rollouts = (datafile.rollouts || []).map(function(rollout) { + var rolloutCopy = fns.assign({}, rollout); + rolloutCopy.experiments = (rollout.experiments || []).map(function(experiment) { + return fns.assign({}, experiment); + }); + return rolloutCopy; + }); + projectConfig.rolloutIdMap = fns.keyBy(projectConfig.rollouts || [], 'id'); objectValues(projectConfig.rolloutIdMap || {}).forEach(function (rollout) { (rollout.experiments || []).forEach(function(experiment) { @@ -89,6 +113,10 @@ export var createProjectConfig = function(datafile) { // for checking that experiment is a feature experiment or not. projectConfig.experimentFeatureMap = {}; + projectConfig.featureFlags = (datafile.featureFlags || []).map(function(featureFlag) { + return fns.assign({}, featureFlag); + }); + projectConfig.featureKeyMap = fns.keyBy(projectConfig.featureFlags || [], 'key'); objectValues(projectConfig.featureKeyMap || {}).forEach(function(feature) { feature.variableKeyMap = fns.keyBy(feature.variables, 'key'); diff --git a/packages/optimizely-sdk/lib/core/project_config/index.tests.js b/packages/optimizely-sdk/lib/core/project_config/index.tests.js index a76bdfea4..fadc8a866 100644 --- a/packages/optimizely-sdk/lib/core/project_config/index.tests.js +++ b/packages/optimizely-sdk/lib/core/project_config/index.tests.js @@ -33,14 +33,13 @@ import configValidator from '../../utils/config_validator'; var logger = getLogger(); describe('lib/core/project_config', function() { - var parsedAudiences = testDatafile.getParsedAudiences; describe('createProjectConfig method', function() { it('should set properties correctly when createProjectConfig is called', function() { var testData = testDatafile.getTestProjectConfig(); var configObj = projectConfig.createProjectConfig(testData); forEach(testData.audiences, function(audience) { - audience.conditions = audience.conditions; + audience.conditions = JSON.parse(audience.conditions); }); assert.strictEqual(configObj.accountId, testData.accountId); @@ -48,6 +47,12 @@ describe('lib/core/project_config', function() { assert.strictEqual(configObj.revision, testData.revision); assert.deepEqual(configObj.events, testData.events); assert.deepEqual(configObj.audiences, testData.audiences); + testData.groups.forEach(function(group) { + group.experiments.forEach(function(experiment) { + experiment.groupId = group.id; + experiment.variationKeyMap = fns.keyBy(experiment.variations, 'key'); + }); + }); assert.deepEqual(configObj.groups, testData.groups); var expectedGroupIdMap = { @@ -170,6 +175,13 @@ describe('lib/core/project_config', function() { assert.deepEqual(configObj.variationIdMap, expectedVariationIdMap); }); + it('should not mutate the datafile', function() { + var datafile = testDatafile.getTypedAudiencesConfig(); + var datafileClone = cloneDeep(datafile); + projectConfig.createProjectConfig(datafile); + assert.deepEqual(datafileClone, datafile); + }); + describe('feature management', function() { var configObj; beforeEach(function() { diff --git a/packages/optimizely-sdk/lib/optimizely/index.tests.js b/packages/optimizely-sdk/lib/optimizely/index.tests.js index 917820493..6e9ec01f1 100644 --- a/packages/optimizely-sdk/lib/optimizely/index.tests.js +++ b/packages/optimizely-sdk/lib/optimizely/index.tests.js @@ -245,6 +245,28 @@ describe('lib/optimizely', function() { }); }); }); + + it('should support constructing two instances using the same datafile object', function() { + var datafile = testData.getTypedAudiencesConfig(); + var optlyInstance = new Optimizely({ + clientEngine: 'node-sdk', + datafile: datafile, + errorHandler: stubErrorHandler, + eventDispatcher: stubEventDispatcher, + jsonSchemaValidator: jsonSchemaValidator, + logger: createdLogger, + }); + assert.instanceOf(optlyInstance, Optimizely); + var optlyInstance2 = new Optimizely({ + clientEngine: 'node-sdk', + datafile: datafile, + errorHandler: stubErrorHandler, + eventDispatcher: stubEventDispatcher, + jsonSchemaValidator: jsonSchemaValidator, + logger: createdLogger, + }); + assert.instanceOf(optlyInstance2, Optimizely); + }); }); }); From 240479748aaf7e3d7941439a0a22963844881e6b Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Tue, 21 Apr 2020 14:12:15 -0700 Subject: [PATCH 2/3] createMutationSafeDatafileCopy --- .../lib/core/project_config/index.js | 72 +++++++++---------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/packages/optimizely-sdk/lib/core/project_config/index.js b/packages/optimizely-sdk/lib/core/project_config/index.js index a120775d6..b2d39ca15 100644 --- a/packages/optimizely-sdk/lib/core/project_config/index.js +++ b/packages/optimizely-sdk/lib/core/project_config/index.js @@ -28,44 +28,56 @@ var EXPERIMENT_RUNNING_STATUS = 'Running'; var RESERVED_ATTRIBUTE_PREFIX = '$opt_'; var MODULE_NAME = 'PROJECT_CONFIG'; +function createMutationSafeDatafileCopy(datafile) { + var datafileCopy = fns.assign({}, datafile); + datafileCopy.audiences = (datafile.audiences || []).map(function(audience) { + return fns.assign({}, audience); + }); + datafileCopy.groups = (datafile.groups || []).map(function(group) { + var groupCopy = fns.assign({}, group); + groupCopy.experiments = (group.experiments || []).map(function(experiment) { + return fns.assign({}, experiment); + }); + return groupCopy; + }); + datafileCopy.experiments = (datafile.experiments || []).map(function(experiment) { + return fns.assign({}, experiment); + }); + datafileCopy.rollouts = (datafile.rollouts || []).map(function(rollout) { + var rolloutCopy = fns.assign({}, rollout); + rolloutCopy.experiments = (rollout.experiments || []).map(function(experiment) { + return fns.assign({}, experiment); + }); + return rolloutCopy; + }); + datafileCopy.featureFlags = (datafile.featureFlags || []).map(function(featureFlag) { + return fns.assign({}, featureFlag); + }); + return datafileCopy; +} + /** * Creates projectConfig object to be used for quick project property lookup * @param {Object} datafile JSON datafile representing the project * @return {Object} Object representing project configuration */ export var createProjectConfig = function(datafile) { - var projectConfig = fns.assign({}, datafile); - - // Shallow copy audiences to avoid mutating datafile object (below we will overwrite - // the conditions string with the parsed conditions) - projectConfig.audiences = (datafile.audiences || []).map(function(audience) { - var audienceCopy = fns.assign({}, audience); - /* - * Note: Conditions of audiences in projectConfig.typedAudiences are not - * expected to be string-encoded as they are here in projectConfig.audiences. - */ - audienceCopy.conditions = JSON.parse(audienceCopy.conditions); - return audienceCopy; + var projectConfig = createMutationSafeDatafileCopy(datafile); + + /* + * Conditions of audiences in projectConfig.typedAudiences are not + * expected to be string-encoded as they are here in projectConfig.audiences. + */ + (projectConfig.audiences || []).forEach(function(audience) { + audience.conditions = JSON.parse(audience.conditions); }); projectConfig.audiencesById = fns.keyBy(projectConfig.audiences, 'id'); fns.assign(projectConfig.audiencesById, fns.keyBy(projectConfig.typedAudiences, 'id')); projectConfig.attributeKeyMap = fns.keyBy(projectConfig.attributes, 'key'); projectConfig.eventKeyMap = fns.keyBy(projectConfig.events, 'key'); - - projectConfig.groups = (datafile.groups || []).map(function(group) { - var groupCopy = fns.assign({}, group); - groupCopy.experiments = (group.experiments || []).map(function(experiment) { - return fns.assign({}, experiment); - }); - return groupCopy; - }); projectConfig.groupIdMap = fns.keyBy(projectConfig.groups, 'id'); - projectConfig.experiments = (datafile.experiments || []).map(function(experiment) { - return fns.assign({}, experiment); - }); - var experiments; Object.keys(projectConfig.groupIdMap || {}).forEach(function(Id) { experiments = projectConfig.groupIdMap[Id].experiments; @@ -74,14 +86,6 @@ export var createProjectConfig = function(datafile) { }); }); - projectConfig.rollouts = (datafile.rollouts || []).map(function(rollout) { - var rolloutCopy = fns.assign({}, rollout); - rolloutCopy.experiments = (rollout.experiments || []).map(function(experiment) { - return fns.assign({}, experiment); - }); - return rolloutCopy; - }); - projectConfig.rolloutIdMap = fns.keyBy(projectConfig.rollouts || [], 'id'); objectValues(projectConfig.rolloutIdMap || {}).forEach(function (rollout) { (rollout.experiments || []).forEach(function(experiment) { @@ -113,10 +117,6 @@ export var createProjectConfig = function(datafile) { // for checking that experiment is a feature experiment or not. projectConfig.experimentFeatureMap = {}; - projectConfig.featureFlags = (datafile.featureFlags || []).map(function(featureFlag) { - return fns.assign({}, featureFlag); - }); - projectConfig.featureKeyMap = fns.keyBy(projectConfig.featureFlags || [], 'key'); objectValues(projectConfig.featureKeyMap || {}).forEach(function(feature) { feature.variableKeyMap = fns.keyBy(feature.variables, 'key'); From 310dd5a645a0585275fe3870cbcdfaf085ad5325 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Tue, 21 Apr 2020 14:49:05 -0700 Subject: [PATCH 3/3] Move shallower copies to top --- .../optimizely-sdk/lib/core/project_config/index.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/optimizely-sdk/lib/core/project_config/index.js b/packages/optimizely-sdk/lib/core/project_config/index.js index b2d39ca15..bcc9e20dd 100644 --- a/packages/optimizely-sdk/lib/core/project_config/index.js +++ b/packages/optimizely-sdk/lib/core/project_config/index.js @@ -33,6 +33,12 @@ function createMutationSafeDatafileCopy(datafile) { datafileCopy.audiences = (datafile.audiences || []).map(function(audience) { return fns.assign({}, audience); }); + datafileCopy.experiments = (datafile.experiments || []).map(function(experiment) { + return fns.assign({}, experiment); + }); + datafileCopy.featureFlags = (datafile.featureFlags || []).map(function(featureFlag) { + return fns.assign({}, featureFlag); + }); datafileCopy.groups = (datafile.groups || []).map(function(group) { var groupCopy = fns.assign({}, group); groupCopy.experiments = (group.experiments || []).map(function(experiment) { @@ -40,9 +46,6 @@ function createMutationSafeDatafileCopy(datafile) { }); return groupCopy; }); - datafileCopy.experiments = (datafile.experiments || []).map(function(experiment) { - return fns.assign({}, experiment); - }); datafileCopy.rollouts = (datafile.rollouts || []).map(function(rollout) { var rolloutCopy = fns.assign({}, rollout); rolloutCopy.experiments = (rollout.experiments || []).map(function(experiment) { @@ -50,9 +53,6 @@ function createMutationSafeDatafileCopy(datafile) { }); return rolloutCopy; }); - datafileCopy.featureFlags = (datafile.featureFlags || []).map(function(featureFlag) { - return fns.assign({}, featureFlag); - }); return datafileCopy; }