Skip to content

Replace strict parsing mode with response headers assertions #22130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions core/src/main/java/org/elasticsearch/common/ParseField.java
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
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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.common.SuppressLoggerChecks;
import org.elasticsearch.common.util.concurrent.ThreadContext;

Expand All @@ -42,7 +41,7 @@ public class DeprecationLogger {
*
* https://tools.ietf.org/html/rfc7234#section-5.5
*/
public static final String DEPRECATION_HEADER = "Warning";
public static final String WARNING_HEADER = "Warning";

/**
* This is set once by the {@code Node} constructor, but it uses {@link CopyOnWriteArraySet} to ensure that tests can run in parallel.
Expand Down Expand Up @@ -128,7 +127,7 @@ void deprecated(Set<ThreadContext> threadContexts, String message, Object... par

while (iterator.hasNext()) {
try {
iterator.next().addResponseHeader(DEPRECATION_HEADER, formattedMessage);
iterator.next().addResponseHeader(WARNING_HEADER, formattedMessage);
} catch (IllegalStateException e) {
// ignored; it should be removed shortly
}
Expand Down
51 changes: 18 additions & 33 deletions core/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java
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
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,14 @@ public void testBeatsTemplatesBWC() throws Exception {
client().prepareIndex("packetbeat-foo", "doc", "1").setSource("message", "foo").get();
client().prepareIndex("filebeat-foo", "doc", "1").setSource("message", "foo").get();
client().prepareIndex("winlogbeat-foo", "doc", "1").setSource("message", "foo").get();
assertWarnings("Deprecated field [template] used, replaced by [index_patterns]");
}

public void testLogstashTemplatesBWC() throws Exception {
String ls5x = copyToStringFromClasspath("/org/elasticsearch/action/admin/indices/template/logstash-5.0.template.json");
client().admin().indices().preparePutTemplate("logstash-5x").setSource(ls5x).get();
client().prepareIndex("logstash-foo", "doc", "1").setSource("message", "foo").get();
assertWarnings("Deprecated field [template] used, replaced by [index_patterns]");
}

}
70 changes: 19 additions & 51 deletions core/src/test/java/org/elasticsearch/common/ParseFieldTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,77 +20,45 @@

import org.elasticsearch.test.ESTestCase;

import static org.hamcrest.CoreMatchers.containsString;
import java.io.IOException;

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.CoreMatchers.sameInstance;
import static org.hamcrest.collection.IsArrayContainingInAnyOrder.arrayContainingInAnyOrder;

public class ParseFieldTests extends ESTestCase {
public void testParse() {
public void testParse() throws IOException {
String name = "foo_bar";
ParseField field = new ParseField(name);
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));
for (String deprecatedName : deprecated) {
assertThat(field.match(deprecatedName, false), is(false));
}

assertThat(withDeprecations.match(name, false), is(true));
assertThat(withDeprecations.match("foo bar", false), 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));
assertThat(field.match(name), is(true));
assertThat(field.match("foo bar"), is(false));
for (String deprecatedName : deprecated) {
assertThat(field.match(deprecatedName, true), is(false));
assertThat(field.match(deprecatedName), is(false));
}

assertThat(withDeprecations.match(name, true), is(true));
assertThat(withDeprecations.match("foo bar", true), is(false));
assertThat(withDeprecations.match(name), is(true));
assertThat(withDeprecations.match("foo bar"), 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));
assertWarnings("Deprecated field [" + deprecatedName + "] used, expected [foo_bar] instead");
}
}

public void testAllDeprecated() {
public void testAllDeprecated() throws IOException {
String name = "like_text";

boolean withDeprecatedNames = randomBoolean();
String[] deprecated = new String[]{"text", "same_as_text"};
String[] allValues;
if (withDeprecatedNames) {
String[] newArray = new String[1 + deprecated.length];
newArray[0] = name;
System.arraycopy(deprecated, 0, newArray, 1, deprecated.length);
allValues = newArray;
} else {
allValues = new String[] {name};
}

ParseField field;
if (withDeprecatedNames) {
field = new ParseField(name).withDeprecation(deprecated).withAllDeprecated("like");
} else {
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]"));
ParseField field = new ParseField(name).withDeprecation(deprecated).withAllDeprecated("like");
assertFalse(field.match("not a field name"));
assertTrue(field.match("text"));
assertWarnings("Deprecated field [text] used, replaced by [like]");
assertTrue(field.match("same_as_text"));
assertWarnings("Deprecated field [same_as_text] used, replaced by [like]");
assertTrue(field.match("like_text"));
assertWarnings("Deprecated field [like_text] used, replaced by [like]");
}

public void testGetAllNamesIncludedDeprecated() {
Expand Down
Loading