Skip to content

refactor: response generation and header aggregation#692

Closed
ardatan wants to merge 4 commits intomainfrom
retake-response-header-handling
Closed

refactor: response generation and header aggregation#692
ardatan wants to merge 4 commits intomainfrom
retake-response-header-handling

Conversation

@ardatan
Copy link
Member

@ardatan ardatan commented Jan 20, 2026

Retake of #652 after #665

  • Extract the execution result generation from PipelineError to a impl From<PipelineError> for FailedExecutionResult { so that the result(as an object) is generated independent from HTTP
  • PlanExecutionResult which is the return type of the plan executor now returns the response bytes as body: Vec<u8> and response_headers_aggregator instead of creating a HeaderMap and returning it. Previously, the plan executor was creating http::HeaderMap, then filling it, then returning it, then the request handler was iterating over it to modify the created ntex::Response's headers. Now the aggregator directly modifies the created Response object in the pipeline handler
  • Since the response header aggregation is done in the pipeline handler, HeaderPropagationError is added to PipelineError enum.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @ardatan, 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 response generation and HTTP header handling within the router. By decoupling the creation of FailedExecutionResult from HTTP-specific constructs and shifting header aggregation responsibilities to the pipeline handler, the changes aim to improve modularity, reduce intermediate data structures, and provide more direct control over the final HTTP response. This leads to a cleaner separation of concerns and potentially more efficient header processing.

Highlights

  • Decoupled Error Response Generation: The logic for generating FailedExecutionResult from PipelineError has been extracted into a dedicated impl From<PipelineError> for FailedExecutionResult, making error responses independent of HTTP specifics.
  • Streamlined Header Aggregation: The PlanExecutionOutput now directly provides a response_headers_aggregator instead of a pre-built http::HeaderMap. The actual modification of the ntex::Response headers now occurs within the pipeline handler, allowing for direct manipulation of the HTTP response object.
  • Enhanced Error Handling for Headers: A new HeaderPropagationError variant has been added to the PipelineError enum, enabling proper error reporting for issues encountered during header propagation within the pipeline handler.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a valuable refactoring of response generation and header aggregation. By moving header aggregation to the pipeline handler and changing PlanExecutionResult to return a response_headers_aggregator, you've effectively eliminated an intermediate HeaderMap creation, which is a good performance optimization. The extraction of FailedExecutionResult generation logic into a From implementation for PipelineError also improves separation of concerns and makes the code cleaner. Overall, these are solid improvements to the codebase. I have one suggestion to further improve readability.

@github-actions
Copy link

github-actions bot commented Jan 20, 2026

k6-benchmark results

     ✓ response code was 200
     ✓ no graphql errors
     ✓ valid response structure

     █ setup

     checks.........................: 100.00% ✓ 231045      ✗ 0    
     data_received..................: 6.7 GB  224 MB/s
     data_sent......................: 90 MB   3.0 MB/s
     http_req_blocked...............: avg=4.12µs   min=751ns   med=1.83µs  max=7.77ms   p(90)=2.6µs   p(95)=2.98µs  
     http_req_connecting............: avg=1.21µs   min=0s      med=0s      max=2.89ms   p(90)=0s      p(95)=0s      
     http_req_duration..............: avg=18.99ms  min=1.99ms  med=18.03ms max=128.31ms p(90)=26.07ms p(95)=29.2ms  
       { expected_response:true }...: avg=18.99ms  min=1.99ms  med=18.03ms max=128.31ms p(90)=26.07ms p(95)=29.2ms  
     http_req_failed................: 0.00%   ✓ 0           ✗ 77035
     http_req_receiving.............: avg=144.21µs min=25.62µs med=40.52µs max=37.74ms  p(90)=96.62µs p(95)=413.69µs
     http_req_sending...............: avg=25.78µs  min=5.53µs  med=10.85µs max=25.57ms  p(90)=16.71µs p(95)=29.9µs  
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s      
     http_req_waiting...............: avg=18.82ms  min=1.93ms  med=17.9ms  max=100.17ms p(90)=25.82ms p(95)=28.87ms 
     http_reqs......................: 77035   2562.318954/s
     iteration_duration.............: avg=19.47ms  min=4.79ms  med=18.4ms  max=257.15ms p(90)=26.55ms p(95)=29.74ms 
     iterations.....................: 77015   2561.653719/s
     vus............................: 50      min=50        max=50 
     vus_max........................: 50      min=50        max=50 

@github-actions
Copy link

github-actions bot commented Jan 20, 2026

🐋 This PR was built and pushed to the following Docker images:

Image Names: ghcr.io/graphql-hive/router

Platforms: linux/amd64,linux/arm64

Image Tags: ghcr.io/graphql-hive/router:pr-692 ghcr.io/graphql-hive/router:sha-99410d7

Docker metadata
{
"buildx.build.ref": "builder-cbf52df3-9af8-4c41-ad67-b6d0e0c5acb2/builder-cbf52df3-9af8-4c41-ad67-b6d0e0c5acb20/jnvpj4ezm1ohp93td3j6mnuap",
"containerimage.descriptor": {
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "digest": "sha256:39bf35b68a2b1f3161c721a06ec01f78daac475d9db6888d3791237d2c445f30",
  "size": 1609
},
"containerimage.digest": "sha256:39bf35b68a2b1f3161c721a06ec01f78daac475d9db6888d3791237d2c445f30",
"image.name": "ghcr.io/graphql-hive/router:pr-692,ghcr.io/graphql-hive/router:sha-99410d7"
}

@ardatan ardatan closed this Jan 20, 2026
@ardatan ardatan reopened this Jan 20, 2026
@ardatan ardatan closed this Jan 20, 2026
@ardatan ardatan deleted the retake-response-header-handling branch January 21, 2026 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant