From 551ce873354b45ce1b8d85f2beace88b0030aa33 Mon Sep 17 00:00:00 2001
From: Dmitri Bourlatchkov <dmitri.bourlatchkov@dremio.com>
Date: Thu, 9 Jan 2025 18:26:23 -0500
Subject: [PATCH] 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.
---
 .../StreamReadConstraintsExceptionMapper.java |  2 +-
 .../PolarisApplicationIntegrationTest.java    | 24 +++++++++++--------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/dropwizard/service/src/main/java/org/apache/polaris/service/dropwizard/throttling/StreamReadConstraintsExceptionMapper.java b/dropwizard/service/src/main/java/org/apache/polaris/service/dropwizard/throttling/StreamReadConstraintsExceptionMapper.java
index d29f2de5d..5ec8fd79a 100644
--- a/dropwizard/service/src/main/java/org/apache/polaris/service/dropwizard/throttling/StreamReadConstraintsExceptionMapper.java
+++ b/dropwizard/service/src/main/java/org/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/dropwizard/service/src/test/java/org/apache/polaris/service/dropwizard/PolarisApplicationIntegrationTest.java b/dropwizard/service/src/test/java/org/apache/polaris/service/dropwizard/PolarisApplicationIntegrationTest.java
index 651581382..e7f7cd4a2 100644
--- a/dropwizard/service/src/test/java/org/apache/polaris/service/dropwizard/PolarisApplicationIntegrationTest.java
+++ b/dropwizard/service/src/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");
     }
   }