From 672e84b2575b3846e9476edce1c9bcb9bd0bfcd1 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Tue, 7 May 2019 18:16:04 -0700 Subject: [PATCH 1/6] Deprecate support for chained multi-fields. We now issue a deprecation warning if a multi-field definition contains a [`fields]` entry. This PR also simplifies the definition of `MultiFieldParserContext`. --- .../elasticsearch/index/mapper/Mapper.java | 8 ++--- .../index/mapper/TypeParsers.java | 13 +++++-- .../index/mapper/TypeParsersTests.java | 34 +++++++++++++++++++ 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java index d98630e5f765e..5de5394a94abe 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java @@ -136,10 +136,7 @@ public Supplier queryShardContextSupplier() { protected Function similarityLookupService() { return similarityLookupService; } public ParserContext createMultiFieldContext(ParserContext in) { - return new MultiFieldParserContext(in) { - @Override - public boolean isWithinMultiField() { return true; } - }; + return new MultiFieldParserContext(in); } static class MultiFieldParserContext extends ParserContext { @@ -147,6 +144,9 @@ static class MultiFieldParserContext extends ParserContext { super(in.type(), in.similarityLookupService(), in.mapperService(), in.typeParsers(), in.indexVersionCreated(), in.queryShardContextSupplier()); } + + @Override + public boolean isWithinMultiField() { return true; } } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java index 77d7be62fc1b9..369d338cc366b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java @@ -19,8 +19,10 @@ package org.elasticsearch.index.mapper; +import org.apache.logging.log4j.LogManager; import org.apache.lucene.index.IndexOptions; import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.index.analysis.AnalysisMode; @@ -37,6 +39,8 @@ import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeStringValue; public class TypeParsers { + private static final DeprecationLogger deprecationLogger = new DeprecationLogger( + LogManager.getLogger(TypeParsers.class)); public static final String DOC_VALUES = "doc_values"; public static final String INDEX_OPTIONS_DOCS = "docs"; @@ -214,11 +218,16 @@ public static void parseField(FieldMapper.Builder builder, String name, Map multiFieldsPropNodes; + parserContext = parserContext.createMultiFieldContext(parserContext); + final Map multiFieldsPropNodes; if (propNode instanceof List && ((List) propNode).isEmpty()) { multiFieldsPropNodes = Collections.emptyMap(); } else if (propNode instanceof Map) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/TypeParsersTests.java b/server/src/test/java/org/elasticsearch/index/mapper/TypeParsersTests.java index 7e216c37686ee..2e1364d79ab30 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/TypeParsersTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/TypeParsersTests.java @@ -24,7 +24,11 @@ import org.apache.lucene.analysis.standard.StandardAnalyzer; import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.IndexMetaData; +import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.analysis.AbstractTokenFilterFactory; import org.elasticsearch.index.analysis.AnalysisMode; @@ -36,6 +40,7 @@ import org.elasticsearch.index.analysis.TokenFilterFactory; import org.elasticsearch.test.ESTestCase; +import java.io.IOException; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -152,6 +157,35 @@ public void testParseTextFieldCheckAnalyzerWithSearchAnalyzerAnalysisMode() { TypeParsers.parseTextField(builder, "name", new HashMap<>(fieldNode), parserContext); } + public void testMultiFieldWithinMultiField() throws IOException { + TextFieldMapper.Builder builder = new TextFieldMapper.Builder("textField"); + + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() + .field("type", "keyword") + .startObject("fields") + .startObject("sub-field") + .field("type", "keyword") + .startObject("fields") + .startObject("sub-sub-field") + .field("type", "keyword") + .endObject() + .endObject() + .endObject() + .endObject() + .endObject(); + + Map fieldNode = XContentHelper.convertToMap( + BytesReference.bytes(mapping), true, mapping.contentType()).v2(); + + Mapper.TypeParser typeParser = new KeywordFieldMapper.TypeParser(); + Mapper.TypeParser.ParserContext parserContext = new Mapper.TypeParser.ParserContext("type", + null, null, type -> typeParser, Version.CURRENT, null); + + TypeParsers.parseField(builder, "some-field", fieldNode, parserContext); + assertWarnings("The multi-field named [sub-field] contains its own [fields] entry. Defining " + + "multi-fields within a multi-field is deprecated and will no longer be supported in 8.0."); + } + private Analyzer createAnalyzerWithMode(String name, AnalysisMode mode) { TokenFilterFactory tokenFilter = new AbstractTokenFilterFactory(indexSettings, name, Settings.EMPTY) { @Override From 98b65a9d17ab4317a36f1ee73870bbb2d02ca5ea Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Tue, 7 May 2019 21:44:16 -0700 Subject: [PATCH 2/6] Fix failing test cases in ExternalFieldMapperTests. --- .../index/mapper/ExternalFieldMapperTests.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java index d80776007aba8..13c7288661c2c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java @@ -169,6 +169,9 @@ public void testExternalValuesWithMultifield() throws Exception { assertThat(raw, notNullValue()); assertThat(raw.binaryValue(), is(new BytesRef("foo"))); + + assertWarnings("The multi-field named [field] contains its own [fields] entry. Defining " + + "multi-fields within a multi-field is deprecated and will no longer be supported in 8.0."); } public void testExternalValuesWithMultifieldTwoLevels() throws Exception { @@ -234,5 +237,8 @@ public void testExternalValuesWithMultifieldTwoLevels() throws Exception { assertThat(doc.rootDoc().getField("field.raw"), notNullValue()); assertThat(doc.rootDoc().getField("field.raw").stringValue(), is("foo")); + + assertWarnings("The multi-field named [field] contains its own [fields] entry. Defining " + + "multi-fields within a multi-field is deprecated and will no longer be supported in 8.0."); } } From 43cc14ec13b42a192ce7181db627e8a68adf1ec4 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Mon, 20 May 2019 17:36:29 -0700 Subject: [PATCH 3/6] Fix some formatting. --- .../main/java/org/elasticsearch/index/mapper/TypeParsers.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java index 369d338cc366b..4113bb0b24283 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java @@ -39,8 +39,7 @@ import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeStringValue; public class TypeParsers { - private static final DeprecationLogger deprecationLogger = new DeprecationLogger( - LogManager.getLogger(TypeParsers.class)); + private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(TypeParsers.class)); public static final String DOC_VALUES = "doc_values"; public static final String INDEX_OPTIONS_DOCS = "docs"; From 80e321f853718c53a483bd8cb3b7d0fd331fbfdf Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Mon, 20 May 2019 18:25:59 -0700 Subject: [PATCH 4/6] Clarify and expand on the deprecation message. --- .../java/org/elasticsearch/index/mapper/TypeParsers.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java index 4113bb0b24283..b6c337527ff5d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java @@ -219,9 +219,10 @@ public static boolean parseMultiField(FieldMapper.Builder builder, String name, String propName, Object propNode) { if (propName.equals("fields")) { if (parserContext.isWithinMultiField()) { - deprecationLogger.deprecatedAndMaybeLog("multifield_within_multifield", "The multi-field " + - "named [" + name + "] contains its own [fields] entry. Defining multi-fields within a " + - "multi-field is deprecated and will no longer be supported in 8.0."); + deprecationLogger.deprecatedAndMaybeLog("multifield_within_multifield", "A multifield named [" + name + "] was " + + "encountered that contains its own [fields] entry. Defining multi-fields within a multi-field is deprecated and will " + + "no longer be supported in 8.0. It is recommended to remove all instances of [fields] that occur within a [fields] " + + "entry, either by flattening the chained multi-fields into a single level, or switching to [copy_to] if appropriate."); } parserContext = parserContext.createMultiFieldContext(parserContext); From bbc7962c7826232d53d23cbd7df06e429807cb48 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Mon, 20 May 2019 19:32:44 -0700 Subject: [PATCH 5/6] Fix ExternalFieldMapperTests and further clarify the message. --- .../elasticsearch/index/mapper/TypeParsers.java | 9 +++++---- .../index/mapper/ExternalFieldMapperTests.java | 14 ++++++++++---- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java index b6c337527ff5d..9848a23cac11b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java @@ -219,10 +219,11 @@ public static boolean parseMultiField(FieldMapper.Builder builder, String name, String propName, Object propNode) { if (propName.equals("fields")) { if (parserContext.isWithinMultiField()) { - deprecationLogger.deprecatedAndMaybeLog("multifield_within_multifield", "A multifield named [" + name + "] was " + - "encountered that contains its own [fields] entry. Defining multi-fields within a multi-field is deprecated and will " + - "no longer be supported in 8.0. It is recommended to remove all instances of [fields] that occur within a [fields] " + - "entry, either by flattening the chained multi-fields into a single level, or switching to [copy_to] if appropriate."); + deprecationLogger.deprecatedAndMaybeLog("multifield_within_multifield", "At least one multi-field, [" + name + "], was " + + "encountered that itself contains a multi-field. Defining multi-fields within a multi-field is deprecated and will " + + "no longer be supported in 8.0. To resolve the issue, all instances of [fields] that occur within a [fields] block " + + "should be removed from the mappings, either by flattening the chained [fields] blocks into a single level, or " + + "switching to [copy_to] if appropriate."); } parserContext = parserContext.createMultiFieldContext(parserContext); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java index 13c7288661c2c..e5d3040f7a3bc 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java @@ -170,8 +170,11 @@ public void testExternalValuesWithMultifield() throws Exception { assertThat(raw, notNullValue()); assertThat(raw.binaryValue(), is(new BytesRef("foo"))); - assertWarnings("The multi-field named [field] contains its own [fields] entry. Defining " + - "multi-fields within a multi-field is deprecated and will no longer be supported in 8.0."); + assertWarnings("At least one multi-field, [field], was " + + "encountered that itself contains a multi-field. Defining multi-fields within a multi-field is deprecated and will " + + "no longer be supported in 8.0. To resolve the issue, all instances of [fields] that occur within a [fields] block " + + "should be removed from the mappings, either by flattening the chained [fields] blocks into a single level, or " + + "switching to [copy_to] if appropriate."); } public void testExternalValuesWithMultifieldTwoLevels() throws Exception { @@ -238,7 +241,10 @@ public void testExternalValuesWithMultifieldTwoLevels() throws Exception { assertThat(doc.rootDoc().getField("field.raw"), notNullValue()); assertThat(doc.rootDoc().getField("field.raw").stringValue(), is("foo")); - assertWarnings("The multi-field named [field] contains its own [fields] entry. Defining " + - "multi-fields within a multi-field is deprecated and will no longer be supported in 8.0."); + assertWarnings("At least one multi-field, [field], was " + + "encountered that itself contains a multi-field. Defining multi-fields within a multi-field is deprecated and will " + + "no longer be supported in 8.0. To resolve the issue, all instances of [fields] that occur within a [fields] block " + + "should be removed from the mappings, either by flattening the chained [fields] blocks into a single level, or " + + "switching to [copy_to] if appropriate."); } } From d5fe1a247520da8faddd58c319dcf4e8bab57f77 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Tue, 21 May 2019 10:26:26 -0700 Subject: [PATCH 6/6] Fix TypeParsersTests. --- .../org/elasticsearch/index/mapper/TypeParsersTests.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/TypeParsersTests.java b/server/src/test/java/org/elasticsearch/index/mapper/TypeParsersTests.java index 801dbe14766c8..70f469b96370c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/TypeParsersTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/TypeParsersTests.java @@ -187,8 +187,11 @@ public void testMultiFieldWithinMultiField() throws IOException { null, null, type -> typeParser, Version.CURRENT, null); TypeParsers.parseField(builder, "some-field", fieldNode, parserContext); - assertWarnings("The multi-field named [sub-field] contains its own [fields] entry. Defining " + - "multi-fields within a multi-field is deprecated and will no longer be supported in 8.0."); + assertWarnings("At least one multi-field, [sub-field], was " + + "encountered that itself contains a multi-field. Defining multi-fields within a multi-field is deprecated and will " + + "no longer be supported in 8.0. To resolve the issue, all instances of [fields] that occur within a [fields] block " + + "should be removed from the mappings, either by flattening the chained [fields] blocks into a single level, or " + + "switching to [copy_to] if appropriate."); } private Analyzer createAnalyzerWithMode(String name, AnalysisMode mode) {