From 975bf8466928df502a8e7f26764a19b5acb92501 Mon Sep 17 00:00:00 2001 From: Rashid Siddique Date: Thu, 30 Aug 2018 15:28:40 +0500 Subject: [PATCH 1/2] fix(datafile-parsing): Prevent newer versions datafile --- lib/optimizely.rb | 10 ++++------ lib/optimizely/exceptions.rb | 7 +++---- lib/optimizely/helpers/constants.rb | 6 ++++++ lib/optimizely/project_config.rb | 17 +---------------- spec/project_config_spec.rb | 11 ----------- spec/project_spec.rb | 15 +++++++++++++-- spec/spec_params.rb | 2 +- 7 files changed, 28 insertions(+), 40 deletions(-) diff --git a/lib/optimizely.rb b/lib/optimizely.rb index bce9c82b..6a362b15 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -64,17 +64,15 @@ def initialize(datafile, event_dispatcher = nil, logger = nil, error_handler = n begin @config = ProjectConfig.new(datafile, @logger, @error_handler) - rescue + rescue InvalidDatafileVersionError => e @is_valid = false @logger = SimpleLogger.new - @logger.log(Logger::ERROR, InvalidInputError.new('datafile').message) + @logger.log(Logger::ERROR, e.message) return - end - - unless @config.parsing_succeeded? + rescue @is_valid = false @logger = SimpleLogger.new - @logger.log(Logger::ERROR, InvalidDatafileVersionError.new.message) + @logger.log(Logger::ERROR, InvalidInputError.new('datafile').message) return end diff --git a/lib/optimizely/exceptions.rb b/lib/optimizely/exceptions.rb index 818ab610..ce1c768a 100644 --- a/lib/optimizely/exceptions.rb +++ b/lib/optimizely/exceptions.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # -# Copyright 2016-2017, Optimizely and contributors +# Copyright 2016-2018, Optimizely and contributors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -85,9 +85,8 @@ def initialize(aborted_method) class InvalidDatafileVersionError < Error # Raised when a datafile with an unsupported version is provided - def initialize(msg = 'Provided datafile is an unsupported version. Please use SDK version 1.1.2 or earlier '\ - 'for datafile version 1.') - super + def initialize(version) + super("This version of the Ruby SDK does not support the given datafile version: #{version}.") end end diff --git a/lib/optimizely/helpers/constants.rb b/lib/optimizely/helpers/constants.rb index 93756a07..c35fcd8b 100644 --- a/lib/optimizely/helpers/constants.rb +++ b/lib/optimizely/helpers/constants.rb @@ -321,6 +321,12 @@ module Constants 'BUCKETING_ID' => '$opt_bucketing_id', 'USER_AGENT' => '$opt_user_agent' }.freeze + + SUPPORTED_VERSIONS = { + 'v2' => '2', + 'v3' => '3', + 'v4' => '4' + }.freeze end end end diff --git a/lib/optimizely/project_config.rb b/lib/optimizely/project_config.rb index f05c51e3..f452c4e4 100644 --- a/lib/optimizely/project_config.rb +++ b/lib/optimizely/project_config.rb @@ -19,10 +19,6 @@ require_relative 'helpers/validator' module Optimizely - V1_CONFIG_VERSION = '1' - - UNSUPPORTED_VERSIONS = [V1_CONFIG_VERSION].freeze - class ProjectConfig # Representation of the Optimizely project config. RUNNING_EXPERIMENT_STATUS = ['Running'].freeze @@ -39,7 +35,6 @@ class ProjectConfig attr_reader :experiments attr_reader :feature_flags attr_reader :groups - attr_reader :parsing_succeeded attr_reader :project_id # Boolean - denotes if Optimizely should remove the last block of visitors' IP address before storing event data attr_reader :anonymize_ip @@ -75,12 +70,11 @@ def initialize(datafile, logger, error_handler) config = JSON.parse(datafile) - @parsing_succeeded = false @error_handler = error_handler @logger = logger @version = config['version'] - return if UNSUPPORTED_VERSIONS.include?(@version) + raise InvalidDatafileVersionError, @version unless Helpers::Constants::SUPPORTED_VERSIONS.value?(@version) @account_id = config['accountId'] @attributes = config.fetch('attributes', []) @@ -147,7 +141,6 @@ def initialize(datafile, logger, error_handler) @feature_flag_key_map.each do |key, feature_flag| @feature_variable_key_map[key] = generate_key_map(feature_flag['variables'], 'key') end - @parsing_succeeded = true end def experiment_running?(experiment) @@ -389,14 +382,6 @@ def get_attribute_id(attribute_key) nil end - def parsing_succeeded? - # Helper method to determine if parsing the datafile was successful. - # - # Returns Boolean depending on whether parsing the datafile succeeded or not. - - @parsing_succeeded - end - def variation_id_exists?(experiment_id, variation_id) # Determines if a given experiment ID / variation ID pair exists in the datafile # diff --git a/spec/project_config_spec.rb b/spec/project_config_spec.rb index b74ba388..03aa894e 100644 --- a/spec/project_config_spec.rb +++ b/spec/project_config_spec.rb @@ -40,7 +40,6 @@ expect(project_config.groups).to eq(config_body['groups']) expect(project_config.project_id).to eq(config_body['projectId']) expect(project_config.revision).to eq(config_body['revision']) - expect(project_config.parsing_succeeded).to be(true) expected_attribute_key_map = { 'browser_type' => config_body['attributes'][0] @@ -659,16 +658,6 @@ end end - describe 'parsing_succeeded?' do - let(:config_body_v2) { OptimizelySpec::VALID_CONFIG_BODY } - let(:config_body_v2_JSON) { OptimizelySpec::VALID_CONFIG_BODY_JSON } - - it 'should be true for version 2' do - project_config_v2 = Optimizely::ProjectConfig.new(config_body_v2_JSON, logger, error_handler) - expect(project_config_v2.parsing_succeeded?).to be(true) - end - end - describe '@logger' do let(:spy_logger) { spy('logger') } let(:config) { Optimizely::ProjectConfig.new(config_body_JSON, spy_logger, error_handler) } diff --git a/spec/project_spec.rb b/spec/project_spec.rb index c44813a3..655bcbde 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -68,6 +68,16 @@ def handle_error(error) expect(instance_with_error_handler.error_handler.handle_error('test_message')). to eq('test_message') end + it 'should log an error when datafile is null' do + expect_any_instance_of(Optimizely::SimpleLogger).to receive(:log).once.with(Logger::ERROR, 'Provided datafile is in an invalid format.') + Optimizely::Project.new(nil) + end + + it 'should log an error when datafile is empty' do + expect_any_instance_of(Optimizely::SimpleLogger).to receive(:log).once.with(Logger::ERROR, 'Provided datafile is in an invalid format.') + Optimizely::Project.new('') + end + it 'should log an error when given a datafile that does not conform to the schema' do expect_any_instance_of(Optimizely::SimpleLogger).to receive(:log).once.with(Logger::ERROR, 'Provided datafile is in an invalid format.') Optimizely::Project.new('{"foo": "bar"}') @@ -109,11 +119,12 @@ class InvalidErrorHandler; end it 'should log an error when provided an invalid JSON datafile and skip_json_validation is true' do expect_any_instance_of(Optimizely::SimpleLogger).to receive(:log).once.with(Logger::ERROR, 'Provided datafile is in an invalid format.') - Optimizely::Project.new('{"foo": "bar"}', nil, nil, nil, true) + Optimizely::Project.new('{"version": "2", "foo": "bar"}', nil, nil, nil, true) end it 'should log an error when provided a datafile of unsupported version' do - expect_any_instance_of(Optimizely::SimpleLogger).to receive(:log).once.with(Logger::ERROR, 'Provided datafile is an unsupported version. Please use SDK version 1.1.2 or earlier for datafile version 1.') + config_body_invalid_json = JSON.parse(config_body_invalid_JSON) + expect_any_instance_of(Optimizely::SimpleLogger).to receive(:log).once.with(Logger::ERROR, "This version of the Ruby SDK does not support the given datafile version: #{config_body_invalid_json['version']}.") Optimizely::Project.new(config_body_invalid_JSON, nil, nil, nil, true) end diff --git a/spec/spec_params.rb b/spec/spec_params.rb index ebfb627b..caaf8aa6 100644 --- a/spec/spec_params.rb +++ b/spec/spec_params.rb @@ -657,6 +657,6 @@ module OptimizelySpec VALID_CONFIG_BODY_JSON = JSON.dump(VALID_CONFIG_BODY) INVALID_CONFIG_BODY = VALID_CONFIG_BODY.dup - INVALID_CONFIG_BODY['version'] = '1' + INVALID_CONFIG_BODY['version'] = '5' INVALID_CONFIG_BODY_JSON = JSON.dump(INVALID_CONFIG_BODY) end From fbf3910d77958c57a26c98cd25e26396ac3c7289 Mon Sep 17 00:00:00 2001 From: Rashid Siddique Date: Wed, 5 Sep 2018 18:21:33 +0500 Subject: [PATCH 2/2] addressing comments --- lib/optimizely.rb | 12 +++++------- spec/project_spec.rb | 10 ++++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/optimizely.rb b/lib/optimizely.rb index 6a362b15..c506ed9e 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -64,15 +64,13 @@ def initialize(datafile, event_dispatcher = nil, logger = nil, error_handler = n begin @config = ProjectConfig.new(datafile, @logger, @error_handler) - rescue InvalidDatafileVersionError => e + rescue StandardError => e @is_valid = false @logger = SimpleLogger.new - @logger.log(Logger::ERROR, e.message) - return - rescue - @is_valid = false - @logger = SimpleLogger.new - @logger.log(Logger::ERROR, InvalidInputError.new('datafile').message) + error_msg = e.class == InvalidDatafileVersionError ? e.message : InvalidInputError.new('datafile').message + error_to_handle = e.class == InvalidDatafileVersionError ? InvalidDatafileVersionError : InvalidInputError + @logger.log(Logger::ERROR, error_msg) + @error_handler.handle_error error_to_handle return end diff --git a/spec/project_spec.rb b/spec/project_spec.rb index 655bcbde..c80a39ea 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -110,10 +110,11 @@ class InvalidErrorHandler; end Optimizely::Project.new(config_body_JSON, nil, nil, nil, true) end - it 'should log an error when provided a datafile that is not JSON and skip_json_validation is true' do + it 'should log and raise an error when provided a datafile that is not JSON and skip_json_validation is true' do expect_any_instance_of(Optimizely::SimpleLogger).to receive(:log).once.with(Logger::ERROR, 'Provided datafile is in an invalid format.') + expect_any_instance_of(Optimizely::RaiseErrorHandler).to receive(:handle_error).once.with(Optimizely::InvalidInputError) - Optimizely::Project.new('this is not JSON', nil, nil, nil, true) + Optimizely::Project.new('this is not JSON', nil, nil, Optimizely::RaiseErrorHandler.new, true) end it 'should log an error when provided an invalid JSON datafile and skip_json_validation is true' do @@ -122,11 +123,12 @@ class InvalidErrorHandler; end Optimizely::Project.new('{"version": "2", "foo": "bar"}', nil, nil, nil, true) end - it 'should log an error when provided a datafile of unsupported version' do + it 'should log and raise an error when provided a datafile of unsupported version' do config_body_invalid_json = JSON.parse(config_body_invalid_JSON) expect_any_instance_of(Optimizely::SimpleLogger).to receive(:log).once.with(Logger::ERROR, "This version of the Ruby SDK does not support the given datafile version: #{config_body_invalid_json['version']}.") + expect_any_instance_of(Optimizely::RaiseErrorHandler).to receive(:handle_error).once.with(Optimizely::InvalidDatafileVersionError) - Optimizely::Project.new(config_body_invalid_JSON, nil, nil, nil, true) + Optimizely::Project.new(config_body_invalid_JSON, nil, nil, Optimizely::RaiseErrorHandler.new, true) end end