fix (datafile-parsing): Prevent newer versions datafile #141
Conversation
optimizely/helpers/enums.py
Outdated
|
|
||
|
|
||
| class DatafileVersions(object): | ||
| V1 = '1' |
There was a problem hiding this comment.
This is no longer needed
tests/test_optimizely.py
Outdated
| @@ -125,16 +145,41 @@ class InvalidErrorHandler(object): | |||
| def test_init__v1_datafile__logs_error(self): | |||
| """ Test that v1 datafile logs error on init. """ | |||
|
|
|||
| self.config_dict['version'] = project_config.V1_CONFIG_VERSION | |||
| self.config_dict['version'] = enums.DatafileVersions.V1 | |||
There was a problem hiding this comment.
I think we can get rid of this test case as it tests the exact same thing as the one below
optimizely/optimizely.py
Outdated
| # We actually want to log this error to stderr, so make sure the logger | ||
| # has a handler capable of doing that. | ||
| self.logger.error(enums.Errors.UNSUPPORTED_DATAFILE_VERSION) | ||
| self.logger = _logging.reset_logger(self.logger_name) | ||
| self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('datafile')) |
There was a problem hiding this comment.
Can we check the type of the exception, and if it's exceptions.UnsupportedDatafileVersionException, then we log differently? I'd like that better than having two except clauses here.
Also, we should pass the exception to the error_handler instance
tests/test_optimizely.py
Outdated
|
|
||
| self.config_dict['version'] = project_config.V1_CONFIG_VERSION | ||
| self.config_dict['version'] = '5' |
There was a problem hiding this comment.
Please create a mock unsupported datafile and load into this test case.
| # limitations under the License. | ||
|
|
||
|
|
||
| class InvalidAttributeException(Exception): |
There was a problem hiding this comment.
nit. Can you arrange all these classes alphabetically.
optimizely/helpers/enums.py
Outdated
| @@ -65,3 +64,9 @@ class ControlAttributes(object): | |||
| BOT_FILTERING = '$opt_bot_filtering' | |||
| BUCKETING_ID = '$opt_bucketing_id' | |||
| USER_AGENT = '$opt_user_agent' | |||
|
|
|||
|
|
|||
| class DatafileVersions(object): | |||
There was a problem hiding this comment.
nit. Lets arrange classes in this file alphabetically.
optimizely/optimizely.py
Outdated
| @@ -63,20 +63,22 @@ def __init__(self, | |||
|
|
|||
| try: | |||
| self.config = project_config.ProjectConfig(datafile, self.logger, self.error_handler) | |||
| except: | |||
|
|
|||
There was a problem hiding this comment.
nit. Unnecessary newline.
optimizely/optimizely.py
Outdated
| # We actually want to log this error to stderr, so make sure the logger | ||
| # has a handler capable of doing that. | ||
| self.logger.error(enums.Errors.UNSUPPORTED_DATAFILE_VERSION) | ||
| if type(error) is exceptions.UnsupportedDatafileVersionException: |
There was a problem hiding this comment.
I'd recommend instead of doing this having multiple except clauses
There was a problem hiding this comment.
Hmm, I actually asked them to combine the multiple except clauses because the only thing that differs is the messaging. Otherwise we'd be duplicating (just 1 thing right now) the part where we pass the error to the handler.
There was a problem hiding this comment.
After thinking about it a bit more, I think having the multiple clauses is the more idiomatic way of dealing with this. We can instead, save the message and error objects in variables in each of the except clauses and introduce a finally clause that checks if we recorded any errors, and if so, then log + send to error handler. WDYT? @aliabbasrizvi
There was a problem hiding this comment.
Hey @mikeng13 ... yes that is more idiomatic and I believe we should adopt that.
No description provided.