From 1c5da09ee90eb598b01f31a60114171cccf73c34 Mon Sep 17 00:00:00 2001 From: Tony Dillon Date: Tue, 28 Aug 2018 15:55:41 +0100 Subject: [PATCH 01/10] [Issue #33200] null completion values should not throw IAE and be added to ignored fields --- .../index/mapper/CompletionFieldMapper.java | 9 +++++++-- .../index/mapper/CompletionFieldMapperTests.java | 13 +++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java index 83d9a8178ca5a..460f821d70d8c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java @@ -435,9 +435,14 @@ public Mapper parse(ParseContext context) throws IOException { XContentParser parser = context.parser(); Token token = parser.currentToken(); Map inputMap = new HashMap<>(1); + + // ignore null values 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()); + return null; + } + + if (token == Token.START_ARRAY) { while ((token = parser.nextToken()) != Token.END_ARRAY) { parse(context, token, parser, inputMap); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java index a01ddccc9398b..d1eea644159fa 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java @@ -448,6 +448,19 @@ public void testFieldValueValidation() throws Exception { assertNotNull(doc.docs().get(0).getField("_ignored")); IndexableField ignoredFields = doc.docs().get(0).getField("_ignored"); assertThat(ignoredFields.stringValue(), equalTo("completion")); + + // null inputs are ignored + ParsedDocument doc = defaultMapper.parse(SourceToParse.source("test", "type1", "1", BytesReference + .bytes(XContentFactory.jsonBuilder() + .startObject() + .array("completion", null) + .endObject()), + XContentType.JSON)); + assertThat(doc.docs().size(), equalTo(1)); + assertNull(doc.docs().get(0).get("completion")); + assertNotNull(doc.docs().get(0).getField("_ignored")); + IndexableField ignoredFields = doc.docs().get(0).getField("_ignored"); + assertThat(ignoredFields.stringValue(), equalTo("completion")); } public void testPrefixQueryType() throws Exception { From 9d96f19838c75697467d3682c54ec9be170cec3a Mon Sep 17 00:00:00 2001 From: Tony Dillon Date: Thu, 30 Aug 2018 08:00:54 +0100 Subject: [PATCH 02/10] [Issue #33200] fixed test --- .../index/mapper/CompletionFieldMapperTests.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java index d1eea644159fa..eb943fb4559f1 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java @@ -450,17 +450,17 @@ public void testFieldValueValidation() throws Exception { assertThat(ignoredFields.stringValue(), equalTo("completion")); // null inputs are ignored - ParsedDocument doc = defaultMapper.parse(SourceToParse.source("test", "type1", "1", BytesReference + ParsedDocument nullDoc = defaultMapper.parse(SourceToParse.source("test", "type1", "1", BytesReference .bytes(XContentFactory.jsonBuilder() .startObject() - .array("completion", null) + .array("completion", null, null) .endObject()), XContentType.JSON)); assertThat(doc.docs().size(), equalTo(1)); assertNull(doc.docs().get(0).get("completion")); assertNotNull(doc.docs().get(0).getField("_ignored")); - IndexableField ignoredFields = doc.docs().get(0).getField("_ignored"); - assertThat(ignoredFields.stringValue(), equalTo("completion")); + IndexableField nullIgnoredFields = doc.docs().get(0).getField("_ignored"); + assertThat(nullIgnoredFields.stringValue(), equalTo("completion")); } public void testPrefixQueryType() throws Exception { From 681b868f5a0f7886537ef14e3cfee1edaa3181db Mon Sep 17 00:00:00 2001 From: Tony Dillon Date: Thu, 30 Aug 2018 10:37:59 +0100 Subject: [PATCH 03/10] [Issue #33200] updated test to use Token.NULL_VALUE --- .../elasticsearch/index/mapper/CompletionFieldMapperTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java index eb943fb4559f1..5f8295399bcda 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java +++ b/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; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.index.IndexService; import org.elasticsearch.index.analysis.NamedAnalyzer; @@ -453,7 +454,7 @@ public void testFieldValueValidation() throws Exception { ParsedDocument nullDoc = defaultMapper.parse(SourceToParse.source("test", "type1", "1", BytesReference .bytes(XContentFactory.jsonBuilder() .startObject() - .array("completion", null, null) + .array("completion", Token.VALUE_NULL, Token.VALUE_NULL) .endObject()), XContentType.JSON)); assertThat(doc.docs().size(), equalTo(1)); From 47513b90540d88e1b1a123f1d9136c39bf3850d5 Mon Sep 17 00:00:00 2001 From: Tony Dillon Date: Thu, 30 Aug 2018 12:51:11 +0100 Subject: [PATCH 04/10] [Issue #33200] stopped adding null fields to ignored field list. restored else if. updated test to assert ignored fields is null --- .../elasticsearch/index/mapper/CompletionFieldMapper.java | 5 +---- .../index/mapper/CompletionFieldMapperTests.java | 4 +--- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java index 460f821d70d8c..d1490ae4c46fc 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java @@ -438,11 +438,8 @@ public Mapper parse(ParseContext context) throws IOException { // ignore null values if (token == Token.VALUE_NULL) { - context.addIgnoredField(fieldType.name()); return null; - } - - if (token == Token.START_ARRAY) { + } else if (token == Token.START_ARRAY) { while ((token = parser.nextToken()) != Token.END_ARRAY) { parse(context, token, parser, inputMap); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java index 5f8295399bcda..df4d76e1ae5ed 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java @@ -459,9 +459,7 @@ public void testFieldValueValidation() throws Exception { XContentType.JSON)); assertThat(doc.docs().size(), equalTo(1)); assertNull(doc.docs().get(0).get("completion")); - assertNotNull(doc.docs().get(0).getField("_ignored")); - IndexableField nullIgnoredFields = doc.docs().get(0).getField("_ignored"); - assertThat(nullIgnoredFields.stringValue(), equalTo("completion")); + assertNull(doc.docs().get(0).getField("_ignored")); } public void testPrefixQueryType() throws Exception { From 113245bdba8d9e2eb36e6241fd9d4b20e1334e16 Mon Sep 17 00:00:00 2001 From: Tony Dillon Date: Thu, 30 Aug 2018 18:24:33 +0100 Subject: [PATCH 05/10] [Issue #33200] fixed error where wrong document was being asserted on --- .../index/mapper/CompletionFieldMapperTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java index df4d76e1ae5ed..bae8055cbdea3 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java @@ -457,9 +457,9 @@ public void testFieldValueValidation() throws Exception { .array("completion", Token.VALUE_NULL, Token.VALUE_NULL) .endObject()), XContentType.JSON)); - assertThat(doc.docs().size(), equalTo(1)); - assertNull(doc.docs().get(0).get("completion")); - assertNull(doc.docs().get(0).getField("_ignored")); + assertThat(nullDoc.docs().size(), equalTo(1)); + assertNull(nullDoc.docs().get(0).get("completion")); + assertNull(nullDoc.docs().get(0).getField("_ignored")); } public void testPrefixQueryType() throws Exception { From 94790b13b8cb62e9a343b480f3c1ecc3317e8d5f Mon Sep 17 00:00:00 2001 From: Tony Dillon Date: Thu, 30 Aug 2018 19:58:14 +0100 Subject: [PATCH 06/10] [Issue #33200] set correct null field value on test document --- .../elasticsearch/index/mapper/CompletionFieldMapperTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java index bae8055cbdea3..d7d7f6663e40e 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java @@ -441,7 +441,7 @@ public void testFieldValueValidation() throws Exception { ParsedDocument doc = defaultMapper.parse(SourceToParse.source("test", "type1", "1", BytesReference .bytes(XContentFactory.jsonBuilder() .startObject() - .array("completion", " ", "") + .nullField("completion") .endObject()), XContentType.JSON)); assertThat(doc.docs().size(), equalTo(1)); From 576dc7bc3d745d7141099dabfdf0c6f3de027d9f Mon Sep 17 00:00:00 2001 From: Tony Dillon Date: Thu, 30 Aug 2018 20:04:43 +0100 Subject: [PATCH 07/10] [Issue #33200] set nullField on correct document --- .../index/mapper/CompletionFieldMapperTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java index d7d7f6663e40e..3d7ad4b12f62d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java @@ -441,7 +441,7 @@ public void testFieldValueValidation() throws Exception { ParsedDocument doc = defaultMapper.parse(SourceToParse.source("test", "type1", "1", BytesReference .bytes(XContentFactory.jsonBuilder() .startObject() - .nullField("completion") + .array("completion", " ", "") .endObject()), XContentType.JSON)); assertThat(doc.docs().size(), equalTo(1)); @@ -454,7 +454,7 @@ public void testFieldValueValidation() throws Exception { ParsedDocument nullDoc = defaultMapper.parse(SourceToParse.source("test", "type1", "1", BytesReference .bytes(XContentFactory.jsonBuilder() .startObject() - .array("completion", Token.VALUE_NULL, Token.VALUE_NULL) + .nullField("completion") .endObject()), XContentType.JSON)); assertThat(nullDoc.docs().size(), equalTo(1)); From 563de3f0f090bbaeebff1ede0ad1f3bc0da8d0e0 Mon Sep 17 00:00:00 2001 From: Tony Dillon Date: Thu, 30 Aug 2018 20:27:19 +0100 Subject: [PATCH 08/10] [Issue #33200] remove unused use statement --- .../elasticsearch/index/mapper/CompletionFieldMapperTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java index 3d7ad4b12f62d..e3739eed3362d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java @@ -38,7 +38,6 @@ 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; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.index.IndexService; import org.elasticsearch.index.analysis.NamedAnalyzer; From 27adc3f1eb483632fdb2f5dccf9712fb8d16f2db Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Mon, 3 Sep 2018 12:42:21 +0200 Subject: [PATCH 09/10] remote IT for null completion field value --- .../suggest/CompletionSuggestSearchIT.java | 29 ------------------- 1 file changed, 29 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java b/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java index 58b2b86396317..0c7fb723ea99c 100644 --- a/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java +++ b/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java @@ -1111,35 +1111,6 @@ public void testIssue5930() throws IOException { } } - // see issue #6399 - public void testIndexingUnrelatedNullValue() throws Exception { - String mapping = Strings - .toString(jsonBuilder() - .startObject() - .startObject(TYPE) - .startObject("properties") - .startObject(FIELD) - .field("type", "completion") - .endObject() - .endObject() - .endObject() - .endObject()); - - assertAcked(client().admin().indices().prepareCreate(INDEX).addMapping(TYPE, mapping, XContentType.JSON).get()); - ensureGreen(); - - client().prepareIndex(INDEX, TYPE, "1").setSource(FIELD, "strings make me happy", FIELD + "_1", "nulls make me sad") - .setRefreshPolicy(IMMEDIATE).get(); - - try { - client().prepareIndex(INDEX, TYPE, "2").setSource(FIELD, null, FIELD + "_1", "nulls make me sad").get(); - fail("Expected MapperParsingException for null value"); - } catch (MapperParsingException e) { - // make sure that the exception has the name of the field causing the error - assertTrue(e.getDetailedMessage().contains(FIELD)); - } - } - public void testMultiDocSuggestions() throws Exception { final CompletionMappingBuilder mapping = new CompletionMappingBuilder(); createIndexAndMapping(mapping); From 75db758e1fde46ebcc035dff395617bc091e5c0f Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Mon, 3 Sep 2018 13:03:44 +0200 Subject: [PATCH 10/10] add local change --- .../elasticsearch/search/suggest/CompletionSuggestSearchIT.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java b/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java index 0c7fb723ea99c..65c58e631ec0e 100644 --- a/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java +++ b/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java @@ -31,12 +31,10 @@ import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.support.master.AcknowledgedResponse; import org.elasticsearch.common.FieldMemoryStats; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.Fuzziness; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; -import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.mapper.MapperParsingException; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.search.aggregations.AggregationBuilders;