Skip to content

Commit

Permalink
Merge pull request #15213 from brwe/copy-to-in-multi-fields-exception
Browse files Browse the repository at this point in the history
throw exception if a copy_to is within a multi field

Copy to within multi field is ignored from 2.0 on, see #10802.
Instead of just ignoring it, we should throw an exception if this
is found in the mapping when a mapping is added. For already
existing indices we should at least log a warning.

related to #14946
  • Loading branch information
brwe committed Dec 8, 2015
1 parent 0ad7431 commit e240152
Show file tree
Hide file tree
Showing 5 changed files with 218 additions and 7 deletions.
18 changes: 18 additions & 0 deletions core/src/main/java/org/elasticsearch/index/mapper/Mapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,24 @@ public Version indexVersionCreated() {
public ParseFieldMatcher parseFieldMatcher() {
return parseFieldMatcher;
}

public boolean isWithinMultiField() { return false; }

protected Map<String, TypeParser> typeParsers() { return typeParsers; }

public ParserContext createMultiFieldContext(ParserContext in) {
return new MultiFieldParserContext(in) {
@Override
public boolean isWithinMultiField() { return true; }
};
}

class MultiFieldParserContext extends ParserContext {
MultiFieldParserContext(ParserContext in) {
super(in.type(), in.analysisService, in.similarityLookupService(), in.mapperService(), ImmutableMap.<String, TypeParser>builder().putAll(in.typeParsers()).build(), in.indexVersionCreated(), in.parseFieldMatcher());
}
}

}

Mapper.Builder<?,?> parse(String name, Map<String, Object> node, ParserContext parserContext) throws MapperParsingException;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.index.mapper.core;

import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.inject.Injector;
import org.elasticsearch.common.inject.ModulesBuilder;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsModule;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.EnvironmentModule;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexNameModule;
import org.elasticsearch.index.analysis.AnalysisModule;
import org.elasticsearch.index.analysis.AnalysisService;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.settings.IndexSettingsModule;
import org.elasticsearch.index.similarity.SimilarityLookupService;
import org.elasticsearch.indices.analysis.IndicesAnalysisService;

import java.nio.file.Path;


public class MapperTestUtils {

public static MapperService newMapperService(Path tempDir, Settings indexSettings) {
Settings.Builder settingsBuilder = Settings.builder()
.put("path.home", tempDir)
.put(indexSettings);
if (indexSettings.get(IndexMetaData.SETTING_VERSION_CREATED) == null) {
settingsBuilder.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT);
}
Settings finalSettings = settingsBuilder.build();
return new MapperService(new Index("test"),
finalSettings,
newAnalysisService(finalSettings),
newSimilarityLookupService(finalSettings),
null);
}

private static AnalysisService newAnalysisService(Settings indexSettings) {
Injector parentInjector = new ModulesBuilder().add(new SettingsModule(indexSettings), new EnvironmentModule(new Environment(indexSettings))).createInjector();
Index index = new Index("test");
Injector injector = new ModulesBuilder().add(
new IndexSettingsModule(index, indexSettings),
new IndexNameModule(index),
new AnalysisModule(indexSettings, parentInjector.getInstance(IndicesAnalysisService.class))).createChildInjector(parentInjector);

return injector.getInstance(AnalysisService.class);
}

private static SimilarityLookupService newSimilarityLookupService(Settings indexSettings) {
return new SimilarityLookupService(new Index("test"), indexSettings);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.joda.FormatDateTimeFormatter;
import org.elasticsearch.common.joda.Joda;
import org.elasticsearch.common.logging.ESLoggerFactory;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.loader.SettingsLoader;
import org.elasticsearch.index.analysis.NamedAnalyzer;
Expand Down Expand Up @@ -184,11 +185,12 @@ public static void parseNumberField(NumberFieldMapper.Builder builder, String na
public static void parseField(FieldMapper.Builder builder, String name, Map<String, Object> fieldNode, Mapper.TypeParser.ParserContext parserContext) {
NamedAnalyzer indexAnalyzer = builder.fieldType().indexAnalyzer();
NamedAnalyzer searchAnalyzer = builder.fieldType().searchAnalyzer();
Version indexVersionCreated = parserContext.indexVersionCreated();
for (Iterator<Map.Entry<String, Object>> iterator = fieldNode.entrySet().iterator(); iterator.hasNext();) {
Map.Entry<String, Object> entry = iterator.next();
final String propName = Strings.toUnderscoreCase(entry.getKey());
final Object propNode = entry.getValue();
if (propName.equals("index_name") && parserContext.indexVersionCreated().before(Version.V_2_0_0_beta1)) {
if (propName.equals("index_name") && indexVersionCreated.before(Version.V_2_0_0_beta1)) {
builder.indexName(propNode.toString());
iterator.remove();
} else if (propName.equals("store")) {
Expand Down Expand Up @@ -242,7 +244,7 @@ public static void parseField(FieldMapper.Builder builder, String name, Map<Stri
iterator.remove();
} else if (propName.equals("omit_term_freq_and_positions")) {
final IndexOptions op = nodeBooleanValue(propNode) ? IndexOptions.DOCS : IndexOptions.DOCS_AND_FREQS_AND_POSITIONS;
if (parserContext.indexVersionCreated().onOrAfter(Version.V_1_0_0_RC2)) {
if (indexVersionCreated.onOrAfter(Version.V_1_0_0_RC2)) {
throw new ElasticsearchParseException("'omit_term_freq_and_positions' is not supported anymore - use ['index_options' : 'docs'] instead");
}
// deprecated option for BW compat
Expand All @@ -252,8 +254,8 @@ public static void parseField(FieldMapper.Builder builder, String name, Map<Stri
builder.indexOptions(nodeIndexOptionValue(propNode));
iterator.remove();
} else if (propName.equals("analyzer") || // for backcompat, reading old indexes, remove for v3.0
propName.equals("index_analyzer") && parserContext.indexVersionCreated().before(Version.V_2_0_0_beta1)) {
propName.equals("index_analyzer") && indexVersionCreated.before(Version.V_2_0_0_beta1)) {

NamedAnalyzer analyzer = parserContext.analysisService().analyzer(propNode.toString());
if (analyzer == null) {
throw new MapperParsingException("analyzer [" + propNode.toString() + "] not found for field [" + name + "]");
Expand All @@ -270,10 +272,10 @@ public static void parseField(FieldMapper.Builder builder, String name, Map<Stri
} else if (propName.equals("include_in_all")) {
builder.includeInAll(nodeBooleanValue(propNode));
iterator.remove();
} else if (propName.equals("postings_format") && parserContext.indexVersionCreated().before(Version.V_2_0_0_beta1)) {
} else if (propName.equals("postings_format") && indexVersionCreated.before(Version.V_2_0_0_beta1)) {
// ignore for old indexes
iterator.remove();
} else if (propName.equals("doc_values_format") && parserContext.indexVersionCreated().before(Version.V_2_0_0_beta1)) {
} else if (propName.equals("doc_values_format") && indexVersionCreated.before(Version.V_2_0_0_beta1)) {
// ignore for old indexes
iterator.remove();
} else if (propName.equals("similarity")) {
Expand All @@ -284,14 +286,22 @@ public static void parseField(FieldMapper.Builder builder, String name, Map<Stri
builder.fieldDataSettings(settings);
iterator.remove();
} else if (propName.equals("copy_to")) {
if (parserContext.isWithinMultiField()) {
if (indexVersionCreated.after(Version.V_2_0_1)) {
throw new MapperParsingException("copy_to in multi fields is not allowed. Found the copy_to in field [" + name + "] which is within a multi field.");
} else {
ESLoggerFactory.getLogger("mapping [" + parserContext.type() + "]").warn("Found a copy_to in field [" + name + "] which is within a multi field. This feature has been removed and the copy_to will be ignored.");
// we still parse this, otherwise the message will only appear once and the copy_to removed. After that it will appear again. Better to have it always.
}
}
parseCopyFields(propNode, builder);
iterator.remove();
}
}

if (indexAnalyzer == null) {
if (searchAnalyzer != null) {
// If the index was created before 2.0 then we are trying to upgrade the mappings so use the default indexAnalyzer
// If the index was created before 2.0 then we are trying to upgrade the mappings so use the default indexAnalyzer
// instead of throwing an exception so the user is able to upgrade
if (parserContext.indexVersionCreated().before(Version.V_2_0_0_beta1)) {
indexAnalyzer = parserContext.analysisService().defaultIndexAnalyzer();
Expand All @@ -307,6 +317,7 @@ public static void parseField(FieldMapper.Builder builder, String name, Map<Stri
}

public static boolean parseMultiField(FieldMapper.Builder builder, String name, Mapper.TypeParser.ParserContext parserContext, String propName, Object propNode) {
parserContext = parserContext.createMultiFieldContext(parserContext);
if (propName.equals("path") && parserContext.indexVersionCreated().before(Version.V_2_0_0_beta1)) {
builder.multiFieldPathType(parsePathType(name, propNode.toString()));
return true;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/


package org.elasticsearch.index.mapper.core;

import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.index.mapper.MapperParsingException;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.VersionUtils;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.hamcrest.core.IsEqual.equalTo;

public class MultiFieldCopyToMapperTests extends ESTestCase {

public void testExceptionForCopyToInMultiFields() throws IOException {
XContentBuilder mapping = createMappinmgWithCopyToInMultiField();
Tuple<List<Version>, List<Version>> versionsWithAndWithoutExpectedExceptions = versionsWithAndWithoutExpectedExceptions();

// first check that for newer versions we throw exception if copy_to is found withing multi field
Version indexVersion = randomFrom(versionsWithAndWithoutExpectedExceptions.v1());
MapperService mapperService = MapperTestUtils.newMapperService(createTempDir(), Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, indexVersion).build());
try {
mapperService.parse("type", new CompressedXContent(mapping.string()), true);
fail("Parsing should throw an exception because the mapping contains a copy_to in a multi field");
} catch (MapperParsingException e) {
assertThat(e.getMessage(), equalTo("copy_to in multi fields is not allowed. Found the copy_to in field [c] which is within a multi field."));
}

// now test that with an older version the pasring just works
indexVersion = randomFrom(versionsWithAndWithoutExpectedExceptions.v2());
mapperService = MapperTestUtils.newMapperService(createTempDir(), Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, indexVersion).build());
mapperService.parse("type", new CompressedXContent(mapping.string()), true);
}

private static XContentBuilder createMappinmgWithCopyToInMultiField() throws IOException {
XContentBuilder mapping = jsonBuilder();
mapping.startObject()
.startObject("type")
.startObject("properties")
.startObject("a")
.field("type", "string")
.endObject()
.startObject("b")
.field("type", "string")
.startObject("fields")
.startObject("c")
.field("type", "string")
.field("copy_to", "a")
.endObject()
.endObject()
.endObject()
.endObject()
.endObject()
.endObject();
return mapping;
}

// returs a tuple where
// v1 is a list of versions for which we expect an excpetion when a copy_to in multi fields is found and
// v2 is older versions where we throw no exception and we just log a warning
private static Tuple<List<Version>, List<Version>> versionsWithAndWithoutExpectedExceptions() {
List<Version> versionsWithException = new ArrayList<>();
List<Version> versionsWithoutException = new ArrayList<>();
for (Version version : VersionUtils.allVersions()) {
if (version.after(Version.V_2_0_1)) {
versionsWithException.add(version);
} else {
versionsWithoutException.add(version);
}
}
return new Tuple<>(versionsWithException, versionsWithoutException);
}
}
8 changes: 8 additions & 0 deletions docs/reference/migration/migrate_2_0/mapping.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -429,3 +429,11 @@ to use the old default of 0. This was done to prevent phrase queries from
matching across different values of the same term unexpectedly. Specifically,
100 was chosen to cause phrase queries with slops up to 99 to match only within
a single value of a field.

==== copy_to and multi fields

A <<copy-to,copy_to>> within a <<multi-fields,multi field>> is ignored from version 2.0 on. With any version after
2.1 or 2.0.1 creating a mapping that has a copy_to within a multi field will result
in an exception.


0 comments on commit e240152

Please sign in to comment.