Report parser name and location in XContent deprecation warnings (#53805

)

It's simple to deprecate a field used in an ObjectParser just by adding deprecation
markers to the relevant ParseField objects. The warnings themselves don't currently
have any context - they simply say that a deprecated field has been used, but not
where in the input xcontent it appears. This commit adds the parent object parser
name and XContentLocation to these deprecation messages.

Note that the context is automatically stripped from warning messages when they
are asserted on by integration tests and REST tests, because randomization of
xcontent type during these tests means that the XContentLocation is not constant

libs/x-content/src/main/java/org/elasticsearch/common/ParseField.java

@@ -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
@@ -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
@@ -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;
             }

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/DeprecationHandler.java

@@ -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.
@@ -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");
+            }
         }
     };
 
@@ -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) {
 
         }
     };
@@ -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);
+
 }

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

@@ -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);
             }

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentLocation.java

@@ -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;
 

libs/x-content/src/test/java/org/elasticsearch/common/ParseFieldTests.java

@@ -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");
 
 
     }

libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java

@@ -224,7 +224,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(false, "[foo][1:15] Deprecated field [old_test] used, expected [test] instead");
     }
 
     public void testFailOnValueType() throws IOException {

modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java

@@ -96,7 +96,7 @@ private XContentParser extractRequestSpecificFields(RestRequest restRequest,
     }
 
     private void setMaxDocsFromSearchSize(Request request, int size) {
-        LoggingDeprecationHandler.INSTANCE.usedDeprecatedName("size", "max_docs");
+        LoggingDeprecationHandler.INSTANCE.usedDeprecatedName(null, null, "size", "max_docs");
         setMaxDocsValidateIdentical(request, size);
     }
 }

qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java

@@ -164,7 +164,8 @@ public void testConcurrentDeprecationLogger() throws IOException, UserException,
                  */
                 final List<String> warnings = threadContext.getResponseHeaders().get("Warning");
                 final Set<String> actualWarningValues =
-                        warnings.stream().map(DeprecationLogger::extractWarningValueFromWarningHeader).collect(Collectors.toSet());
+                        warnings.stream().map(s -> DeprecationLogger.extractWarningValueFromWarningHeader(s, true))
+                            .collect(Collectors.toSet());
                 for (int j = 0; j < 128; j++) {
                     assertThat(
                             actualWarningValues,

qa/smoke-test-http/src/test/java/org/elasticsearch/http/DeprecationHttpIT.java

@@ -187,7 +187,8 @@ private void doTestDeprecationWarningsAppearInHeaders() throws IOException {
             assertThat(deprecatedWarning, matches(WARNING_HEADER_PATTERN.pattern()));
         }
         final List<String> actualWarningValues =
-                deprecatedWarnings.stream().map(DeprecationLogger::extractWarningValueFromWarningHeader).collect(Collectors.toList());
+                deprecatedWarnings.stream().map(s -> DeprecationLogger.extractWarningValueFromWarningHeader(s, true))
+                    .collect(Collectors.toList());
         for (Matcher<String> headerMatcher : headerMatchers) {
             assertThat(actualWarningValues, hasItem(headerMatcher));
         }

server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java

@@ -174,14 +174,16 @@ public void deprecatedAndMaybeLog(final String key, final String msg, final Obje
                     "GMT" + // GMT
                     "\")?"); // closing quote (optional, since an older version can still send a warn-date)
 
+    public static final Pattern WARNING_XCONTENT_LOCATION_PATTERN = Pattern.compile("^\\[.*?]\\[-?\\d+:-?\\d+] ");
+
     /**
      * Extracts the warning value from the value of a warning header that is formatted according to RFC 7234. That is, given a string
      * {@code 299 Elasticsearch-6.0.0 "warning value"}, the return value of this method would be {@code warning value}.
      *
      * @param s the value of a warning header formatted according to RFC 7234.
      * @return the extracted warning value
      */
-    public static String extractWarningValueFromWarningHeader(final String s) {
+    public static String extractWarningValueFromWarningHeader(final String s, boolean stripXContentPosition) {
         /*
          * We know the exact format of the warning header, so to extract the warning value we can skip forward from the front to the first
          * quote and we know the last quote is at the end of the string
@@ -196,8 +198,14 @@ public static String extractWarningValueFromWarningHeader(final String s) {
          */
         final int firstQuote = s.indexOf('\"');
         final int lastQuote = s.length() - 1;
-        final String warningValue = s.substring(firstQuote + 1, lastQuote);
+        String warningValue = s.substring(firstQuote + 1, lastQuote);
         assert assertWarningValue(s, warningValue);
+        if (stripXContentPosition) {
+            Matcher matcher = WARNING_XCONTENT_LOCATION_PATTERN.matcher(warningValue);
+            if (matcher.find()) {
+                warningValue = warningValue.substring(matcher.end());
+            }
+        }
         return warningValue;
     }
 
@@ -232,7 +240,7 @@ void deprecated(final Set<ThreadContext> threadContexts, final String message, f
             final String formattedMessage = LoggerMessageFormat.format(message, params);
             final String warningHeaderValue = formatWarning(formattedMessage);
             assert WARNING_HEADER_PATTERN.matcher(warningHeaderValue).matches();
-            assert extractWarningValueFromWarningHeader(warningHeaderValue).equals(escapeAndEncode(formattedMessage));
+            assert extractWarningValueFromWarningHeader(warningHeaderValue, false).equals(escapeAndEncode(formattedMessage));
             while (iterator.hasNext()) {
                 try {
                     final ThreadContext next = iterator.next();

server/src/main/java/org/elasticsearch/common/xcontent/LoggingDeprecationHandler.java

@@ -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>
@@ -49,17 +51,23 @@ 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() + "] ";
+        deprecationLogger.deprecated("{}Deprecated field [{}] used, this field is unused and will be removed entirely",
+            prefix, usedName);
     }
 }

server/src/test/java/org/elasticsearch/common/logging/DeprecationLoggerTests.java

@@ -216,10 +216,18 @@ public void testFailsWhenRemovingUnknownThreadContext() throws IOException {
         expectThrows(IllegalStateException.class, () -> DeprecationLogger.removeThreadContext(threadContext));
     }
 
-    public void testWarningValueFromWarningHeader() throws InterruptedException {
+    public void testWarningValueFromWarningHeader() {
         final String s = randomAlphaOfLength(16);
         final String first = DeprecationLogger.formatWarning(s);
-        assertThat(DeprecationLogger.extractWarningValueFromWarningHeader(first), equalTo(s));
+        assertThat(DeprecationLogger.extractWarningValueFromWarningHeader(first, false), equalTo(s));
+
+        final String withPos = "[context][1:11] Blah blah blah";
+        final String formatted = DeprecationLogger.formatWarning(withPos);
+        assertThat(DeprecationLogger.extractWarningValueFromWarningHeader(formatted, true), equalTo("Blah blah blah"));
+
+        final String withNegativePos = "[context][-1:-1] Blah blah blah";
+        assertThat(DeprecationLogger.extractWarningValueFromWarningHeader(DeprecationLogger.formatWarning(withNegativePos), true),
+            equalTo("Blah blah blah"));
     }
 
     public void testEscapeBackslashesAndQuotes() {

server/src/test/java/org/elasticsearch/common/util/concurrent/ThreadContextTests.java

@@ -208,11 +208,11 @@ public void testResponseHeaders() {
         }
 
         final String value = DeprecationLogger.formatWarning("qux");
-        threadContext.addResponseHeader("baz", value, DeprecationLogger::extractWarningValueFromWarningHeader);
+        threadContext.addResponseHeader("baz", value, s -> DeprecationLogger.extractWarningValueFromWarningHeader(s, false));
         // pretend that another thread created the same response at a different time
         if (randomBoolean()) {
             final String duplicateValue = DeprecationLogger.formatWarning("qux");
-            threadContext.addResponseHeader("baz", duplicateValue, DeprecationLogger::extractWarningValueFromWarningHeader);
+            threadContext.addResponseHeader("baz", duplicateValue, s -> DeprecationLogger.extractWarningValueFromWarningHeader(s, false));
         }
 
         threadContext.addResponseHeader("Warning", "One is the loneliest number");

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

@@ -432,6 +432,10 @@ protected final void assertSettingDeprecationsAndWarnings(final String[] setting
     }
 
     protected final void assertWarnings(String... expectedWarnings) {
+        assertWarnings(true, expectedWarnings);
+    }
+
+    protected final void assertWarnings(boolean stripXContentPosition, String... expectedWarnings) {
         if (enableWarningsCheck() == false) {
             throw new IllegalStateException("unable to check warning headers if the test is not set to do so");
         }
@@ -457,7 +461,8 @@ private List<String> filterJodaDeprecationWarnings(List<String> actualWarnings)
     private void assertWarnings(List<String> actualWarnings, String[] expectedWarnings) {
         assertNotNull("no warnings, expected: " + Arrays.asList(expectedWarnings), actualWarnings);
         final Set<String> actualWarningValues =
-                actualWarnings.stream().map(DeprecationLogger::extractWarningValueFromWarningHeader).collect(Collectors.toSet());
+                actualWarnings.stream().map(s -> DeprecationLogger.extractWarningValueFromWarningHeader(s, true))
+                    .collect(Collectors.toSet());
         for (String msg : expectedWarnings) {
             assertThat(actualWarningValues, hasItem(DeprecationLogger.escapeAndEncode(msg)));
         }