From 382fbe0a3009294374ebd62d82d8b61569ec8e55 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Wed, 20 Nov 2024 17:53:10 +0200 Subject: [PATCH] fix(file_upload): remove placeholders from query variables (#6293) Co-authored-by: Bryn Cooke --- ...ix_fix_file_upload_variable_placeholder.md | 7 + apollo-router/src/plugins/file_uploads/mod.rs | 4 +- .../tests/integration/file_upload.rs | 128 +++++++++++++++++- 3 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 .changesets/fix_fix_file_upload_variable_placeholder.md diff --git a/.changesets/fix_fix_file_upload_variable_placeholder.md b/.changesets/fix_fix_file_upload_variable_placeholder.md new file mode 100644 index 0000000000..e656525589 --- /dev/null +++ b/.changesets/fix_fix_file_upload_variable_placeholder.md @@ -0,0 +1,7 @@ +### File Uploads: Remove Placeholders from Query Variables ([PR #6293](https://github.com/apollographql/router/pull/6293)) + +Fixed an issue where file upload query variables in subgraph requests contained internal placeholders. +According to the [GraphQL Multipart Request Spec](https://github.com/jaydenseric/graphql-multipart-request-spec?tab=readme-ov-file#multipart-form-field-structure), these variables should be set to null. +This fix ensures that the Router complies with the specification and improves compatibility with subgraphs handling file uploads. + +By [@IvanGoncharov](https://github.com/IvanGoncharov) in https://github.com/apollographql/router/pull/6293 diff --git a/apollo-router/src/plugins/file_uploads/mod.rs b/apollo-router/src/plugins/file_uploads/mod.rs index fb44aa6b25..6932495ebc 100644 --- a/apollo-router/src/plugins/file_uploads/mod.rs +++ b/apollo-router/src/plugins/file_uploads/mod.rs @@ -254,7 +254,9 @@ fn replace_value_at_path<'a>( // Removes value at path. fn remove_value_at_path<'a>(variables: &'a mut json_ext::Object, path: &'a [String]) { - let _ = get_value_at_path(variables, path).take(); + if let Some(v) = get_value_at_path(variables, path) { + *v = serde_json_bytes::Value::Null; + } } fn get_value_at_path<'a>( diff --git a/apollo-router/tests/integration/file_upload.rs b/apollo-router/tests/integration/file_upload.rs index add9e81a90..fd272f9d28 100644 --- a/apollo-router/tests/integration/file_upload.rs +++ b/apollo-router/tests/integration/file_upload.rs @@ -22,6 +22,124 @@ macro_rules! make_handler { } } +#[tokio::test(flavor = "multi_thread")] +async fn it_uploads_file_to_subgraph() -> Result<(), BoxError> { + use reqwest::multipart::Form; + use reqwest::multipart::Part; + + const FILE: &str = "Hello, world!"; + const FILE_NAME: &str = "example.txt"; + + let request = Form::new() + .part( + "operations", + Part::text( + serde_json::json!({ + "query": "mutation SomeMutation($file: Upload) { + file: singleUpload(file: $file) { filename body } + }", + "variables": { "file": null }, + }) + .to_string(), + ), + ) + .part( + "map", + Part::text(serde_json::json!({ "0": ["variables.file"] }).to_string()), + ) + .part("0", Part::text(FILE).file_name(FILE_NAME)); + + async fn subgraph_handler( + mut request: http::Request, + ) -> impl axum::response::IntoResponse { + let boundary = request + .headers() + .get(CONTENT_TYPE) + .and_then(|v| multer::parse_boundary(v.to_str().ok()?).ok()) + .expect("subgraph request should have valid Content-Type header"); + let mut multipart = multer::Multipart::new(request.body_mut(), boundary); + + let operations_field = multipart + .next_field() + .await + .ok() + .flatten() + .expect("subgraph request should have valid `operations` field"); + assert_eq!(operations_field.name(), Some("operations")); + let operations: helper::Operation = + serde_json::from_slice(&operations_field.bytes().await.unwrap()).unwrap(); + insta::assert_json_snapshot!(operations, @r#" + { + "query": "mutation SomeMutation__uploads__0($file:Upload){file:singleUpload(file:$file){filename body}}", + "variables": { + "file": null + } + } + "#); + + let map_field = multipart + .next_field() + .await + .ok() + .flatten() + .expect("subgraph request should have valid `map` field"); + assert_eq!(map_field.name(), Some("map")); + let map: BTreeMap> = + serde_json::from_slice(&map_field.bytes().await.unwrap()).unwrap(); + insta::assert_json_snapshot!(map, @r#" + { + "0": [ + "variables.file" + ] + } + "#); + + let file_field = multipart + .next_field() + .await + .ok() + .flatten() + .expect("subgraph request should have file field"); + + ( + http::StatusCode::OK, + axum::Json(serde_json::json!({ + "data": { + "file": { + "filename": file_field.file_name().unwrap(), + "body": file_field.text().await.unwrap(), + }, + } + })), + ) + } + + // Run the test + helper::FileUploadTestServer::builder() + .config(FILE_CONFIG) + .handler(make_handler!(subgraph_handler)) + .request(request) + .subgraph_mapping("uploads", "/") + .build() + .run_test(|response| { + // FIXME: workaround to not update bellow snapshot if one of snapshots inside 'subgraph_handler' fails + // This would be fixed if subgraph shapshots are moved out of 'subgraph_handler' + assert_eq!(response.errors.len(), 0); + + insta::assert_json_snapshot!(response, @r###" + { + "data": { + "file": { + "filename": "example.txt", + "body": "Hello, world!" + } + } + } + "###); + }) + .await +} + #[tokio::test(flavor = "multi_thread")] async fn it_uploads_a_single_file() -> Result<(), BoxError> { const FILE: &str = "Hello, world!"; @@ -1043,7 +1161,7 @@ mod helper { pub body: Option, } - #[derive(Serialize, Deserialize)] + #[derive(Debug, Serialize, Deserialize)] pub struct Operation { // TODO: Can we verify that this is a valid graphql query? query: String, @@ -1168,10 +1286,8 @@ mod helper { return Err(FileUploadError::UnexpectedFile); } - let field_name: String = map - .into_keys() - .take(1) - .next() + let (field_name, _) = map + .first_key_value() .ok_or(FileUploadError::MissingMapping)?; // Extract the single expected file @@ -1181,7 +1297,7 @@ mod helper { .await? .ok_or(FileUploadError::MissingFile(field_name.clone()))?; - let file_name = f.file_name().unwrap_or(&field_name).to_string(); + let file_name = f.file_name().unwrap_or(field_name).to_string(); let body = f.bytes().await?; Upload {