feature: http badrequest code instead of 500, update tests, modernize#225
Merged
feature: http badrequest code instead of 500, update tests, modernize#225
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR improves error handling by returning HTTP 400 (Bad Request) instead of HTTP 500 for broken multipart requests, modernizes tests throughout the tests/ package, and replaces the brittle io.Pipe/stderr-based OTEL span verification with an in-memory exporter.
Changes:
handler/handler.go: Return HTTP 400 whenio.EOForio.ErrUnexpectedEOFis returned during request parsing (e.g. truncated multipart body), instead of 500.tests/: Modernize all test goroutines to usewg.Go(func(){...})(Go 1.22+sync.WaitGroup.Go),for range Nidiom, replaceotel.Plugin+ stderr pipe with an in-memorytracetestexporter via a newtracetestMiddleware, and add a newhandler/handler_test.gowith unit tests for the 400/413 behavior.tests/php_test_files/composer.json/tests/go.mod: Replaceguzzlehttp/guzzlev6 adapter with guzzle v7 adapter; promote several indirect Go module dependencies to direct.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
handler/handler.go |
Adds io.EOF/io.ErrUnexpectedEOF check to return HTTP 400 for malformed multipart requests |
handler/uploads.go |
Modernizes goroutine spawning to use wg.Go |
handler/handler_test.go |
New unit tests covering 400/413 multipart/stream errors and handleError behaviour |
tests/http_plugin_test.go |
Replaces wg.Add(1); go func(){ defer wg.Done() ... }() with wg.Go(...) throughout |
tests/http_plugin2_test.go |
Same goroutine modernisation + for range N idiom |
tests/http_plugin3_test.go |
Same goroutine modernisation + for range N idiom |
tests/http_plugin4_test.go |
Same goroutine modernisation |
tests/http_otlp_test.go |
Replaces stderr pipe OTEL verification with in-memory tracetest exporter |
tests/configs/.rr-http-otel.yaml / .rr-http-otel2.yaml |
Remove otel: config block; use tracetestOtel middleware name |
tests/go.mod |
Promotes OTEL and context deps to direct; marks otel/v5 indirect |
tests/php_test_files/composer.json |
Upgrades Guzzle v6 → v7 adapter, removes deprecated packages |
.golangci.yml |
Raises goconst min-occurrences from 3 to 10 |
go.work.sum |
Updated dependency checksums |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 5 to 8. - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](actions/download-artifact@v5...v8) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-version: '8' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [golangci/golangci-lint-action](https://github.com/golangci/golangci-lint-action) from 7 to 9. - [Release notes](https://github.com/golangci/golangci-lint-action/releases) - [Commits](golangci/golangci-lint-action@v7...v9) --- updated-dependencies: - dependency-name: golangci/golangci-lint-action dependency-version: '9' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v6) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 4 to 5. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v4...v5) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/setup-go](https://github.com/actions/setup-go) from 4 to 6. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@v4...v6) --- updated-dependencies: - dependency-name: actions/setup-go dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
📝 WalkthroughWalkthroughThis pull request updates GitHub Actions workflows to newer action versions, refactors goroutine patterns across test files to use WaitGroup.Go, adds EOF error handling in the HTTP handler to return 400 Bad Request for truncated multipart requests, replaces otel instrumentation with custom test middleware for tracing, and updates dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/http_plugin2_test.go (1)
402-426: Inconsistent goroutine pattern within the same test.
TestHTTPBigRespuses the modernizedwg.Gopattern for the main loop (lines 374-398) but still uses the oldgo func() { defer wg2.Done() ... }()pattern for the parallel HTTP requests.Consider applying the same
wg.Gopattern consistently:♻️ Suggested refactor for consistency
- wg2 := &sync.WaitGroup{} - wg2.Add(2) - go func() { - defer wg2.Done() + wg2 := &sync.WaitGroup{} + wg2.Go(func() { req, err1 := http.NewRequest(http.MethodGet, "http://127.0.0.1:15399", nil) require.NoError(t, err1) r, err1 := http.DefaultClient.Do(req) require.NoError(t, err1) require.Equal(t, 200, r.StatusCode) _ = r.Body.Close() - }() + }) - go func() { - defer wg2.Done() + wg2.Go(func() { req, err2 := http.NewRequest(http.MethodGet, "http://127.0.0.1:15399", nil) //nolint:noctx require.NoError(t, err2) r, err2 := http.DefaultClient.Do(req) require.NoError(t, err2) require.Equal(t, 200, r.StatusCode) _ = r.Body.Close() - }() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/http_plugin2_test.go` around lines 402 - 426, The two anonymous goroutines that manually call defer wg2.Done() should be converted to the consistent wg2.Go pattern used earlier in TestHTTPBigResp: replace each "go func() { defer wg2.Done() ... }()" with "wg2.Go(func() error { ... return nil })" (keep the same request logic and require assertions inside), remove the explicit Done calls, and return nil at the end of each wg2.Go closure so wg2.Wait() still synchronizes correctly.handler/handler_test.go (1)
23-33:mockPool.Execreturns nil channel on success — document or guard.When
execErrisnil,Execreturns(nil, nil). If a future test expects successful execution, reading from the nil channel will panic. Current tests are safe since they all setexecErr.Consider either documenting this limitation more explicitly or returning a closed channel for the success case:
♻️ Defensive fix for future-proofing
func (m *mockPool) Exec(_ context.Context, _ *payload.Payload, _ chan struct{}) (chan *staticPool.PExec, error) { + if m.execErr != nil { + return nil, m.execErr + } + // Return closed channel to prevent deadlock in success path. + // Note: real pool behavior is more complex; extend mockPool as needed. + ch := make(chan *staticPool.PExec) + close(ch) - return nil, m.execErr + return ch, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@handler/handler_test.go` around lines 23 - 33, The mockPool.Exec currently returns (nil, nil) when execErr is nil which will cause a panic if a test tries to read from the returned channel; update mockPool.Exec to return a non-nil, already-closed channel on success so consumers can safely range/read from it. Modify the mockPool.Exec implementation to check m.execErr and when nil create a channel of *staticPool.PExec, close it, and return it with nil error; keep the existing behavior of returning (nil, m.execErr) when execErr is non-nil. Ensure references are to mockPool.Exec and the execErr field so the change is easy to locate..github/workflows/codeql-analysis.yml (1)
48-48: Consider updating CodeQL actions for consistency.Other actions in this workflow and across the repository are being upgraded, but
github/codeql-actionremains at v2. CodeQL v3 has been available; consider updating for consistency if compatible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/codeql-analysis.yml at line 48, The workflow references github/codeql-action/init@v2; update the CodeQL actions to v3 for consistency by changing the `uses: github/codeql-action/init@v2` entry (and any other CodeQL steps like `github/codeql-action/analyze`) to the corresponding `@v3` releases, ensure any v3-required inputs or option names used by the `init` and `analyze` steps are adjusted to match the v3 docs, and run a quick local or CI validation to confirm the workflow syntax and behavior remain compatible..golangci.yml (1)
41-41: Relaxedgoconstthreshold from 3 to 10 occurrences.This significantly reduces the sensitivity of the constant detection linter. While this can reduce noise (especially in test files with repeated literals), it may also miss opportunities to extract meaningful constants that appear 3-9 times.
If this change is to accommodate test patterns in this PR, consider instead adding goconst to the test file exclusions (alongside
dupl,funlen, etc. at lines 71-76) rather than raising the global threshold.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml at line 41, Revert the global goconst threshold change (set min-occurrences back to 3) and instead add goconst to the test-file exclusion block used for dupl/funlen so the linter stays strict globally but ignores repeated literals in tests; update the .golangci.yml: restore the min-occurrences value and add "goconst" to the same exclusion/disable list (the block alongside dupl and funlen) so only tests are exempt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/codeql-analysis.yml:
- Line 48: The workflow references github/codeql-action/init@v2; update the
CodeQL actions to v3 for consistency by changing the `uses:
github/codeql-action/init@v2` entry (and any other CodeQL steps like
`github/codeql-action/analyze`) to the corresponding `@v3` releases, ensure any
v3-required inputs or option names used by the `init` and `analyze` steps are
adjusted to match the v3 docs, and run a quick local or CI validation to confirm
the workflow syntax and behavior remain compatible.
In @.golangci.yml:
- Line 41: Revert the global goconst threshold change (set min-occurrences back
to 3) and instead add goconst to the test-file exclusion block used for
dupl/funlen so the linter stays strict globally but ignores repeated literals in
tests; update the .golangci.yml: restore the min-occurrences value and add
"goconst" to the same exclusion/disable list (the block alongside dupl and
funlen) so only tests are exempt.
In `@handler/handler_test.go`:
- Around line 23-33: The mockPool.Exec currently returns (nil, nil) when execErr
is nil which will cause a panic if a test tries to read from the returned
channel; update mockPool.Exec to return a non-nil, already-closed channel on
success so consumers can safely range/read from it. Modify the mockPool.Exec
implementation to check m.execErr and when nil create a channel of
*staticPool.PExec, close it, and return it with nil error; keep the existing
behavior of returning (nil, m.execErr) when execErr is non-nil. Ensure
references are to mockPool.Exec and the execErr field so the change is easy to
locate.
In `@tests/http_plugin2_test.go`:
- Around line 402-426: The two anonymous goroutines that manually call defer
wg2.Done() should be converted to the consistent wg2.Go pattern used earlier in
TestHTTPBigResp: replace each "go func() { defer wg2.Done() ... }()" with
"wg2.Go(func() error { ... return nil })" (keep the same request logic and
require assertions inside), remove the explicit Done calls, and return nil at
the end of each wg2.Go closure so wg2.Wait() still synchronizes correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1eff3079-3238-4624-8adc-c11b710b7541
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
.github/workflows/codeql-analysis.yml.github/workflows/linters.yml.github/workflows/linux.yml.golangci.ymlhandler/handler.gohandler/handler_test.gohandler/uploads.gotests/configs/.rr-http-otel.yamltests/configs/.rr-http-otel2.yamltests/go.modtests/http_otlp_test.gotests/http_plugin2_test.gotests/http_plugin3_test.gotests/http_plugin4_test.gotests/http_plugin_test.gotests/php_test_files/composer.json
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #225 +/- ##
===========================================
+ Coverage 0 61.20% +61.20%
===========================================
Files 0 6 +6
Lines 0 250 +250
===========================================
+ Hits 0 153 +153
- Misses 0 81 +81
- Partials 0 16 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or 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
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.
Reason for This PR
Description of Changes
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
Bug Fixes
Chores