fix: [#589] Context in PrepareForValidation always nil #151
Conversation
WalkthroughThe pull request updates how request validation is handled by passing the context ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant ContextRequest
participant Validator
Client->>Server: Send GET request with context parameter
Server->>ContextRequest: Invoke ValidateRequest method
ContextRequest->>Validator: Call PrepareForValidation(ctx, validationFunc)
Validator-->>ContextRequest: Return validation result
ContextRequest->>Server: Return JSON response or error status
Server->>Client: Respond with validation result
Possibly related PRs
Suggested reviewers
✨ 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 (
|
There was a problem hiding this comment.
PR Overview
This PR fixes an issue where the context passed to PrepareForValidation was always nil by updating the PrepareForValidation call and adding a dedicated test. It also includes refactoring changes such as renaming config mocks and updating log messages for consistency.
- Added a new test to validate that context values are correctly passed in PrepareForValidation.
- Updated the PrepareForValidation call to include the context.
- Renamed config mock imports and refined log output in route methods.
Reviewed Changes
| File | Description |
|---|---|
| context_request_test.go | Added a test to verify the context value after validation. |
| route_test.go | Renamed references from configmocks to mocksconfig. |
| route.go | Updated logging and refactored Listen/ListenTLS methods. |
| context_request.go | Fixed the PrepareForValidation call by passing the context. |
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
| ctx.WithValue("test", "-ctx") | ||
|
|
||
| var createUser CreateUser | ||
| validateErrors, err := ctx.Request().ValidateRequest(&createUser) | ||
| if err != nil { | ||
| return ctx.Response().String(http.StatusBadRequest, "Validate error: "+err.Error()) | ||
| } | ||
| if validateErrors != nil { | ||
| return ctx.Response().String(http.StatusBadRequest, fmt.Sprintf("Validate fail: %+v", validateErrors.All())) | ||
| } | ||
|
|
||
| return ctx.Response().Success().Json(contractshttp.Json{ |
There was a problem hiding this comment.
Verify that ctx.WithValue properly updates the context. If WithValue returns a new context instance rather than modifying in place, consider capturing and using the returned context.
| ctx.WithValue("test", "-ctx") | |
| var createUser CreateUser | |
| validateErrors, err := ctx.Request().ValidateRequest(&createUser) | |
| if err != nil { | |
| return ctx.Response().String(http.StatusBadRequest, "Validate error: "+err.Error()) | |
| } | |
| if validateErrors != nil { | |
| return ctx.Response().String(http.StatusBadRequest, fmt.Sprintf("Validate fail: %+v", validateErrors.All())) | |
| } | |
| return ctx.Response().Success().Json(contractshttp.Json{ | |
| newCtx := ctx.WithValue("test", "-ctx") | |
| var createUser CreateUser | |
| validateErrors, err := newCtx.Request().ValidateRequest(&createUser) | |
| if err != nil { | |
| return newCtx.Response().String(http.StatusBadRequest, "Validate error: "+err.Error()) | |
| } | |
| if validateErrors != nil { | |
| return newCtx.Response().String(http.StatusBadRequest, fmt.Sprintf("Validate fail: %+v", validateErrors.All())) | |
| } | |
| return newCtx.Response().Success().Json(contractshttp.Json{ |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
route.go (1)
26-26: Replaced termlink with internal str package.Removed dependency on external termlink package in favor of the internal str package from the framework.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
context_request.go(1 hunks)context_request_test.go(1 hunks)go.mod(2 hunks)route.go(5 hunks)route_test.go(19 hunks)
🔇 Additional comments (16)
context_request.go (1)
485-485: Bug fix: Added context to PrepareForValidation.The change passes the request context (
r.ctx) to the validation preparation function, addressing issue #589 where context was previously nil in PrepareForValidation. This fix ensures that context-dependent validation can work properly.context_request_test.go (1)
1406-1432: Good addition of test for context-aware validation.This test effectively verifies the bugfix by:
- Setting a context value (
-ctx)- Validating a request with that context
- Confirming the context value is correctly appended to the final name
The test demonstrates that the context is now properly passed to the PrepareForValidation function.
route_test.go (2)
21-22: Updated import aliases for consistency.The import for config mock package has been renamed from
configmockstomocksconfig, and thecastpackage has been added for string type conversion.
753-757: Enhanced PrepareForValidation to use context values.The updated implementation now reads the "test" value from the context and appends it to the name field. This works in conjunction with the fix in context_request.go to demonstrate that context values are properly passed to PrepareForValidation.
route.go (4)
156-158: Improved logging sequence and formatting.Now routes are output before the server listening message, and the URL formatting uses the internal str package instead of termlink.
185-190: Improved HTTPS logging format.Server listening message now uses a more consistent format with str package, and routes are output before starting the server.
206-208: Updated HTTP server logging format.Server startup message now uses the internal str package for URL formatting.
241-243: Updated HTTPS server logging format.Server startup message for HTTPS now uses the internal str package for URL formatting.
go.mod (8)
11-11: Dependency Update:github.com/goravel/frameworkto v1.15.4
This update bumps the framework version from v1.15.0 to v1.15.4. Ensure that any breaking changes (if any) are handled in the codebase.
22-22: Dependency Update:github.com/containerd/consolev1.0.4
The version forcontainerd/consolehas been updated. Verify that this indirect dependency update does not introduce any conflicts downstream.
25-26: Dependency Updates:fsnotifyandmimetype
github.com/fsnotify/fsnotifyhas been updated to v1.8.0.github.com/gabriel-vasile/mimetypehas been updated to v1.4.8.
These updates likely include bug fixes or performance improvements. Please ensure that any consumers of these packages in the project remain compatible.
37-37: Dependency Update:github.com/magiconair/propertiesto v1.8.9
This update brings a new version of the properties package. A quick check for any API changes on the consumer side is recommended.
42-42: Dependency Update:github.com/pelletier/go-toml/v2to v2.2.3
The TOML parser has been updated from v2.2.2 to v2.2.3. Confirm that the latest fixes or improvements align with your configuration needs.
47-47: Dependency Update:github.com/sagikazarmark/locaferoto v0.6.0
This update represents a jump from v0.4.0 to v0.6.0. Review the release notes for any breaking changes or additional features that might affect filesystem abstraction logic within the codebase.
59-63: Multiple Dependency Updates: Go Ecosystem Packages
The following dependencies have been updated:
go.uber.org/multierrto v1.11.0golang.org/x/expto v0.0.0-20250106191152-7588d65b2ba8golang.org/x/netto v0.34.0golang.org/x/systo v0.29.0golang.org/x/termto v0.28.0
These updates aim to streamline improvements and security fixes in common Go libraries. Verify that the indirect dependencies used by the project integrate well with these versions, and consider runninggo mod tidyto clean up the module graph if necessary.
65-68: Multiple Dependency Updates: Google & YAML Packages
The following updates have been applied:
google.golang.org/genproto/googleapis/rpcto v0.0.0-20250124145028-65684f501c47google.golang.org/grpcto v1.70.0google.golang.org/protobufto v1.36.4gopkg.in/check.v1to v1.0.0-20201130134442-10cb98267c6c
Review these changes to ensure that any grpc or protobuf-related functionalities are compatible with the new versions. These updates can bring performance improvements and additional features, but verifying against your integration tests is advisable.
📑 Description
Closes goravel/goravel#589
Summary by CodeRabbit
Enhancements
Dependency Updates
Tests
✅ Checks