Use HTTP status code 413 for exceedingly large payloads (#631)

The 413 status code seems to be more appropriate than 400 (Bad Request) in this case.
apache · Jan 9, 2025 · 551ce87 · 551ce87
1 parent 5564515
commit 551ce87
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 11 deletions.
diff --git a/...rg/apache/polaris/service/dropwizard/throttling/StreamReadConstraintsExceptionMapper.java b/...rg/apache/polaris/service/dropwizard/throttling/StreamReadConstraintsExceptionMapper.java
@@ -34,7 +34,7 @@ public class StreamReadConstraintsExceptionMapper
 
   @Override
   public Response toResponse(StreamConstraintsException exception) {
-    return Response.status(Response.Status.BAD_REQUEST)
+    return Response.status(Response.Status.REQUEST_ENTITY_TOO_LARGE)
         .type(MediaType.APPLICATION_JSON_TYPE)
         .entity(new RequestThrottlingErrorResponse(REQUEST_TOO_LARGE))
         .build();

diff --git a/...rc/test/java/org/apache/polaris/service/dropwizard/PolarisApplicationIntegrationTest.java b/...rc/test/java/org/apache/polaris/service/dropwizard/PolarisApplicationIntegrationTest.java
@@ -19,7 +19,6 @@
 package org.apache.polaris.service.dropwizard;
 
 import static org.apache.polaris.service.context.DefaultRealmContextResolver.REALM_PROPERTY_KEY;
-import static org.apache.polaris.service.dropwizard.throttling.RequestThrottlingErrorResponse.RequestThrottlingErrorType.REQUEST_TOO_LARGE;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
@@ -87,7 +86,6 @@
 import org.apache.polaris.service.dropwizard.test.PolarisRealm;
 import org.apache.polaris.service.dropwizard.test.SnowmanCredentialsExtension;
 import org.apache.polaris.service.dropwizard.test.TestEnvironmentExtension;
-import org.apache.polaris.service.dropwizard.throttling.RequestThrottlingErrorResponse;
 import org.assertj.core.api.Assertions;
 import org.assertj.core.api.InstanceOfAssertFactories;
 import org.junit.jupiter.api.AfterAll;
@@ -712,7 +710,10 @@ public void testRequestHeaderTooLarge() {
 
   @Test
   public void testRequestBodyTooLarge() {
-    // The size is set to be higher than the limit in polaris-server-integrationtest.yml
+    // The behaviour in case of large requests depends on the specific server configuration.
+    // This test assumes that the server under test is configured to deny requests larger than
+    // 1000000 bytes. The test payload below assumes UTF8 encoding of ASCII charts plus a bit of
+    // JSON overhead.
     Entity<PrincipalRole> largeRequest = Entity.json(new PrincipalRole("r".repeat(1000001)));
 
     try (Response response =
@@ -724,13 +725,16 @@ public void testRequestBodyTooLarge() {
             .header("Authorization", "Bearer " + userToken)
             .header(REALM_PROPERTY_KEY, realm)
             .post(largeRequest)) {
-      assertThat(response)
-          .returns(Response.Status.BAD_REQUEST.getStatusCode(), Response::getStatus)
-          .matches(
-              r ->
-                  r.readEntity(RequestThrottlingErrorResponse.class)
-                      .errorType()
-                      .equals(REQUEST_TOO_LARGE));
+      // Note we only validate the status code here because per RFC 9110, the server MAY not provide
+      // a response body. The HTTP status line is still expected to be provided.
+      assertThat(response.getStatus())
+          .isEqualTo(Response.Status.REQUEST_ENTITY_TOO_LARGE.getStatusCode());
+    } catch (ProcessingException e) {
+      // Per RFC 9110 servers MAY close the connection in case of 413 responses, which
+      // might cause the client to fail to read the status code (cf. RFC 9112, section 9.6).
+      // TODO: servers are expected to close connections gracefully. It might be worth investigating
+      // whether "connection closed" exceptions are a client-side bug.
+      assertThat(e).hasMessageContaining("Connection was closed");
     }
   }