fix: use bytes as values in proto to support non-utf8 payloads#209
fix: use bytes as values in proto to support non-utf8 payloads#209
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
WalkthroughThis pull request introduces several updates across the project's configuration and code files. The changes include upgrading the GolangCI linter version, updating PHP and Go dependency versions, and modifying type conversion and error handling in HTTP-related code. The modifications focus on improving type safety, error handling, and dependency management in the project's GitHub Actions workflows and Go code. Changes
Assessment against linked issues
Poem
Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
handler/convert.go (1)
20-23: LGTM! Proper handling of non-UTF-8 header values.The conversion of header values to byte slices ensures proper handling of non-UTF-8 encoded data, which aligns well with the PR objective.
Consider pre-allocating the Value slice to reduce allocations:
if resp[k] == nil { - resp[k] = &httpV1proto.HeaderValue{} + resp[k] = &httpV1proto.HeaderValue{ + Value: make([][]byte, 0, len(v)) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.github/workflows/linters.yml(1 hunks).github/workflows/linux.yml(1 hunks)go.mod(1 hunks)handler/convert.go(2 hunks)handler/response.go(3 hunks)tests/go.mod(1 hunks)tests/handler_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: HTTP plugin (Go stable, PHP 8.4, OS ubuntu-latest)
🔇 Additional comments (8)
handler/convert.go (1)
41-41: LGTM! Consistent byte-based handling for cookie values.Converting cookie values to byte slices maintains consistency with header handling and supports non-UTF-8 data.
handler/response.go (3)
57-57: LGTM! Safe conversion for HTTP/2 push paths.Converting byte slices to strings for HTTP/2 push paths is necessary as
http.Pusherrequires string paths. The conversion is safe here since paths should be valid UTF-8.
72-72: LGTM! Proper header value conversion.Converting byte slices to strings for header values is required as
http.Headeroperates with strings. This maintains compatibility with Go's HTTP package while allowing internal byte-based storage.
95-96: LGTM! Simplified flusher implementation.The new flusher implementation is more idiomatic and efficient, using type assertion instead of the deprecated
http.NewResponseController..github/workflows/linters.yml (1)
21-21: Verify golangci-lint v1.62 changelog.The linter version upgrade from v1.61 to v1.62 may introduce new checks or changes in existing ones.
Please check the changelog at https://github.com/golangci/golangci-lint/releases/tag/v1.62.0 to ensure compatibility with the codebase.
.github/workflows/linux.yml (1)
20-20: Consider deferring PHP 8.4 upgrade.Using PHP 8.4 in the test matrix might be premature as it's currently in alpha/development stage. This could potentially lead to CI instability.
Let's verify the current status of PHP 8.4:
tests/go.mod (2)
84-84: Verify API version compatibility with non-UTF-8 payload changes.The update to
roadrunner-server/api/v4 v4.18.1should be verified to ensure it contains the necessary changes for supporting non-UTF-8 payloads.Let's check the changelog and related commits:
✅ Verification successful
API update is aligned with non-UTF-8 payload handling
The update to v4.18.1 introduces necessary changes for handling non-UTF-8 payloads by switching to byte-based values in the HTTP protocol implementation. This version bump is appropriate and required for the PR's objective.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check changelog and commits for the API update # Get the changelog or commit history between v4.17.0 and v4.18.1 gh api repos/roadrunner-server/api/compare/v4.17.0...v4.18.1 --jq '.commits[].commit.message'Length of output: 476
Line range hint
1-1: Request core implementation files.The current files show only peripheral changes (PHP version update and API dependency bump). To properly review the non-UTF-8 payload handling fix, please provide the core implementation files where the actual changes for handling non-UTF-8 payloads are made.
Let's check for related files:
| go func() { | ||
| err = hs.ListenAndServe() | ||
| if err != nil && !errors.Is(http.ErrServerClosed, err) { | ||
| if err != nil && !errors.Is(err, http.ErrServerClosed) { |
There was a problem hiding this comment.
💡 Codebase verification
Multiple instances of incorrect error comparison order found
The argument order in errors.Is and stderr.Is calls should be fixed across the codebase. The error being checked should be the first argument, followed by the target error.
Locations needing fixes:
tests/handler_test.go: Multiple instances oferrors.Is(http.ErrServerClosed, err)tests/uploads_test.go: Multiple instances oferrors.Is(http.ErrServerClosed, errL)servers/fcgi/fcgi.go: Two instances ofstderr.Is(err, http.ErrServerClosed)servers/http11/http.go: Two instances ofstderr.Is(err, http.ErrServerClosed)servers/https/https.go: Three instances ofstderr.Is(err, http.ErrServerClosed)
🔗 Analysis chain
Fixed incorrect error comparison order.
The change corrects the argument order in errors.Is(). The error being checked should be the first argument, followed by the target error.
Let's check if this issue exists elsewhere in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potentially incorrect error comparisons
ast-grep --pattern 'errors.Is($target, $err)' | grep -v 'errors.Is(err,'
Length of output: 74
Script:
#!/bin/bash
# Search for all errors.Is usage with context
rg "errors\.Is\(" -A 2 -B 2
# Also search for any potential type assertions or comparisons with error types
rg "ErrServerClosed" -A 2 -B 2
Length of output: 17665
1 participant
Reason for This PR
closes: roadrunner-server/roadrunner#2104
Description of Changes
[]bytesinstead of utf-8 strings to pass any data to the PHP worker (even non utf-8).License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
Chores
github.com/roadrunner-server/api/v4to v4.18.1Bug Fixes
Tests