refactor: move server responseHeaders to server headers custom #1402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent updates involve transitioning the 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 (
|
github-actions
bot
added
the
type: chore
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1402 +/- ##
==========================================
+ Coverage 88.44% 88.50% +0.06%
==========================================
Files 127 127
Lines 13470 13532 +62
==========================================
+ Hits 11914 11977 +63
+ Misses 1556 1555 -1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
tests/snapshots/execution_spec__custom-headers.md_merged.snapis excluded by:!**/*.snaptests/snapshots/execution_spec__test-response-header-value.md_errors.snapis excluded by:!**/*.snaptests/snapshots/execution_spec__test-response-headers-multi.md_errors.snapis excluded by:!**/*.snaptests/snapshots/execution_spec__test-response-headers-name.md_errors.snapis excluded by:!**/*.snap
Files selected for processing (10)
- generated/.tailcallrc.graphql (2 hunks)
- generated/.tailcallrc.schema.json (2 hunks)
- src/blueprint/server.rs (1 hunks)
- src/config/headers.rs (2 hunks)
- src/config/mod.rs (1 hunks)
- src/config/server.rs (4 hunks)
- 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: 13
tests/execution/test-response-headers-name.md (1)
- 6-6: The change from
responseHeaderstoheaders: {custom: [{key: "🤣", value: "a"}]}aligns with the PR objectives of structuring response headers more clearly. This update in the test documentation ensures that the new method is correctly represented.tests/execution/test-response-header-value.md (1)
- 6-6: The update to use
headers: {custom: [{key: "a", value: "a \n b"}]}correctly demonstrates handling header values with special characters, such as newline characters, in the newheaders.customstructure. This ensures that the test cases cover scenarios with complex header values.tests/execution/test-response-headers-multi.md (1)
- 6-6: The transition to
headers: {custom: [{key: "a b", value: "a \n b"}, {key: "a c", value: "a \n b"}]}for specifying multiple custom headers is correctly implemented in this test case. It showcases the flexibility of the new structure in handling multiple headers, including those with spaces in keys and special characters in values.src/config/mod.rs (1)
- 4-4: Exporting the
headersmodule publicly inconfig/mod.rsis a necessary step to support the newheaders.customstructure for response headers. This change ensures that theHeadersstruct and related functionality are accessible where needed.tests/execution/custom-headers.md (1)
- 6-17: The JSON configuration example correctly demonstrates the new way of specifying custom headers using
headers.customwithin theserverobject. This change aligns with the PR objectives and ensures that the documentation and test cases reflect the new structure for response headers.src/config/headers.rs (1)
- 15-19: Adding the
customfield of typeVec<KeyValue>to theHeadersstruct is a key part of the refactor, enabling the structured specification of custom headers in server responses. This change is well-implemented and aligns with the PR objectives.src/blueprint/server.rs (1)
- 176-177: The addition of tracing for "custom" and "headers" in the
handle_response_headersfunction enhances the visibility and debugging capabilities for handling custom headers. This update is a thoughtful improvement that aligns with the overall refactor's goals.src/config/server.rs (4)
- 163-171: The refactoring of the
get_response_headersmethod to utilize the newheaders.customstructure is correctly implemented. This change ensures that the server struct aligns with the new way of handling response headers, enhancing the structure and maintainability of the code.- 182-196: The introduction of the
merge_key_value_iteratorsmethod is a significant improvement for merging key-value pairs, especially in the context of handling custom headers. This method provides a more robust and flexible way to merge headers, aligning with the refactor's objectives.- 200-210: The updates to the
merge_rightmethod, particularly the handling of theheadersfield, demonstrate a thoughtful approach to merging server configurations. The use ofmerge_key_value_iteratorsfor merging custom headers ensures that the new structure is correctly maintained during merges.- 223-223: The update to handle the merging of
varsusing themerge_key_value_iteratorsmethod is a good practice, ensuring consistency in how key-value pairs are merged across different fields of theServerstruct.generated/.tailcallrc.graphql (1)
- 574-579: The introduction of the
customfield within theHeadersinput type is a positive change, enhancing the flexibility and structure in specifying custom headers for server responses. However, it would be beneficial to include detailed documentation for thecustomfield to ensure users understand its purpose and how to use it effectively.generated/.tailcallrc.schema.json (1)
- 1218-1224: The addition of the
customfield underheadersis well-defined, with a clear description and appropriate type specification (arrayof key-value pairs). This aligns with the PR objectives to transition to a more structured approach for specifying response headers. The schema reference ($ref) toKeyValuefor items ensures that each header is represented as a key-value pair, which is a standard and flexible way to handle headers. This change should make the handling of custom headers more intuitive and flexible for developers.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tests/snapshots/execution_spec__test-response-header-merge.md_merged.snapis excluded by:!**/*.snap
Files selected for processing (1)
- tests/execution/test-response-header-merge.md (1 hunks)
Additional comments: 2
tests/execution/test-response-header-merge.md (2)
- 3-16: The transition to using
headers.customfor specifying response headers is correctly implemented in this schema definition. This aligns well with the PR objectives of refactoring the codebase for more structured header management.- 18-31: Similarly, this schema definition correctly utilizes the
headers.customdirective for response headers. It's good to see consistency in applying the new method across different test cases.
ologbonowiwi
force-pushed
the
refactor/#1400/move-server-responseHeaders-to-server-headers-custom
branch
from
0a9ebe8 to
a784132
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (5)
tests/snapshots/execution_spec__custom-headers.md_merged.snapis excluded by:!**/*.snaptests/snapshots/execution_spec__test-response-header-merge.md_merged.snapis excluded by:!**/*.snaptests/snapshots/execution_spec__test-response-header-value.md_errors.snapis excluded by:!**/*.snaptests/snapshots/execution_spec__test-response-headers-multi.md_errors.snapis excluded by:!**/*.snaptests/snapshots/execution_spec__test-response-headers-name.md_errors.snapis excluded by:!**/*.snap
Files selected for processing (11)
- generated/.tailcallrc.graphql (2 hunks)
- generated/.tailcallrc.schema.json (2 hunks)
- src/blueprint/server.rs (1 hunks)
- src/config/headers.rs (2 hunks)
- src/config/mod.rs (1 hunks)
- src/config/server.rs (4 hunks)
- tests/execution/custom-headers.md (1 hunks)
- tests/execution/test-response-header-merge.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)
Check Runs (1)
Check Examples completed (6)
Files skipped from review as they are similar to previous changes (5)
- generated/.tailcallrc.graphql
- generated/.tailcallrc.schema.json
- src/blueprint/server.rs
- src/config/headers.rs
- src/config/mod.rs
Additional comments: 8
tests/execution/test-response-headers-name.md (1)
- 6-6: The update from
responseHeaderstoheaders.customaligns well with the PR objectives, ensuring that the test case reflects the new method of specifying response headers. This change is correctly implemented in the test case.tests/execution/test-response-header-value.md (1)
- 6-6: The update to use
headers.customwith values containing newline characters is correctly implemented, reflecting the PR's objective to handle response headers more flexibly. This test case accurately captures the intended functionality.tests/execution/test-response-headers-multi.md (1)
- 6-6: The implementation of multiple custom headers using
headers.customis correctly demonstrated in this test case, aligning with the PR's objectives to enhance header management. This change is accurately reflected.tests/execution/test-response-header-merge.md (1)
- 4-4: The test cases for merging response headers using
headers.customare correctly implemented, showcasing the intended functionality as per the PR objectives. This demonstrates the flexibility and robustness of the new header management approach.tests/execution/custom-headers.md (1)
- 6-6: The renaming and new structure for specifying custom headers within the JSON configuration is correctly implemented in this test case. This aligns with the PR's objectives to enhance header management through a more structured approach.
src/config/server.rs (3)
- 163-171: The refactoring of the
get_response_headersmethod to handle the newheaders.customstructure is correctly implemented, aligning with the PR's objectives for enhanced header management. This change improves the method's clarity and maintainability.- 182-196: The introduction of the
merge_key_value_iteratorsmethod for merging key-value pairs is a good practice, enhancing the code's maintainability and modularity. This method is correctly implemented and aligns with the PR's objectives for robust header management.- 179-213: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [197-223]
The updates to the
merge_rightmethod, including the use ofmerge_key_value_iteratorsfor merging headers and vars, are correctly implemented. These changes improve the method's robustness and maintainability, aligning with the PR's objectives for enhanced header management.
…eHeaders-to-server-headers-custom
…eHeaders-to-server-headers-custom
tusharmath
reviewed
Mar 13, 2024
tusharmath
reviewed
Mar 13, 2024
tusharmath
reviewed
Mar 13, 2024
…-server-headers-custom' of github.com:ologbonowiwi/tailcall into refactor/tailcallhq#1400/move-server-responseHeaders-to-server-headers-custom
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tests/snapshots/execution_spec__test-response-header-merge.md_merged.snapis excluded by:!**/*.snap
Files selected for processing (4)
- src/blueprint/server.rs (2 hunks)
- src/config/headers.rs (2 hunks)
- src/config/key_values.rs (1 hunks)
- src/config/server.rs (5 hunks)
Check Runs (3)
codecov/patch completed (1)
- src/config/key_values.rs: 37-38: Added lines #L37 - L38 were not covered by tests
Check Examples completed (6)
Run Formatter and Lint Check completed (3)
- src/config/key_values.rs: 27-27: Diff in /home/runner/work/tailcall/tailcall/src/config/key_values.rs
- src/config/server.rs: 195-195: Diff in /home/runner/work/tailcall/tailcall/src/config/server.rs
Files skipped from review as they are similar to previous changes (2)
- src/blueprint/server.rs
- src/config/headers.rs
Additional comments: 2
src/config/server.rs (2)
- 163-172: The refactoring of the
get_response_headersmethod to utilize the newheaders.customfield aligns well with the updated approach to managing response headers. The implementation is concise and correctly handles optional fields, ensuring backward compatibility and ease of use.- 195-213: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [185-210]
The updates to the
merge_rightmethod, including the use ofmerge_headersandmerge_key_value_vecs, enhance the robustness and consistency of header and variable merging. However, there's a redundant operation in mergingvarstwice. Consider removing the fold operation beforemerge_key_value_vecsto streamline the process.- self.vars = other - .vars - .iter() - .fold(self.vars.to_vec(), |mut acc, kv| { - let position = acc.iter().position(|x| x.key == kv.key); - if let Some(pos) = position { - acc[pos] = kv.clone(); - } else { - acc.push(kv.clone()); - }; - acc - }); self.vars = merge_key_value_vecs(&self.vars, &other.vars);Overall, the method's updates align well with the PR's objectives and improve the codebase's maintainability and readability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/config/key_values.rs (1 hunks)
- src/config/server.rs (6 hunks)
Check Runs (1)
Check Examples completed (6)
Files skipped from review as they are similar to previous changes (1)
- src/config/key_values.rs
Additional comments: 3
src/config/server.rs (3)
- 163-172: The refactoring of
get_response_headersto utilizeheaders.customfor response headers is aligned with the PR objectives. This change simplifies the access to custom headers and makes the code more readable. However, ensure that all instances whereget_response_headersis called are updated to handle the new return type if there was any change.- 185-185: The use of
merge_headersin themerge_rightmethod to handle merging ofheadersfields is a good practice. It encapsulates the logic for merging headers, making themerge_rightmethod cleaner and more maintainable. Ensure thatmerge_headerscorrectly handles all edge cases, such as duplicate headers and case sensitivity.- 198-198: The initial approach to merging
varsusing an iterative method is replaced withmerge_key_value_vecs. This change likely improves the readability and maintainability of the code by abstracting the merging logic into a separate function. However, it's important to ensure thatmerge_key_value_vecscorrectly handles duplicates and prioritizes the values fromother.varsoverself.varswhen conflicts arise.
…ailcallhq#1400/move-server-responseHeaders-to-server-headers-custom
ologbonowiwi
commented
Mar 13, 2024
tusharmath
reviewed
Mar 13, 2024
tusharmath
reviewed
Mar 13, 2024
tusharmath
approved these changes
Mar 13, 2024
auto-merge was automatically disabled
March 13, 2024 15:14
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/config/key_values.rs (2 hunks)
Check Runs (2)
Check Examples completed (6)
Run Formatter and Lint Check completed (3)
- src/config/key_values.rs: 162-162: Diff in /home/runner/work/tailcall/tailcall/src/config/key_values.rs
- src/config/key_values.rs: 39-39: iterating on a map's values
Additional comments: 3
src/config/key_values.rs (3)
- 29-40: The
merge_key_value_vecsfunction is implemented correctly and fulfills its purpose of merging two vectors ofKeyValuestructs. However, consider the performance implications of cloning eachKeyValuestruct, especially for large vectors. If performance becomes a concern, you might explore more efficient ways to handle the merging, such as using references or optimizing the cloning process.- 114-167: The tests for
merge_key_value_vecsare comprehensive and well-structured, covering all the essential scenarios including empty vectors, unique keys, and overlapping keys. Good job ensuring that the function behaves as expected across different cases.- 39-39: Iterating over the values of a map to collect them into a vector, as done in the final step of
merge_key_value_vecs, is a standard and efficient pattern for this use case. The implementation is appropriate and does not require changes.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/config/key_values.rs (2 hunks)
Check Runs (1)
Check Examples completed (6)
Files skipped from review as they are similar to previous changes (1)
- src/config/key_values.rs
ologbonowiwi
deleted the
refactor/#1400/move-server-responseHeaders-to-server-headers-custom
branch
ssddOnTop
pushed a commit
that referenced
this pull request
Mar 13, 2024
Co-authored-by: Tushar Mathur <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
headers.customto hold response headersresponseHeaderscases to useheaders.custominstead./lint.sh --mode=fixIssue Reference(s):
Fixes #1400
/claim #1400
Build & Testing:
cargo testsuccessfully../lint.sh --mode=fixto fix all linting issues raised by./lint.sh --mode=check.Checklist:
<type>(<optional scope>): <title>Summary by CodeRabbit
Access-Control-Allow-Originand other headers for downstream services.