fix(initialize): Prevent SDK from initializing if the datafile version in invalid. #161

mfahadahmed · 2018-09-03T15:08:58Z

No description provided.

coveralls · 2018-09-03T15:14:27Z

Coverage increased (+0.02%) to 97.135% when pulling ad1a30b on fahad/unsupported-datafile into a72082b on master.

oakbani

Looks good. Just minor changes

oakbani · 2018-09-04T07:13:17Z

packages/optimizely-sdk/lib/core/project_config/index.tests.js

@@ -146,6 +146,22 @@ describe('lib/core/project_config', function() {
      assert.deepEqual(configObj.variationIdMap, expectedVariationIdMap);
    });

+    it('should throw exception with invalid config version', function() {


nit. throw an error with unsupported datafile version
Same below.

oakbani · 2018-09-04T07:15:48Z

packages/optimizely-sdk/lib/optimizely/index.tests.js

@@ -152,6 +152,27 @@ describe('lib/optimizely', function() {
        assert.strictEqual(logMessage, sprintf(ERROR_MESSAGES.INVALID_DATAFILE, 'JSON_SCHEMA_VALIDATOR', 'projectId', 'is missing and it is required'));
      });

+      it('should log an error if the datafile version is not valid', function() {


nit. is not supported

oakbani · 2018-09-04T07:19:39Z

packages/optimizely-sdk/lib/utils/enums/index.js

+/*
+ * Possible types of variables attached to features
+ */
+exports.CONFIG_VERSIONS = {


nit. DATAFILE_VERSIONS

oakbani · 2018-09-04T07:20:01Z

packages/optimizely-sdk/lib/utils/enums/index.js

@@ -186,3 +187,12 @@ exports.FEATURE_VARIABLE_TYPES = {
  INTEGER: 'integer',
  STRING: 'string',
 };
+
+/*
+ * Possible types of variables attached to features


nit. Remove comment

oakbani · 2018-09-04T07:21:45Z

packages/optimizely-sdk/lib/utils/enums/index.js

@@ -59,6 +59,7 @@ exports.ERROR_MESSAGES = {
  VARIATION_ID_NOT_IN_DATAFILE: '%s: No variation ID %s defined in datafile for experiment %s.',
  VARIATION_ID_NOT_IN_DATAFILE_NO_EXPERIMENT: '%s: Variation ID %s is not in the datafile.',
  INVALID_INPUT_FORMAT: '%s: Provided %s is in an invalid format.',
+  INVALID_CONFIG_VERSION: '%s: This version of the Javascript SDK does not support the given datafile version: %s',


nit UNSUPPORTED_DATAFILE_VERSION

mikeproeng37

We already have all the validation in the config_validator module. Please move this logic there are well: https://github.com/optimizely/javascript-sdk/blob/master/packages/optimizely-sdk/lib/utils/config_validator/index.js#L35

mfahadahmed · 2018-09-05T14:43:40Z

Currently most of the datafile parsing and validations are performed inside Optimizely constructor. The config_validator module actually validates logger and error handler instances.
Should we move all those datafile validations from Optimizely constructor to config_validator?

mikeproeng37 · 2018-09-05T18:18:01Z

Yep, we should move the version validation out of the project_config into the config validator, as well as lines 63 through 77 from the Optimizely constructor into the config validator as well. Moving code out of that constructor makes it more maintainable and easier to follow

…dator's validate method.

msohailhussain · 2018-09-06T17:32:34Z

Please don't review this PR. Some of the changes needed to make in this PR, will let you know when ready.

…dator's validateDatafile method.

mfahadahmed · 2018-09-07T14:20:43Z

@mikeng13 : The approach we should take to isolate datafile validations from Optimizely constructor requires more rework. I have some queries related to that.

Right now, I moved datafile validations in a separate config_validator method and called it from Optimizely constructor.

To completely isolate validations from Optimizely constructor, we should perform it in createInstance method and move all the instance creation unit tests from optimizely.tests to index.node.tests.js and index.browser.tests.js files. Should I go with that or the current implementation is enough?

mikeproeng37

@mfahadahmed no need to move the other validation code for now. This is enough of a refactor. I've also requested some changes in a few places.

mikeproeng37 · 2018-09-07T20:39:20Z

packages/optimizely-sdk/lib/utils/config_validator/index.js

+   * Validates the datafile
+   * @param {string}  datafile
+   * @return {Boolean} True if the datafile is valid
+   * @throws If any of the datafile validations get failed


@throws If the datafile is not valid for any of the following reasons: - The datafile string is undefined - The datafile string cannot be parsed as a JSON object - The datafile version is not supported

mikeproeng37 · 2018-09-07T20:40:43Z

packages/optimizely-sdk/lib/utils/config_validator/index.tests.js

+        }, sprintf(ERROR_MESSAGES.INVALID_DATAFILE_MALFORMED, 'CONFIG_VALIDATOR'));
+      });
+
+      it('should complain if datafile version is not supported', function() {


Can you add some more test cases for things like boolean, number, array

nvm this, it is not really needed.

mikeproeng37 · 2018-09-07T20:40:59Z

packages/optimizely-sdk/lib/utils/enums/index.js

@@ -59,6 +59,7 @@ exports.ERROR_MESSAGES = {
  VARIATION_ID_NOT_IN_DATAFILE: '%s: No variation ID %s defined in datafile for experiment %s.',
  VARIATION_ID_NOT_IN_DATAFILE_NO_EXPERIMENT: '%s: Variation ID %s is not in the datafile.',
  INVALID_INPUT_FORMAT: '%s: Provided %s is in an invalid format.',
+  INVALID_DATAFILE_VERSION: '%s: This version of the Javascript SDK does not support the given datafile version: %s',


JavaScript

mikeproeng37 · 2018-09-07T20:42:27Z

packages/optimizely-sdk/lib/index.node.tests.js

+        createdLogger.log.restore();
+      });
+
+      it('should log an error when datafile is not provided', function() {


These are duplicate test cases from the config validator. We just need two test cases:

datafile is valid

datafile validation throws error (any) error

I think that we need to add only 1 test case here. That config validator does get called from createInstance and if it throws an error, assert that the isValidInstance is set to False.
The datafile validations are already taken care of in config validator tests.

oakbani · 2018-09-11T11:47:07Z

packages/optimizely-sdk/lib/index.node.tests.js

+
+      it('should create optimizely instance with valid datafile', function() {
+        var optlyInstance = optimizelyFactory.createInstance({
+          datafile: JSON.stringify(testData.getUnsupportedVersionConfig()),


In case, you keep this test case, assert that validator doesn't thrown an error and isValidInstance is true. Instance will always be created like it does here with UnsupportedVersionConfig

mfahadahmed · 2018-09-12T16:18:55Z

Oh I think none of the datafile validation tests are needed in index.node.tests.js as all of these test cases are already covered in optimizely.tests. Sorry, I forgot to remove these test cases from index.node.tests.js after changing validation logic.

oakbani · 2018-09-13T05:16:01Z

packages/optimizely-sdk/lib/utils/config_validator/index.js

@@ -17,6 +17,13 @@ var sprintf = require('sprintf-js').sprintf;

 var ERROR_MESSAGES = require('../enums').ERROR_MESSAGES;
 var MODULE_NAME = 'CONFIG_VALIDATOR';
+var DATAFILE_VERSIONS = require('../enums').DATAFILE_VERSIONS;


@mfahadahmed Please update header for this file and it's test file.

mikeproeng37

Lgtm

Prevent SDK from initializing if the datafile version in invalid.

d9acc22

mfahadahmed changed the title ~~Prevent SDK from initializing if the datafile version in invalid.~~ fix(initialize): Prevent SDK from initializing if the datafile version in invalid. Sep 3, 2018

oakbani suggested changes Sep 4, 2018

View reviewed changes

Addressed code review defects.

c88fe3a

mikeproeng37 suggested changes Sep 4, 2018

View reviewed changes

Moved datafile validations from Optimizely constructor to config_vali…

65705fc

…dator's validate method.

Moved datafile validations from Optimizely constructor to config_vali…

e9f3def

…dator's validateDatafile method.

Update headers.

a310249

mikeproeng37 suggested changes Sep 7, 2018

View reviewed changes

oakbani reviewed Sep 11, 2018

View reviewed changes

Remove unit tests from index.node.tests.js

aad73d5

oakbani reviewed Sep 13, 2018

View reviewed changes

Update headers.

ad1a30b

oakbani approved these changes Sep 13, 2018

View reviewed changes

mikeproeng37 approved these changes Sep 13, 2018

View reviewed changes

mikeproeng37 merged commit 0d263fc into master Sep 13, 2018

msohailhussain deleted the fahad/unsupported-datafile branch September 20, 2018 21:28

fix(initialize): Prevent SDK from initializing if the datafile version in invalid. #161

fix(initialize): Prevent SDK from initializing if the datafile version in invalid. #161

Uh oh!

Conversation

mfahadahmed commented Sep 3, 2018

Uh oh!

coveralls commented Sep 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oakbani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

mfahadahmed commented Sep 5, 2018

Uh oh!

mikeproeng37 commented Sep 5, 2018

Uh oh!

msohailhussain commented Sep 6, 2018

Uh oh!

mfahadahmed commented Sep 7, 2018

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfahadahmed commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Sep 3, 2018 •

edited

Loading