Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Comment on lines +714 to +715
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to do this here, but I wonder if we could somehow add a check for the limit to this test.

// JSON overhead.
Entity<PrincipalRole> largeRequest = Entity.json(new PrincipalRole("r".repeat(1000001)));

try (Response response =
Expand All @@ -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");
Comment on lines +733 to +737
Copy link
Contributor

@eric-maynard eric-maynard Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch makes sense to me. It might be worth filing an issue to see if we can reproduce the disconnect elsewhere

}
}

Expand Down
Loading