coprocessor: fix parsing of graphql responses with null as data#7141
coprocessor: fix parsing of graphql responses with null as data#7141IvanGoncharov merged 14 commits intodevfrom
data#7141Conversation
This comment has been minimized.
This comment has been minimized.
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: fb1f29e54dfdeafbe06042a0 |
IvanGoncharov
force-pushed
the
i1g/fix_coprocessor_data_null
branch
from
e5c2803 to
43052be
Compare
IvanGoncharov
force-pushed
the
i1g/fix_coprocessor_data_null
branch
from
43052be to
554654e
Compare
bnjjj
left a comment
bnjjj
left a comment
There was a problem hiding this comment.
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. |
| Ok(Some(s)) => Ok(s.as_str().to_string()), | ||
| Ok(None) => Err(MalformedResponseError { | ||
| reason: "missing required `message` property within error".to_owned(), | ||
| }), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -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. | |||
There was a problem hiding this comment.
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.
| pub(crate) struct MalformedResponseError { | ||
| /// The reason the deserialization failed. | ||
| pub(crate) reason: String, | ||
| } |
There was a problem hiding this comment.
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.
|
|
||
| let body_to_send = request_config | ||
| .body | ||
| .then(|| serde_json::from_slice::<serde_json::Value>(&bytes)) |
There was a problem hiding this comment.
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.
| 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() |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread")] | ||
| async fn test_coprocessor_proxying_error_response() -> Result<(), BoxError> { |
There was a problem hiding this comment.
Here is a test that tests the customer's issue:
|
@mergify backport 1.x |
✅ Backports have been createdDetails
|
) (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
4 participants
Coprocessor deserialized GraphQL response using serde_json directly that resulted in
data: nullwere 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
Footnotes
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. ↩