feat: make request body size configurable via MAX_REQUEST_BODY_MB#128
feat: make request body size configurable via MAX_REQUEST_BODY_MB#128Omkarop0808 wants to merge 3 commits intoAnkanMisra:mainfrom
Conversation
- Add getMaxBodySize() helper in gateway/config.go - Replace hardcoded 10MB limit with dynamic configuration - Add MAX_REQUEST_BODY_MB to .env.example with default 10MB - Add comprehensive unit tests for getMaxBodySize() - Update error response to show actual configured limit Fixes AnkanMisra#116
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR makes the gateway's request body size configurable via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Gateway as Gateway Handler
participant Config as Config (getMaxBodySize)
participant Reader as http.MaxBytesReader
Client->>Gateway: POST /api/ai/summarize (body)
Gateway->>Config: getMaxBodySize()
Config-->>Gateway: maxBodySize (bytes)
Gateway->>Reader: wrap Request.Body with MaxBytesReader(maxBodySize)
Gateway->>Gateway: read body -> if exceeds -> 413 error with max_size_mb
Gateway-->>Client: 200/413 response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func getMaxBodySize() int64 { | ||
| mb := getEnvAsInt("MAX_REQUEST_BODY_MB", 10) | ||
| return int64(mb) * 1024 * 1024 | ||
| } |
There was a problem hiding this comment.
Missing validation for non-positive values - allows MAX_REQUEST_BODY_MB=0 or negative values, unlike getPositiveTimeout() which validates positive values
| func getMaxBodySize() int64 { | |
| mb := getEnvAsInt("MAX_REQUEST_BODY_MB", 10) | |
| return int64(mb) * 1024 * 1024 | |
| } | |
| func getMaxBodySize() int64 { | |
| mb := getEnvAsInt("MAX_REQUEST_BODY_MB", 10) | |
| if mb <= 0 { | |
| mb = 10 | |
| } | |
| return int64(mb) * 1024 * 1024 | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: gateway/config.go
Line: 25-28
Comment:
Missing validation for non-positive values - allows `MAX_REQUEST_BODY_MB=0` or negative values, unlike `getPositiveTimeout()` which validates positive values
```suggestion
func getMaxBodySize() int64 {
mb := getEnvAsInt("MAX_REQUEST_BODY_MB", 10)
if mb <= 0 {
mb = 10
}
return int64(mb) * 1024 * 1024
}
```
How can I resolve this? If you propose a fix, please make it concise.| func TestGetMaxBodySize(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| envValue string | ||
| expected int64 | ||
| }{ | ||
| { | ||
| name: "default 10MB", | ||
| envValue: "", | ||
| expected: 10 * 1024 * 1024, | ||
| }, | ||
| { | ||
| name: "custom 1MB", | ||
| envValue: "1", | ||
| expected: 1 * 1024 * 1024, | ||
| }, | ||
| { | ||
| name: "custom 50MB", | ||
| envValue: "50", | ||
| expected: 50 * 1024 * 1024, | ||
| }, | ||
| { | ||
| name: "custom 100MB", | ||
| envValue: "100", | ||
| expected: 100 * 1024 * 1024, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if tt.envValue != "" { | ||
| t.Setenv("MAX_REQUEST_BODY_MB", tt.envValue) | ||
| } | ||
| result := getMaxBodySize() | ||
| if result != tt.expected { | ||
| t.Errorf("expected %d bytes, got %d bytes", tt.expected, result) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test cases for invalid values (zero, negative) - all timeout helpers test non-positive fallback behavior (see line 255-258)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: gateway/config_test.go
Line: 261-300
Comment:
Missing test cases for invalid values (zero, negative) - all timeout helpers test non-positive fallback behavior (see line 255-258)
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
gateway/main.go
Outdated
| var maxBytesErr *http.MaxBytesError | ||
| if errors.As(err, &maxBytesErr) { | ||
| c.JSON(413, gin.H{"error": "Payload too large", "max_size": "10MB"}) | ||
| c.JSON(413, gin.H{"error": "Payload too large", "max_size_mb": getEnvAsInt("MAX_REQUEST_BODY_MB", 10)}) |
There was a problem hiding this comment.
Calling getEnvAsInt() directly here duplicates the logic from getMaxBodySize() - should reuse the same value from line 371
| c.JSON(413, gin.H{"error": "Payload too large", "max_size_mb": getEnvAsInt("MAX_REQUEST_BODY_MB", 10)}) | |
| c.JSON(413, gin.H{"error": "Payload too large", "max_size_mb": maxBodySize / (1024 * 1024)}) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: gateway/main.go
Line: 377
Comment:
Calling `getEnvAsInt()` directly here duplicates the logic from `getMaxBodySize()` - should reuse the same value from line 371
```suggestion
c.JSON(413, gin.H{"error": "Payload too large", "max_size_mb": maxBodySize / (1024 * 1024)})
```
How can I resolve this? If you propose a fix, please make it concise.| // getMaxBodySize returns the maximum request body size in bytes. | ||
| // Configurable via MAX_REQUEST_BODY_MB environment variable (default: 10MB). | ||
| func getMaxBodySize() int64 { | ||
| mb := getEnvAsInt("MAX_REQUEST_BODY_MB", 10) | ||
| return int64(mb) * 1024 * 1024 | ||
| } |
There was a problem hiding this comment.
Critical: gateway/cache.go lines 50, 52, 62, 54, 69 still use hardcoded const maxBodySize = 10 * 1024 * 1024 - when caching is enabled, requests will be limited to 10MB regardless of this configuration
Prompt To Fix With AI
This is a comment left during a code review.
Path: gateway/config.go
Line: 23-28
Comment:
**Critical**: `gateway/cache.go` lines 50, 52, 62, 54, 69 still use hardcoded `const maxBodySize = 10 * 1024 * 1024` - when caching is enabled, requests will be limited to 10MB regardless of this configuration
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
gateway/main.go (1)
371-377: Derivemax_size_mbfrom the already-computedmaxBodySizeinstead of a second env read.Line 377 calls
getEnvAsInt("MAX_REQUEST_BODY_MB", 10)independently. The value it reports could theoretically differ from what was passed toMaxBytesReaderon line 372 if the env var changes between the two reads. Using integer division from the in-scopemaxBodySizeis both simpler and guaranteed consistent.♻️ Proposed fix
- c.JSON(413, gin.H{"error": "Payload too large", "max_size_mb": getEnvAsInt("MAX_REQUEST_BODY_MB", 10)}) + c.JSON(413, gin.H{"error": "Payload too large", "max_size_mb": maxBodySize / (1024 * 1024)})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/main.go` around lines 371 - 377, The 413 error response currently calls getEnvAsInt("MAX_REQUEST_BODY_MB", 10) again instead of using the already-computed maxBodySize; update the error branch in the handler to derive max_size_mb from maxBodySize (e.g., divide maxBodySize by (1024*1024) with appropriate integer rounding) and pass that value to c.JSON so the reported limit matches the value used with http.MaxBytesReader; adjust references in the request-reading block around maxBodySize, http.MaxBytesReader, and the c.JSON(413, ...) call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/config_test.go`:
- Around line 289-299: The default subtest does not clear the environment so
MAX_REQUEST_BODY_MB may leak into tests; unconditionally call
t.Setenv("MAX_REQUEST_BODY_MB", tt.envValue) in the table-driven loop (pass ""
for the default case) so getMaxBodySize() sees an absent value as intended, and
add two edge-case table entries where envValue is "0" and "-1" and expected is
the default 10*1024*1024 to verify the non-positive guard in
getMaxBodySize()/getEnvAsInt; update the tests loop and test cases referencing
getMaxBodySize and MAX_REQUEST_BODY_MB accordingly.
In `@gateway/config.go`:
- Around line 25-28: The getMaxBodySize function should guard against
non-positive environment values: read mb via getEnvAsInt("MAX_REQUEST_BODY_MB",
10), and if mb <= 0 set mb = 10 (mirroring getPositiveTimeout) before converting
to int64 and multiplying by 1024*1024 so http.MaxBytesReader never receives n ≤
0; update getMaxBodySize to enforce this default behavior using the existing
symbols getMaxBodySize and getEnvAsInt.
---
Nitpick comments:
In `@gateway/main.go`:
- Around line 371-377: The 413 error response currently calls
getEnvAsInt("MAX_REQUEST_BODY_MB", 10) again instead of using the
already-computed maxBodySize; update the error branch in the handler to derive
max_size_mb from maxBodySize (e.g., divide maxBodySize by (1024*1024) with
appropriate integer rounding) and pass that value to c.JSON so the reported
limit matches the value used with http.MaxBytesReader; adjust references in the
request-reading block around maxBodySize, http.MaxBytesReader, and the
c.JSON(413, ...) call.
- Add validation for non-positive values in getMaxBodySize() - Add test cases for zero and negative values - Reuse maxBodySize value instead of calling getEnvAsInt() multiple times - Fix cache.go to use calculated maxBodySize value consistently
- Always call t.Setenv() to prevent MAX_REQUEST_BODY_MB leakage between tests - Ensures default test case properly clears environment
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@gateway/config_test.go`:
- Around line 299-309: The "default" subtest can inherit an existing
MAX_REQUEST_BODY_MB from the environment because t.Setenv is only called when
tt.envValue != ""—fix by unconditionally calling t.Setenv("MAX_REQUEST_BODY_MB",
tt.envValue) inside the test loop so an empty tt.envValue clears the env for
that subtest; update the loop around the table-driven tests that call
getMaxBodySize() (referencing tt.envValue and getMaxBodySize) to always invoke
t.Setenv before calling getMaxBodySize().
|
@greptile give the confidence score and point out where the bugs are in this pr |
Confidence Score: 1/5This is a very low score indicating critical issues that make the PR unsuitable for merging. Critical Bugs1. Incomplete Implementation - Cache Handler IgnoredLocation: The PR updates
Impact: When caching is enabled, requests will still be limited to 10MB regardless of the 2. Missing Input ValidationLocation: The function lacks validation for non-positive values (0, negative numbers). Other timeout helpers in the codebase validate their inputs, but this one doesn't. Impact: Setting Minor Issues3. Duplicated Logic in Error ResponseLocation: The error response calls 4. Incomplete Test CoverageLocation: Tests are missing edge cases for:
Bottom Line: The feature appears to work but has a critical blind spot in the cache handler, making it only partially functional. The PR needs updates to |
AnkanMisra
left a comment
AnkanMisra
left a comment
There was a problem hiding this comment.
@Omkarop0808 Follow and do the changes that greptile said
|
Hi! All the issues mentioned have already been addressed in commits 0e04eb3 and 7625ff8: |
2 participants
🎯 Description
This PR makes the request body size limit configurable via the
MAX_REQUEST_BODY_MBenvironment variable, replacing the hardcoded 10MB limit in the Gateway service.Fixes #116
🔧 Changes Made
1. Added Configuration Helper (
gateway/config.go)getMaxBodySize()function that readsMAX_REQUEST_BODY_MBfrom environment2. Updated Request Handler (
gateway/main.go)const maxBodySize = 10 * 1024 * 1024getMaxBodySize()max_size_mbfield3. Added Documentation (
.env.example)MAX_REQUEST_BODY_MB=10with clear comments4. Added Tests (
gateway/config_test.go)getMaxBodySize()📊 Impact
✅ Testing
getMaxBodySize()📝 Usage Example
Ready for review!
Summary by CodeRabbit
New Features
Chores
Tests
Documentation
Greptile Summary
This PR makes request body size configurable via
MAX_REQUEST_BODY_MB, but critically, it only updatesmain.goand missescache.go, which still has a hardcoded 10MB limit. This creates inconsistent behavior where cached requests ignore the configuration.Critical Issues:
gateway/cache.golines 50, 52, 62, 54, 69 still use hardcoded10 * 1024 * 1024and"10MB"stringsgetMaxBodySize()lacks validation for non-positive values (unlike other timeout helpers)Minor Issues:
getEnvAsInt()again instead of reusingmaxBodySizeConfidence Score: 1/5
cache.gostill has hardcoded 10MB limits in multiple places (lines 50, 52, 62, 54, 69), which means the configurable body size only works when caching is disabled. Additionally, missing validation allows invalid values like 0 or negative numbers.gateway/cache.gomust be updated to usegetMaxBodySize()instead of hardcoded limits, andgateway/config.goneeds validation for non-positive valuesImportant Files Changed
getMaxBodySize()helper but missing validation for non-positive valuesgetMaxBodySize()but missing edge cases for zero/negative valuesgetMaxBodySize()but incomplete -cache.gostill has hardcoded 10MB limit causing inconsistent behavior, and error response duplicates logicMAX_REQUEST_BODY_MBconfiguration with clear documentationLast reviewed commit: 4e494c3
(5/5) You can turn off certain types of comments like style here!