Skip to content

test(file_uploads): buffer uploads the router rejects before reading the body #10411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BrynCooke merged 1 commit into from
Oct 5, 2026
Merged

test(file_uploads): buffer uploads the router rejects before reading the body #10411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 25 additions & 5 deletions apollo-router/tests/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use fred::interfaces::KeysInterface;
use fred::prelude::Config as RedisConfig;
use fred::types::scan::Scanner;
use futures::StreamExt;
use futures::TryStreamExt;
use http::header::ACCEPT;
use http::header::CONTENT_TYPE;
use mime::APPLICATION_JSON;
Expand Down Expand Up @@ -1445,11 +1446,18 @@ impl IntegrationTest {
}

/// Make a raw multipart request to the router.
///
/// By default the form is streamed part by part. With `buffered`, it is collected first and
/// sent as a single body with a `Content-Length`, so the whole upload is written before the

@rohan-b99 rohan-b99 Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doc says buffering works because the upload "is written before the router can respond". The router can still respond after reading only the first parts. What matters is what happens next. Once the whole body has arrived, the router reads the rest of it and keeps the connection open, so there's no reset. A streamed body is still arriving, so the router closes the connection with data still on the way.

I checked this with a raw-socket client against a local router on macOS. Sent in one write, the 100-file upload left the connection reusable for a second request in 45 out of 45 runs. Sent part by part, the client's write failed and the connection was reset in 15 out of 15. When I held back even the last 100 bytes, the router closed the connection.

Suggested wording:

    /// By default the form is streamed part by part. With `buffered`, it is collected first and
    /// sent as a single body with a `Content-Length`. Use this when the router rejects the request
    /// after reading only the first parts: if the rest of a streamed body is still arriving, the
    /// router closes the connection and the client can hit a reset before it reads the response.

/// router can respond. Use this when the router rejects the request without reading the rest
/// of the body: a streamed upload can then hit a connection reset before the client has read
/// the response.
#[allow(dead_code)]
pub fn execute_multipart_request(
&self,
request: reqwest::multipart::Form,
transform: Option<fn(reqwest::Request) -> reqwest::Request>,
buffered: bool,
) -> impl std::future::Future<Output = (String, reqwest::Response)> + use<> {
assert!(
self.router.is_some(),
Expand All @@ -1464,14 +1472,26 @@ impl IntegrationTest {
let span_id = span.context().span().span_context().trace_id().to_string();

async move {
let mut request = client
let builder = client
.post(url)
.header("apollographql-client-name", "custom_name")
.header("apollographql-client-version", "1.0")
.header("apollo-require-preflight", "test")
.multipart(request)
.build()
.unwrap();
.header("apollo-require-preflight", "test");
let builder = if buffered {
let content_type =
format!("multipart/form-data; boundary={}", request.boundary());
let chunks: Vec<bytes::Bytes> = request
.into_stream()
.try_collect()
.await
.expect("multipart form can be buffered");
builder
.header(CONTENT_TYPE, content_type)
.body(chunks.concat())
} else {
builder.multipart(request)
};
let mut request = builder.build().unwrap();

// Optionally transform the request if needed
let transformer = transform.unwrap_or(core::convert::identity);
Expand Down
16 changes: 14 additions & 2 deletions apollo-router/tests/integration/file_upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,12 +751,14 @@ async fn it_fails_with_file_count_limits() -> Result<(), BoxError> {
.collect::<Vec<_>>(),
);

// Run the test
// Run the test. The router rejects this as soon as it reads the map, so buffer the

@rohan-b99 rohan-b99 Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment repeats the explanation on execute_multipart_request. The other three tests that opt in have no comment, so a reader might wonder what is special here. The doc added to FileUploadTestServer::new (lines 1782-1784) repeats it a third time. I'd go back to // Run the test here, delete line 755, and cut the builder doc to a pointer:

        /// Set `buffered` for requests the router rejects before reading the files. See
        /// [IntegrationTest::execute_multipart_request].

// upload: a streamed one can be reset before the response has been read.
helper::FileUploadTestServer::builder()
.config(FILE_CONFIG)
.handler(make_handler!(helper::always_fail))
.request(request)
.subgraph_mapping("uploads", "/")
.buffered(true)
.build()
.run_test(|response| {
insta::assert_json_snapshot!(response, @r###"
Expand Down Expand Up @@ -845,6 +847,7 @@ async fn it_fails_invalid_multipart_order() -> Result<(), BoxError> {
.handler(make_handler!(helper::always_fail))
.request(request)
.subgraph_mapping("uploads", "/")
.buffered(true)
.build()
.run_test(|response| {
insta::assert_json_snapshot!(response, @r###"
Expand Down Expand Up @@ -961,6 +964,7 @@ async fn it_fails_with_no_boundary_in_multipart() -> Result<(), BoxError> {
.handler(make_handler!(helper::always_fail))
.request(request)
.subgraph_mapping("uploads", "/")
.buffered(true)
.transformer(strip_boundary)
.build()
.run_test(|response| {
Expand Down Expand Up @@ -1028,6 +1032,7 @@ async fn it_fails_incompatible_query_order() -> Result<(), BoxError> {
.request(request)
.subgraph_mapping("uploads", "/s1")
.subgraph_mapping("uploads_clone", "/s2")
.buffered(true)
.build()
.run_test(|response| {
insta::assert_json_snapshot!(response, @r###"
Expand Down Expand Up @@ -1763,6 +1768,7 @@ mod helper {
request: Form,
subgraph_mappings: HashMap<String, String>,
transformer: Option<fn(reqwest::Request) -> reqwest::Request>,
buffered: bool,
}

#[buildstructor]
Expand All @@ -1772,20 +1778,26 @@ mod helper {
/// Prefer the builder so that tests are more descriptive.
///
/// See [make_handler] and [create_request].
///
/// Set `buffered` for requests that the router rejects before reading the whole body,
/// so that the upload is sent in full before the router responds. See
/// [IntegrationTest::execute_multipart_request].
#[builder]
pub fn new(
config: &'static str,
handler: Router,
subgraph_mappings: HashMap<String, String>,
request: Form,
transformer: Option<fn(reqwest::Request) -> reqwest::Request>,
buffered: Option<bool>,
) -> Self {
Self {
config,
handler,
request,
subgraph_mappings,
transformer,
buffered: buffered.unwrap_or_default(),
}
}

Expand Down Expand Up @@ -1843,7 +1855,7 @@ mod helper {

// Make the request and pass it into the validator callback
let (_span, response) = router
.execute_multipart_request(self.request, self.transformer)
.execute_multipart_request(self.request, self.transformer, self.buffered)
.await;
let response = serde_json::from_slice(&response.bytes().await?)?;
validation_fn(response);
Expand Down
Loading