feat: [#515] Input* methods of http.Request support retrieve data from query and route #124
Conversation
…m query and route
WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
context_request.go (2)
295-312: Consider preserving empty values in array.While the changes improve error handling and add query parameter support, filtering out empty values in lines 306-308 might be too aggressive. Empty strings could be valid values that users want to preserve.
Consider this alternative approach:
- if itemStr := utils.UnsafeString(item); itemStr != "" { - value = append(value, itemStr) - } + value = append(value, utils.UnsafeString(item))
330-338: Consider adding JSON structure validation.While the changes improve error handling and add query parameter support, there's no validation of the JSON structure. Invalid JSON objects could be silently converted to an empty map.
Consider adding structure validation:
if r.instance.Context().QueryArgs().Has(key) { valueStr := r.instance.Query(key) var value map[string]string if err := json.Unmarshal([]byte(valueStr), &value); err != nil { + // Log the error for debugging + LogFacade.Debug(fmt.Sprintf("Failed to unmarshal JSON for key %s: %v", key, err)) return map[string]string{} } + // Validate that we got a non-nil map + if value == nil { + return map[string]string{} + } return value }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
context_request.go(3 hunks)context_request_test.go(5 hunks)
🔇 Additional comments (6)
context_request.go (1)
286-290: LGTM! Input method changes look good.
The changes improve the input retrieval logic by:
- Using
QueryArgs().Has(key)for more precise query parameter existence check - Adding route parameter support as a fallback
- Maintaining proper precedence: body > query > route
Let's verify the precedence order works as expected:
✅ Verification successful
Precedence order is correctly implemented and verified
The code implementation and tests confirm that the Input method follows the correct precedence order:
- Body values (lines 269-284)
- Query parameters (lines 286-288)
- Route parameters (line 290)
The test cases TestInput_KeyInBodyIsEmpty and TestInput_KeyInQueryIsEmpty verify that empty values are properly handled and maintain the precedence order.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Input method's precedence order (body > query > route)
# Test: Search for test cases that verify this precedence
rg -A 5 "TestInput.*" context_request_test.go
Length of output: 7435
Script:
#!/bin/bash
# Let's check the test implementations to verify the precedence order
rg -A 20 "TestInput_KeyInBodyIsEmpty|TestInput_KeyInQueryIsEmpty" context_request_test.go
Length of output: 1405
Script:
#!/bin/bash
# Let's check the implementation of Input method to verify the precedence order
ast-grep --pattern 'func (r *ContextRequest) Input($_, $$$) $_ {
$$$
}'
Length of output: 1297
context_request_test.go (5)
652-670: LGTM! Test case properly verifies empty string handling.
The test ensures that empty strings in the request body are preserved and not replaced with default values.
Line range hint 672-685: LGTM! Test case properly verifies empty query parameter handling.
The test ensures that empty strings in query parameters are preserved and not replaced with default values.
805-823: LGTM! Test case properly verifies empty array handling in body.
The test ensures that empty arrays in the request body are returned as empty slices rather than nil or default values.
Line range hint 825-839: LGTM! Test case properly verifies empty array handling in query.
The test ensures that empty arrays in query parameters are returned as empty slices rather than nil or default values.
Line range hint 932-969: LGTM! Test cases properly verify empty map handling.
The tests ensure that empty maps in both request body and query parameters are returned as empty maps rather than nil or default values.
📑 Description
Closes goravel/goravel#515
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✅ Checks