Added WithResponseReadSize function to allow SDK users to modify max response read opt #5961
Conversation
…response read opt
WalkthroughThe pull request introduces modifications to the configuration handling in the Changes
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
lib/config.go (1)
148-157: LGTM! Clean refactoring of concurrency options.The simplified assignments improve code readability while maintaining the same validation logic. Good job on reducing cognitive complexity.
Consider using a struct literal assignment for better maintainability:
- e.opts.TemplateThreads = opts.TemplateConcurrency - e.opts.BulkSize = opts.HostConcurrency - e.opts.HeadlessBulkSize = opts.HeadlessHostConcurrency - e.opts.HeadlessTemplateThreads = opts.HeadlessTemplateConcurrency - e.opts.JsConcurrency = opts.JavascriptTemplateConcurrency - e.opts.PayloadConcurrency = opts.TemplatePayloadConcurrency - e.opts.ProbeConcurrency = opts.ProbeConcurrency + e.opts = struct { + TemplateThreads int + BulkSize int + HeadlessBulkSize int + HeadlessTemplateThreads int + JsConcurrency int + PayloadConcurrency int + ProbeConcurrency int + }{ + TemplateThreads: opts.TemplateConcurrency, + BulkSize: opts.HostConcurrency, + HeadlessBulkSize: opts.HeadlessHostConcurrency, + HeadlessTemplateThreads: opts.HeadlessTemplateConcurrency, + JsConcurrency: opts.JavascriptTemplateConcurrency, + PayloadConcurrency: opts.TemplatePayloadConcurrency, + ProbeConcurrency: opts.ProbeConcurrency, + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/config.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
🔇 Additional comments (1)
lib/config.go (1)
159-163: Verify the implementation of response size limit.Let's verify how this option is used in the HTTP request processing code:
✅ Verification successful
Response size limit implementation is correct and properly integrated.
The option is:
- Correctly propagated from configuration to HTTP handling
- Used properly with ResponseChain for efficient reading
- Has a reasonable default of 10MB
- Doesn't conflict with other size limits
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ResponseReadSize usage in the codebase echo "Searching for ResponseReadSize usage..." rg -A 5 "ResponseReadSize" # Search for other response size limits in HTTP handling code echo "Searching for other response size limits..." rg -A 5 "response.*size|size.*response" # Search for HTTP response reading implementation echo "Searching for HTTP response reading implementation..." ast-grep --pattern 'func $_(response http.Response) { $$$ }'Length of output: 14015
improved comment, changed casing of param name, added negative input check Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/config.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint
lib/config.go
[failure] 170-170:
syntax error: non-declaration statement outside function body (typecheck)
[failure] 170-170:
expected declaration, found '}' (typecheck)
[failure] 170-170:
syntax error: non-declaration statement outside function body) (typecheck)
[failure] 170-170:
syntax error: non-declaration statement outside function body) (typecheck)
[failure] 170-170:
syntax error: non-declaration statement outside function body) (typecheck)
🪛 golangci-lint (1.62.2)
lib/config.go
170-170: expected declaration, found '}'
(typecheck)
🪛 GitHub Actions: 🔨 Tests
lib/config.go
[error] 170-170: syntax error: non-declaration statement outside function body
🔇 Additional comments (2)
lib/config.go (2)
148-154: Great simplification of the concurrency options assignment!The removal of conditional branches while maintaining all necessary validation checks makes the code more maintainable and easier to understand.
159-169: Well-implemented response size configuration!The implementation includes all the essential elements:
- Clear documentation with units (bytes)
- Input validation for negative values
- Recommended value ranges
lib/config.go
Outdated
| } | ||
| } |
There was a problem hiding this comment.
Remove extra closing braces causing syntax errors.
The extra closing braces on these lines are causing compilation failures. They don't belong to any opening brace and should be removed.
Apply this diff to fix the syntax errors:
- }
-}🧰 Tools
🪛 GitHub Check: Lint
[failure] 170-170:
syntax error: non-declaration statement outside function body (typecheck)
[failure] 170-170:
expected declaration, found '}' (typecheck)
[failure] 170-170:
syntax error: non-declaration statement outside function body) (typecheck)
[failure] 170-170:
syntax error: non-declaration statement outside function body) (typecheck)
[failure] 170-170:
syntax error: non-declaration statement outside function body) (typecheck)
🪛 golangci-lint (1.62.2)
170-170: expected declaration, found '}'
(typecheck)
🪛 GitHub Actions: 🔨 Tests
[error] 170-170: syntax error: non-declaration statement outside function body
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/config.go (1)
159-167: Implementation looks good and addresses previous review comments.Consider defining constants for the recommended values to improve maintainability:
+const ( + // DefaultMinResponseReadSize represents 1MB in bytes + DefaultMinResponseReadSize = 1048576 + // DefaultMaxResponseReadSize represents 10MB in bytes + DefaultMaxResponseReadSize = 10485760 +) // WithResponseReadSize sets the maximum size of response to read in bytes. -// A value of 0 means no limit. Recommended values: 1MB (1048576) to 10MB (10485760). +// A value of 0 means no limit. Recommended values: DefaultMinResponseReadSize to DefaultMaxResponseReadSize.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/config.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
🔇 Additional comments (2)
lib/config.go (2)
148-157: Great refactoring of the concurrency options!The simplified assignments improve readability while maintaining the same validation constraints.
159-167: Verify integration and add test coverage.
Please add unit tests for the new
WithResponseReadSizefunction to verify:
- Validation of negative values
- Setting of valid values
- Default behavior with zero value
Let's verify the integration with the HTTP client:
✅ Verification successful
Implementation verified, tests still needed
The
ResponseReadSizeoption is properly integrated with the HTTP client and the overall system. Please proceed with adding unit tests as originally suggested to ensure the robustness of the implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for HTTP client configurations to ensure ResponseReadSize is properly used rg -A 5 "ResponseReadSize" --type goLength of output: 2687
|
Thanks for your contribution @meme-lord ! :) |
Proposed changes
This adds a WithResponseReadSize() function so that SDK users can change the max response read size to suit their needs. I also simplified the WithConcurrency() function to reduce the cognitive complexity of it (no functional change).
Related Github issue: #5895
Checklist
Summary by CodeRabbit