coprocessor: fix parsing of graphql responses with null as `data` #7141

IvanGoncharov · 2025-03-28T14:36:15Z

Coprocessor deserialized GraphQL response using serde_json directly that resulted in data: null were ignored.
After my change, it's using Response::from_value, which applies the same steps for parsing GraphQL response that we are already using for parsing subgraph response.

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Note any exceptions here

Notes

It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

svc-apollo-docs · 2025-03-28T14:37:00Z

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: fb1f29e54dfdeafbe06042a0

bnjjj

It contains a lot of changes unrelated to the fix itself (formatting fixes), I tried to read everything carefully to make sure I didn't miss anything but feel free to tell me in which files changes are really important to look at.

apollo-router/src/graphql/request.rs

apollo-router/src/graphql/mod.rs

IvanGoncharov · 2025-04-10T11:53:11Z

It contains a lot of changes unrelated to the fix itself (formatting fixes), I tried to read everything carefully to make sure I didn't miss anything but feel free to tell me in which files changes are really important to look at.

@bnjjj I will add comments on all important changes.

IvanGoncharov · 2025-04-10T11:58:02Z

apollo-router/src/graphql/mod.rs

+            Ok(Some(s)) => Ok(s.as_str().to_string()),
+            Ok(None) => Err(MalformedResponseError {
+                reason: "missing required `message` property within error".to_owned(),
+            }),


This is a new behaviour for subgraph response parsing.
Previously, if the description was missing, it was set up by unwrap_or_default, which resulted in an empty string.
Since the coprocessor used serde for parsion, it was erroring on missing message.
Since the GraphQL spec requires message I just added validation here.

Is it potentially a breaking change or could it break existing implementation ? I think it's a bug fix but as it's more strict maybe it could be interesting to add notes about this in the changelog to avoid confusions

BrynCooke · 2025-04-10T12:12:37Z

.changesets/fix_fix_coprocessor_data_null.md

@@ -0,0 +1,9 @@
+### Fix Parsing of Coprocessor GraphQL Responses ([PR #7141](https://github.com/apollographql/router/pull/7141))
+
+Standardized GraphQL response parsing and validation between coprocessors and subgraphs to ensure consistent behavior throughout the Router. 


I think we need a user focused changset entry. Something like.

Previously coprocessors allowed null body in graphql responses, hoever this is not allowed in the graphql spec. The router now correctly enforces that the body of a coprocessor response is valid, and will reject such responses.

If your coprocessor relied on this behaviour then you must update your coprocessor to return a calid graphql response.

Make sense 👍

IvanGoncharov · 2025-04-10T12:16:41Z

apollo-router/src/graphql/response.rs

+pub(crate) struct MalformedResponseError {
+    /// The reason the deserialization failed.
+    pub(crate) reason: String,
+}


Previously, we reported all errors related to parsing/validation graphql response as FetchError::SubrequestMalformedResponse which made this code only usable for subgraph responses. Since now we have generic error it could be reused for coprocessor responses.

IvanGoncharov · 2025-04-10T12:21:37Z

apollo-router/src/plugins/coprocessor/execution.rs

@@ -206,7 +207,7 @@ where

    let body_to_send = request_config
        .body
-        .then(|| serde_json::from_slice::<serde_json::Value>(&bytes))


The general idea was to switch the entire coprocessor plugin to use serde_json_bytes since it's the only way to reuse code written for subgraph response, which is already tied to serde_json_bytes. In general, I think it's a good idea since even if the coprocessor changed the GraphQL response in a minor way, it still needs to be deserialized in its entirety, and serde_json_bytes provides necessary optimisation for large responses.

IvanGoncharov · 2025-04-10T12:26:56Z

apollo-router/src/plugins/coprocessor/mod.rs

@@ -1137,29 +1138,28 @@ where
        let code = control.get_http_status()?;

        let res = {
-            let graphql_response: crate::graphql::Response =
-                match co_processor_output.body.unwrap_or(serde_json::Value::Null) {
-                    serde_json::Value::String(s) => crate::graphql::Response::builder()


This part is weird. I think it would be better to just have a custom error saying you need to provide body if you return Break. Instead, we push null instead of body, which results in a parsing error.
But I decided to stop myself here since I had already gone too deep into refactoring this code.

IvanGoncharov · 2025-04-10T12:28:32Z

apollo-router/tests/integration/coprocessor.rs

@@ -274,3 +266,51 @@ async fn test_coprocessor_demand_control_access() -> Result<(), BoxError> {

    Ok(())
 }
+
+#[tokio::test(flavor = "multi_thread")]
+async fn test_coprocessor_proxying_error_response() -> Result<(), BoxError> {


Here is a test that tests the customer's issue:

IvanGoncharov · 2025-04-18T12:33:55Z

@mergify backport 1.x

mergify · 2025-04-18T12:34:05Z

backport 1.x

✅ Backports have been created

#7315 coprocessor: fix parsing of graphql responses with null as data (backport #7141) has been created for branch 1.x but encountered conflicts

) (cherry picked from commit 388afab) # Conflicts: # apollo-router/src/graphql/request.rs # apollo-router/src/metrics/mod.rs # apollo-router/src/plugins/coprocessor/execution.rs # apollo-router/src/plugins/coprocessor/supergraph.rs # apollo-router/src/plugins/coprocessor/test.rs

…ckport #7141) (#7315)

This comment has been minimized.

Sign in to view

IvanGoncharov force-pushed the i1g/fix_coprocessor_data_null branch from e5c2803 to 43052be Compare March 28, 2025 14:37

coprocessor: fix parsing of graphql responses with null as data

554654e

IvanGoncharov force-pushed the i1g/fix_coprocessor_data_null branch from 43052be to 554654e Compare April 7, 2025 20:21

IvanGoncharov added 6 commits April 10, 2025 06:10

fix tests

b7550ec

fix formating

aa9f029

fix one more test

7093221

fix formating

2d62b29

fix formating

a85b32d

Merge branch 'dev' into i1g/fix_coprocessor_data_null

5608ca9

IvanGoncharov marked this pull request as ready for review April 10, 2025 06:15

IvanGoncharov requested a review from a team as a code owner April 10, 2025 06:15

abernix requested review from goto-bus-stop, bnjjj and BrynCooke April 10, 2025 07:36

bnjjj reviewed Apr 10, 2025

View reviewed changes

apollo-router/src/graphql/request.rs Show resolved Hide resolved

apollo-router/src/graphql/request.rs Show resolved Hide resolved

apollo-router/src/graphql/mod.rs Show resolved Hide resolved

add changelog

b44ca74

IvanGoncharov requested a review from a team as a code owner April 10, 2025 11:36

IvanGoncharov commented Apr 10, 2025

View reviewed changes

BrynCooke requested changes Apr 10, 2025

View reviewed changes

IvanGoncharov commented Apr 10, 2025

View reviewed changes

IvanGoncharov added 4 commits April 10, 2025 16:14

Merge branch 'dev' into i1g/fix_coprocessor_data_null

1c706e2

Merge branch 'dev' into i1g/fix_coprocessor_data_null

2444a2c

review changes

2e6e01e

improve changeset

02a9031

BrynCooke approved these changes Apr 17, 2025

View reviewed changes

Merge branch 'dev' into i1g/fix_coprocessor_data_null

9687b0c

IvanGoncharov enabled auto-merge (squash) April 17, 2025 20:17

fix after merge dev

5e7e8fe

IvanGoncharov disabled auto-merge April 18, 2025 07:57

IvanGoncharov enabled auto-merge (squash) April 18, 2025 07:58

IvanGoncharov disabled auto-merge April 18, 2025 09:46

IvanGoncharov merged commit 388afab into dev Apr 18, 2025
15 checks passed

IvanGoncharov deleted the i1g/fix_coprocessor_data_null branch April 18, 2025 12:33

IvanGoncharov mentioned this pull request Apr 18, 2025

coprocessor: fix parsing of graphql responses with null as data (backport #7141) #7315

Merged

6 tasks

IvanGoncharov added a commit that referenced this pull request Apr 22, 2025

coprocessor: fix parsing of graphql responses with null as data (ba…

d69fda9

…ckport #7141) (#7315)

abernix mentioned this pull request Apr 29, 2025

prep release: v2.2.0 #7361

Merged

abernix mentioned this pull request May 6, 2025

prep release: v1.61.6 #7404

Merged

This was referenced Jun 13, 2025

fix(subscription): set a valid graphql response for websocket handshake response #7680

Merged

fix(subscription): set a valid graphql response for websocket handshake response (backport #7680) #7683

Merged

abernix mentioned this pull request Jul 1, 2025

prep release: v2.4.0 #7789

Merged

		@@ -0,0 +1,9 @@
		### Fix Parsing of Coprocessor GraphQL Responses ([PR #7141](https://github.com/apollographql/router/pull/7141))

		Standardized GraphQL response parsing and validation between coprocessors and subgraphs to ensure consistent behavior throughout the Router.

coprocessor: fix parsing of graphql responses with null as data #7141

coprocessor: fix parsing of graphql responses with null as data #7141

Uh oh!

Conversation

IvanGoncharov commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

This comment has been minimized.

svc-apollo-docs commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Docs preview has no changes

Uh oh!

bnjjj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IvanGoncharov commented Apr 10, 2025

Uh oh!

IvanGoncharov Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

bnjjj Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

BrynCooke Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IvanGoncharov commented Apr 18, 2025

Uh oh!

mergify bot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Uh oh!

coprocessor: fix parsing of graphql responses with null as `data` #7141

coprocessor: fix parsing of graphql responses with null as `data` #7141

IvanGoncharov commented Mar 28, 2025 •

edited

Loading

svc-apollo-docs commented Mar 28, 2025 •

edited

Loading

IvanGoncharov Apr 10, 2025 •

edited

Loading

mergify bot commented Apr 18, 2025 •

edited

Loading