Skip to content

Commit

Permalink
Remove support for strict parsing mode
Browse files Browse the repository at this point in the history 
We return deprecation warnings as response headers, besides logging them. Strict parsing mode stayed around, but was only used in query tests, though we also introduced checks for deprecation warnings there that don't need strict parsing anymore (see elastic#20993).

 We can then safely remove support for strict parsing mode. The final goal is to remove the ParseFieldMatcher class, but there are many many users of it. This commit prepares the field for the removal, by deprecating ParseFieldMatcher and making it effectively not needed. Strict parsing is removed from ParseFieldMatcher, and strict parsing is replaced in tests where needed with deprecation warnings checks.

 Note that the setting to enable strict parsing was never ported to the new settings infra hance it cannot be set in production. It is really only used in our own tests.

 Relates to elastic#19552
  • Loading branch information
@javanna
javanna committed Dec 19, 2016
1 parent 63af03a commit ae4caff
Show file tree
Hide file tree
Showing 21 changed files with 172 additions and 230 deletions.
12 changes: 2 additions & 10 deletions core/src/main/java/org/elasticsearch/common/ParseField.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,10 @@ public ParseField withAllDeprecated(String allReplacedWith) {
/**
* @param fieldName
* the field name to match against this {@link ParseField}
* @param strict
* if true an exception will be thrown if a deprecated field name
* is given. If false the deprecated name will be matched but a
* message will also be logged to the {@link DeprecationLogger}
* @return true if <code>fieldName</code> matches any of the acceptable
* names for this {@link ParseField}.
*/
boolean match(String fieldName, boolean strict) {
public boolean match(String fieldName) {
Objects.requireNonNull(fieldName, "fieldName cannot be null");
// if this parse field has not been completely deprecated then try to
// match the preferred name
Expand All @@ -128,11 +124,7 @@ boolean match(String fieldName, boolean strict) {
// message to indicate what should be used instead
msg = "Deprecated field [" + fieldName + "] used, replaced by [" + allReplacedWith + "]";
}
if (strict) {
throw new IllegalArgumentException(msg);
} else {
DEPRECATION_LOGGER.deprecated(msg);
}
DEPRECATION_LOGGER.deprecated(msg);
return true;
}
}
Expand Down
31 changes: 11 additions & 20 deletions core/src/main/java/org/elasticsearch/common/ParseFieldMatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,38 +22,29 @@
import org.elasticsearch.common.settings.Settings;

/**
* 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 #PARSE_STRICT} setting.
* Matcher to use in combination with {@link ParseField} while parsing requests.
*
* @deprecated This class used to be useful to parse in strict mode and emit errors rather than deprecation warnings. Now that we return
* warnings as response headers all the time, it is no longer useful and will soon be removed. The removal is in progress and there is
* already no strict mode in fact. Use {@link ParseField} directly.
*/
@Deprecated
public class ParseFieldMatcher {
public static final String PARSE_STRICT = "index.query.parse.strict";
public static final ParseFieldMatcher EMPTY = new ParseFieldMatcher(false);
public static final ParseFieldMatcher STRICT = new ParseFieldMatcher(true);

private final boolean strict;
public static final ParseFieldMatcher EMPTY = new ParseFieldMatcher(Settings.EMPTY);
public static final ParseFieldMatcher STRICT = new ParseFieldMatcher(Settings.EMPTY);

public ParseFieldMatcher(Settings settings) {
this(settings.getAsBoolean(PARSE_STRICT, false));
}

public ParseFieldMatcher(boolean strict) {
this.strict = strict;
}

/** Should deprecated settings be rejected? */
public boolean isStrict() {
return strict;
//we don't do anything with the settings argument, this whole class will be soon removed
}

/**
* Matches a {@link ParseField} against a field name, and throws deprecation exception depending on the current
* value of the {@link #PARSE_STRICT} setting.
* Matches a {@link ParseField} against a field name,
* @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, strict);
return parseField.match(fieldName);
}
}
51 changes: 18 additions & 33 deletions core/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import org.elasticsearch.common.joda.FormatDateTimeFormatter;
import org.elasticsearch.common.joda.Joda;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.ESLoggerFactory;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.index.analysis.NamedAnalyzer;
Expand All @@ -43,7 +42,6 @@
import static org.elasticsearch.common.xcontent.support.XContentMapValues.isArray;
import static org.elasticsearch.common.xcontent.support.XContentMapValues.lenientNodeBooleanValue;
import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeFloatValue;
import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeIntegerValue;
import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeMapValue;
import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeStringValue;

Expand All @@ -59,16 +57,11 @@ public class TypeParsers {
private static final Set<String> BOOLEAN_STRINGS = new HashSet<>(Arrays.asList("true", "false"));

public static boolean nodeBooleanValue(String name, Object node, Mapper.TypeParser.ParserContext parserContext) {
// Hook onto ParseFieldMatcher so that parsing becomes strict when setting index.query.parse.strict
if (parserContext.parseFieldMatcher().isStrict()) {
return XContentMapValues.nodeBooleanValue(node);
} else {
// TODO: remove this leniency in 6.0
if (BOOLEAN_STRINGS.contains(node.toString()) == false) {
DEPRECATION_LOGGER.deprecated("Expected a boolean for property [{}] but got [{}]", name, node);
}
return XContentMapValues.lenientNodeBooleanValue(node);
// TODO: remove this leniency in 6.0
if (BOOLEAN_STRINGS.contains(node.toString()) == false) {
DEPRECATION_LOGGER.deprecated("Expected a boolean for property [{}] but got [{}]", name, node);
}
return XContentMapValues.lenientNodeBooleanValue(node);
}

private static void parseAnalyzersAndTermVectors(FieldMapper.Builder builder, String name, Map<String, Object> fieldNode, Mapper.TypeParser.ParserContext parserContext) {
Expand Down Expand Up @@ -211,10 +204,10 @@ public static void parseField(FieldMapper.Builder builder, String name, Map<Stri
throw new MapperParsingException("[" + propName + "] must not have a [null] value");
}
if (propName.equals("store")) {
builder.store(parseStore(name, propNode.toString(), parserContext));
builder.store(parseStore(propNode.toString()));
iterator.remove();
} else if (propName.equals("index")) {
builder.index(parseIndex(name, propNode.toString(), parserContext));
builder.index(parseIndex(name, propNode.toString()));
iterator.remove();
} else if (propName.equals(DOC_VALUES)) {
builder.docValues(nodeBooleanValue(DOC_VALUES, propNode, parserContext));
Expand Down Expand Up @@ -346,7 +339,7 @@ public static void parseTermVector(String fieldName, String termVector, FieldMap
}
}

public static boolean parseIndex(String fieldName, String index, Mapper.TypeParser.ParserContext parserContext) throws MapperParsingException {
private static boolean parseIndex(String fieldName, String index) throws MapperParsingException {
switch (index) {
case "true":
return true;
Expand All @@ -355,31 +348,23 @@ public static boolean parseIndex(String fieldName, String index, Mapper.TypePars
case "not_analyzed":
case "analyzed":
case "no":
if (parserContext.parseFieldMatcher().isStrict() == false) {
DEPRECATION_LOGGER.deprecated("Expected a boolean for property [index] but got [{}]", index);
return "no".equals(index) == false;
} else {
throw new IllegalArgumentException("Can't parse [index] value [" + index + "] for field [" + fieldName + "], expected [true] or [false]");
}
DEPRECATION_LOGGER.deprecated("Expected a boolean for property [index] but got [{}]", index);
return "no".equals(index) == false;
default:
throw new IllegalArgumentException("Can't parse [index] value [" + index + "] for field [" + fieldName + "], expected [true] or [false]");
}
}

public static boolean parseStore(String fieldName, String store, Mapper.TypeParser.ParserContext parserContext) throws MapperParsingException {
if (parserContext.parseFieldMatcher().isStrict()) {
return XContentMapValues.nodeBooleanValue(store);
private static boolean parseStore(String store) throws MapperParsingException {
if (BOOLEAN_STRINGS.contains(store) == false) {
DEPRECATION_LOGGER.deprecated("Expected a boolean for property [store] but got [{}]", store);
}
if ("no".equals(store)) {
return false;
} else if ("yes".equals(store)) {
return true;
} else {
if (BOOLEAN_STRINGS.contains(store) == false) {
DEPRECATION_LOGGER.deprecated("Expected a boolean for property [store] but got [{}]", store);
}
if ("no".equals(store)) {
return false;
} else if ("yes".equals(store)) {
return true;
} else {
return lenientNodeBooleanValue(store);
}
return lenientNodeBooleanValue(store);
}
}

Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ public class GeoBoundingBoxQueryBuilder extends AbstractQueryBuilder<GeoBounding
private static final ParseField TYPE_FIELD = new ParseField("type");
private static final ParseField VALIDATION_METHOD_FIELD = new ParseField("validation_method");
private static final ParseField COERCE_FIELD =new ParseField("coerce", "normalize")
.withAllDeprecated("use field validation_method instead");
.withAllDeprecated("validation_method");
private static final ParseField IGNORE_MALFORMED_FIELD = new ParseField("ignore_malformed")
.withAllDeprecated("use field validation_method instead");
.withAllDeprecated("validation_method");
private static final ParseField FIELD_FIELD = new ParseField("field");
private static final ParseField TOP_FIELD = new ParseField("top");
private static final ParseField BOTTOM_FIELD = new ParseField("bottom");
Expand Down
6 changes: 2 additions & 4 deletions core/src/main/java/org/elasticsearch/index/query/GeoDistanceQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,8 @@ public class GeoDistanceQueryBuilder extends AbstractQueryBuilder<GeoDistanceQue
public static final boolean DEFAULT_IGNORE_UNMAPPED = false;

private static final ParseField VALIDATION_METHOD_FIELD = new ParseField("validation_method");
private static final ParseField IGNORE_MALFORMED_FIELD = new ParseField("ignore_malformed")
.withAllDeprecated("use validation_method instead");
private static final ParseField COERCE_FIELD = new ParseField("coerce", "normalize")
.withAllDeprecated("use validation_method instead");
private static final ParseField IGNORE_MALFORMED_FIELD = new ParseField("ignore_malformed").withAllDeprecated("validation_method");
private static final ParseField COERCE_FIELD = new ParseField("coerce", "normalize").withAllDeprecated("validation_method");
@Deprecated
private static final ParseField OPTIMIZE_BBOX_FIELD = new ParseField("optimize_bbox")
.withAllDeprecated("no replacement: `optimize_bbox` is no longer supported due to recent improvements");
Expand Down
6 changes: 2 additions & 4 deletions core/src/main/java/org/elasticsearch/index/query/GeoPolygonQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,8 @@ public class GeoPolygonQueryBuilder extends AbstractQueryBuilder<GeoPolygonQuery
*/
public static final boolean DEFAULT_IGNORE_UNMAPPED = false;

private static final ParseField COERCE_FIELD = new ParseField("coerce", "normalize")
.withAllDeprecated("use validation_method instead");
private static final ParseField IGNORE_MALFORMED_FIELD = new ParseField("ignore_malformed")
.withAllDeprecated("use validation_method instead");
private static final ParseField COERCE_FIELD = new ParseField("coerce", "normalize").withAllDeprecated("validation_method");
private static final ParseField IGNORE_MALFORMED_FIELD = new ParseField("ignore_malformed").withAllDeprecated("validation_method");
private static final ParseField VALIDATION_METHOD = new ParseField("validation_method");
private static final ParseField POINTS_FIELD = new ParseField("points");
private static final ParseField IGNORE_UNMAPPED_FIELD = new ParseField("ignore_unmapped");
Expand Down
6 changes: 2 additions & 4 deletions core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,8 @@ public class GeoDistanceSortBuilder extends SortBuilder<GeoDistanceSortBuilder>
private static final ParseField UNIT_FIELD = new ParseField("unit");
private static final ParseField DISTANCE_TYPE_FIELD = new ParseField("distance_type");
private static final ParseField VALIDATION_METHOD_FIELD = new ParseField("validation_method");
private static final ParseField IGNORE_MALFORMED_FIELD = new ParseField("ignore_malformed")
.withAllDeprecated("use validation_method instead");
private static final ParseField COERCE_FIELD = new ParseField("coerce", "normalize")
.withAllDeprecated("use validation_method instead");
private static final ParseField IGNORE_MALFORMED_FIELD = new ParseField("ignore_malformed").withAllDeprecated("validation_method");
private static final ParseField COERCE_FIELD = new ParseField("coerce", "normalize").withAllDeprecated("validation_method");
private static final ParseField SORTMODE_FIELD = new ParseField("mode", "sort_mode");

private final String fieldName;
Expand Down
38 changes: 8 additions & 30 deletions core/src/test/java/org/elasticsearch/common/ParseFieldTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

import org.elasticsearch.test.ESTestCase;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.CoreMatchers.sameInstance;
Expand All @@ -33,32 +32,16 @@ public void testParse() {
String[] deprecated = new String[]{"barFoo", "bar_foo", "Foobar"};
ParseField withDeprecations = field.withDeprecation(deprecated);
assertThat(field, not(sameInstance(withDeprecations)));
assertThat(field.match(name, false), is(true));
assertThat(field.match("foo bar", false), is(false));
assertThat(field.match(name), is(true));
assertThat(field.match("foo bar"), is(false));
for (String deprecatedName : deprecated) {
assertThat(field.match(deprecatedName, false), is(false));
assertThat(field.match(deprecatedName), is(false));
}

assertThat(withDeprecations.match(name, false), is(true));
assertThat(withDeprecations.match("foo bar", false), is(false));
assertThat(withDeprecations.match(name), is(true));
assertThat(withDeprecations.match("foo bar"), is(false));
for (String deprecatedName : deprecated) {
assertThat(withDeprecations.match(deprecatedName, false), is(true));
}

// now with strict mode
assertThat(field.match(name, true), is(true));
assertThat(field.match("foo bar", true), is(false));
for (String deprecatedName : deprecated) {
assertThat(field.match(deprecatedName, true), is(false));
}

assertThat(withDeprecations.match(name, true), is(true));
assertThat(withDeprecations.match("foo bar", true), is(false));
for (String deprecatedName : deprecated) {
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
withDeprecations.match(deprecatedName, true);
});
assertThat(e.getMessage(), containsString("used, expected [foo_bar] instead"));
assertThat(withDeprecations.match(deprecatedName), is(true));
}
}

Expand All @@ -84,13 +67,8 @@ public void testAllDeprecated() {
field = new ParseField(name).withAllDeprecated("like");
}

// strict mode off
assertThat(field.match(randomFrom(allValues), false), is(true));
assertThat(field.match("not a field name", false), is(false));

// now with strict mode
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> field.match(randomFrom(allValues), true));
assertThat(e.getMessage(), containsString(" used, replaced by [like]"));
assertThat(field.match(randomFrom(allValues)), is(true));
assertThat(field.match("not a field name"), is(false));
}

public void testGetAllNamesIncludedDeprecated() {
Expand Down
39 changes: 22 additions & 17 deletions core/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
import org.elasticsearch.common.ParseFieldMatcher;
import org.elasticsearch.common.ParseFieldMatcherSupplier;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.AbstractObjectParser.NoContextParser;
import org.elasticsearch.common.xcontent.ObjectParser.NamedObjectParser;
import org.elasticsearch.common.xcontent.ObjectParser.ValueType;
Expand All @@ -35,6 +38,8 @@
import java.util.Arrays;
import java.util.List;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.hasSize;

public class ObjectParserTests extends ESTestCase {
Expand Down Expand Up @@ -218,27 +223,27 @@ public void setTest(int test) {
}
}

public void testDeprecationFail() throws IOException {
XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"old_test\" : \"foo\"}");
class TestStruct {
public String test;
}
ObjectParser<TestStruct, ParseFieldMatcherSupplier> objectParser = new ObjectParser<>("foo");
TestStruct s = new TestStruct();
public void testDeprecationWarnings() throws IOException {
try (ThreadContext threadContext = new ThreadContext(Settings.EMPTY)) {
DeprecationLogger.setThreadContext(threadContext);
class TestStruct {
public String test;
}
ObjectParser<TestStruct, ParseFieldMatcherSupplier> objectParser = new ObjectParser<>("foo");
TestStruct s = new TestStruct();

objectParser.declareField((i, v, c) -> v.test = i.text(), new ParseField("test", "old_test"), ObjectParser.ValueType.STRING);
objectParser.declareField((i, v, c) -> v.test = i.text(), new ParseField("test", "old_test"), ObjectParser.ValueType.STRING);

try {
objectParser.parse(parser, s, STRICT_PARSING);
fail("deprecated value");
} catch (IllegalArgumentException ex) {
assertEquals(ex.getMessage(), "Deprecated field [old_test] used, expected [test] instead");
XContentParser parser = createParser(XContentType.JSON.xContent(), "{\"old_test\" : \"foo\"}");
objectParser.parse(parser, s, () -> ParseFieldMatcher.EMPTY);

assertEquals("foo", s.test);

final List<String> warnings = threadContext.getResponseHeaders().get(DeprecationLogger.DEPRECATION_HEADER);
assertThat(warnings, hasSize(1));
assertThat(warnings, hasItem(equalTo("Deprecated field [old_test] used, expected [test] instead")));
DeprecationLogger.removeThreadContext(threadContext);
}
assertNull(s.test);
parser = createParser(JsonXContent.jsonXContent, "{\"old_test\" : \"foo\"}");
objectParser.parse(parser, s, () -> ParseFieldMatcher.EMPTY);
assertEquals("foo", s.test);
}

public void testFailOnValueType() throws IOException {
Expand Down
Loading

0 comments on commit ae4caff

Please sign in to comment.