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.

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

Fix boolean parameters without value with following comment #1883

Merged

Conversation

tagomoris
Copy link
Member

As reported at tagomoris/fluent-plugin-route#10, config_param :name, :bool returns falsely value (nil) when it's configured without explicit value with following comment.

# parameter definition
config_param :b1, :bool, default: false
config_param :b2, :bool, default: false

# configuration
# b1 will be true
b1
# b2 will be nil
b2 # comment here

This bug cannot be reproducible via test cases of v1 parser, but I succeeded to reproduce it using plugin code and test driver. This bug was reported to be reproducible on actual Fluentd processes.

@tagomoris
Copy link
Member Author

@repeatedly This looks a bug related with literal parser, v1 parser and value parser, and takes a time to fix it. Could you take ownership on this?

@repeatedly
Copy link
Member

The problem is this line:

spacing_without_comment

Maybe, this is for this test case:
assert_text_parsed_as(e('ROOT', '', {"k1" => "#not_comment"}), " k1 #not_comment")

I think this parsed result should be {"k1" => ""} because we can use '# foo' like syntax for # started string. But changing this behaviour may break existing environment. So we can't use this fix.
For keeping compatibility, changing bool handling is better.

diff --git a/lib/fluent/config/types.rb b/lib/fluent/config/types.rb
index f41d451..beccc18 100644
--- a/lib/fluent/config/types.rb
+++ b/lib/fluent/config/types.rb
@@ -60,7 +60,11 @@ module Fluent
       when ''
         true
       else
-        nil
+        if str.start_with?('#')
+          true
+        else
+          nil
+        end
       end
     end

@repeatedly
Copy link
Member

Hmm... return nil seems error prone. Should it raise an exception for unexpected value?

@repeatedly repeatedly changed the title [WIP] boolean parameters without value with following comment Fix boolean parameters without value with following comment Mar 12, 2018
@repeatedly repeatedly force-pushed the boolean-params-without-value-with-following-comment branch from a23d826 to e84208f Compare March 12, 2018 14:44
@repeatedly
Copy link
Member

@tagomoris How about this patch?

@repeatedly repeatedly merged commit a12d368 into master Mar 14, 2018
@ganmacs ganmacs deleted the boolean-params-without-value-with-following-comment branch July 11, 2019 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants