Revert "refactor(executor): handle header propagation outside of the plan executor"

[ardatan]

Reverts #652

[gemini-code-assist]

Summary of Changes

Hello @ardatan, I'm Gemini Code Assist¹! 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 serves as a revert of a prior refactoring that attempted to externalize header propagation from the plan executor. The core impact is a simplification of the header management within the router's pipeline, moving from an optional header aggregator to a direct HeaderMap for response headers. This change removes specific error types related to header propagation and modifies how HTTP responses are constructed to directly incorporate the aggregated headers, ensuring a more integrated and potentially less complex approach to header handling.

Highlights

• Revert of Header Propagation Refactor: This pull request fully reverts a previous refactoring (refactor(executor): handle header propagation outside of the plan executor #652) that aimed to handle header propagation logic outside of the plan executor, effectively rolling back the changes introduced in that PR.
• Simplified Header Handling in Pipeline: The PipelineError enum no longer includes HeaderPropagation errors, and the PlanExecutionOutput now directly uses an http::HeaderMap to store response headers, simplifying the overall header management within the execution pipeline.
• Direct HeaderMap Manipulation: The modify_client_response_headers function and related logic have been updated to directly manipulate an http::HeaderMap instead of an ntex::http::ResponseBuilder, streamlining how headers are added to the final HTTP response.
• Usage Reporting Update: The usage reporting mechanism has been adjusted to pass the complete PlanExecutionOutput object, providing more context than just the error count for reporting purposes.

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. ↩

[gemini-code-assist]

Code Review

This pull request refactors the handling of HTTP response headers within the router. It removes the HeaderRuleRuntimeError from the PipelineError enum and streamlines error response generation to directly build ResponseBuilder instances. The PlanExecutionOutput struct is updated to return a HeaderMap directly instead of an optional ResponseHeaderAggregator. Consequently, the execute_query_plan function now populates this HeaderMap using modify_client_response_headers before returning it. The graphql_request_handler in the pipeline is updated to iterate over this HeaderMap to set response headers. Additionally, the usage reporting mechanism is modified to accept the full PlanExecutionOutput instead of just the error count. A review comment highlights a critical issue in the graphql_request_handler where the iteration over response_headers (a HeaderMap) incorrectly handles multi-valued headers like Set-Cookie, causing all but the first value to be dropped due to only processing Some(HeaderName) entries, which could lead to security vulnerabilities.

[gemini-code-assist]

The loop used to copy aggregated headers from the query plan execution result to the final HTTP response incorrectly handles multi-valued headers. In Rust's http::HeaderMap, iteration yields (Option<HeaderName>, HeaderValue), where the name is None for subsequent values of the same header name. The current implementation only processes iterations where the header name is Some, effectively dropping all but the first value for headers like Set-Cookie. This can lead to security issues if critical cookies (like session or CSRF cookies) are lost.

    for (header_name, header_value) in response_headers.iter() {
        response_builder.header(header_name.clone(), header_value.clone());
    }

[github-actions]

✅ k6-benchmark results

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

     █ setup

     checks.........................: 100.00% ✓ 242493      ✗ 0    
     data_received..................: 7.1 GB  235 MB/s
     data_sent......................: 95 MB   3.2 MB/s
     http_req_blocked...............: avg=3.82µs   min=662ns   med=1.73µs  max=13.26ms  p(90)=2.51µs  p(95)=2.9µs   
     http_req_connecting............: avg=995ns    min=0s      med=0s      max=2.62ms   p(90)=0s      p(95)=0s      
     http_req_duration..............: avg=18.13ms  min=1.61ms  med=17.07ms max=177.14ms p(90)=25.13ms p(95)=28.42ms 
       { expected_response:true }...: avg=18.13ms  min=1.61ms  med=17.07ms max=177.14ms p(90)=25.13ms p(95)=28.42ms 
     http_req_failed................: 0.00%   ✓ 0           ✗ 80851
     http_req_receiving.............: avg=183.05µs min=27.58µs med=45.52µs max=125.04ms p(90)=88.87µs p(95)=401.35µs
     http_req_sending...............: avg=22.8µs   min=4.68µs  med=10.26µs max=19.8ms   p(90)=15.41µs p(95)=24.69µs 
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s      
     http_req_waiting...............: avg=17.93ms  min=1.55ms  med=16.93ms max=53.19ms  p(90)=24.86ms p(95)=28.08ms 
     http_reqs......................: 80851   2690.157204/s
     iteration_duration.............: avg=18.55ms  min=5.08ms  med=17.41ms max=295.48ms p(90)=25.56ms p(95)=28.89ms 
     iterations.....................: 80831   2689.491743/s
     vus............................: 50      min=50        max=50 
     vus_max........................: 50      min=50        max=50 

[github-actions]

🐋 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-710 ghcr.io/graphql-hive/router:sha-de96faa

Docker metadata

{
"buildx.build.ref": "builder-856cf22b-8cd8-411d-a537-ec3bde5d98ae/builder-856cf22b-8cd8-411d-a537-ec3bde5d98ae0/6ho8hz9pufs5zfexq3bslcbcb",
"containerimage.descriptor": {
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "digest": "sha256:f52ee63f6e726af7a83f2c03952d1598d40ceeb52ccca56dc2fe27ed68baceae",
  "size": 1609
},
"containerimage.digest": "sha256:f52ee63f6e726af7a83f2c03952d1598d40ceeb52ccca56dc2fe27ed68baceae",
"image.name": "ghcr.io/graphql-hive/router:pr-710,ghcr.io/graphql-hive/router:sha-de96faa"
}