-
Notifications
You must be signed in to change notification settings - Fork 258
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
refactor: move server.responseHeaders to server.headers.custom #1401
Conversation
WalkthroughThe update involves shifting the handling of server response headers, replacing Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1401 +/- ##
==========================================
- Coverage 88.31% 88.31% -0.01%
==========================================
Files 127 127
Lines 13398 13389 -9
==========================================
- Hits 11833 11825 -8
+ Misses 1565 1564 -1 ☔ View full report in Codecov by Sentry. |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
tests/snapshots/execution_spec__cache-control.md_merged.snap
is excluded by:!**/*.snap
tests/snapshots/execution_spec__custom-headers.md_merged.snap
is excluded by:!**/*.snap
Files selected for processing (8)
- generated/.tailcallrc.graphql (2 hunks)
- generated/.tailcallrc.schema.json (2 hunks)
- src/config/headers.rs (2 hunks)
- src/config/server.rs (3 hunks)
- tests/execution/custom-headers.md (2 hunks)
- tests/execution/test-response-header-value.md (1 hunks)
- tests/execution/test-response-headers-multi.md (1 hunks)
- tests/execution/test-response-headers-name.md (1 hunks)
Additional comments: 8
tests/execution/test-response-headers-name.md (1)
- 6-6: The updated schema declaration for custom headers correctly implements the proposed changes in the PR. This test case effectively demonstrates the new structure for defining custom headers under
server.headers.custom
.tests/execution/test-response-header-value.md (1)
- 6-6: The test case effectively verifies the handling of newline characters in header values within the new
headers.custom
structure. This ensures that complex header values are supported in the updated configuration.tests/execution/test-response-headers-multi.md (1)
- 6-6: The test case effectively verifies the handling of multiple custom headers within the new
headers.custom
structure. This ensures that the updated configuration supports multiple custom headers without issues.tests/execution/custom-headers.md (1)
- 3-10: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [6-17]
The JSON configuration correctly implements the new structure for custom headers, aligning with the PR objectives. This change ensures that custom headers are organized under
headers.custom
, enhancing clarity and maintainability.src/config/headers.rs (1)
- 17-17: The addition of the
custom
field to theHeaders
struct is correctly implemented, allowing for a structured approach to defining custom headers in server responses. This change aligns with the PR objectives and enhances the flexibility and clarity of header management.src/config/server.rs (1)
- 163-171: The refactoring of the
Server
struct to utilize the newheaders.custom
structure for managing custom headers is correctly implemented. This change ensures that the handling of response headers aligns with the updated configuration, enhancing clarity and maintainability.generated/.tailcallrc.graphql (1)
- 574-579: The introduction of the
custom
directive for handling key-value pairs in server responses, along with the update to thecacheControl
input, correctly implements the proposed refactor. These changes enhance the clarity and flexibility of header management, aligning with the PR objectives.generated/.tailcallrc.schema.json (1)
- 1218-1227: The addition of the
custom
field underserver.headers
is aligned with the PR's objectives to streamline the configuration of custom headers in server responses. This change makes the schema more intuitive and organized, facilitating easier management of custom headers. The structure and type definitions appear to be correctly implemented, ensuring that custom headers can be defined as key-value pairs.One thing to consider is ensuring that all existing configurations and documentation that previously used
responseHeaders
are updated to reflect this change. This includes updating any examples, guides, or external documentation that reference the old configuration.
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- tests/execution/custom-headers.md (1 hunks)
- tests/execution/test-response-header-value.md (1 hunks)
- tests/execution/test-response-headers-multi.md (1 hunks)
- tests/execution/test-response-headers-name.md (1 hunks)
Additional comments: 4
tests/execution/test-response-headers-name.md (1)
- 6-6: The schema declaration correctly transitions from
responseHeaders
toheaders
with acustom
field, aligning with the PR's objectives. Using emojis as keys is unconventional but acceptable if it fits your use case. Ensure consistent usage across your application.tests/execution/test-response-header-value.md (1)
- 6-6: The schema declaration correctly implements the transition to
headers
with acustom
field. Including newline characters (\n
) in header values is unusual and should be validated to ensure it behaves as expected in server responses.tests/execution/test-response-headers-multi.md (1)
- 6-6: The schema declaration correctly supports multiple custom headers. Caution: using spaces in header keys (
"a b"
,"a c"
) is unconventional and might not be universally supported. Verify compatibility with your server and client implementations.tests/execution/custom-headers.md (1)
- 6-17: The JSON configuration correctly transitions from
"responseHeaders"
to"headers"
with a nested"custom"
array, aligning with the PR's objectives. The structure and key-value pairs for custom headers are correctly formatted.
This PR moves server.responseHeaders to server.headers.custom
Issue Reference(s):
Fixes #1400
Build & Testing:
cargo test
successfully../lint.sh --mode=fix
to fix all linting issues raised by./lint.sh --mode=check
./claim #1400
Summary by CodeRabbit
custom
directive for specifying key-value pairs in server responses, enhancing customization.