Merge pull request #1119 from fluent/prevent-config-section-overwriting

Fix not to overwrite something unexpectedly

lib/fluent/configurable.rb

@@ -144,9 +144,10 @@ def config_set_desc(name, desc)
       end
 
       def config_section(name, **kwargs, &block)
+        section_already_exists = !!merged_configure_proxy.sections[name]
         configure_proxy(self.name).config_section(name, **kwargs, &block)
         variable_name = configure_proxy(self.name).sections[name].variable_name
-        unless self.respond_to?(variable_name)
+        if !section_already_exists && !self.respond_to?(variable_name)
           attr_accessor variable_name
         end
       end

lib/fluent/plugin_helper/formatter.rb

@@ -17,6 +17,7 @@
 require 'fluent/plugin'
 require 'fluent/plugin/formatter'
 require 'fluent/config/element'
+require 'fluent/configurable'
 
 module Fluent
   module PluginHelper
@@ -52,16 +53,19 @@ def formatter_create(usage: '', type: nil, conf: nil)
         formatter
       end
 
-      def self.included(mod)
-        mod.instance_eval do
-          # minimum section definition to instantiate formatter plugin instances
-          config_section :format, required: false, multi: true, param_name: :formatter_configs do
-            config_argument :usage, :string, default: ''
-            config_param    :@type, :string
-          end
+      module FormatterParams
+        include Fluent::Configurable
+        # minimum section definition to instantiate formatter plugin instances
+        config_section :format, required: false, multi: true, param_name: :formatter_configs do
+          config_argument :usage, :string, default: ''
+          config_param    :@type, :string
         end
       end
 
+      def self.included(mod)
+        mod.include FormatterParams
+      end
+
       attr_reader :_formatters # for tests
 
       def initialize

lib/fluent/plugin_helper/inject.rb

@@ -16,6 +16,7 @@
 
 require 'fluent/event'
 require 'time'
+require 'fluent/configurable'
 
 module Fluent
   module PluginHelper
@@ -58,22 +59,26 @@ def inject_values_to_event_stream(tag, es)
         new_es
       end
 
-      def self.included(mod)
-        mod.instance_eval do
-          config_section :inject, required: false, multi: false, param_name: :inject_config do
-            config_param :hostname_key, :string, default: nil
-            config_param :hostname, :string, default: nil
-            config_param :tag_key, :string, default: nil
-            config_param :time_key, :string, default: nil
-            config_param :time_type, :enum, list: [:float, :unixtime, :string], default: :float
-            config_param :time_format, :string, default: nil
-            config_param :timezone, :string, default: "#{Time.now.strftime('%z')}" # localtime
-          end
+      module InjectParams
+        include Fluent::Configurable
+        config_section :inject, required: false, multi: false, param_name: :inject_config do
+          config_param :hostname_key, :string, default: nil
+          config_param :hostname, :string, default: nil
+          config_param :tag_key, :string, default: nil
+          config_param :time_key, :string, default: nil
+          config_param :time_type, :enum, list: [:float, :unixtime, :string], default: :float
+          config_param :time_format, :string, default: nil
+          config_param :timezone, :string, default: "#{Time.now.strftime('%z')}" # localtime
         end
       end
 
+      def self.included(mod)
+        mod.include InjectParams
+      end
+
       def initialize
         super
+        @_inject_enabled = false
         @_inject_hostname_key = nil
         @_inject_hostname = nil
         @_inject_tag_key = nil

lib/fluent/plugin_helper/parser.rb

@@ -17,6 +17,7 @@
 require 'fluent/plugin'
 require 'fluent/plugin/parser'
 require 'fluent/config/element'
+require 'fluent/configurable'
 
 module Fluent
   module PluginHelper
@@ -52,16 +53,19 @@ def parser_create(usage: '', type: nil, conf: nil)
         parser
       end
 
-      def self.included(mod)
-        mod.instance_eval do
-          # minimum section definition to instantiate parser plugin instances
-          config_section :parse, required: false, multi: true, param_name: :parser_configs do
-            config_argument :usage, :string, default: ''
-            config_param    :@type, :string
-          end
+      module ParserParams
+        include Fluent::Configurable
+        # minimum section definition to instantiate parser plugin instances
+        config_section :parse, required: false, multi: true, param_name: :parser_configs do
+          config_argument :usage, :string, default: ''
+          config_param    :@type, :string
         end
       end
 
+      def self.included(mod)
+        mod.include ParserParams
+      end
+
       attr_reader :_parsers # for tests
 
       def initialize

lib/fluent/plugin_helper/storage.rb

@@ -20,6 +20,7 @@
 require 'fluent/plugin/storage'
 require 'fluent/plugin_helper/timer'
 require 'fluent/config/element'
+require 'fluent/configurable'
 
 module Fluent
   module PluginHelper
@@ -63,16 +64,19 @@ def storage_create(usage: '', type: nil, conf: nil)
         s.storage
       end
 
-      def self.included(mod)
-        mod.instance_eval do
-          # minimum section definition to instantiate storage plugin instances
-          config_section :storage, required: false, multi: true, param_name: :storage_configs do
-            config_argument :usage, :string, default: ''
-            config_param    :@type, :string, default: Fluent::Plugin::Storage::DEFAULT_TYPE
-          end
+      module StorageParams
+        include Fluent::Configurable
+        # minimum section definition to instantiate storage plugin instances
+        config_section :storage, required: false, multi: true, param_name: :storage_configs do
+          config_argument :usage, :string, default: ''
+          config_param    :@type, :string, default: Fluent::Plugin::Storage::DEFAULT_TYPE
         end
       end
 
+      def self.included(mod)
+        mod.include StorageParams
+      end
+
       attr_reader :_storages # for tests
 
       def initialize

test/plugin_helper/test_formatter.rb

@@ -19,6 +19,13 @@ class Dummy < Fluent::Plugin::TestBase
     helpers :formatter
   end
 
+  class Dummy2 < Fluent::Plugin::TestBase
+    helpers :formatter
+    config_section :format do
+      config_set_default :@type, 'example2'
+    end
+  end
+
   setup do
     @d = nil
   end
@@ -37,6 +44,19 @@ class Dummy < Fluent::Plugin::TestBase
     assert_equal 0, d._formatters.size
   end
 
+  test 'can override default configuration parameters, but not overwrite whole definition' do
+    d = Dummy.new
+    assert_equal [], d.formatter_configs
+
+    d = Dummy2.new
+    d.configure(config_element('ROOT', '', {}, [config_element('format', '', {}, [])]))
+    assert_raise NoMethodError do
+      d.format
+    end
+    assert_equal 1, d.formatter_configs.size
+    assert_equal 'example2', d.formatter_configs.first[:@type]
+  end
+
   test 'can be configured without format sections' do
     d = Dummy.new
     assert_nothing_raised do

test/plugin_helper/test_inject.rb

@@ -8,6 +8,13 @@ class Dummy < Fluent::Plugin::TestBase
     helpers :inject
   end
 
+  class Dummy2 < Fluent::Plugin::TestBase
+    helpers :inject
+    config_section :inject do
+      config_set_default :hostname_key, 'host'
+    end
+  end
+
   def config_inject_section(hash = {})
     config_element('ROOT', '', {}, [config_element('inject', '', hash)])
   end
@@ -26,6 +33,17 @@ def config_inject_section(hash = {})
     end
   end
 
+  test 'can override default parameters, but not overwrite whole definition' do
+    d = Dummy.new
+    d.configure(config_element())
+    assert_nil d.inject_config
+
+    d = Dummy2.new
+    d.configure(config_element('ROOT', '', {}, [config_element('inject')]))
+    assert d.inject_config
+    assert_equal 'host', d.inject_config.hostname_key
+  end
+
   test 'do nothing in default' do
     @d.configure(config_inject_section())
     @d.start

test/plugin_helper/test_parser.rb

@@ -30,6 +30,13 @@ class Dummy < Fluent::Plugin::TestBase
     helpers :parser
   end
 
+  class Dummy2 < Fluent::Plugin::TestBase
+    helpers :parser
+    config_section :parse do
+      config_set_default :@type, 'example2'
+    end
+  end
+
   setup do
     @d = nil
   end
@@ -48,6 +55,19 @@ class Dummy < Fluent::Plugin::TestBase
     assert_equal 0, d._parsers.size
   end
 
+  test 'can override default configuration parameters, but not overwrite whole definition' do
+    d = Dummy.new
+    assert_equal [], d.parser_configs
+
+    d = Dummy2.new
+    d.configure(config_element('ROOT', '', {}, [config_element('parse', '', {}, [])]))
+    assert_raise NoMethodError do
+      d.parse
+    end
+    assert_equal 1, d.parser_configs.size
+    assert_equal 'example2', d.parser_configs.first[:@type]
+  end
+
   test 'can be configured without parse sections' do
     d = Dummy.new
     assert_nothing_raised do

test/plugin_helper/test_storage.rb

@@ -73,6 +73,14 @@ class Dummy < Fluent::Plugin::TestBase
     helpers :storage
   end
 
+  class Dummy2 < Fluent::Plugin::TestBase
+    helpers :storage
+    config_section :storage do
+      config_set_default :@type, 'ex2'
+      config_set_default :dummy_path, '/tmp/yay'
+    end
+  end
+
   setup do
     @d = nil
   end
@@ -91,6 +99,21 @@ class Dummy < Fluent::Plugin::TestBase
     assert_equal 0, d._storages.size
   end
 
+  test 'can override default configuration parameters, but not overwrite whole definition' do
+    d = Dummy.new
+    d.configure(config_element())
+    assert_equal [], d.storage_configs
+
+    d = Dummy2.new
+    d.configure(config_element('ROOT', '', {}, [config_element('storage', '', {}, [])]))
+    assert_raise NoMethodError do
+      d.storage
+    end
+    assert_equal 1, d.storage_configs.size
+    assert_equal 'ex2', d.storage_configs.first[:@type]
+    assert_equal '/tmp/yay', d.storage_configs.first.dummy_path
+  end
+
   test 'can be configured without storage sections' do
     d = Dummy.new
     assert_nothing_raised do