Make sure parser returns hash #4474

daipom · 2024-04-26T10:14:14Z

Which issue(s) this PR fixes:

Partially Fixes JSON parser plugin can sometimes output records as strings rather than Hash #4100
Remake Make sure JSON parser returns Hash #4106
- Add fixes for parser_msgpack and some refactoring

What this PR does / why we need it:

Make sure parser_json and parser_msgpack return Hash.
Make parser_json and parser_msgpack accept only Hash or Array of Hash

It is wrong for Parser to return a record that is not Hash.
Subsequent processing may result in errors.

parser_json and parser_msgpack could return non-Hash objects, so they have been fixed in this PR.
In addition, in_http was designed based on this wrong behavior, so it has been fixed as well in this PR.

However, we have a remaining problem with filter_parser.
filter_parser could return Array based on this wrong behavior.
This PR disables it, so it can not return multiple parsed results anymore.
Even though it was wrong to begin with, it's possible that this change in specifications would be unacceptable.
We need to consider this change carefully.
I explain some examples in detail below.

Docs Changes:
TODO

Release Note:

Make sure parser_json and parser_msgpack return Hash.
Make parser_json and parser_msgpack accept only Hash or Array of Hash

Example: parser_json

Case: No subsequent processing (without `convert_values`)

Before: Array => Single record
After: Array => Multiple records

Config

<source>
  @type tcp
  tag test.tcp
  <parse>
    @type json
  </parse>
</source>

<match test.**>
  @type stdout
</match>

Operation

$ netcat 0.0.0.0 5170
[{"k":"v"}, {"k2":"v2"}]

Result BEFORE this fix

2023-03-21 23:30:30.623063804 +0900 test.tcp: [{"k":"v"},{"k2":"v2"}]

Result AFTER this fix

2023-03-21 23:29:20.147728222 +0900 test.tcp: {"k":"v"}
2023-03-21 23:29:20.147754433 +0900 test.tcp: {"k2":"v2"}

Case: No subsequent processing (with `convert_values`)

Before: Array => NoMethodError
After: Array => Multiple records

Config

<source>
  @type tcp
  tag test.tcp
  <parse>
    @type json
    null_empty_string
  </parse>
</source>

<match test.**>
  @type stdout
</match>

Operation

$ netcat 0.0.0.0 5170
[{"k":"v"}, {"k2":"v2"}]

Result BEFORE this fix

2024-04-26 18:48:43 +0900 [error]: #0 unexpected error on reading data host="127.0.0.1" port=33268 error_class=NoMethodError error="undefined method `each_key' for [{\"k\"=>\"v\"}, {\"k2\"=>\"v2\"}]:Array"

Result AFTER this fix

2024-04-26 18:51:02.763961229 +0900 test.tcp: {"k":"v"}
2024-04-26 18:51:02.763971914 +0900 test.tcp: {"k2":"v2"}

Case: With subsequent filter processing

Before: Array => NoMethodError
After: Array => Multiple records

Config

<source>
  @type tcp
  tag test.tcp
  <parse>
    @type json
  </parse>
</source>

<filter test.**>
  @type record_transformer
  enable_ruby true
  <record>
    class ${record.class}
  </record>
</filter>

<match test.**>
  @type stdout
</match>

Operation

$ netcat 0.0.0.0 5170
[{"k":"v"}, {"k2":"v2"}]

Result BEFORE this fix

2023-03-21 23:30:51 +0900 [warn]: #0 dump an error event: error_class=NoMethodError error="undefined method `merge!' for [{\"k\"=>\"v\"}, {\"k2\"=>\"v2\"}]:Array" location="/home/daipom/work/fluentd/fluentd/lib/fluent/plugin/filter_record_transformer.rb:135:in `reform'" tag="test.tcp" time=2023-03-21 23:30:51.040692881 +0900 record=[{"k"=>"v"}, {"k2"=>"v2"}]

Result AFTER this fix

2023-03-21 23:29:55.783183244 +0900 test.tcp: {"k":"v","class":"Hash"}
2023-03-21 23:29:55.783215076 +0900 test.tcp: {"k2":"v2","class":"Hash"}

Example: filter_parser

Case: No subsequent processing (without any option)

Before: Array => Single record
After: Array => Imcomleted single record that has only the first value !!Remaining Problem!!

Config

<source>
  @type sample
  tag test.array
  sample {"message": "[{\"k\":\"v\"}, {\"k2\":\"v2\"}]"}
</source>

<filter test.**>
  @type parser
  key_name message
  <parse>
    @type json
  </parse>
</filter>

<match test.**>
  @type stdout
</match>

Result BEFORE this fix

2024-04-26 18:53:30.029602646 +0900 test.array: [{"k":"v"},{"k2":"v2"}]

Result AFTER this fix

2024-04-26 18:55:28.010836436 +0900 test.array: {"k":"v"}

Case: No subsequent processing (with some options such as `reserve_data`)

Before: Array => ParserError and return the original record
- parse failed no implicit conversion of Array into Hash
After: Array => Imcomleted record that has only the first value !!Remaining Problem!!

Config

<source>
  @type sample
  tag test.array
  sample {"message": "[{\"k\":\"v\"}, {\"k2\":\"v2\"}]"}
</source>

<filter test.**>
  @type parser
  key_name message
  reserve_data
  <parse>
    @type json
  </parse>
</filter>

<match test.**>
  @type stdout
</match>

Result BEFORE this fix

2024-04-26 18:58:25 +0900 [warn]: #0 dump an error event: error_class=Fluent::Plugin::Parser::ParserError error="parse failed no implicit conversion of Array into Hash" location="/home/daipom/work/fluentd/fluentd/lib/fluent/plugin/filter_parser.rb:110:in `rescue in filter_with_time'" tag="test.array" time=2024-04-26 18:58:25.027466823 +0900 record={"message"=>"[{\"k\":\"v\"}, {\"k2\":\"v2\"}]"}
2024-04-26 18:58:25.027466823 +0900 test.array: {"message":"[{\"k\":\"v\"}, {\"k2\":\"v2\"}]"}

Result AFTER this fix

2024-04-26 18:58:08.091274876 +0900 test.array: {"message":"[{\"k\":\"v\"}, {\"k2\":\"v2\"}]","k":"v"}

Case: With subsequent filter processing

Before: Array => dump an error event: error_class=NoMethodError
After: Array => Imcomleted record that has only the first value !!Remaining Problem!!

Config

<source>
  @type sample
  tag test.array
  sample {"message": "[{\"k\":\"v\"}, {\"k2\":\"v2\"}]"}
</source>

<filter test.**>
  @type parser
  key_name message
  <parse>
    @type json
  </parse>
</filter>

<filter test.**>
  @type record_transformer
  enable_ruby true
  <record>
    class ${record.class}
  </record>
</filter>

<match test.**>
  @type stdout
</match>

Result BEFORE this fix

2024-04-26 19:06:23 +0900 [warn]: #0 dump an error event: error_class=NoMethodError error="undefined method `merge!' for [{\"k\"=>\"v\"}, {\"k2\"=>\"v2\"}]:Array" location="/home/daipom/work/fluentd/fluentd/lib/fluent/plugin/filter_record_transformer.rb:135:in `reform'" tag="test.array" time=2024-04-26 19:06:23.032939419 +0900 record=[{"k"=>"v"}, {"k2"=>"v2"}]

Result AFTER this fix

2024-04-26 19:07:25.096004693 +0900 test.array: {"k":"v","class":"Hash"}

It is wrong for Parser to return a record that is not Hash. Subsequent processing may result in errors. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

in_http didn't support yield of Parser. The specification assumed that Parser could return Array. However, this is wrong. Parser shouldn't return Array. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

@type

Config to reproduce: <source> @type sample tag test.array sample {"message": "[{\"k\":\"v\"}, {\"k2\":\"v2\"}]"} </source> <filter test.**> @type parser key_name message <parse> @type json </parse> </filter> <match test.**> @type stdout </match> Result: 2023-03-21 23:24:52.004470792 +0900 test.array: {"k":"v"} 2023-03-21 23:24:52.004470792 +0900 test.array: {"k":"v"} ... Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

lib/fluent/plugin/parser_json.rb

ashie

LGTM

daipom · 2024-04-30T06:47:48Z

Thanks! I have added Release Note to #4474 (comment).

daipom mentioned this pull request Apr 26, 2024

Make sure JSON parser returns Hash #4106

Closed

daipom force-pushed the make-sure-parser-returns-hash branch 3 times, most recently from 900854d to 85e20ad Compare April 30, 2024 02:05

daipom added 4 commits April 30, 2024 11:05

parser_json: Make sure return Hash

b010a09

It is wrong for Parser to return a record that is not Hash. Subsequent processing may result in errors. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

parser_mgspack: make sure return Hash

8433ca7

It is wrong for Parser to return a record that is not Hash. Subsequent processing may result in errors. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

in_http: support Parser yield

4bf78e6

in_http didn't support yield of Parser. The specification assumed that Parser could return Array. However, this is wrong. Parser shouldn't return Array. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

daipom force-pushed the make-sure-parser-returns-hash branch from 85e20ad to 6cace97 Compare April 30, 2024 02:06

ashie reviewed Apr 30, 2024

View reviewed changes

lib/fluent/plugin/parser_json.rb Show resolved Hide resolved

ashie approved these changes Apr 30, 2024

View reviewed changes

ashie added this to the v1.17.0 milestone Apr 30, 2024

ashie merged commit 35e2210 into master Apr 30, 2024
15 of 16 checks passed

ashie deleted the make-sure-parser-returns-hash branch April 30, 2024 03:02

ashie mentioned this pull request Apr 30, 2024

JSON parser plugin can sometimes output records as strings rather than Hash #4100

Open

daipom mentioned this pull request Apr 30, 2024

filter_parser: add error event for multiple parsed results #4478

Merged

daipom mentioned this pull request Jul 31, 2024

messagepack bug in http (in) plugin #4346

Open

daipom mentioned this pull request Sep 18, 2024

filter_parser: Transition from single record to stream #4620

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure parser returns hash #4474

Make sure parser returns hash #4474

daipom commented Apr 26, 2024 •

edited

Loading

ashie left a comment

daipom commented Apr 30, 2024

Make sure parser returns hash #4474

Make sure parser returns hash #4474

Conversation

daipom commented Apr 26, 2024 • edited Loading

Example: parser_json

Case: No subsequent processing (without convert_values)

Case: No subsequent processing (with convert_values)

Case: With subsequent filter processing

Example: filter_parser

Case: No subsequent processing (without any option)

Case: No subsequent processing (with some options such as reserve_data)

Case: With subsequent filter processing

ashie left a comment

Choose a reason for hiding this comment

daipom commented Apr 30, 2024

daipom commented Apr 26, 2024 •

edited

Loading

Case: No subsequent processing (without `convert_values`)

Case: No subsequent processing (with `convert_values`)

Case: No subsequent processing (with some options such as `reserve_data`)