refactor: Execution pipeline without HTTP request dependency and better accept header parsing#665
refactor: Execution pipeline without HTTP request dependency and better accept header parsing#665
Conversation
Summary of ChangesHello @enisdenjo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the GraphQL execution pipeline to separate HTTP concerns from the core logic. By centralizing HTTP request processing in a dedicated handler and making the pipeline functions operate on more abstract data, the codebase becomes more modular and easier to extend. This change not only simplifies error management but also lays crucial groundwork for implementing advanced features like GraphQL subscriptions over WebSockets and other streaming protocols, improving the router's flexibility and future capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring that successfully decouples the core execution pipeline from HTTP-specific concerns. The simplification of error handling and the much-improved Accept header parsing logic are excellent changes that enhance modularity and robustness.
My main feedback is regarding the graphql_request_handler function in bin/router/src/pipeline/mod.rs, which has grown quite large as it now orchestrates all the pre-execution steps. I've left a comment suggesting ways to break it down into smaller, more focused functions to improve maintainability, in line with the repository's style guide.
Regarding your question in the PR description, I agree that renaming PipelineErrorVariant to PipelineError would be a good cleanup step now that the old PipelineError struct is gone.
✅
|
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: Docker metadata{
"buildx.build.ref": "builder-a3a09607-20cb-4804-940e-76e63dd8ff47/builder-a3a09607-20cb-4804-940e-76e63dd8ff470/unsvemggwgcfkhl03hyiibhch",
"containerimage.descriptor": {
"mediaType": "application/vnd.oci.image.index.v1+json",
"digest": "sha256:63f653716f55bb46a49fef7dd662aaf9170df3f470325951711425051ae8072b",
"size": 1609
},
"containerimage.digest": "sha256:63f653716f55bb46a49fef7dd662aaf9170df3f470325951711425051ae8072b",
"image.name": "ghcr.io/graphql-hive/router:pr-665,ghcr.io/graphql-hive/router:sha-38f6694"
} |
dotansimha
left a comment
There was a problem hiding this comment.
LGTM. changesets are missing (for internal changes that matters, and dry-run fix).
Let's wait for @kamilkisiela 's response as well before merging
# Conflicts: # Cargo.toml # bin/router/Cargo.toml # bin/router/src/lib.rs
| fn try_from(media_type: &MediaType) -> Result<Self, Self::Error> { | ||
| let essence_media_type_string = media_type.essence().to_string(); | ||
| if essence_media_type_string == SingleContentType::GraphQLResponseJSON.as_ref() { | ||
| Ok(SingleContentType::GraphQLResponseJSON) | ||
| } else if essence_media_type_string == SingleContentType::JSON.as_ref() { | ||
| Ok(SingleContentType::JSON) | ||
| } else { | ||
| Err("Unsupported single content type") | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
We could avoid allocating the String by laveraging those individual components https://github.com/picoHz/mediatype/blob/8150f29ff62a0214cb9ee534f9a63ce694723987/src/media_type.rs#L34-L41 to match the media type with the enum variant.
That's one allocation that does not dot affect performance too much, so it's fine to keep as is.
There was a problem hiding this comment.
I did some numbers and it'd be 4x faster, but we're talking 70-113 ns vs 16-36 ns
There was a problem hiding this comment.
I originally used consts everywhere, which is the most optimal way. But I had to fork the crate, because of this since the maintainer does not budge.
On @dotansimha's request I removed the fork dependency and refactored all this in
90c2657.
There was a problem hiding this comment.
Do you think we should go back to the fork @kamilkisiela?
There was a problem hiding this comment.
Or just avoid the String alloc?
There was a problem hiding this comment.
It's fine as is :) there's a way to improve it, but we're talking about 4x speed-up on a small number, so it does not affect the overall performance noticeably anyway
There was a problem hiding this comment.
if we ever see this being a culprit (I don't think we ever will), we will fight it
There was a problem hiding this comment.
let's not leave anything up to chance. I changed it to avoid allocations, wdyt?
|
@enisdenjo if @dotansimha 's comments are covered, feel free to merge it |
Ref ROUTER-240
execution_pipelinedoes not depend on the HTTP anymore,graphql_request_handlerdoes all of the HTTP request handling.As a result, this allows the
execution_pipelineand all of its dependencies to return HTTP request independant errors,simplifying the error handling and removing the(mostly already done in #637).PipelineErrorleaving onlyPipelineErrorVariantAlso, this PR allows the upcoming subscriptions PR (#620) to include also WebSockets implementation1 and not be too huge for review. Because of this PR being a prerequisite, there's some subscriptions stuff with a
// coming soonjust to expidite the process of subscriptions and to have better understanding on whats coming.Besides the WS stuff, removing the HTTP request dependency from
execution_pipelineallows us to have onit tests for the pipeline as well as mocking (which might be a thing when we get the #305).Furthermore we now use
headers-acceptcrate to negotiate the content type as per the official HTTP spec (correct parameter handling and q-weighting).Bonus points go to: expose query plan dry-run is now really dry. Also added tests.
Why do we return both single and streaming accepted content type when parsing the accept header?
Some clients (like Apollo) sends
Accept: multipart/mixed;subscriptionSpec="1.0", application/jsonheaders indicating that the client can also stream. This enables router to send single results as JSON responses but stream multiple results as multipart. Router will inteligently choose one or the other.As a bonus, it also allows the router to stream single responses.
TODOs
Footnotes
At the time of writing feat: Subscriptions #620 does not implement WebSockets, but merging this PR will allow it inthe PR. ↩