Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config param option validator #1437

Merged
merged 2 commits into from
Jan 30, 2017
Merged

Add config param option validator #1437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91b7665
add configuration parameter option validator and its tests
tagomoris Jan 27, 2017
19c60be
stop to use implicit default type :string (for process_name), and enu…
tagomoris Jan 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions lib/fluent/config/configure_proxy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)

Expand Down
24 changes: 14 additions & 10 deletions lib/fluent/system_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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|
Expand Down
181 changes: 180 additions & 1 deletion test/config/test_configure_proxy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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") }
Expand Down