Null completion field should not throw IAE #33268

tony-dillon · 2018-08-30T10:51:54Z

null completion values should not throw IAE and be added to ignored fields

@jimczi would you mind taking a look at this PR please? I believe all tests run via gradlew check pass now.

many thanks,
Tony

… be added to ignored fields

ghost · 2018-08-30T10:51:58Z

Hi @tony-dillon, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

tony-dillon · 2018-08-30T10:54:05Z

Please note, I have signed the CLA and have received the document via email. I have also added the email used to make the commit to my profile.

jimczi

Thanks @tony-dillon , it looks good.
I left a comment regarding the usage of the _ignored field.

jimczi · 2018-08-30T11:04:52Z

server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java

        if (token == Token.VALUE_NULL) {
-            throw new MapperParsingException("completion field [" + fieldType().name() + "] does not support null values");
-        } else if (token == Token.START_ARRAY) {
+            context.addIgnoredField(fieldType.name());


I don't think we need to add explicit null values to the _ignored field. It should be treated as if the field was missing.

stopped adding the field to the ignored list

jimczi · 2018-08-30T11:10:48Z

server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java

+            XContentType.JSON));
+        assertThat(doc.docs().size(), equalTo(1));
+        assertNull(doc.docs().get(0).get("completion"));
+        assertNotNull(doc.docs().get(0).getField("_ignored"));


should be null

now checking for assertNull

jimczi · 2018-08-30T11:11:25Z

server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java

+            return null;
+        }
+
+        if (token == Token.START_ARRAY) {


can you restore the } else if here for readability ?

reinstated the else if

…t. restored else if. updated test to assert ignored fields is null

tony-dillon · 2018-08-30T15:13:29Z

Hi @jimczi - thanks for taking the time to review this.

I've addressed your comments, but I'm afraid that there is now an error during an integration test for the CustomSuggestion plugin.

I've attached the report. The only hint I can see from the report is that there may be a JVM crash:

[ant:junit4] ERROR: JVM J0 ended with an exception: Forked process returned with error code: 1. Very likely a JVM crash.

Would you be able to advise how I should tackle this?

reports.zip

jimczi

Thanks @tony-dillon . The jvm crash seems unrelated to this change. I'll trigger a CI build and will merge if it passes.

jimczi · 2018-08-30T15:35:37Z

test this please

tony-dillon · 2018-08-30T15:58:24Z

@jimczi : wow - looks like nothing worked on the elasticsearch-ci build. Almost every single class failed to load. can't imagine how I've managed that 😄

jimczi · 2018-08-30T16:06:50Z

Can you merge your branch with latest master and push ? I have no idea of what is going on but this is not due to this change.

tony-dillon · 2018-08-30T16:13:26Z

@jimczi merge from master done. thanks again

jimczi · 2018-08-30T16:20:08Z

test this please

tony-dillon · 2018-08-30T16:56:35Z

@jimczi

The build failed again, this time with:

17:35:43    > Throwable #1: java.lang.AssertionError: expected null, but was:<stored,indexed,omitNorms,indexOptions=DOCS<_ignored:completion>>

Seems to be triggered by this test:

assertNull(doc.docs().get(0).getField("_ignored"));

I wonder, could it be that null fields automatically get added to the ignoredFields list?

Cheers

jimczi

I left a comment in the tests, they don't use the right variable when asserting.

jimczi · 2018-08-30T17:18:06Z

server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java

+                    .array("completion", Token.VALUE_NULL, Token.VALUE_NULL)
+                    .endObject()),
+            XContentType.JSON));
+        assertThat(doc.docs().size(), equalTo(1));


it should be nullDoc?

jimczi · 2018-08-30T17:18:13Z

server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java

+                    .endObject()),
+            XContentType.JSON));
+        assertThat(doc.docs().size(), equalTo(1));
+        assertNull(doc.docs().get(0).get("completion"));


jimczi · 2018-08-30T17:18:21Z

server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java

+            XContentType.JSON));
+        assertThat(doc.docs().size(), equalTo(1));
+        assertNull(doc.docs().get(0).get("completion"));
+        assertNull(doc.docs().get(0).getField("_ignored"));


and here ;)

…rted on

jimczi · 2018-08-30T17:27:51Z

test this please

tony-dillon · 2018-08-30T17:32:36Z

Ahhhhh schoolboy error! thank you!

elasticmachine · 2018-08-30T17:35:31Z

Pinging @elastic/es-search-aggs

tony-dillon · 2018-08-30T17:48:27Z

Build failed:

18:39:12 FAILURE 0.05s J3 | CompletionFieldMapperTests.testFieldValueValidation <<< FAILURES!
18:39:12    > Throwable #1: java.lang.AssertionError: expected null, but was:<VALUE_NULL>

should the assert be changed?

assertThat(nullDoc.docs().get(0).get("completion"), equalTo(Token.VALUE_NULL));

jimczi · 2018-08-30T18:13:00Z

Yes it should be assertNull(nullDoc.docs().get(0).get("completion")), can you make sure that the test passes locally. If you have troubles to run the tests I can help but you need to run them locally (at least the ones you added) before we run them on CI.

tony-dillon · 2018-08-30T18:40:25Z

Hi @jimczi

I really appreciate your time with this, and I do always run the tests locally first which actually lead to my last comment.

I run my local tests like this:

/gradlew :server:test -Dtests.class=org.elasticsearch.index.mapper.CompletionFieldMapperTests -Dtests.method="testFieldValueValidation" -Dbuild.snapshot=false -Dlicense.key=./x-pack/qa/smoke-test-plugins-ssl/testnode.pem

When I run the tests locally, I get this:

  2> REPRODUCE WITH: ./gradlew :server:test -Dtests.seed=8CC2CA7D1B1C6586 -Dtests.class=org.elasticsearch.index.mapper.CompletionFieldMapperTests -Dtests.method="testFieldValueValidation" -Dtests.security.manager=true -Dtests.locale=fr-SC -Dtests.timezone=Africa/Sao_Tome -Dcompiler.java=10 -Druntime.java=10
FAILURE 4.56s | CompletionFieldMapperTests.testFieldValueValidation <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: expected null, but was:<VALUE_NULL>

The standout for me is: expected null, but was:<VALUE_NULL> which is why I thought maybe I was asserting on the wrong value (i.e. not assertNull but assertThat and use the VALUE_NULL value)

I apologise for taking up your time on this, it's my first contribution and I'm very much just working out how this stuff works 😄

Many thanks!
Tony

jimczi · 2018-08-30T18:47:45Z

I apologise for taking up your time on this, it's my first contribution and I'm very much just working out how this stuff works 😄

No apologies needed, I just wanted to make sure that you can test locally (which is important if you become a contributor ;) ).

Regarding the failure, we need assertNull because we don't add any field in the document if the completion is null.

tony-dillon · 2018-08-30T18:59:16Z

Local tests pass! Thank you Sir!

> Task :server:test
==> Test Summary: 1 suite, 1 test
   [junit4] JVM J0:     1.01 ..     9.08 =     8.07s
   [junit4] Execution time total: 9.14 sec.
   [junit4] Tests summary: 1 suite, 1 test

BUILD SUCCESSFUL in 1m 0s
29 actionable tasks: 2 executed, 27 up-to-date

jimczi · 2018-08-30T19:01:24Z

server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java

@@ -440,14 +441,25 @@ public void testFieldValueValidation() throws Exception {
        ParsedDocument doc = defaultMapper.parse(SourceToParse.source("test", "type1", "1", BytesReference
                .bytes(XContentFactory.jsonBuilder()
                    .startObject()
-                    .array("completion", "   ", "")


you changed the wrong doc ?

jimczi · 2018-08-30T19:01:45Z

server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java

+        ParsedDocument nullDoc = defaultMapper.parse(SourceToParse.source("test", "type1", "1", BytesReference
+                .bytes(XContentFactory.jsonBuilder()
+                    .startObject()
+                    .array("completion", Token.VALUE_NULL, Token.VALUE_NULL)


it should be .nullField("completion")

jimczi · 2018-08-30T19:02:59Z

@tony-dillon you pushed the change to the wrong doc ;)

jimczi · 2018-08-30T19:13:13Z

test this please

jimczi · 2018-08-30T19:28:54Z

server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java

@@ -38,6 +38,7 @@
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentType;
+import org.elasticsearch.common.xcontent.XContentParser.Token;


Sorry I missed that we forbid unused import in our build so this needs to be removed.

already fixed :) thanks

jimczi · 2018-08-30T19:30:11Z

test this please

tony-dillon · 2018-08-30T19:31:00Z

may I ask @jimczi - what's the recommended way to run the full suite of tests before submitting a PR?

./gradlew check or....?

jimczi · 2018-08-30T19:38:33Z

Sure ./gradlew clean check is what you should run but we can run it on the CI too so you don't need to rerun everything when you fix a small thing in your pr. You can run ./gradlew clean precommit to check compilation and styles and then run the failing tests locally like you did. It's good to run ./gradlew clean check before submitting the initial submission though.

tony-dillon · 2018-08-30T19:42:41Z

Thanks for the info.

I notice that you tagged this with with 6.4.1. Does that mean that if we finally get this to pass a build, it will make it into the next minor release?

That would be amazing! 😄

jimczi · 2018-08-31T11:22:10Z

@tony-dillon the plan is to have the fix in the next patch or minor release so 6.4.1 assuming we release it and 6.5.0 otherwise.

Can you check the failure, there's an IT test that needs to be adapted.

jimczi · 2018-09-03T10:45:12Z

test this please

jimczi · 2018-09-03T10:49:03Z

@tony-dillon, I hope you don't mind that I pushed a fix directly in your branch. The change looks good but we need a green build so I triggered a new one.

jimczi · 2018-09-03T11:09:44Z

test this please

tony-dillon · 2018-09-03T11:58:54Z

Of course not @jimczi ! Appreciate you taking the time to dig in and find the issue.

tony-dillon · 2018-09-03T14:07:49Z

Success! @jimczi

Ignore null value on the completion field Closes #33200

jimczi · 2018-09-03T14:52:06Z

I merged in master and 6.x and 6.4.1, thanks @tony-dillon !

tony-dillon · 2018-09-04T08:12:33Z

Thank you @jimczi - you really helped a lot getting this through, and it solves a real-world problem we are having trying to add completion fields to a schema then re-indexing when there are null values present.

Although the code change was small, getting to the point where I could even start to look at the issue was rather slow. To help others get started developing, I've created a Docker image, and posted an article on Discuss on how to use it.

https://discuss.elastic.co/t/contributing-to-elasticsearch-using-a-docker-image/147162

This should allow anybody to get developing in minutes rather then the hours I spent trying to get the environment setup correctly.

Once again, many thanks for your time and patience getting this live.
Do you have an ETA for the 6.4.1 release?

Cheers!

jimczi · 2018-09-04T10:56:34Z

You're welcome @tony-dillon.

Do you have an ETA for the 6.4.1 release?

We don't disclose release dates in advance so the best that I can tell you is that it will probably be soon.

…e-default-distribution * elastic/master: (213 commits) ML: Fix build after HLRC change Fix inner hits retrieval when stored fields are disabled (_none_) (elastic#33018) SQL: Show/desc commands now support table ids (elastic#33363) Mute testValidateFollowingIndexSettings HLRC: Add delete by query API (elastic#32782) [ML] The sort field on get records should default to the record_score (elastic#33358) [ML] Minor improvements to categorization Grok pattern creation (elastic#33353) [DOCS] fix a couple of typos (elastic#33356) Disable assemble task instead of removing it (elastic#33348) Simplify the return type of FieldMapper#parse. (elastic#32654) [ML] Delete forecast API (elastic#31134) (elastic#33218) Introduce private settings (elastic#33327) [Docs] Add search timeout caveats (elastic#33354) TESTS: Fix Race Condition in Temp Path Creation (elastic#33352) Fix from_range in search_after in changes snapshot (elastic#33335) TESTS+DISTR.: Fix testIndexCheckOnStartup Flake (elastic#33349) Null completion field should not throw IAE (elastic#33268) Adds code to help with IndicesRequestCacheIT failures (elastic#33313) Prevent NPE parsing the stop datafeed request. (elastic#33347) HLRC: Add ML get overall buckets API (elastic#33297) ...

tony-dillon added 3 commits August 28, 2018 15:55

[Issue elastic#33200] null completion values should not throw IAE and…

1c5da09

… be added to ignored fields

[Issue elastic#33200] fixed test

9d96f19

[Issue elastic#33200] updated test to use Token.NULL_VALUE

681b868

jimczi reviewed Aug 30, 2018

View reviewed changes

[Issue elastic#33200] stopped adding null fields to ignored field lis…

47513b9

…t. restored else if. updated test to assert ignored fields is null

jimczi approved these changes Aug 30, 2018

View reviewed changes

Merge branch 'master' into null_completion

886e18f

jimczi reviewed Aug 30, 2018

View reviewed changes

[Issue elastic#33200] fixed error where wrong document was being asse…

113245b

…rted on

jimczi added >bug :Search Relevance/Suggesters "Did you mean" and suggestions as you type v7.0.0 v6.5.0 v6.4.1 labels Aug 30, 2018

jimczi reviewed Aug 30, 2018

View reviewed changes

[Issue elastic#33200] set nullField on correct document

576dc7b

[Issue elastic#33200] remove unused use statement

563de3f

jimczi reviewed Aug 30, 2018

View reviewed changes

jimczi added 2 commits September 3, 2018 12:42

remote IT for null completion field value

27adc3f

Merge branch 'master' into pr/33268

7e0ad6e

add local change

75db758

jimczi merged commit a9d2b1d into elastic:master Sep 3, 2018

jimczi pushed a commit that referenced this pull request Sep 3, 2018

Null completion field should not throw IAE (#33268)

bc37003

Ignore null value on the completion field Closes #33200

jimczi pushed a commit that referenced this pull request Sep 3, 2018

Null completion field should not throw IAE (#33268)

021e352

Ignore null value on the completion field Closes #33200

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

Null completion field should not throw IAE #33268

Null completion field should not throw IAE #33268

Conversation

tony-dillon commented Aug 30, 2018

ghost commented Aug 30, 2018

tony-dillon commented Aug 30, 2018 • edited Loading

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tony-dillon commented Aug 30, 2018

jimczi left a comment

Choose a reason for hiding this comment

jimczi commented Aug 30, 2018

tony-dillon commented Aug 30, 2018

jimczi commented Aug 30, 2018

tony-dillon commented Aug 30, 2018

jimczi commented Aug 30, 2018

tony-dillon commented Aug 30, 2018 • edited Loading

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi commented Aug 30, 2018

tony-dillon commented Aug 30, 2018

elasticmachine commented Aug 30, 2018

tony-dillon commented Aug 30, 2018 • edited Loading

jimczi commented Aug 30, 2018

tony-dillon commented Aug 30, 2018

jimczi commented Aug 30, 2018

tony-dillon commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi commented Aug 30, 2018

jimczi commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi commented Aug 30, 2018

tony-dillon commented Aug 30, 2018

jimczi commented Aug 30, 2018

tony-dillon commented Aug 30, 2018

jimczi commented Aug 31, 2018

jimczi commented Sep 3, 2018

jimczi commented Sep 3, 2018

jimczi commented Sep 3, 2018

tony-dillon commented Sep 3, 2018

tony-dillon commented Sep 3, 2018

jimczi commented Sep 3, 2018

tony-dillon commented Sep 4, 2018

jimczi commented Sep 4, 2018

tony-dillon commented Aug 30, 2018 •

edited

Loading

tony-dillon commented Aug 30, 2018 •

edited

Loading

tony-dillon commented Aug 30, 2018 •

edited

Loading