Skip to content
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

Report parser name and location in XContent deprecation warnings #53752

Merged
merged 5 commits into from
Mar 19, 2020
Merged
Show file tree
Hide file tree
Changes from 4 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
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@
package org.elasticsearch.common;

import org.elasticsearch.common.xcontent.DeprecationHandler;
import org.elasticsearch.common.xcontent.XContentLocation;

import java.util.Collections;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
import java.util.function.Supplier;

/**
* Holds a field that can be found in a request while parsing and its different
Expand Down Expand Up @@ -115,6 +117,22 @@ public ParseField withAllDeprecated() {
* names for this {@link ParseField}.
*/
public boolean match(String fieldName, DeprecationHandler deprecationHandler) {
return match(null, () -> XContentLocation.UNKNOWN, fieldName, deprecationHandler);
}

/**
* Does {@code fieldName} match this field?
* @param parserName
* the name of the parent object holding this field
* @param location
* the XContentLocation of the field
* @param fieldName
* the field name to match against this {@link ParseField}
* @param deprecationHandler called if {@code fieldName} is deprecated
* @return true if <code>fieldName</code> matches any of the acceptable
* names for this {@link ParseField}.
*/
public boolean match(String parserName, Supplier<XContentLocation> location, String fieldName, DeprecationHandler deprecationHandler) {
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 @@ -127,11 +145,11 @@ public boolean match(String fieldName, DeprecationHandler deprecationHandler) {
for (String depName : deprecatedNames) {
if (fieldName.equals(depName)) {
if (fullyDeprecated) {
deprecationHandler.usedDeprecatedField(fieldName);
deprecationHandler.usedDeprecatedField(parserName, location, fieldName);
} else if (allReplacedWith == null) {
deprecationHandler.usedDeprecatedName(fieldName, name);
deprecationHandler.usedDeprecatedName(parserName, location, fieldName, name);
} else {
deprecationHandler.usedDeprecatedField(fieldName, allReplacedWith);
deprecationHandler.usedDeprecatedField(parserName, location, fieldName, allReplacedWith);
}
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

package org.elasticsearch.common.xcontent;

import java.util.function.Supplier;

/**
* Callback for notifying the creator of the {@link XContentParser} that
* parsing hit a deprecated field.
Expand All @@ -32,20 +34,35 @@ public interface DeprecationHandler {
*/
DeprecationHandler THROW_UNSUPPORTED_OPERATION = new DeprecationHandler() {
@Override
public void usedDeprecatedField(String usedName, String replacedWith) {
throw new UnsupportedOperationException("deprecated fields not supported here but got ["
+ usedName + "] which is a deprecated name for [" + replacedWith + "]");
public void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName, String replacedWith) {
if (parserName != null) {
throw new UnsupportedOperationException("deprecated fields not supported in [" + parserName + "] but got ["
+ usedName + "] at [" + location.get() + "] which is a deprecated name for [" + replacedWith + "]");
} else {
throw new UnsupportedOperationException("deprecated fields not supported here but got ["
+ usedName + "] which is a deprecated name for [" + replacedWith + "]");
}
}
@Override
public void usedDeprecatedName(String usedName, String modernName) {
throw new UnsupportedOperationException("deprecated fields not supported here but got ["
+ usedName + "] which has been replaced with [" + modernName + "]");
public void usedDeprecatedName(String parserName, Supplier<XContentLocation> location, String usedName, String modernName) {
if (parserName != null) {
throw new UnsupportedOperationException("deprecated fields not supported in [" + parserName + "] but got ["
+ usedName + "] at [" + location.get() + "] which has been replaced with [" + modernName + "]");
} else {
throw new UnsupportedOperationException("deprecated fields not supported here but got ["
+ usedName + "] which has been replaced with [" + modernName + "]");
}
}

@Override
public void usedDeprecatedField(String usedName) {
throw new UnsupportedOperationException("deprecated fields not supported here but got ["
+ usedName + "] which has been deprecated entirely");
public void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName) {
if (parserName != null) {
throw new UnsupportedOperationException("deprecated fields not supported in [" + parserName + "] but got ["
+ usedName + "] at [" + location.get() + "] which has been deprecated entirely");
} else {
throw new UnsupportedOperationException("deprecated fields not supported here but got ["
+ usedName + "] which has been deprecated entirely");
}
}
};

Expand All @@ -54,17 +71,17 @@ public void usedDeprecatedField(String usedName) {
*/
DeprecationHandler IGNORE_DEPRECATIONS = new DeprecationHandler() {
@Override
public void usedDeprecatedName(String usedName, String modernName) {
public void usedDeprecatedName(String parserName, Supplier<XContentLocation> location, String usedName, String modernName) {

}

@Override
public void usedDeprecatedField(String usedName, String replacedWith) {
public void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName, String replacedWith) {

}

@Override
public void usedDeprecatedField(String usedName) {
public void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName) {

}
};
Expand All @@ -74,20 +91,21 @@ public void usedDeprecatedField(String usedName) {
* @param usedName the provided field name
* @param modernName the modern name for the field
*/
void usedDeprecatedName(String usedName, String modernName);
void usedDeprecatedName(String parserName, Supplier<XContentLocation> location, String usedName, String modernName);

/**
* Called when the provided field name matches the current field but the entire
* field has been marked as deprecated and another field should be used
* @param usedName the provided field name
* @param replacedWith the name of the field that replaced this field
*/
void usedDeprecatedField(String usedName, String replacedWith);
void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName, String replacedWith);

/**
* Called when the provided field name matches the current field but the entire
* field has been marked as deprecated with no replacement
* @param usedName the provided field name
*/
void usedDeprecatedField(String usedName);
void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName);

}
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ private class FieldParser {
}

void assertSupports(String parserName, XContentParser parser, String currentFieldName) {
if (parseField.match(currentFieldName, parser.getDeprecationHandler()) == false) {
if (parseField.match(parserName, parser::getTokenLocation, currentFieldName, parser.getDeprecationHandler()) == false) {
throw new XContentParseException(parser.getTokenLocation(),
"[" + parserName + "] parsefield doesn't accept: " + currentFieldName);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@
* position of a parsing error to end users and consequently have line and
* column numbers starting from 1.
*/
public class XContentLocation {
public final class XContentLocation {

public static final XContentLocation UNKNOWN = new XContentLocation(-1, -1);

public final int lineNumber;
public final int columnNumber;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,11 @@ public void testDeprecatedWithNoReplacement() {
ParseField field = new ParseField(name).withDeprecation(alternatives).withAllDeprecated();
assertFalse(field.match("not a field name", LoggingDeprecationHandler.INSTANCE));
assertTrue(field.match("dep", LoggingDeprecationHandler.INSTANCE));
assertWarnings("Deprecated field [dep] used, which has been removed entirely");
assertWarnings("Deprecated field [dep] used, this field is unused and will be removed entirely");
assertTrue(field.match("old_dep", LoggingDeprecationHandler.INSTANCE));
assertWarnings("Deprecated field [old_dep] used, which has been removed entirely");
assertWarnings("Deprecated field [old_dep] used, this field is unused and will be removed entirely");
assertTrue(field.match("new_dep", LoggingDeprecationHandler.INSTANCE));
assertWarnings("Deprecated field [new_dep] used, which has been removed entirely");
assertWarnings("Deprecated field [new_dep] used, this field is unused and will be removed entirely");


}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ class TestStruct {
objectParser.declareField((i, v, c) -> v.test = i.text(), new ParseField("test", "old_test"), ObjectParser.ValueType.STRING);
objectParser.parse(parser, s, null);
assertEquals("foo", s.test);
assertWarnings("Deprecated field [old_test] used, expected [test] instead");
assertWarnings("[foo][1:15] Deprecated field [old_test] used, expected [test] instead");
}

public void testFailOnValueType() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public void testInlineBackcompat() throws Exception {
configMap.put("inline", "code");

factory.create(null, randomAlphaOfLength(10), configMap);
assertWarnings("Deprecated field [inline] used, expected [source] instead");
assertWarnings("[script][1:11] Deprecated field [inline] used, expected [source] instead");
}

public void testFactoryInvalidateWithInvalidCompiledScript() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.logging.DeprecationLogger;

import java.util.function.Supplier;

/**
* Logs deprecations to the {@link DeprecationLogger}.
* <p>
Expand All @@ -49,17 +51,27 @@ private LoggingDeprecationHandler() {
}

@Override
public void usedDeprecatedName(String usedName, String modernName) {
deprecationLogger.deprecated("Deprecated field [{}] used, expected [{}] instead", usedName, modernName);
public void usedDeprecatedName(String parserName, Supplier<XContentLocation> location, String usedName, String modernName) {
String prefix = parserName == null ? "" : "[" + parserName + "][" + location.get() + "] ";
deprecationLogger.deprecated("{}Deprecated field [{}] used, expected [{}] instead",
prefix, usedName, modernName);
}

@Override
public void usedDeprecatedField(String usedName, String replacedWith) {
deprecationLogger.deprecated("Deprecated field [{}] used, replaced by [{}]", usedName, replacedWith);
public void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName, String replacedWith) {
String prefix = parserName == null ? "" : "[" + parserName + "][" + location.get() + "] ";
deprecationLogger.deprecated("{} Deprecated field [{}] used, replaced by [{}]",
prefix, usedName, replacedWith);
}

@Override
public void usedDeprecatedField(String usedName) {
deprecationLogger.deprecated("Deprecated field [{}] used, which has been removed entirely", usedName);
public void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName) {
String prefix = parserName == null ? "" : "[" + parserName + "][" + location.get() + "] ";
if (true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, out-of-date diff 😅

deprecationLogger.deprecated("{}Deprecated field [{}] used, this field is unused and will be removed entirely",
prefix, usedName);
} else {
deprecationLogger.deprecated("Deprecated field [{}] used, this field is unused and will be removed entirely", usedName);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ public void testDeprecation() throws IOException {
QueryBuilder q = parseQuery(query);
QueryBuilder expected = new BoolQueryBuilder().mustNot(new MatchAllQueryBuilder());
assertEquals(expected, q);
assertWarnings("Deprecated field [mustNot] used, expected [must_not] instead");
assertWarnings("[bool][1:24] Deprecated field [mustNot] used, expected [must_not] instead");
}

public void testRewrite() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public void testSourceParsing() throws Exception {

assertThat(parsed, equalTo(source));
}
assertWarnings("Deprecated field [code] used, expected [source] instead");
assertWarnings("[stored script source][1:33] Deprecated field [code] used, expected [source] instead");

// complex script with script object and empty options
try (XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@
import org.elasticsearch.common.logging.LoggerMessageFormat;
import org.elasticsearch.common.xcontent.DeprecationHandler;
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
import org.elasticsearch.common.xcontent.XContentLocation;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.function.Supplier;

/**
* Very similar to {@link org.elasticsearch.common.xcontent.LoggingDeprecationHandler} main differences are:
Expand All @@ -26,24 +28,27 @@ public class LoggingDeprecationAccumulationHandler implements DeprecationHandler
private final List<String> deprecations = new ArrayList<>();

@Override
public void usedDeprecatedName(String usedName, String modernName) {
LoggingDeprecationHandler.INSTANCE.usedDeprecatedName(usedName, modernName);
deprecations.add(LoggerMessageFormat.format("Deprecated field [{}] used, expected [{}] instead",
new Object[] {usedName, modernName}));
public void usedDeprecatedName(String parserName, Supplier<XContentLocation> location, String usedName, String modernName) {
LoggingDeprecationHandler.INSTANCE.usedDeprecatedName(parserName, location, usedName, modernName);
String prefix = parserName == null ? "" : "[" + parserName + "][" + location.get() + "] ";
deprecations.add(LoggerMessageFormat.format("{}Deprecated field [{}] used, expected [{}] instead",
new Object[]{prefix, usedName, modernName}));
}

@Override
public void usedDeprecatedField(String usedName, String replacedWith) {
LoggingDeprecationHandler.INSTANCE.usedDeprecatedField(usedName, replacedWith);
deprecations.add(LoggerMessageFormat.format("Deprecated field [{}] used, replaced by [{}]",
new Object[] {usedName, replacedWith}));
public void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName, String replacedWith) {
LoggingDeprecationHandler.INSTANCE.usedDeprecatedField(parserName, location, usedName, replacedWith);
String prefix = parserName == null ? "" : "[" + parserName + "][" + location.get() + "] ";
deprecations.add(LoggerMessageFormat.format("{}Deprecated field [{}] used, replaced by [{}]",
new Object[]{prefix, usedName, replacedWith}));
}

@Override
public void usedDeprecatedField(String usedName) {
LoggingDeprecationHandler.INSTANCE.usedDeprecatedField(usedName);
deprecations.add(LoggerMessageFormat.format("Deprecated field [{}] used, which has been deprecated entirely",
new Object[]{ usedName }));
public void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName) {
LoggingDeprecationHandler.INSTANCE.usedDeprecatedField(parserName, location, usedName);
String prefix = parserName == null ? "" : "[" + parserName + "][" + location.get() + "] ";
deprecations.add(LoggerMessageFormat.format("{}Deprecated field [{}] used, unused and will be removed entirely",
new Object[]{prefix, usedName}));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ public void testToMap() throws IOException {
public void testDeprecationWarnings() throws IOException {
XContentObjectTransformer<QueryBuilder> queryBuilderTransformer = new XContentObjectTransformer<>(NamedXContentRegistry.EMPTY,
(p)-> {
p.getDeprecationHandler().usedDeprecatedField("oldField", "newField");
p.getDeprecationHandler().usedDeprecatedName("oldName", "modernName");
p.getDeprecationHandler().usedDeprecatedField(null, null, "oldField", "newField");
p.getDeprecationHandler().usedDeprecatedName(null, null, "oldName", "modernName");
return new BoolQueryBuilder();
});
List<String> deprecations = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ public InferenceProcessor create(Map<String, Processor.Factory> processorFactori
fieldMap = ConfigurationUtils.readOptionalMap(TYPE, tag, config, FIELD_MAPPINGS);
//TODO Remove in 8.x
if (fieldMap != null) {
LoggingDeprecationHandler.INSTANCE.usedDeprecatedName(FIELD_MAPPINGS, FIELD_MAP);
LoggingDeprecationHandler.INSTANCE.usedDeprecatedName(null, () -> null, FIELD_MAPPINGS, FIELD_MAP);
}
}
InferenceConfig inferenceConfig = inferenceConfigFromMap(ConfigurationUtils.readMap(TYPE, tag, config, INFERENCE_CONFIG));
Expand Down
Loading