-
Notifications
You must be signed in to change notification settings - Fork 2
feat(router): CORS #473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(router): CORS #473
Conversation
Summary of ChangesHello @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 PR implements comprehensive CORS support for the router, allowing flexible configuration of cross-origin policies including origin matching (exact, wildcard, regex), method/header allowances, credential handling, and preflight caching. It integrates CORS processing directly into the request pipeline, ensuring proper header management for both preflight and actual requests. Highlights
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.
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 CORS support to the router, which is a great addition. The implementation is quite thorough, covering various CORS configurations and including a good set of tests. I've found a critical issue in the handling of preflight requests and a few other areas for improvement regarding correctness, maintainability, and documentation. Please see my detailed comments below.
✅
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: Docker metadata{
"buildx.build.ref": "builder-b5d7873b-46ec-4063-9f7b-f8b5a171e4a3/builder-b5d7873b-46ec-4063-9f7b-f8b5a171e4a30/s084xdtd0lz4yknb6ewkx9waf",
"containerimage.descriptor": {
"mediaType": "application/vnd.oci.image.index.v1+json",
"digest": "sha256:63b34b013e15025521d553ba06483487a8893a3cb501f5c52b8625f535604239",
"size": 1609
},
"containerimage.digest": "sha256:63b34b013e15025521d553ba06483487a8893a3cb501f5c52b8625f535604239",
"image.name": "ghcr.io/graphql-hive/router:pr-473,ghcr.io/graphql-hive/router:sha-454ddff"
} |
@gemini-code-assist review again |
There was a problem hiding this 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 comprehensive CORS support, including new configuration options, a dedicated CORS processing module, and integration into the request pipeline. The implementation is well-structured, featuring optimizations for various CORS scenarios and thorough documentation updates. However, I've identified a critical compilation error and two high-severity issues concerning configuration validation and consistency. These issues could negatively affect the developer experience by silently ignoring parts of the configuration or causing deserialization errors. Addressing these points will significantly improve the robustness and usability of the new CORS feature.
@gemini-code-assist review again |
There was a problem hiding this 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 CORS support to the router, including adding a regex
dependency, implementing CORS policy configuration, and modifying the GraphQL endpoint handler to perform CORS checks. The code adds a new module for CORS handling, updates the shared state to include CORS configuration, and modifies the router's configuration to support CORS policies. I have identified a critical issue related to error handling in the CORS configuration loading process.
@gemini-code-assist review again |
There was a problem hiding this 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 comprehensive CORS support to the router. The implementation is well-structured, adding a new cors.rs
module for the core logic, along with corresponding configuration options, documentation, and tests. My review focuses on enhancing the robustness, readability, and performance of the new CORS implementation. I've suggested changes to avoid unnecessary allocations, handle non-UTF8 headers more gracefully, prevent duplicate Vary
header values, and use more idiomatic Rust patterns for safer and clearer code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made some changes
Ref ROUTER-112
Closes #319
[x] Policies