Add allowed_warnings to yaml tests (backport of elastic#53139) (el…

…astic#53173)

When we test backwards compatibility we often end up in a situation
where we *sometimes* get a warning, and sometimes don't. Like, we won't
get the warning if we're testing against an older version, but we will
in a newer one. Or we won't get the warning if the request randomly
lands on a node with an old version of the code. But we wouldn't if it
randomed into a node with newer code.

This adds `allowed_warnings` to our yaml test runner for those cases:
warnings declared this way are "allowed" but not "required".

Blocks elastic#52959

Co-authored-by: Benjamin Trent <[email protected]>

rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc

@@ -198,6 +198,23 @@ header. The warnings must match exactly. Using it looks like this:
             id:        1
 ....
 
+If the arguments to `do` include `allowed_warnings` then matching `Warning`
+headers do not fail the request. Unlike the `warnings` argument, these aren't
+expected so much as "allowed". This usually comes up in backwards compatibility
+testing. Using it looks like this:
+
+....
+    - do:
+        allowed_warnings:
+            - some warning
+            - this argument is also always a list, never a single string
+            - no matter how many warnings you expect
+        get:
+            index:    test
+            type:    test
+            id:        1
+....
+
 If the arguments to `do` include `node_selector` then the request is only
 sent to nodes that match the `node_selector`. It looks like this:
 

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Features.java

@@ -49,8 +49,9 @@ public final class Features {
             "yaml",
             "contains",
             "transform_and_set",
-            "arbitrary_key"
-    ));
+            "arbitrary_key",
+            "allowed_warnings"));
+
     private static final String SPI_ON_CLASSPATH_SINCE_JDK_9 = "spi_on_classpath_jdk9";
 
     private Features() {

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSuite.java

@@ -163,6 +163,14 @@ private static Stream<String> validateExecutableSections(List<ExecutableSection>
                 "without a corresponding [\"skip\": \"features\": \"warnings\"] so runners that do not support the [warnings] " +
                 "section can skip the test at line [" + section.getLocation().lineNumber + "]");
 
+        errors = Stream.concat(errors, sections.stream().filter(section -> section instanceof DoSection)
+                .map(section -> (DoSection) section)
+                .filter(section -> false == section.getAllowedWarningHeaders().isEmpty())
+                .filter(section -> false == hasSkipFeature("allowed_warnings", testSection, setupSection, teardownSection))
+                .map(section -> "attempted to add a [do] with a [allowed_warnings] section " +
+                    "without a corresponding [\"skip\": \"features\": \"allowed_warnings\"] so runners that do not " +
+                    "support the [allowed_warnings] section can skip the test at line [" + section.getLocation().lineNumber + "]"));
+
         errors = Stream.concat(errors, sections.stream().filter(section -> section instanceof DoSection)
             .map(section -> (DoSection) section)
             .filter(section -> NodeSelector.ANY != section.getApiCallSection().getNodeSelector())

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java

@@ -50,10 +50,11 @@
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.regex.Matcher;
-import java.util.stream.Collectors;
 
 import static java.util.Collections.emptyList;
 import static java.util.Collections.unmodifiableList;
+import static java.util.stream.Collectors.toCollection;
+import static java.util.stream.Collectors.toSet;
 import static org.elasticsearch.common.collect.Tuple.tuple;
 import static org.elasticsearch.common.logging.DeprecationLogger.WARNING_HEADER_PATTERN;
 import static org.elasticsearch.test.hamcrest.RegexMatcher.matches;
@@ -77,6 +78,9 @@
  *          - Stuff is deprecated, yo
  *          - Don't use deprecated stuff
  *          - Please, stop. It hurts.
+ *      allowed_warnings:
+ *          - Maybe this warning shows up
+ *          - But it isn't actually required for the test to pass.
  *      update:
  *          index:  test_1
  *          type:   test
@@ -94,6 +98,7 @@ public static DoSection parse(XContentParser parser) throws IOException {
         NodeSelector nodeSelector = NodeSelector.ANY;
         Map<String, String> headers = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
         List<String> expectedWarnings = new ArrayList<>();
+        List<String> allowedWarnings = new ArrayList<>();
 
         if (parser.nextToken() != XContentParser.Token.START_OBJECT) {
             throw new IllegalArgumentException("expected [" + XContentParser.Token.START_OBJECT + "], " +
@@ -117,6 +122,14 @@ public static DoSection parse(XContentParser parser) throws IOException {
                     if (token != XContentParser.Token.END_ARRAY) {
                         throw new ParsingException(parser.getTokenLocation(), "[warnings] must be a string array but saw [" + token + "]");
                     }
+                } else if ("allowed_warnings".equals(currentFieldName)) {
+                    while ((token = parser.nextToken()) == XContentParser.Token.VALUE_STRING) {
+                        allowedWarnings.add(parser.text());
+                    }
+                    if (token != XContentParser.Token.END_ARRAY) {
+                        throw new ParsingException(parser.getTokenLocation(),
+                                "[allowed_warnings] must be a string array but saw [" + token + "]");
+                    }
                 } else {
                     throw new ParsingException(parser.getTokenLocation(), "unknown array [" + currentFieldName + "]");
                 }
@@ -172,10 +185,16 @@ public static DoSection parse(XContentParser parser) throws IOException {
             if (apiCallSection == null) {
                 throw new IllegalArgumentException("client call section is mandatory within a do section");
             }
+            for (String w : expectedWarnings) {
+                if (allowedWarnings.contains(w)) {
+                    throw new IllegalArgumentException("the warning [" + w + "] was both allowed and expected");
+                }
+            }
             apiCallSection.addHeaders(headers);
             apiCallSection.setNodeSelector(nodeSelector);
             doSection.setApiCallSection(apiCallSection);
             doSection.setExpectedWarningHeaders(unmodifiableList(expectedWarnings));
+            doSection.setAllowedWarningHeaders(unmodifiableList(allowedWarnings));
         } finally {
             parser.nextToken();
         }
@@ -188,6 +207,7 @@ public static DoSection parse(XContentParser parser) throws IOException {
     private String catchParam;
     private ApiCallSection apiCallSection;
     private List<String> expectedWarningHeaders = emptyList();
+    private List<String> allowedWarningHeaders = emptyList();
 
     public DoSection(XContentLocation location) {
         this.location = location;
@@ -225,6 +245,22 @@ void setExpectedWarningHeaders(List<String> expectedWarningHeaders) {
         this.expectedWarningHeaders = expectedWarningHeaders;
     }
 
+    /**
+     * Warning headers that we allow from this response. These warning
+     * headers don't cause the test to fail. Defaults to emptyList.
+     */
+    List<String> getAllowedWarningHeaders() {
+        return allowedWarningHeaders;
+    }
+
+    /**
+     * Set the warning headers that we expect from this response. These
+     * warning headers don't cause the test to fail. Defaults to emptyList.
+     */
+    void setAllowedWarningHeaders(List<String> allowedWarningHeaders) {
+        this.allowedWarningHeaders = allowedWarningHeaders;
+    }
+
     @Override
     public XContentLocation getLocation() {
         return location;
@@ -284,15 +320,18 @@ void checkWarningHeaders(final List<String> warningHeaders, final Version master
         final List<String> unexpected = new ArrayList<>();
         final List<String> unmatched = new ArrayList<>();
         final List<String> missing = new ArrayList<>();
+        Set<String> allowed = allowedWarningHeaders.stream()
+                .map(DeprecationLogger::escapeAndEncode)
+                .collect(toSet());
         // LinkedHashSet so that missing expected warnings come back in a predictable order which is nice for testing
-        final Set<String> expected =
-                new LinkedHashSet<>(expectedWarningHeaders.stream().map(DeprecationLogger::escapeAndEncode).collect(Collectors.toList()));
+        final Set<String> expected = expectedWarningHeaders.stream()
+                .map(DeprecationLogger::escapeAndEncode)
+                .collect(toCollection(LinkedHashSet::new));
         for (final String header : warningHeaders) {
             final Matcher matcher = WARNING_HEADER_PATTERN.matcher(header);
             final boolean matches = matcher.matches();
             if (matches) {
                 final String message = matcher.group(1);
-                // noinspection StatementWithEmptyBody
                 if (masterVersion.before(Version.V_7_0_0)
                         && message.equals("the default number of shards will change from [5] to [1] in 7.0.0; "
                         + "if you wish to continue using the default of [5] shards, "
@@ -301,15 +340,19 @@ void checkWarningHeaders(final List<String> warningHeaders, final Version master
                      * This warning header will come back in the vast majority of our tests that create an index when running against an
                      * older master. Rather than rewrite our tests to assert this warning header, we assume that it is expected.
                      */
-                } else // noinspection StatementWithEmptyBody
-                    if (message.startsWith("[types removal]") || message.startsWith("[_data_frame/transforms/] is deprecated")) {
-                    /*
-                     * We skip warnings related to types deprecation and transform rename so that we can continue to run the many
-                     * mixed-version tests that used typed APIs.
-                     */
-                } else if (expected.remove(message) == false) {
-                    unexpected.add(header);
+                    continue;
+                }
+                if (message.startsWith("[types removal]")) {
+                    // We skip warnings related to types deprecation because they are *everywhere*.
+                    continue;
+                }
+                if (allowed.contains(message)) {
+                    continue;
+                }
+                if (expected.remove(message)) {
+                    continue;
                 }
+                unexpected.add(header);
             } else {
                 unmatched.add(header);
             }

test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSuiteTests.java

@@ -411,6 +411,19 @@ public void testAddingDoWithWarningWithoutSkipWarnings() {
             "at line [" + lineNumber + "]"));
     }
 
+    public void testAddingDoWithAllowedWarningWithoutSkipAllowedWarnings() {
+        int lineNumber = between(1, 10000);
+        DoSection doSection = new DoSection(new XContentLocation(lineNumber, 0));
+        doSection.setAllowedWarningHeaders(singletonList("foo"));
+        doSection.setApiCallSection(new ApiCallSection("test"));
+        ClientYamlTestSuite testSuite = createTestSuite(SkipSection.EMPTY, doSection);
+        Exception e = expectThrows(IllegalArgumentException.class, testSuite::validate);
+        assertThat(e.getMessage(), containsString("api/name:\nattempted to add a [do] with a [allowed_warnings] " +
+            "section without a corresponding [\"skip\": \"features\": \"allowed_warnings\"] so runners that do not " +
+            "support the [allowed_warnings] section can skip the test at line [" + lineNumber + "]"));
+    }
+
+
     public void testAddingDoWithHeaderWithoutSkipHeaders() {
         int lineNumber = between(1, 10000);
         DoSection doSection = new DoSection(new XContentLocation(lineNumber, 0));

test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/DoSectionTests.java

@@ -126,6 +126,15 @@ public void testWarningHeaders() {
                             "did not get expected warning headers [\n\tanother\n\tsome more\n]\n",
                     e.getMessage());
         }
+
+        // "allowed" warnings are fine
+        {
+            final DoSection section = new DoSection(new XContentLocation(1, 1));
+            section.setAllowedWarningHeaders(singletonList("test"));
+            section.checkWarningHeaders(singletonList(testHeader), Version.CURRENT);
+            // and don't throw exceptions if we don't receive them
+            section.checkWarningHeaders(emptyList(), Version.CURRENT);
+        }
     }
 
     public void testIgnoreTypesWarnings() {
@@ -467,6 +476,54 @@ public void testParseDoSectionExpectedWarnings() throws Exception {
                 "just one entry this time")));
     }
 
+    public void testParseDoSectionAllowedWarnings() throws Exception {
+        parser = createParser(YamlXContent.yamlXContent,
+                "indices.get_field_mapping:\n" +
+                "        index: test_index\n" +
+                "        type: test_type\n" +
+                "allowed_warnings:\n" +
+                "    - some test warning they are typically pretty long\n" +
+                "    - some other test warning sometimes they have [in] them"
+        );
+
+        DoSection doSection = DoSection.parse(parser);
+        assertThat(doSection.getCatch(), nullValue());
+        assertThat(doSection.getApiCallSection(), notNullValue());
+        assertThat(doSection.getApiCallSection().getApi(), equalTo("indices.get_field_mapping"));
+        assertThat(doSection.getApiCallSection().getParams().size(), equalTo(2));
+        assertThat(doSection.getApiCallSection().getParams().get("index"), equalTo("test_index"));
+        assertThat(doSection.getApiCallSection().getParams().get("type"), equalTo("test_type"));
+        assertThat(doSection.getApiCallSection().hasBody(), equalTo(false));
+        assertThat(doSection.getApiCallSection().getBodies().size(), equalTo(0));
+        assertThat(doSection.getAllowedWarningHeaders(), equalTo(Arrays.asList(
+                "some test warning they are typically pretty long",
+                "some other test warning sometimes they have [in] them")));
+
+        parser = createParser(YamlXContent.yamlXContent,
+                "indices.get_field_mapping:\n" +
+                "        index: test_index\n" +
+                "allowed_warnings:\n" +
+                "    - just one entry this time"
+        );
+
+        doSection = DoSection.parse(parser);
+        assertThat(doSection.getCatch(), nullValue());
+        assertThat(doSection.getApiCallSection(), notNullValue());
+        assertThat(doSection.getAllowedWarningHeaders(), equalTo(singletonList(
+                "just one entry this time")));
+
+        parser = createParser(YamlXContent.yamlXContent,
+                "indices.get_field_mapping:\n" +
+                "        index: test_index\n" +
+                "warnings:\n" +
+                "    - foo\n" +
+                "allowed_warnings:\n" +
+                "    - foo"
+        );
+        Exception e = expectThrows(IllegalArgumentException.class, () -> DoSection.parse(parser));
+        assertThat(e.getMessage(), equalTo("the warning [foo] was both allowed and expected"));
+    }
+
     public void testNodeSelectorByVersion() throws IOException {
         parser = createParser(YamlXContent.yamlXContent,
                 "node_selector:\n" +