ingest: dot_expander_processor prevent null add/append to source document #35106

jakelandis · 2018-10-30T20:40:32Z

don't allow null values to be added to the source document
if the field does not exist in the source document.
don't allow null values to be appended to an existing field
if the field does not exist in the source document.

I noticed some peculiar behavior w/r/t the dot expander processor adding in null values when either 1) the field declared is missing from the source document 2) the source document already contains the nested objects. For example:

PUT _ingest/pipeline/expander
{
  "processors": [
    {
      "dot_expander": {
        "field": "network.name"
      }
    }
  ]
}
POST test/_doc/1?pipeline=expander
{
  "foo": "bar"
}

POST test/_doc/2?pipeline=expander
{
  "network": {
    "name" :"bar"
  }
}

GET test/_doc/_mget
{
  "docs": [
    {
      "_id": "1"
    },
    {
      "_id": "2"
    }
  ]
}

Results in:

{
  "docs" : [
    {
      "_index" : "test",
      "_type" : "_doc",
      "_id" : "1",
      "_version" : 21,
      "found" : true,
      "_source" : {
        "foo" : "bar",
        "network" : {
          "name" : null
        }
      }
    },
    {
      "_index" : "test",
      "_type" : "_doc",
      "_id" : "2",
      "_version" : 1,
      "found" : true,
      "_source" : {
        "network" : {
          "name" : [
            "bar",
            null
          ]
        }
      }
    }
  ]
}

With this change:

{
  "docs": [
    {
      "_index": "test",
      "_type": "_doc",
      "_id": "1",
      "_version": 1,
      "found": true,
      "_source": {
        "foo": "bar"
      }
    },
    {
      "_index": "test",
      "_type": "_doc",
      "_id": "2",
      "_version": 1,
      "found": true,
      "_source": {
        "network": {
          "name": "bar"
        }
      }
    }
  ]
}

EDIT : updated text to match latest commit (examples did not need to change)

* support for ignore_missing, such that if the field does not exist in source document the processor will be skipped * fix bug such that if the location to expand to already exists as nested objects, don't append a null value.

elasticmachine · 2018-10-30T20:40:41Z

Pinging @elastic/es-core-infra

jakelandis · 2018-10-31T21:20:43Z

After a some discussion with @talevy , I will remove the ignore_missing here since it isn't a good fit w/r/t how the other processors define it's behavior. We will however keep the behavior that prevents the null from appearing the IngestDocument, just will do so without introducing a new option.

This reverts commit 650986d.

jakelandis · 2018-11-01T14:35:12Z

@talevy - changes made as discussed. The GH diff is kinda ugly, but it really a 1 line change.

talevy

left some questions, otherwise looks good!

talevy · 2018-11-02T18:48:42Z

...s/ingest-common/src/test/java/org/elasticsearch/ingest/common/DotExpanderProcessorTests.java

@@ -143,4 +143,41 @@ public void testEscapeFields_path() throws Exception {
        assertThat(document.getFieldValue("field.foo.bar.baz", String.class), equalTo("value"));
    }

+
+    public void testEscapeFields_doNotingIfFieldNotInSourceDoc() throws Exception {


s/doNoting/doNothing

talevy · 2018-11-02T18:49:29Z

...s/ingest-common/src/test/java/org/elasticsearch/ingest/common/DotExpanderProcessorTests.java

+        //hasField returns false since it requires the expanded form, which is not expanded since we did not ask for it to be
+        assertFalse(document.hasField("foo.bar"));
+        //nothing has changed
+        assertEquals(document.getSourceAndMetadata().get("foo.bar"), source.get("foo.bar"));


is this necessary given the next line?

I guess, are any of the other assertions necessary given that we are checking that source has not changed at all in this case (and no metadata was added).

actually, comparing to source is mis-leading since source is the object that is getting changed, so assertEquals(document.getSourceAndMetadata(), source); is like asserting 1==1.

I removed the assertions against source and kept the specific assertions. Thanks for spotting this.

talevy · 2018-11-02T18:50:56Z

...s/ingest-common/src/test/java/org/elasticsearch/ingest/common/DotExpanderProcessorTests.java

+        assertTrue(document.hasField("foo.bar"));
+        //nothing changed
+        assertEquals(document.getSourceAndMetadata().get("foo.bar"), source.get("foo.bar"));
+        assertEquals(document.getSourceAndMetadata(), source);


same question as above, is this not a deep enough equals?

removed all misleading assertions against source

talevy

LGTM. thanks!

…ment (#35106) * don't allow null values to be added or appended to the source document if the field does not exist.

jakelandis · 2018-11-06T15:35:34Z

6.6.0 backport : 1e02754

jakelandis added >bug >enhancement :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v7.0.0 v6.6.0 labels Oct 30, 2018

jakelandis requested a review from rjernst October 30, 2018 20:40

rjernst requested a review from talevy October 31, 2018 16:27

jakelandis removed the >enhancement label Oct 31, 2018

jakelandis added 2 commits November 1, 2018 08:10

Revert "ingest: dot_expander_processor, ignore_missing and prevent null"

a628f29

This reverts commit 650986d.

remove ignore_missing in favor of fixing null insertion bug

a3980e6

jakelandis changed the title ~~ingest: dot_expander_processor, ignore_missing and prevent null~~ ingest: dot_expander_processor prevent null add/append to source document Nov 1, 2018

talevy reviewed Nov 2, 2018

View reviewed changes

fix typo, remove misleading assertions

6fdf05d

talevy approved these changes Nov 5, 2018

View reviewed changes

jakelandis merged commit 9150a93 into elastic:master Nov 5, 2018

jakelandis added the backport pending label Nov 5, 2018

jakelandis added a commit that referenced this pull request Nov 6, 2018

ingest: dot_expander_processor prevent null add/append to source docu…

1e02754

…ment (#35106) * don't allow null values to be added or appended to the source document if the field does not exist.

jakelandis removed the backport pending label Nov 6, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ingest: dot_expander_processor prevent null add/append to source document #35106

ingest: dot_expander_processor prevent null add/append to source document #35106

jakelandis commented Oct 30, 2018 •

edited

elasticmachine commented Oct 30, 2018

jakelandis commented Oct 31, 2018

jakelandis commented Nov 1, 2018

talevy left a comment

talevy Nov 2, 2018

jakelandis Nov 5, 2018

talevy Nov 2, 2018

talevy Nov 2, 2018

jakelandis Nov 5, 2018

talevy Nov 2, 2018

jakelandis Nov 5, 2018

talevy left a comment

jakelandis commented Nov 6, 2018

ingest: dot_expander_processor prevent null add/append to source document #35106

ingest: dot_expander_processor prevent null add/append to source document #35106

Conversation

jakelandis commented Oct 30, 2018 • edited

elasticmachine commented Oct 30, 2018

jakelandis commented Oct 31, 2018

jakelandis commented Nov 1, 2018

talevy left a comment

Choose a reason for hiding this comment

talevy Nov 2, 2018

Choose a reason for hiding this comment

jakelandis Nov 5, 2018

Choose a reason for hiding this comment

talevy Nov 2, 2018

Choose a reason for hiding this comment

talevy Nov 2, 2018

Choose a reason for hiding this comment

jakelandis Nov 5, 2018

Choose a reason for hiding this comment

talevy Nov 2, 2018

Choose a reason for hiding this comment

jakelandis Nov 5, 2018

Choose a reason for hiding this comment

talevy left a comment

Choose a reason for hiding this comment

jakelandis commented Nov 6, 2018

jakelandis commented Oct 30, 2018 •

edited