Skip to content

Commit

Permalink
Internal: make sure ParseField is always used in combination with par…
Browse files Browse the repository at this point in the history
…se flags

Removed ParseField#match variant that accepts the field name only, without parse flags. Such a method is harmful as it defaults to empty parse flags, meaning that no deprecation exceptions will be thrown in strict mode, which defeats the purpose of using ParseField. Unfortunately such a method was used in a lot of places were the parse flags weren't easily accessible (outside of query parsing), and in a lot of other places just by mistake.

Parse flags have been introduced now as part of SearchContext and mappers where needed. There are a few places (e.g. java api requests) where it is not possible to retrieve them as they depend on the index settings, in that case we explicitly pass in EMPTY_FLAGS for now, but this has to be seen as an exception.

Closes #11859
  • Loading branch information
javanna committed Jul 2, 2015
1 parent 72d9914 commit c7887f4
Show file tree
Hide file tree
Showing 97 changed files with 593 additions and 608 deletions.
Expand Up @@ -168,7 +168,7 @@ protected ShardValidateQueryResponse shardOperation(ShardValidateQueryRequest re
DefaultSearchContext searchContext = new DefaultSearchContext(0,
new ShardSearchLocalRequest(request.types(), request.nowInMillis(), request.filteringAliases()),
null, searcher, indexService, indexShard,
scriptService, pageCacheRecycler, bigArrays, threadPool.estimatedTimeInMillisCounter()
scriptService, pageCacheRecycler, bigArrays, threadPool.estimatedTimeInMillisCounter(), parseFieldMatcher
);
SearchContext.setCurrent(searchContext);
try {
Expand All @@ -187,10 +187,7 @@ protected ShardValidateQueryResponse shardOperation(ShardValidateQueryRequest re
} catch (QueryParsingException e) {
valid = false;
error = e.getDetailedMessage();
} catch (AssertionError e) {
valid = false;
error = e.getMessage();
} catch (IOException e) {
} catch (AssertionError|IOException e) {
valid = false;
error = e.getMessage();
} finally {
Expand Down
Expand Up @@ -151,7 +151,7 @@ protected ShardExistsResponse shardOperation(ShardExistsRequest request) {
SearchContext context = new DefaultSearchContext(0,
new ShardSearchLocalRequest(request.types(), request.nowInMillis(), request.filteringAliases()),
shardTarget, indexShard.acquireSearcher("exists"), indexService, indexShard,
scriptService, pageCacheRecycler, bigArrays, threadPool.estimatedTimeInMillisCounter());
scriptService, pageCacheRecycler, bigArrays, threadPool.estimatedTimeInMillisCounter(), parseFieldMatcher);
SearchContext.setCurrent(context);

try {
Expand Down
Expand Up @@ -113,7 +113,7 @@ protected ExplainResponse shardOperation(ExplainRequest request, ShardId shardId
0, new ShardSearchLocalRequest(new String[]{request.type()}, request.nowInMillis, request.filteringAlias()),
null, result.searcher(), indexService, indexShard,
scriptService, pageCacheRecycler,
bigArrays, threadPool.estimatedTimeInMillisCounter()
bigArrays, threadPool.estimatedTimeInMillisCounter(), parseFieldMatcher
);
SearchContext.setCurrent(context);

Expand Down
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.client.Requests;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParseFieldMatcher;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
Expand Down Expand Up @@ -239,7 +240,7 @@ public SearchRequest searchType(SearchType searchType) {
* "query_then_fetch"/"queryThenFetch", and "query_and_fetch"/"queryAndFetch".
*/
public SearchRequest searchType(String searchType) {
return searchType(SearchType.fromString(searchType));
return searchType(SearchType.fromString(searchType, ParseFieldMatcher.EMPTY));
}

/**
Expand Down
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.action.search;

import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.ParseFieldMatcher;

/**
* Search type represent the manner at which the search operation is executed.
Expand Down Expand Up @@ -108,7 +109,7 @@ public static SearchType fromId(byte id) {
* one of "dfs_query_then_fetch"/"dfsQueryThenFetch", "dfs_query_and_fetch"/"dfsQueryAndFetch",
* "query_then_fetch"/"queryThenFetch", "query_and_fetch"/"queryAndFetch", and "scan".
*/
public static SearchType fromString(String searchType) {
public static SearchType fromString(String searchType, ParseFieldMatcher parseFieldMatcher) {
if (searchType == null) {
return SearchType.DEFAULT;
}
Expand All @@ -122,7 +123,7 @@ public static SearchType fromString(String searchType) {
return SearchType.QUERY_AND_FETCH;
} else if ("scan".equals(searchType)) {
return SearchType.SCAN;
} else if (COUNT_VALUE.match(searchType)) {
} else if (parseFieldMatcher.match(searchType, COUNT_VALUE)) {
return SearchType.COUNT;
} else {
throw new IllegalArgumentException("No search type for [" + searchType + "]");
Expand Down
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.action.support;

import org.elasticsearch.action.*;
import org.elasticsearch.common.ParseFieldMatcher;
import org.elasticsearch.common.component.AbstractComponent;
import org.elasticsearch.common.logging.ESLogger;
import org.elasticsearch.common.settings.Settings;
Expand All @@ -37,9 +38,11 @@ public abstract class TransportAction<Request extends ActionRequest, Response ex
protected final ThreadPool threadPool;
protected final String actionName;
private final ActionFilter[] filters;
protected final ParseFieldMatcher parseFieldMatcher;

protected TransportAction(Settings settings, String actionName, ThreadPool threadPool, ActionFilters actionFilters) {
super(settings);
this.parseFieldMatcher = new ParseFieldMatcher(settings);
this.actionName = actionName;
this.filters = actionFilters.filters();
this.threadPool = threadPool;
Expand Down
Expand Up @@ -25,6 +25,7 @@
import org.elasticsearch.action.index.IndexRequest;
import org.elasticsearch.action.support.single.instance.InstanceShardOperationRequest;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParseFieldMatcher;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.StreamInput;
Expand Down Expand Up @@ -291,13 +292,13 @@ public String scriptLang() {
public UpdateRequest addScriptParam(String name, Object value) {
Script script = script();
if (script == null) {
HashMap<String, Object> scriptParams = new HashMap<String, Object>();
HashMap<String, Object> scriptParams = new HashMap<>();
scriptParams.put(name, value);
updateOrCreateScript(null, null, null, scriptParams);
} else {
Map<String, Object> scriptParams = script.getParams();
if (scriptParams == null) {
scriptParams = new HashMap<String, Object>();
scriptParams = new HashMap<>();
scriptParams.put(name, value);
updateOrCreateScript(null, null, null, scriptParams);
} else {
Expand Down Expand Up @@ -648,7 +649,8 @@ public UpdateRequest source(BytesReference source) throws Exception {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
} else if ("script".equals(currentFieldName) && token == XContentParser.Token.START_OBJECT) {
script = Script.parse(parser);
//here we don't have settings available, unable to throw strict deprecation exceptions
script = Script.parse(parser, ParseFieldMatcher.EMPTY);
} else if ("params".equals(currentFieldName)) {
scriptParams = parser.map();
} else if ("scripted_upsert".equals(currentFieldName)) {
Expand All @@ -666,7 +668,8 @@ public UpdateRequest source(BytesReference source) throws Exception {
} else if ("detect_noop".equals(currentFieldName)) {
detectNoop(parser.booleanValue());
} else {
scriptParameterParser.token(currentFieldName, token, parser);
//here we don't have settings available, unable to throw deprecation exceptions
scriptParameterParser.token(currentFieldName, token, parser, ParseFieldMatcher.EMPTY);
}
}
// Don't have a script using the new API so see if it is specified with the old API
Expand Down
14 changes: 6 additions & 8 deletions core/src/main/java/org/elasticsearch/common/ParseField.java
Expand Up @@ -23,16 +23,18 @@
import java.util.HashSet;

/**
* Holds a field that can be found in a request while parsing and its different variants, which may be deprecated.
*/
public class ParseField {
private final String camelCaseName;
private final String underscoreName;
private final String[] deprecatedNames;
private String allReplacedWith = null;

public static final EnumSet<Flag> EMPTY_FLAGS = EnumSet.noneOf(Flag.class);
static final EnumSet<Flag> EMPTY_FLAGS = EnumSet.noneOf(Flag.class);
static final EnumSet<Flag> STRICT_FLAGS = EnumSet.of(Flag.STRICT);

public static enum Flag {
enum Flag {
STRICT
}

Expand All @@ -47,7 +49,7 @@ public ParseField(String value, String... deprecatedNames) {
set.add(Strings.toCamelCase(depName));
set.add(Strings.toUnderscoreCase(depName));
}
this.deprecatedNames = set.toArray(new String[0]);
this.deprecatedNames = set.toArray(new String[set.size()]);
}
}

Expand Down Expand Up @@ -78,11 +80,7 @@ public ParseField withAllDeprecated(String allReplacedWith) {
return parseField;
}

public boolean match(String currentFieldName) {
return match(currentFieldName, EMPTY_FLAGS);
}

public boolean match(String currentFieldName, EnumSet<Flag> flags) {
boolean match(String currentFieldName, EnumSet<Flag> flags) {
if (allReplacedWith == null && (currentFieldName.equals(camelCaseName) || currentFieldName.equals(underscoreName))) {
return true;
}
Expand Down
61 changes: 61 additions & 0 deletions core/src/main/java/org/elasticsearch/common/ParseFieldMatcher.java
@@ -0,0 +1,61 @@
/*
* 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.common;

import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.query.IndexQueryParserService;

import java.util.EnumSet;

/**
* Matcher to use in combination with {@link ParseField} while parsing requests. Matches a {@link ParseField}
* against a field name and throw deprecation exception depending on the current value of the {@link IndexQueryParserService#PARSE_STRICT} setting.
*/
public class ParseFieldMatcher {

public static final ParseFieldMatcher EMPTY = new ParseFieldMatcher(ParseField.EMPTY_FLAGS);
public static final ParseFieldMatcher STRICT = new ParseFieldMatcher(ParseField.STRICT_FLAGS);

private final EnumSet<ParseField.Flag> parseFlags;

public ParseFieldMatcher(Settings settings) {
if (settings.getAsBoolean(IndexQueryParserService.PARSE_STRICT, false)) {
this.parseFlags = EnumSet.of(ParseField.Flag.STRICT);
} else {
this.parseFlags = ParseField.EMPTY_FLAGS;
}
}

public ParseFieldMatcher(EnumSet<ParseField.Flag> parseFlags) {
this.parseFlags = parseFlags;
}

/**
* Matches a {@link ParseField} against a field name, and throws deprecation exception depending on the current
* value of the {@link IndexQueryParserService#PARSE_STRICT} setting.
* @param fieldName the field name found in the request while parsing
* @param parseField the parse field that we are looking for
* @throws IllegalArgumentException whenever we are in strict mode and the request contained a deprecated field
* @return true whenever the parse field that we are looking for was found, false otherwise
*/
public boolean match(String fieldName, ParseField parseField) {
return parseField.match(fieldName, parseFlags);
}
}
Expand Up @@ -21,9 +21,9 @@

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;

import org.elasticsearch.Version;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParseFieldMatcher;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.MapBuilder;
import org.elasticsearch.common.collect.Tuple;
Expand All @@ -38,35 +38,10 @@
import org.elasticsearch.index.AbstractIndexComponent;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.analysis.AnalysisService;
import org.elasticsearch.index.mapper.core.BinaryFieldMapper;
import org.elasticsearch.index.mapper.core.BooleanFieldMapper;
import org.elasticsearch.index.mapper.core.ByteFieldMapper;
import org.elasticsearch.index.mapper.core.CompletionFieldMapper;
import org.elasticsearch.index.mapper.core.DateFieldMapper;
import org.elasticsearch.index.mapper.core.DoubleFieldMapper;
import org.elasticsearch.index.mapper.core.FloatFieldMapper;
import org.elasticsearch.index.mapper.core.IntegerFieldMapper;
import org.elasticsearch.index.mapper.core.LongFieldMapper;
import org.elasticsearch.index.mapper.core.Murmur3FieldMapper;
import org.elasticsearch.index.mapper.core.ShortFieldMapper;
import org.elasticsearch.index.mapper.core.StringFieldMapper;
import org.elasticsearch.index.mapper.core.TokenCountFieldMapper;
import org.elasticsearch.index.mapper.core.TypeParsers;
import org.elasticsearch.index.mapper.core.*;
import org.elasticsearch.index.mapper.geo.GeoPointFieldMapper;
import org.elasticsearch.index.mapper.geo.GeoShapeFieldMapper;
import org.elasticsearch.index.mapper.internal.AllFieldMapper;
import org.elasticsearch.index.mapper.internal.FieldNamesFieldMapper;
import org.elasticsearch.index.mapper.internal.IdFieldMapper;
import org.elasticsearch.index.mapper.internal.IndexFieldMapper;
import org.elasticsearch.index.mapper.internal.ParentFieldMapper;
import org.elasticsearch.index.mapper.internal.RoutingFieldMapper;
import org.elasticsearch.index.mapper.internal.SizeFieldMapper;
import org.elasticsearch.index.mapper.internal.SourceFieldMapper;
import org.elasticsearch.index.mapper.internal.TTLFieldMapper;
import org.elasticsearch.index.mapper.internal.TimestampFieldMapper;
import org.elasticsearch.index.mapper.internal.TypeFieldMapper;
import org.elasticsearch.index.mapper.internal.UidFieldMapper;
import org.elasticsearch.index.mapper.internal.VersionFieldMapper;
import org.elasticsearch.index.mapper.internal.*;
import org.elasticsearch.index.mapper.ip.IpFieldMapper;
import org.elasticsearch.index.mapper.object.ObjectMapper;
import org.elasticsearch.index.mapper.object.RootObjectMapper;
Expand Down Expand Up @@ -96,13 +71,15 @@ public class DocumentMapperParser extends AbstractIndexComponent {

private final Object typeParsersMutex = new Object();
private final Version indexVersionCreated;
private final ParseFieldMatcher parseFieldMatcher;

private volatile ImmutableMap<String, Mapper.TypeParser> typeParsers;
private volatile ImmutableMap<String, Mapper.TypeParser> rootTypeParsers;

public DocumentMapperParser(Index index, @IndexSettings Settings indexSettings, MapperService mapperService, AnalysisService analysisService,
SimilarityLookupService similarityLookupService, ScriptService scriptService) {
super(index, indexSettings);
this.parseFieldMatcher = new ParseFieldMatcher(indexSettings);
this.mapperService = mapperService;
this.analysisService = analysisService;
this.similarityLookupService = similarityLookupService;
Expand Down Expand Up @@ -168,7 +145,7 @@ public void putRootTypeParser(String type, Mapper.TypeParser typeParser) {
}

public Mapper.TypeParser.ParserContext parserContext() {
return new Mapper.TypeParser.ParserContext(analysisService, similarityLookupService, mapperService, typeParsers, indexVersionCreated);
return new Mapper.TypeParser.ParserContext(analysisService, similarityLookupService, mapperService, typeParsers, indexVersionCreated, parseFieldMatcher);
}

public DocumentMapper parse(String source) throws MapperParsingException {
Expand Down Expand Up @@ -296,7 +273,7 @@ private static String getRemainingFields(Map<String, ?> map) {
}

private void parseTransform(DocumentMapper.Builder docBuilder, Map<String, Object> transformConfig, Version indexVersionCreated) {
Script script = Script.parse(transformConfig, true);
Script script = Script.parse(transformConfig, true, parseFieldMatcher);
if (script != null) {
docBuilder.transform(scriptService, script);
}
Expand Down
12 changes: 10 additions & 2 deletions core/src/main/java/org/elasticsearch/index/mapper/Mapper.java
Expand Up @@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableMap;
import org.elasticsearch.Version;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParseFieldMatcher;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.ToXContent;
Expand Down Expand Up @@ -92,14 +93,17 @@ class ParserContext {

private final Version indexVersionCreated;

private final ParseFieldMatcher parseFieldMatcher;

public ParserContext(AnalysisService analysisService, SimilarityLookupService similarityLookupService,
MapperService mapperService,
ImmutableMap<String, TypeParser> typeParsers, Version indexVersionCreated) {
MapperService mapperService, ImmutableMap<String, TypeParser> typeParsers,
Version indexVersionCreated, ParseFieldMatcher parseFieldMatcher) {
this.analysisService = analysisService;
this.similarityLookupService = similarityLookupService;
this.mapperService = mapperService;
this.typeParsers = typeParsers;
this.indexVersionCreated = indexVersionCreated;
this.parseFieldMatcher = parseFieldMatcher;
}

public AnalysisService analysisService() {
Expand All @@ -121,6 +125,10 @@ public TypeParser typeParser(String type) {
public Version indexVersionCreated() {
return indexVersionCreated;
}

public ParseFieldMatcher parseFieldMatcher() {
return parseFieldMatcher;
}
}

Mapper.Builder<?,?> parse(String name, Map<String, Object> node, ParserContext parserContext) throws MapperParsingException;
Expand Down
Expand Up @@ -21,8 +21,6 @@

import com.carrotsearch.hppc.ObjectArrayList;
import org.apache.lucene.document.Field;
import org.apache.lucene.document.FieldType;
import org.apache.lucene.index.DocValuesType;
import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.store.ByteArrayDataOutput;
import org.apache.lucene.util.BytesRef;
Expand All @@ -35,7 +33,6 @@
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.compress.CompressorFactory;
import org.elasticsearch.common.compress.NotXContentException;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.CollectionUtils;
import org.elasticsearch.common.xcontent.XContentParser;
Expand Down Expand Up @@ -98,7 +95,7 @@ public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext
Map.Entry<String, Object> entry = iterator.next();
String fieldName = entry.getKey();
if (parserContext.indexVersionCreated().before(Version.V_2_0_0) &&
(COMPRESS.match(fieldName) || COMPRESS_THRESHOLD.match(fieldName))) {
(parserContext.parseFieldMatcher().match(fieldName, COMPRESS) || parserContext.parseFieldMatcher().match(fieldName, COMPRESS_THRESHOLD))) {
iterator.remove();
}
}
Expand Down

0 comments on commit c7887f4

Please sign in to comment.