Handle exceptions thrown from RestCompatibleVersionHelper (elastic#80253

)

RestCompatibleVersionHelper is used to validate versions on Accept and Content-Type headers
When a validation fails, an exception is being thrown indicating which headers are incorrect.
That exception was not handled in RestRequest where the helper was used.
This commit gracefully handles the exception from validation failure. A bad request response is returned to a user.

closes elastic#79060
closes elastic#78214

modules/transport-netty4/src/javaRestTest/java/org/elasticsearch/rest/Netty4BadRequestIT.java

@@ -89,6 +89,6 @@ public void testInvalidHeaderValue() throws IOException {
         final ObjectPath objectPath = ObjectPath.createFromResponse(response);
         final Map<String, Object> map = objectPath.evaluate("error");
         assertThat(map.get("type"), equalTo("media_type_header_exception"));
-        assertThat(map.get("reason"), equalTo("Invalid media-type value on header [Content-Type]"));
+        assertThat(map.get("reason"), equalTo("Invalid media-type value on headers [Content-Type]"));
     }
 }

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

@@ -413,7 +413,7 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan
                 innerRestRequest = RestRequest.request(parserConfig, httpRequest, httpChannel);
             } catch (final RestRequest.MediaTypeHeaderException e) {
                 badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e);
-                innerRestRequest = requestWithoutFailedHeader(httpRequest, httpChannel, badRequestCause, e.getFailedHeaderName());
+                innerRestRequest = requestWithoutFailedHeader(httpRequest, httpChannel, badRequestCause, e.getFailedHeaderNames());
             } catch (final RestRequest.BadParameterException e) {
                 badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e);
                 innerRestRequest = RestRequest.requestWithoutParameters(parserConfig, httpRequest, httpChannel);
@@ -468,11 +468,18 @@ private RestRequest requestWithoutFailedHeader(
         HttpRequest httpRequest,
         HttpChannel httpChannel,
         Exception badRequestCause,
-        String failedHeaderName
+        Set<String> failedHeaderNames
     ) {
-        HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader(failedHeaderName);
+        assert failedHeaderNames.size() > 0;
+        HttpRequest httpRequestWithoutContentType = httpRequest;
+        for (String failedHeaderName : failedHeaderNames) {
+            httpRequestWithoutContentType = httpRequestWithoutContentType.removeHeader(failedHeaderName);
+        }
         try {
             return RestRequest.request(parserConfig, httpRequestWithoutContentType, httpChannel);
+        } catch (final RestRequest.MediaTypeHeaderException e) {
+            badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e);
+            return requestWithoutFailedHeader(httpRequest, httpChannel, badRequestCause, e.getFailedHeaderNames());
         } catch (final RestRequest.BadParameterException e) {
             badRequestCause.addSuppressed(e);
             return RestRequest.requestWithoutParameters(parserConfig, httpRequestWithoutContentType, httpChannel);

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

@@ -10,9 +10,11 @@
 
 import org.apache.lucene.util.SetOnce;
 import org.elasticsearch.ElasticsearchParseException;
+import org.elasticsearch.ElasticsearchStatusException;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.logging.LoggerMessageFormat;
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.core.Booleans;
 import org.elasticsearch.core.CheckedConsumer;
@@ -106,7 +108,11 @@ private RestRequest(
             throw new MediaTypeHeaderException(e, "Content-Type");
         }
         this.httpRequest = httpRequest;
-        this.restApiVersion = RestCompatibleVersionHelper.getCompatibleVersion(parsedAccept, parsedContentType, hasContent());
+        try {
+            this.restApiVersion = RestCompatibleVersionHelper.getCompatibleVersion(parsedAccept, parsedContentType, hasContent());
+        } catch (ElasticsearchStatusException e) {
+            throw new MediaTypeHeaderException(e, "Accept", "Content-Type");
+        }
         this.parserConfig = parserConfig.restApiVersion().equals(restApiVersion)
             ? parserConfig
             : parserConfig.withRestApiVersion(restApiVersion);
@@ -607,20 +613,23 @@ public RestApiVersion getRestApiVersion() {
 
     public static class MediaTypeHeaderException extends RuntimeException {
 
-        private String failedHeaderName;
+        private String message;
+        private Set<String> failedHeaderNames;
+        private Object[] params;
 
-        MediaTypeHeaderException(final IllegalArgumentException cause, String failedHeaderName) {
+        MediaTypeHeaderException(final RuntimeException cause, String... failedHeaderNames) {
             super(cause);
-            this.failedHeaderName = failedHeaderName;
+            this.failedHeaderNames = Set.of(failedHeaderNames);
+            this.message = "Invalid media-type value on headers " + this.failedHeaderNames;
         }
 
-        public String getFailedHeaderName() {
-            return failedHeaderName;
+        public Set<String> getFailedHeaderNames() {
+            return failedHeaderNames;
         }
 
         @Override
         public String getMessage() {
-            return "Invalid media-type value on header [" + failedHeaderName + "]";
+            return LoggerMessageFormat.format(message, params);
         }
     }
 

server/src/test/java/org/elasticsearch/http/AbstractHttpServerTransportTests.java

@@ -51,6 +51,7 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
 import static java.net.InetAddress.getByName;
@@ -195,10 +196,32 @@ public HttpStats stats() {
         }
     }
 
+    public void testHandlingCompatibleVersionParsingErrors() {
+        // a compatible version exception (v7 on accept and v8 on content-type) should be handled gracefully
+        final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
+
+        try (
+            AbstractHttpServerTransport transport = failureAssertingtHttpServerTransport(clusterSettings, Set.of("Accept", "Content-Type"))
+        ) {
+            Map<String, List<String>> headers = new HashMap<>();
+            headers.put("Accept", Collections.singletonList("aaa/bbb;compatible-with=7"));
+            headers.put("Content-Type", Collections.singletonList("aaa/bbb;compatible-with=8"));
+
+            FakeRestRequest.FakeHttpRequest fakeHttpRequest = new FakeRestRequest.FakeHttpRequest(
+                RestRequest.Method.GET,
+                "/",
+                new BytesArray(randomByteArrayOfLength(between(1, 20))),
+                headers
+            );
+
+            transport.incomingRequest(fakeHttpRequest, null);
+        }
+    }
+
     public void testIncorrectHeaderHandling() {
 
         final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
-        try (AbstractHttpServerTransport transport = failureAssertingtHttpServerTransport(clusterSettings, "Accept")) {
+        try (AbstractHttpServerTransport transport = failureAssertingtHttpServerTransport(clusterSettings, Set.of("Accept"))) {
 
             Map<String, List<String>> headers = new HashMap<>();
             headers.put("Accept", Collections.singletonList("incorrectheader"));
@@ -212,7 +235,7 @@ public void testIncorrectHeaderHandling() {
 
             transport.incomingRequest(fakeHttpRequest, null);
         }
-        try (AbstractHttpServerTransport transport = failureAssertingtHttpServerTransport(clusterSettings, "Content-Type")) {
+        try (AbstractHttpServerTransport transport = failureAssertingtHttpServerTransport(clusterSettings, Set.of("Content-Type"))) {
             Map<String, List<String>> headers = new HashMap<>();
             headers.put("Accept", Collections.singletonList("application/json"));
             headers.put("Content-Type", Collections.singletonList("incorrectheader"));
@@ -230,7 +253,7 @@ public void testIncorrectHeaderHandling() {
 
     private AbstractHttpServerTransport failureAssertingtHttpServerTransport(
         ClusterSettings clusterSettings,
-        final String failedHeaderName
+        final Set<String> failedHeaderNames
     ) {
         return new AbstractHttpServerTransport(
             Settings.EMPTY,
@@ -248,11 +271,8 @@ public void dispatchRequest(RestRequest request, RestChannel channel, ThreadCont
                 public void dispatchBadRequest(RestChannel channel, ThreadContext threadContext, Throwable cause) {
                     assertThat(cause, instanceOf(RestRequest.MediaTypeHeaderException.class));
                     RestRequest.MediaTypeHeaderException mediaTypeHeaderException = (RestRequest.MediaTypeHeaderException) cause;
-                    assertThat(mediaTypeHeaderException.getFailedHeaderName(), equalTo(failedHeaderName));
-                    assertThat(
-                        mediaTypeHeaderException.getMessage(),
-                        equalTo("Invalid media-type value on header [" + failedHeaderName + "]")
-                    );
+                    assertThat(mediaTypeHeaderException.getFailedHeaderNames(), equalTo(failedHeaderNames));
+                    assertThat(mediaTypeHeaderException.getMessage(), equalTo("Invalid media-type value on headers " + failedHeaderNames));
                 }
             },
             clusterSettings

server/src/test/java/org/elasticsearch/rest/RestRequestTests.java

@@ -198,7 +198,7 @@ public void testMalformedContentTypeHeader() {
         assertNotNull(e.getCause());
         assertThat(e.getCause(), instanceOf(IllegalArgumentException.class));
         assertThat(e.getCause().getMessage(), equalTo("invalid media-type [" + type + "]"));
-        assertThat(e.getMessage(), equalTo("Invalid media-type value on header [Content-Type]"));
+        assertThat(e.getMessage(), equalTo("Invalid media-type value on headers [Content-Type]"));
     }
 
     public void testNoContentTypeHeader() {
@@ -215,7 +215,7 @@ public void testMultipleContentTypeHeaders() {
         assertNotNull(e.getCause());
         assertThat(e.getCause(), instanceOf((IllegalArgumentException.class)));
         assertThat(e.getCause().getMessage(), equalTo("Incorrect header [Content-Type]. Only one value should be provided"));
-        assertThat(e.getMessage(), equalTo("Invalid media-type value on header [Content-Type]"));
+        assertThat(e.getMessage(), equalTo("Invalid media-type value on headers [Content-Type]"));
     }
 
     public void testRequiredContent() {