From 91b76651e468e7c8c559274fa095dc913b33bf21 Mon Sep 17 00:00:00 2001 From: TAGOMORI Satoshi Date: Fri, 27 Jan 2017 14:21:13 +0900 Subject: [PATCH 1/2] add configuration parameter option validator and its tests --- lib/fluent/config/configure_proxy.rb | 35 +++++- test/config/test_configure_proxy.rb | 181 ++++++++++++++++++++++++++- 2 files changed, 209 insertions(+), 7 deletions(-) diff --git a/lib/fluent/config/configure_proxy.rb b/lib/fluent/config/configure_proxy.rb index c77c42a062..2522d88c15 100644 --- a/lib/fluent/config/configure_proxy.rb +++ b/lib/fluent/config/configure_proxy.rb @@ -226,19 +226,41 @@ def option_value_type!(name, opts, key, klass=nil, type: nil) end end + def config_parameter_option_validate!(name, type, **kwargs, &block) + if type.nil? && !block + type = :string + end + kwargs.each_key do |key| + case key + when :default, :alias, :secret, :skip_accessor, :deprecated, :obsoleted, :desc + # valid for all types + when :list + raise ArgumentError, ":list is valid only for :enum type, but #{type}: #{name}" if type != :enum + when :value_type + raise ArgumentError, ":value_type is valid only for :hash and :array, but #{type}: #{name}" if type != :hash && type != :array + when :symbolize_keys + raise ArgumentError, ":symbolize_keys is valid only for :hash, but #{type}: #{name}" if type != :hash + else + raise ArgumentError, "unknown option '#{key}' for configuration parameter: #{name}" + end + end + end + def parameter_configuration(name, type = nil, **kwargs, &block) - name = name.to_sym + config_parameter_option_validate!(name, type, **kwargs, &block) - opts = {} - opts[:type] = type - opts.merge!(kwargs) + name = name.to_sym if block && type raise ArgumentError, "#{name}: both of block and type cannot be specified" + elsif !block && !type + type = :string end + opts = {} + opts[:type] = type + opts.merge!(kwargs) begin - type = :string if type.nil? block ||= @type_lookup.call(type) rescue ConfigError # override error message @@ -252,10 +274,11 @@ def parameter_configuration(name, type = nil, **kwargs, &block) option_value_type!(name, opts, :deprecated, String) option_value_type!(name, opts, :obsoleted, String) if type == :enum - if !opts.has_key?(:list) || !opts[:list].all?{|v| v.is_a?(Symbol) } + if !opts.has_key?(:list) || !opts[:list].is_a?(Array) || opts[:list].empty? || !opts[:list].all?{|v| v.is_a?(Symbol) } raise ArgumentError, "#{name}: enum parameter requires :list of Symbols" end end + option_value_type!(name, opts, :symbolize_keys, type: :boolean) option_value_type!(name, opts, :value_type, Symbol) # hash, array option_value_type!(name, opts, :skip_accessor, type: :boolean) diff --git a/test/config/test_configure_proxy.rb b/test/config/test_configure_proxy.rb index a5f094a09e..24e865164c 100644 --- a/test/config/test_configure_proxy.rb +++ b/test/config/test_configure_proxy.rb @@ -45,7 +45,7 @@ class TestConfigureProxy < ::Test::Unit::TestCase assert_false(proxy.multi?) end test 'raise error if both of init and required are true' do - assert_raise "init and required are exclusive" do + assert_raise RuntimeError.new("init and required are exclusive") do Fluent::Config::ConfigureProxy.new(:section, init: true, required: true, type_lookup: @type_lookup) end end @@ -164,6 +164,185 @@ class TestConfigureProxy < ::Test::Unit::TestCase @proxy = Fluent::Config::ConfigureProxy.new(:section, type_lookup: @type_lookup) end + test 'handles configuration parameters without type as string' do + @proxy.config_argument(:label) + @proxy.config_param(:name) + assert_equal :label, @proxy.argument[0] + assert_equal :string, @proxy.argument[2][:type] + assert_equal :string, @proxy.params[:name][1][:type] + end + + data( + default: [:default, nil], + alias: [:alias, :alias_name_in_config], + secret: [:secret, true], + skip_accessor: [:skip_accessor, true], + deprecated: [:deprecated, 'it is deprecated'], + obsoleted: [:obsoleted, 'it is obsoleted'], + desc: [:desc, "description"], + ) + test 'always allow options for all types' do |(option, value)| + opt = {option => value} + assert_nothing_raised{ @proxy.config_argument(:param0, **opt) } + assert_nothing_raised{ @proxy.config_param(:p1, :string, **opt) } + assert_nothing_raised{ @proxy.config_param(:p2, :enum, list: [:a, :b, :c], **opt) } + assert_nothing_raised{ @proxy.config_param(:p3, :integer, **opt) } + assert_nothing_raised{ @proxy.config_param(:p4, :float, **opt) } + assert_nothing_raised{ @proxy.config_param(:p5, :size, **opt) } + assert_nothing_raised{ @proxy.config_param(:p6, :bool, **opt) } + assert_nothing_raised{ @proxy.config_param(:p7, :time, **opt) } + assert_nothing_raised{ @proxy.config_param(:p8, :hash, **opt) } + assert_nothing_raised{ @proxy.config_param(:p9, :array, **opt) } + end + + data(string: :string, integer: :integer, float: :float, size: :size, bool: :bool, time: :time, hash: :hash, array: :array) + test 'deny list for non-enum types' do |type| + assert_raise ArgumentError.new(":list is valid only for :enum type, but #{type}: arg") do + @proxy.config_argument(:arg, type, list: [:a, :b]) + end + assert_raise ArgumentError.new(":list is valid only for :enum type, but #{type}: p1") do + @proxy.config_param(:p1, type, list: [:a, :b]) + end + end + + data(string: :string, integer: :integer, float: :float, size: :size, bool: :bool, time: :time) + test 'deny value_type for non-hash/array types' do |type| + assert_raise ArgumentError.new(":value_type is valid only for :hash and :array, but #{type}: arg") do + @proxy.config_argument(:arg, type, value_type: :string) + end + assert_raise ArgumentError.new(":value_type is valid only for :hash and :array, but #{type}: p1") do + @proxy.config_param(:p1, type, value_type: :integer) + end + end + + data(string: :string, integer: :integer, float: :float, size: :size, bool: :bool, time: :time, array: :array) + test 'deny symbolize_keys for non-hash types' do |type| + assert_raise ArgumentError.new(":symbolize_keys is valid only for :hash, but #{type}: arg") do + @proxy.config_argument(:arg, type, symbolize_keys: true) + end + assert_raise ArgumentError.new(":symbolize_keys is valid only for :hash, but #{type}: p1") do + @proxy.config_param(:p1, type, symbolize_keys: true) + end + end + + data(string: :string, integer: :integer, float: :float, size: :size, bool: :bool, time: :time, hash: :hash, array: :array) + test 'deny unknown options' do |type| + assert_raise ArgumentError.new("unknown option 'required' for configuration parameter: arg") do + @proxy.config_argument(:arg, type, required: true) + end + assert_raise ArgumentError.new("unknown option 'param_name' for configuration parameter: p1") do + @proxy.config_argument(:p1, type, param_name: :yay) + end + end + + test 'desc gets string' do + assert_nothing_raised do + @proxy.config_param(:name, :string, desc: "it is description") + end + assert_raise ArgumentError.new("name1: desc must be a String, but Symbol") do + @proxy.config_param(:name1, :string, desc: :yaaaaaaaay) + end + end + + test 'alias gets symbol' do + assert_nothing_raised do + @proxy.config_param(:name, :string, alias: :label) + end + assert_raise ArgumentError.new("name1: alias must be a Symbol, but String") do + @proxy.config_param(:name1, :string, alias: 'label1') + end + end + + test 'secret gets true/false' do + assert_nothing_raised do + @proxy.config_param(:name1, :string, secret: false) + end + assert_nothing_raised do + @proxy.config_param(:name2, :string, secret: true) + end + assert_raise ArgumentError.new("name3: secret must be true or false, but String") do + @proxy.config_param(:name3, :string, secret: 'yes') + end + assert_raise ArgumentError.new("name4: secret must be true or false, but NilClass") do + @proxy.config_param(:name4, :string, secret: nil) + end + end + + test 'symbolize_keys gets true/false' do + assert_nothing_raised do + @proxy.config_param(:data1, :hash, symbolize_keys: false) + end + assert_nothing_raised do + @proxy.config_param(:data2, :hash, symbolize_keys: true) + end + assert_raise ArgumentError.new("data3: symbolize_keys must be true or false, but NilClass") do + @proxy.config_param(:data3, :hash, symbolize_keys: nil) + end + end + + test 'value_type gets symbol' do + assert_nothing_raised do + @proxy.config_param(:data1, :array, value_type: :integer) + end + assert_raise ArgumentError.new("data2: value_type must be a Symbol, but Class") do + @proxy.config_param(:data2, :array, value_type: Integer) + end + end + + test 'list gets an array of symbols' do + assert_nothing_raised do + @proxy.config_param(:proto1, :enum, list: [:a, :b]) + end + assert_raise ArgumentError.new("proto2: enum parameter requires :list of Symbols") do + @proxy.config_param(:proto2, :enum, list: nil) + end + assert_raise ArgumentError.new("proto3: enum parameter requires :list of Symbols") do + @proxy.config_param(:proto3, :enum, list: ['a', 'b']) + end + assert_raise ArgumentError.new("proto4: enum parameter requires :list of Symbols") do + @proxy.config_param(:proto4, :enum, list: []) + end + end + + test 'deprecated gets string' do + assert_nothing_raised do + @proxy.config_param(:name1, :string, deprecated: "use name2 instead") + end + assert_raise ArgumentError.new("name2: deprecated must be a String, but TrueClass") do + @proxy.config_param(:name2, :string, deprecated: true) + end + end + + test 'obsoleted gets string' do + assert_nothing_raised do + @proxy.config_param(:name1, :string, obsoleted: "use name2 instead") + end + assert_raise ArgumentError.new("name2: obsoleted must be a String, but TrueClass") do + @proxy.config_param(:name2, :string, obsoleted: true) + end + end + + test 'skip_accessor gets true/false' do + assert_nothing_raised do + @proxy.config_param(:format1, :string, skip_accessor: false) + end + assert_nothing_raised do + @proxy.config_param(:format2, :string, skip_accessor: true) + end + assert_raise ArgumentError.new("format2: skip_accessor must be true or false, but String") do + @proxy.config_param(:format2, :string, skip_accessor: 'yes') + end + end + + test 'list is required for :enum' do + assert_nothing_raised do + @proxy.config_param(:proto1, :enum, list: [:a, :b]) + end + assert_raise ArgumentError.new("proto1: enum parameter requires :list of Symbols") do + @proxy.config_param(:proto1, :enum, default: :a) + end + end + test 'does not permit config_set_default for param w/ :default option' do @proxy.config_param(:name, :string, default: "name1") assert_raise(ArgumentError) { @proxy.config_set_default(:name, "name2") } From 19c60be2e3a0d1e669ec4eb435c94196050556f1 Mon Sep 17 00:00:00 2001 From: TAGOMORI Satoshi Date: Fri, 27 Jan 2017 14:21:22 +0900 Subject: [PATCH 2/2] stop to use implicit default type :string (for process_name), and enum to show valid log levels --- lib/fluent/system_config.rb | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/fluent/system_config.rb b/lib/fluent/system_config.rb index 6761843122..2b82835f94 100644 --- a/lib/fluent/system_config.rb +++ b/lib/fluent/system_config.rb @@ -29,19 +29,17 @@ class SystemConfig :file_permission, :dir_permission, ] - config_param :workers, :integer, default: 1 - config_param :root_dir, :string, default: nil - config_param :log_level, default: nil do |level| - Log.str_to_level(level) - end + config_param :workers, :integer, default: 1 + config_param :root_dir, :string, default: nil + config_param :log_level, :enum, list: [:trace, :debug, :info, :warn, :error, :fatal], default: nil config_param :suppress_repeated_stacktrace, :bool, default: nil - config_param :emit_error_log_interval, :time, default: nil + config_param :emit_error_log_interval, :time, default: nil config_param :suppress_config_dump, :bool, default: nil - config_param :log_event_verbose, :bool, default: nil - config_param :without_source, :bool, default: nil - config_param :rpc_endpoint, :string, default: nil + config_param :log_event_verbose, :bool, default: nil + config_param :without_source, :bool, default: nil + config_param :rpc_endpoint, :string, default: nil config_param :enable_get_dump, :bool, default: nil - config_param :process_name, default: nil + config_param :process_name, :string, default: nil config_param :file_permission, default: nil do |v| v.to_i(8) end @@ -77,6 +75,12 @@ def initialize(conf=nil) configure(conf) end + def configure(conf) + super + + @log_level = Log.str_to_level(@log_level.to_s) if @log_level + end + def dup s = SystemConfig.new SYSTEM_CONFIG_PARAMETERS.each do |param|