Making JsonProcessor stricter so that it does not silently drop data by masseyke · Pull Request #93179 · elastic/elasticsearch

masseyke · 2023-01-23T23:50:03Z

This PR makes JsonProcessor's JSON parsing a little bit stricter so that we are not silently dropping data when given bad inputs. Previously if the input string began with something that could be parsed as a valid json field, then the processor would grab that and ignore the rest. For example, 123 "foo" would be parsed as 123, dropping the "foo". Now by default it will throw an IllegalArgumentException on a string like this. A user can now set the strict_json_parsing parameter to false to get the old behavior. For example:

POST _ingest/pipeline/_simulate
{
  "pipeline": {
    "description": "",
    "processors" : [
      {
        "json" : {
          "field" : "message",
          "strict_json_parsing": false
        }
      }
    ]
  },
  "docs": [
    {
      "_source": {
        "message": "123 \"foo\""
      }
    }
  ]
}'

Closes #92898

github-actions · 2023-01-23T23:50:16Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2023-01-23T23:50:28Z

Hi @masseyke, I've created a changelog YAML for you.

…elasticsearch into fix/json-processor-too-lenient

elasticsearchmachine · 2023-01-24T20:39:51Z

Pinging @elastic/es-data-management (Team:Data Management)

masseyke · 2023-01-24T21:26:02Z

@elasticmachine run elasticsearch-ci/docs

mattc58

I left comments mostly around style but the core logic is +1 LGTM to me

mattc58 · 2023-01-24T21:21:57Z

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/JsonProcessorTests.java

        String randomField = randomAlphaOfLength(3);
        String randomTargetField = randomAlphaOfLength(2);
-        JsonProcessor jsonProcessor = new JsonProcessor(processorTag, null, randomField, randomTargetField, false, REPLACE, false);
+        JsonProcessor jsonProcessor = new JsonProcessor(processorTag, null, randomField, randomTargetField, false, REPLACE, false, true);


These lines in this test file make me wonder if we should add a constructor to JsonProcessor which has the default strict behavior, and then have the other constructor for allowing the caller to set it. It would at the very least reduce the changes on this file.

Yeah I can do that -- I went back and forth over whether it would be simpler to have another constructor or another required argument to the single constructor.

mattc58 · 2023-01-24T21:23:59Z

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/JsonProcessorTests.java

+                assertThat(resultList.get(i), closeTo(list.get(i), .001));
+            }
+        }
+        expectThrows(IllegalArgumentException.class, () -> JsonProcessor.apply("foo", true, true));


Why are these expected to fail? A comment would help.

mattc58 · 2023-01-24T21:25:53Z

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/JsonProcessorTests.java

+        expectThrows(IllegalArgumentException.class, () -> JsonProcessor.apply("123 foo", true, true));
+        expectThrows(IllegalArgumentException.class, () -> JsonProcessor.apply("45 this is {\"a\": \"json\"}", true, true));
+
+        {


These are all good. Maybe split into a separate test just to make it clear that we're testing for something different than the code above? Basically, maybe the "strict validation" stuff could be its own test.

jbaiera

Changes LGTM, pending green CI of course. Had one question about breaking vs bug classification.

jbaiera · 2023-01-24T21:23:40Z

docs/reference/ingest/processors/json.asciidoc

 | `add_to_root`                   | no        | false     | Flag that forces the parsed JSON to be added at the top level of the document. `target_field` must not be set when this option is chosen.
 | `add_to_root_conflict_strategy` | no        | `replace` | When set to `replace`, root fields that conflict with fields from the parsed JSON will be overridden. When set to `merge`, conflicting fields will be merged. Only applicable if `add_to_root` is set to `true`.
 | `allow_duplicate_keys`          | no        | false     | When set to `true`, the JSON parser will not fail if the JSON contains duplicate keys. Instead, the last encountered value for any duplicate key wins.
+| `strict_json_parsing`           | no        | true      | When set to `true`, the JSON parser will strictly parse the field value. When set to `false`, the JSON parser will be more lenient but also more likely to drop parts of the field value. For example if `strict_json_parsing` is set to `true` and the field value is `123 "foo"` then the processor will throw an IllegalArgumentException. But if `strict_json_parsing` is set to `false` then the field value will be parsed as `123`.


With the default being strict, does this count as a breaking change now or are we classifying this as a bug?

I thought we had agreed it was a bug.

Couldn't remember. If so, all good!

Making JsonProcessor stricter so that it does not silently drop data

62f40cf

masseyke added >bug :Distributed/Ingest Node Execution or management of Ingest Pipelines v8.7.0 labels Jan 23, 2023

masseyke added 4 commits January 23, 2023 17:50

Update docs/changelog/93179.yaml

8218026

suppressing rawtypes warnings

9bf46a6

Merge branch 'fix/json-processor-too-lenient' of github.com:masseyke/…

67caf61

…elasticsearch into fix/json-processor-too-lenient

adding a unit test and a comment

d5f7a6c

masseyke marked this pull request as ready for review January 24, 2023 20:39

elasticsearchmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Jan 24, 2023

mattc58 approved these changes Jan 24, 2023

View reviewed changes

jbaiera approved these changes Jan 24, 2023

View reviewed changes

masseyke added 2 commits January 24, 2023 16:57

code review feedback

fa5a514

code review feedback

37b805e

masseyke added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jan 24, 2023

elasticsearchmachine merged commit f327352 into elastic:main Jan 24, 2023

masseyke deleted the fix/json-processor-too-lenient branch January 24, 2023 23:44

masseyke mentioned this pull request Jan 25, 2023

Ingest JSON processor parses starting object instead of failing #43262

Closed

masseyke mentioned this pull request Feb 7, 2023

Adding release notes for JsonProcessor change #93567

Merged

joegallo mentioned this pull request Apr 6, 2023

Pipeline fails with "cannot access method/field [slowlog] from a null def reference" #95086

Closed

rdner mentioned this pull request Apr 7, 2023

[updatecli] update elastic stack version for testing 8.7.1-1bebafa7 elastic/beats#34589

Closed

andrewkroh mentioned this pull request Apr 18, 2023

Fix pipeline and tests for 8.7 elastic/integrations#5910

Merged

5 tasks

rdner mentioned this pull request Apr 19, 2023

Behaviour of certain Filebeat modules changed with Elasticsearch 8.7.0 elastic/beats#35133

Closed

4 tasks

This was referenced Apr 20, 2023

[Filebeat][MySQL Enterprise] Ingest pipeline parser error for trailing comma elastic/beats#35158

Closed

[MySQL Enterprise] Ingest pipeline parser error for trailing comma elastic/integrations#5941

Closed

ebeahan mentioned this pull request Apr 20, 2023

[Filebeat][envoyproxy] Fix JSON formatting in envoyproxy testing logs elastic/beats#35159

Merged

Conversation

masseyke commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 23, 2023

Uh oh!

elasticsearchmachine commented Jan 23, 2023

Uh oh!

elasticsearchmachine commented Jan 24, 2023

Uh oh!

masseyke commented Jan 24, 2023

Uh oh!

mattc58 left a comment

Choose a reason for hiding this comment

Uh oh!

mattc58 Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

masseyke Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

mattc58 Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

mattc58 Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

jbaiera left a comment

Choose a reason for hiding this comment

Uh oh!

jbaiera Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

masseyke Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

jbaiera Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

masseyke commented Jan 23, 2023 •

edited

Loading