Validate Accept and Content-Type header for compatible API

buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java

@@ -130,6 +130,8 @@ public boolean wait(int durationInMs) throws GeneralSecurityException, Interrupt
 
     protected void checkResource(SSLContext ssl) throws IOException {
         final HttpURLConnection connection = buildConnection(ssl);
+        connection.setRequestProperty("Accept", "application/json");
+
         connection.connect();
         final Integer response = connection.getResponseCode();
         if (validResponseCodes.contains(response)) {
@@ -144,6 +146,7 @@ HttpURLConnection buildConnection(SSLContext ssl) throws IOException {
         final HttpURLConnection connection = (HttpURLConnection) this.url.openConnection();
         configureSslContext(connection, ssl);
         configureBasicAuth(connection);
+
         connection.setRequestMethod("GET");
         return connection;
     }

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

@@ -117,7 +117,7 @@ public XContent xContent() {
     };
 
     private static final Pattern COMPATIBLE_API_HEADER_PATTERN = Pattern.compile(
-        "application/(vnd.elasticsearch\\+)?([^;]+)(\\s*;\\s*compatible-with=(\\d+))?",
+        "(application|text)/(vnd.elasticsearch\\+)?([^;]+)(\\s*;\\s*compatible-with=(\\d+))?",
         Pattern.CASE_INSENSITIVE);
 
     /**
@@ -126,7 +126,9 @@ public XContent xContent() {
      * also supports a wildcard accept for {@code application/*}. This method can be used to parse the {@code Accept} HTTP header or a
      * format query string parameter. This method will return {@code null} if no match is found
      */
-    public static XContentType fromMediaTypeOrFormat(String mediaType) {
+    public static XContentType fromMediaTypeOrFormat(String mediaTypeHeaderValue) {
+        String mediaType = parseMediaType(mediaTypeHeaderValue);
+
         if (mediaType == null) {
             return null;
         }
@@ -136,7 +138,7 @@ public static XContentType fromMediaTypeOrFormat(String mediaType) {
             }
         }
         final String lowercaseMediaType = mediaType.toLowerCase(Locale.ROOT);
-        if (lowercaseMediaType.startsWith("application/*")) {
+        if (lowercaseMediaType.startsWith("application/*") || lowercaseMediaType.equals("*/*")) {
             return JSON;
         }
 
@@ -165,11 +167,12 @@ public static XContentType fromMediaType(String mediaTypeHeaderValue) {
         return null;
     }
 
+    //public scope needed for text formats hack
     public static String parseMediaType(String mediaType) {
         if (mediaType != null) {
             Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType);
             if (matcher.find()) {
-                return "application/" + matcher.group(2).toLowerCase(Locale.ROOT);
+                return matcher.group(1) + "/" + matcher.group(3).toLowerCase(Locale.ROOT);
             }
         }
 
@@ -179,9 +182,9 @@ public static String parseMediaType(String mediaType) {
     public static String parseVersion(String mediaType){
         if(mediaType != null){
             Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType);
-            if (matcher.find() && "vnd.elasticsearch+".equalsIgnoreCase(matcher.group(1))) {
+            if (matcher.find() && "vnd.elasticsearch+".equalsIgnoreCase(matcher.group(2))) {
 
-                return matcher.group(4);
+                return matcher.group(5);
             }
         }
         return null;

modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestGetActionV7.java

@@ -37,12 +37,12 @@ public class RestGetActionV7 extends RestGetAction {
     private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestGetAction.class));
     static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Specifying types in "
         + "document get requests is deprecated, use the /{index}/_doc/{id} endpoint instead.";
+
     @Override
     public String getName() {
         return "document_get_action_v7";
     }
 
-
     @Override
     public List<Route> routes() {
         assert Version.CURRENT.major == 8 : "REST API compatibility for version 7 is only supported on version 8";

modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestIndexActionV7.java

@@ -74,6 +74,7 @@ public boolean compatibilityRequired() {
     }
 
     public static class CompatibleCreateHandler extends RestIndexAction.CreateHandler {
+
         @Override
         public String getName() {
             return "document_create_action_v7";
@@ -100,15 +101,16 @@ public boolean compatibilityRequired() {
     }
 
     public static final class CompatibleAutoIdHandler extends RestIndexAction.AutoIdHandler {
-        @Override
-        public String getName() {
-            return "document_create_action_auto_id_v7";
-        }
 
         public CompatibleAutoIdHandler(Supplier<DiscoveryNodes> nodesInCluster) {
             super(nodesInCluster);
         }
 
+        @Override
+        public String getName() {
+            return "document_create_action_auto_id_v7";
+        }
+
         @Override
         public List<Route> routes() {
             return singletonList(new Route(POST, "/{index}/{type}"));

qa/rest-compat-tests/src/main/java/org/elasticsearch/rest/compat/AbstractCompatRestTest.java

@@ -33,6 +33,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.function.Consumer;
 import java.util.stream.StreamSupport;
 
 /**
@@ -43,7 +44,6 @@ protected AbstractCompatRestTest(@Name("yaml") ClientYamlTestCandidate testCandi
         super(testCandidate);
     }
 
-
     private static final Logger staticLogger = LogManager.getLogger(AbstractCompatRestTest.class);
 
     public static final String COMPAT_TESTS_PATH = "/rest-api-spec/test-compat";
@@ -66,38 +66,48 @@ public static Iterable<Object[]> createParameters() throws Exception {
             });
             finalTestCandidates.add(testCandidates.toArray());
         }
-        localCandidates.keySet().forEach(lc -> finalTestCandidates.add(new Object[]{lc}));
+        localCandidates.keySet().forEach(lc -> finalTestCandidates.add(new Object[] { lc }));
         return finalTestCandidates;
     }
 
-
     private static void mutateTestCandidate(ClientYamlTestCandidate testCandidate) {
-        testCandidate.getTestSection().getExecutableSections().stream().filter(s -> s instanceof DoSection).forEach(ds -> {
+        testCandidate.getSetupSection().getExecutableSections().stream().filter(s -> s instanceof DoSection).forEach(updateDoSection());
+        testCandidate.getTestSection().getExecutableSections().stream().filter(s -> s instanceof DoSection).forEach(updateDoSection());
+    }
+
+    private static Consumer<? super ExecutableSection> updateDoSection() {
+        return ds -> {
             DoSection doSection = (DoSection) ds;
-            //TODO: be more selective here
+            // TODO: be more selective here
             doSection.setIgnoreWarnings(true);
 
             String compatibleHeader = createCompatibleHeader();
-            //TODO decide which one to use - Accept or Content-Type
-            doSection.getApiCallSection()
-                     .addHeaders(Map.of(
-                         CompatibleConstants.COMPATIBLE_HEADER, compatibleHeader,
-                         "Content-Type", compatibleHeader
-                         ));
-        });
+            // for cat apis accept headers would break tests which expect txt response
+            if (doSection.getApiCallSection().getApi().startsWith("cat") == false) {
+                doSection.getApiCallSection()
+                    .addHeaders(
+                        Map.of(
+                            CompatibleConstants.COMPATIBLE_ACCEPT_HEADER,
+                            compatibleHeader,
+                            CompatibleConstants.COMPATIBLE_CONTENT_TYPE_HEADER,
+                            compatibleHeader
+                        )
+                    );
+            }
+
+        };
     }
 
     private static String createCompatibleHeader() {
         return "application/vnd.elasticsearch+json;compatible-with=" + CompatibleConstants.COMPATIBLE_VERSION;
     }
 
-
     private static Map<ClientYamlTestCandidate, ClientYamlTestCandidate> getLocalCompatibilityTests() throws Exception {
-        Iterable<Object[]> candidates =
-            ESClientYamlSuiteTestCase.createParameters(ExecutableSection.XCONTENT_REGISTRY, COMPAT_TESTS_PATH);
+        Iterable<Object[]> candidates = ESClientYamlSuiteTestCase.createParameters(ExecutableSection.XCONTENT_REGISTRY, COMPAT_TESTS_PATH);
         Map<ClientYamlTestCandidate, ClientYamlTestCandidate> localCompatibilityTests = new HashMap<>();
         StreamSupport.stream(candidates.spliterator(), false)
-            .flatMap(Arrays::stream).forEach(o -> localCompatibilityTests.put((ClientYamlTestCandidate) o, (ClientYamlTestCandidate) o));
+            .flatMap(Arrays::stream)
+            .forEach(o -> localCompatibilityTests.put((ClientYamlTestCandidate) o, (ClientYamlTestCandidate) o));
         return localCompatibilityTests;
     }
 }

server/src/main/java/org/elasticsearch/Version.java

@@ -80,6 +80,7 @@ public class Version implements Comparable<Version>, ToXContentFragment {
     public static final Version V_7_8_0 = new Version(7080099, org.apache.lucene.util.Version.LUCENE_8_5_0);
     public static final Version V_8_0_0 = new Version(8000099, org.apache.lucene.util.Version.LUCENE_8_5_0);
     public static final Version CURRENT = V_8_0_0;
+    public static final Version PREVIOUS = V_7_0_0;
 
     private static final ImmutableOpenIntMap<Version> idToVersion;
 
@@ -89,7 +90,7 @@ public class Version implements Comparable<Version>, ToXContentFragment {
         for (final Field declaredField : Version.class.getFields()) {
             if (declaredField.getType().equals(Version.class)) {
                 final String fieldName = declaredField.getName();
-                if (fieldName.equals("CURRENT") || fieldName.equals("V_EMPTY")) {
+                if (fieldName.equals("CURRENT") || fieldName.equals("V_EMPTY") || fieldName.equals("PREVIOUS")) {
                     continue;
                 }
                 assert fieldName.matches("V_\\d+_\\d+_\\d+")
@@ -402,6 +403,7 @@ public static List<Version> getDeclaredVersions(final Class<?> versionClass) {
             }
             switch (field.getName()) {
                 case "CURRENT":
+                case "PREVIOUS":
                 case "V_EMPTY":
                     continue;
             }

server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java

@@ -352,6 +352,11 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan
             } catch (final RestRequest.BadParameterException e) {
                 badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e);
                 innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel);
+            } catch (final RestRequest.CompatibleApiHeadersCombinationException e){
+                badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e);
+                //todo // tempting to just rethrow. removing content type is just making this less obvious
+                throw e;
+//                innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause);
             }
             restRequest = innerRestRequest;
         }

server/src/main/java/org/elasticsearch/rest/CompatibleConstants.java

@@ -26,8 +26,9 @@ public class CompatibleConstants {
     /**
      * TODO revisit when https://github.com/elastic/elasticsearch/issues/52370 is resolved
      */
-    public static final String COMPATIBLE_HEADER = "Accept";
+    public static final String COMPATIBLE_ACCEPT_HEADER = "Accept";
+    public static final String COMPATIBLE_CONTENT_TYPE_HEADER = "Content-Type";
     public static final String COMPATIBLE_PARAMS_KEY = "Compatible-With";
-    public static final String COMPATIBLE_VERSION = "" + (Version.CURRENT.major - 1);
+    public static final String COMPATIBLE_VERSION = String.valueOf(Version.CURRENT.major - 1);
 
 }

server/src/main/java/org/elasticsearch/rest/RestRequest.java

@@ -21,6 +21,7 @@
 
 import org.apache.lucene.util.SetOnce;
 import org.elasticsearch.ElasticsearchParseException;
+import org.elasticsearch.Version;
 import org.elasticsearch.common.Booleans;
 import org.elasticsearch.common.CheckedConsumer;
 import org.elasticsearch.common.Nullable;
@@ -45,6 +46,7 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
@@ -132,29 +134,91 @@ void ensureSafeBuffers() {
     public static RestRequest request(NamedXContentRegistry xContentRegistry, HttpRequest httpRequest, HttpChannel httpChannel) {
         Map<String, String> params = params(httpRequest.uri());
         String path = path(httpRequest.uri());
-        RestRequest restRequest = new RestRequest(xContentRegistry, params, path, httpRequest.getHeaders(), httpRequest, httpChannel,
+        return new RestRequest(xContentRegistry, params, path, httpRequest.getHeaders(), httpRequest, httpChannel,
             requestIdGenerator.incrementAndGet());
-        return restRequest;
     }
 
     private void addCompatibleParameter() {
-        if (isRequestCompatible()) {
-            String compatibleVersion = XContentType.parseVersion(header(CompatibleConstants.COMPATIBLE_HEADER));
-            params().put(CompatibleConstants.COMPATIBLE_PARAMS_KEY, compatibleVersion);
+        Version compatible = compatibleWithVersion();
+        if (Version.PREVIOUS.equals(compatible)) {
+            params().put(CompatibleConstants.COMPATIBLE_PARAMS_KEY, String.valueOf(compatible.major));
             //use it so it won't fail request validation with unused parameter
             param(CompatibleConstants.COMPATIBLE_PARAMS_KEY);
         }
     }
 
-    private boolean isRequestCompatible() {
-        return isHeaderCompatible(header(CompatibleConstants.COMPATIBLE_HEADER));
-    }
+    private Version compatibleWithVersion() {
+        String currentVersion = String.valueOf(Version.CURRENT.major);
+        String previousVersion = String.valueOf(Version.CURRENT.major - 1);
 
-    private boolean isHeaderCompatible(String headerValue) {
-        String version = XContentType.parseVersion(headerValue);
-        return CompatibleConstants.COMPATIBLE_VERSION.equals(version);
-    }
+        String acceptHeader = header(CompatibleConstants.COMPATIBLE_ACCEPT_HEADER);
+        String acceptVersion = XContentType.parseVersion(acceptHeader);
+        String contentTypeHeader = header(CompatibleConstants.COMPATIBLE_CONTENT_TYPE_HEADER);
+        String contentTypeVersion = XContentType.parseVersion(contentTypeHeader);
+
+        //TODO not sure about this one as this does not cover text formats
+        boolean isSupportedMediaTypeAccept = acceptHeader == null || XContentType.parseMediaType(acceptHeader) != null;
 
+        boolean isSupportedMediaTypeContentType = contentTypeHeader == null || XContentType.parseMediaType(contentTypeHeader) != null;
+
+        if (hasContent()) {
+            //both headers versioned
+            if (acceptVersion != null && contentTypeVersion != null) {
+                // both Accept and Content-Type are versioned and set to a previous version
+                if (previousVersion.equals(acceptVersion) && previousVersion.equals(contentTypeVersion)) {
+                    return Version.PREVIOUS;
+                }
+                // both Accept and Content-Type are versioned to a current version
+                if (currentVersion.equals(acceptVersion) && currentVersion.equals(contentTypeVersion)) {
+                    return Version.CURRENT;
+                }
+                // both headers are versioned but set to incorrect version
+                throw new CompatibleApiHeadersCombinationException(
+                    String.format(Locale.ROOT, "Request with a body and incompatible Accept and Content-Type header values. " +
+                            "Accept=%s Content-Type=%s hasContent=%b %s %s %s", acceptHeader,
+                        contentTypeHeader, hasContent(), path(), params.toString(), method().toString()));
+            }
+            // Content-Type is versioned but accept is not present
+            if(previousVersion.equals(contentTypeVersion)
+                && (acceptHeader == null || acceptHeader.equals("*/*") )) {
+                return Version.PREVIOUS;
+            }
+            // Content type is set (not verioned) but accept is not present. It will be defaulted to JSON
+            if(isSupportedMediaTypeContentType &&
+                (acceptHeader == null || acceptHeader.equals("*/*") )){//TODO when do we default this?
+                return Version.CURRENT;
+            }
+            // both Accept and Content-Type are not a compatible format, but are supported and not empty
+            if (isSupportedMediaTypeContentType && isSupportedMediaTypeAccept &&
+                acceptHeader != null && contentTypeHeader != null) {
+                return Version.CURRENT;
+            }
+        } else {
+            if (acceptVersion != null) {
+                //Accept header is versioned and set to previous
+                if (previousVersion.equals(acceptVersion)) {
+                    return Version.PREVIOUS;
+                }
+                if (currentVersion.equals(acceptVersion)) {
+                    return Version.CURRENT;
+                }
+                // Accept header is versioned but set to incorrect version
+                throw new CompatibleApiHeadersCombinationException(
+                    String.format(Locale.ROOT, "Request with a body and incompatible Accept and Content-Type header values. " +
+                            "Accept=%s Content-Type=%s hasContent=%b %s %s %s", acceptHeader,
+                        contentTypeHeader, hasContent(), path(), params.toString(), method().toString()));
+            }
+            //Accept header is not versioned but is supported
+            if (isSupportedMediaTypeAccept) {
+                return Version.CURRENT;
+            }
+        }
+
+        throw new CompatibleApiHeadersCombinationException(
+            String.format(Locale.ROOT, "Request with a body and incompatible Accept and Content-Type header values. " +
+                    "Accept=%s Content-Type=%s hasContent=%b %s %s %s", acceptHeader,
+                contentTypeHeader, hasContent(), path(), params.toString(), method().toString()));
+    }
 
     private static Map<String, String> params(final String uri) {
         final Map<String, String> params = new HashMap<>();
@@ -559,4 +623,12 @@ public static class BadParameterException extends RuntimeException {
 
     }
 
+    public static class CompatibleApiHeadersCombinationException extends RuntimeException {
+
+        CompatibleApiHeadersCombinationException(String cause) {
+            super(cause);
+        }
+
+    }
+
 }

server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java

@@ -121,4 +121,21 @@ public void testVersionParsing() {
         assertThat(XContentType.parseVersion("APPLICATION/JSON"),
             nullValue());
     }
+
+    public void testVersionParsingOnText() {
+        String version = String.valueOf(Math.abs(randomByte()));
+        assertThat(XContentType.parseVersion("text/vnd.elasticsearch+csv;compatible-with=" + version),
+            equalTo(version));
+        assertThat(XContentType.parseVersion("text/vnd.elasticsearch+text;compatible-with=" + version),
+            equalTo(version));
+        assertThat(XContentType.parseVersion("text/vnd.elasticsearch+tab-separated-values;compatible-with=" + version),
+            equalTo(version));
+        assertThat(XContentType.parseVersion("text/csv"),
+            nullValue());
+
+        assertThat(XContentType.parseVersion("TEXT/VND.ELASTICSEARCH+CSV;COMPATIBLE-WITH=" + version),
+            equalTo(version));
+        assertThat(XContentType.parseVersion("TEXT/csv"),
+            nullValue());
+    }
 }