fix: [#589] Context in PrepareForValidation always nil#919
Conversation
WalkthroughThis pull request updates context handling across multiple parts of the codebase. In the HTTP package, a comment in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Validator
participant PrepFunc
Caller->>Validator: Invoke Make() for validation
Validator->>Validator: Check generateOptions for context
alt Context found
Validator->>PrepFunc: Call prepareForValidation(ctx, data)
else Context not found
Validator->>PrepFunc: Call prepareForValidation(nil, data)
end
PrepFunc-->>Validator: Return error/success
Validator-->>Caller: Return validation result
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24.0) ✨ 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #919 +/- ##
==========================================
- Coverage 68.82% 68.82% -0.01%
==========================================
Files 154 154
Lines 10191 10200 +9
==========================================
+ Hits 7014 7020 +6
- Misses 2855 2857 +2
- Partials 322 323 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
validation/validation_test.go (1)
87-91: Consider adding a test for nil contextThere's no test case that verifies the behavior when PrepareForValidation is called without a context, which explains the lack of test coverage noted in static analysis.
Consider adding a test case like:
{ description: "success when PrepareForValidation is called without context", data: map[string]any{"a": " b "}, rules: map[string]string{"a": "required"}, options: []httpvalidate.Option{ Filters(map[string]string{"a": "trim"}), PrepareForValidation(nil, func(ctx contractshttp.Context, data httpvalidate.Data) error { // Test that ctx is nil but validation still works return nil }), }, expectValidator: true, expectData: Data{A: "b"}, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
http/context.go(1 hunks)validation/options.go(1 hunks)validation/validation.go(1 hunks)validation/validation_test.go(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- http/context.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
validation/validation.go
[warning] 63-64: validation/validation.go#L63-L64
Added lines #L63 - L64 were not covered by tests
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: test / windows (1.24)
🔇 Additional comments (6)
validation/validation.go (2)
57-65: Context handling enhancementGood implementation of context handling for PrepareForValidation. The code properly retrieves context from options and handles the case where it doesn't exist.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 63-64: validation/validation.go#L63-L64
Added lines #L63 - L64 were not covered by tests
63-64: Add test coverage for the nil context caseAccording to static analysis, these lines aren't covered by tests, indicating that there's no test for the case when context doesn't exist in options.
Consider adding a test case where PrepareForValidation is called without a context to ensure both code paths are covered.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 63-64: validation/validation.go#L63-L64
Added lines #L63 - L64 were not covered by testsvalidation/options.go (1)
60-62: Function signature update properly integrates contextThe function now correctly accepts a context parameter and stores it in the options map, which fixes the issue where context in PrepareForValidation was always nil.
validation/validation_test.go (3)
110-126: Good test case for context usageThis new test case properly verifies that context values can be retrieved and used in PrepareForValidation, which is essential for the fix.
22-24: Test setup for context properly initializedThe setup code correctly initializes the context with a test value that will be used throughout the tests.
88-89: Verify the error assertion changeThe error assertion changed from a specific error (likely
errors.ErrInvalidData) to the genericassert.AnError. This makes the test less specific about what error is expected.Was this change intentional? It's generally better to assert for specific error types when possible to ensure the exact error condition is being tested.
There was a problem hiding this comment.
PR Overview
This PR fixes the issue where the context passed to PrepareForValidation was always nil, by updating the function signature and propagating the context through the options. Key changes include:
- Updating the PrepareForValidation signature in options.go to accept a context parameter.
- Adjusting the Make function in validation.go to extract and use the provided context.
- Updating tests in validation_test.go to create and pass a proper context for validation.
Reviewed Changes
| File | Description |
|---|---|
| validation/validation_test.go | Updated imports and test cases to create and pass context to PrepareForValidation. |
| validation/validation.go | Modified Make to retrieve the context from options instead of hardcoding nil. |
| validation/options.go | Revised PrepareForValidation to include the context parameter and set it in options. |
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
validation/validation_test.go:87
- Ensure that the context type used in the closure matches the expected type (http.Context) as defined in the options. If contractshttp.Context and http.Context are interchangeable, consider adding a clarifying comment.
PrepareForValidation(ctx, func(ctx contractshttp.Context, data httpvalidate.Data) error {
validation/validation.go:61
- [nitpick] Since the comma-ok type assertion already yields a nil value when the key is missing, explicitly setting ctx to nil in the if-condition might be redundant. Consider simplifying this block.
ctx, exist = generateOptions["ctx"].(http.Context)
📑 Description
Closes goravel/goravel#589
Summary by CodeRabbit
New Features
Tests
✅ Checks