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

Use compat instead of strict to follow json/yajl way. fix #1230 #1239

Merged
merged 2 commits into from Sep 26, 2016

Conversation

repeatedly
Copy link
Member

No description provided.

@repeatedly
Copy link
Member Author

I can write messy test like below but it seems not unittest...

diff --git a/test/plugin/test_formatter_json.rb b/test/plugin/test_formatter_json.rb
index 5f80dd1..f073ea2 100644
--- a/test/plugin/test_formatter_json.rb
+++ b/test/plugin/test_formatter_json.rb
@@ -17,7 +17,7 @@ class JsonFormatterTest < ::Test::Unit::TestCase
   end

   def record
-    {'message' => 'awesome'}
+    {'message' => 'awesome', :message2 => :awesome2}
   end

   data('oj' => 'oj', 'yajl' => 'yajl')
@@ -27,4 +27,14 @@ class JsonFormatterTest < ::Test::Unit::TestCase

     assert_equal("#{Yajl.dump(record)}\n", formatted)
   end
+
+  def test_format_with_symbol_value
+    d = create_driver('json_parser' => 'oj')
+    require 'fluent/plugin/parser_json'
+    parser = Fluent::Plugin::JSONParser.new
+    parser.configure(config_element())
+    formatted = d.instance.format(tag, @time, record)
+
+    assert_equal("#{Yajl.dump(record)}\n", formatted)
+  end
 end

@@ -38,7 +38,7 @@ def configure(conf)
begin
raise LoadError unless @json_parser == 'oj'
require 'oj'
Oj.default_options = {bigdecimal_load: :float, mode: :strict}
Oj.default_options = {bigdecimal_load: :float, mode: :compat}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default options are different from one in formatter_json.
You must make a common Oj default options as a constant, and share it in everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You must make a common Oj default options as a constant,

This is the trick but it seems acceptable.

@tagomoris
Copy link
Member

LGTM.

@tagomoris tagomoris added the bug label Sep 26, 2016
@repeatedly repeatedly merged commit 479257d into master Sep 26, 2016
@repeatedly repeatedly deleted the fix-oj-mode branch November 2, 2016 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants