Fix bug to overwrite all subsections definition by subclass

* overwriting config_section options like required/multi/alias breaks behavior of `super`
  * so subclass must not overwrite these options
* subsection definitions of subsections (Hash) is simply merged
  * it is bug, because same section definition like <a> is overwritten by subclass
  * under this bug, subclass MUST repeat superclass's definition, and add original definition
  * but this is impossible in the real world
  * subsection definitions should be merged correctly

Author: tagomoris

lib/fluent/config/configure_proxy.rb

@@ -60,16 +60,29 @@ def multi?
 
       def merge(other) # self is base class, other is subclass
         options = {
-          param_name: other.param_name,
-          required: (other.required.nil? ? self.required : other.required),
-          multi: (other.multi.nil? ? self.multi : other.multi)
+          param_name: other.param_name, # param_name is used not to ovewrite plugin's instance varible, so this should be able to be overwritten
+          required: (@required.nil? ? other.required : self.required), # subclass cannot overwrite base class's definition
+          multi: (@multi.nil? ? other.multi : self.multi),
+          alias: (@alias.nil? ? other.alias : self.alias),
         }
         merged = self.class.new(other.name, options)
 
         merged.argument = other.argument || self.argument
-        merged.params = self.params.merge(other.params)
+        merged.params = other.params.merge(self.params)
         merged.defaults = self.defaults.merge(other.defaults)
-        merged.sections = self.sections.merge(other.sections)
+        merged.sections = {}
+        (self.sections.keys + other.sections.keys).uniq.each do |section_key|
+          self_section = self.sections[section_key]
+          other_section = other.sections[section_key]
+          merged_section = if self_section && other_section
+                             self_section.merge(other_section)
+                           elsif self_section || other_section
+                             self_section || other_section
+                           else
+                             raise "BUG: both of self and other section are nil"
+                           end
+          merged.sections[section_key] = merged_section
+        end
 
         merged
       end
@@ -148,11 +161,16 @@ def config_section(name, *args, &block)
           raise ArgumentError, "#{self.name}: unknown config_section arguments: #{args.inspect}"
         end
 
-        sub_proxy = ConfigureProxy.new(name, *args)
-        sub_proxy.instance_exec(&block)
+        # if @sections[name]
+        #   @sections[name].instance_exec(&block)
+        # else
+          sub_proxy = ConfigureProxy.new(name, *args)
+          sub_proxy.instance_exec(&block)
+
+          @sections[name] = sub_proxy
+        # end
 
         @params.delete(name)
-        @sections[name] = sub_proxy
 
         name
       end

test/config/test_configurable.rb

@@ -108,6 +108,12 @@ def get_all
       [@name, @detail]
     end
   end
+
+  class Example2 < Example1
+    config_section :detail, required: false, multi: false, alias: "information2" do
+      config_param :phone_no, :string
+    end
+  end
 end
 
 module Fluent::Config
@@ -493,6 +499,59 @@ def e(name, arg = '', attrs = {}, elements = [])
           assert_false(ex5.boolvalue)
         end
       end
+
+      sub_test_case '.config_section' do
+        def e(name, arg = '', attrs = {}, elements = [])
+          attrs_str_keys = {}
+          attrs.each{|key, value| attrs_str_keys[key.to_s] = value }
+          Fluent::Config::Element.new(name, arg, attrs_str_keys, elements)
+        end
+
+        test 'adds only config_param definitions into configuration without overwriting existing configuration elements' do
+          # conf0 = e('ROOT', '', {}, [])
+
+          conf1 = e('ROOT', '', {
+              'name' => 'tagomoris',
+              'bool' => true,
+            },
+          )
+          # <detail> section is required by Example1 config_section spec
+          assert_raise(Fluent::ConfigError.new("'<detail>' sections are required")) { ConfigurableSpec::Example1.new.configure(conf1) }
+          assert_raise(Fluent::ConfigError.new("'<detail>' sections are required")) { ConfigurableSpec::Example2.new.configure(conf1) }
+
+          conf2 = e('ROOT', '', {
+              'name' => 'tagomoris',
+              'bool' => true,
+            },
+            [e('detail', '', { 'phone_no' => "+81-00-0000-0000" }, [])],
+          )
+          # <detail> address </detail> is required by Example1 config_param spec
+          assert_raise(Fluent::ConfigError.new("'address' parameter is required, in section detail")) { ConfigurableSpec::Example2.new.configure(conf2) }
+
+          conf3 = e('ROOT', '', {
+              'name' => 'tagomoris',
+              'bool' => true,
+            },
+            [e('detail', '', { 'address' => "Chiyoda Tokyo Japan" }, [])],
+          )
+          # <detail> phone_no </detail is required by Example2 config_param spec
+          assert_nothing_raised { ConfigurableSpec::Example1.new.configure(conf3) }
+          assert_raise(Fluent::ConfigError.new("'phone_no' parameter is required, in section detail")) { ConfigurableSpec::Example2.new.configure(conf3) }
+
+          conf4 = e('ROOT', '', {
+              'name' => 'tagomoris',
+              'bool' => true,
+            },
+            [e('detail', '', { 'address' => "Chiyoda Tokyo Japan", 'phone_no' => '+81-00-0000-0000' }, [])],
+          )
+          assert_nothing_raised { ConfigurableSpec::Example1.new.configure(conf4) } # phone_no is not used
+          assert_nothing_raised { ConfigurableSpec::Example2.new.configure(conf4) }
+
+          ex2 = ConfigurableSpec::Example2.new.configure(conf4)
+          assert_equal "Chiyoda Tokyo Japan", ex2.detail.address
+          assert_equal "+81-00-0000-0000", ex2.detail.phone_no
+        end
+      end
     end
 
     sub_test_case 'class defined with config_param/config_section having :alias' do