Add solution for gin challenge-1-basic-routing by nbploc08#1430
Add solution for gin challenge-1-basic-routing by nbploc08#1430nbploc08 wants to merge 2 commits intoRezaSi:mainfrom
Conversation
WalkthroughA new Go solution file that implements a complete in-memory REST API using the Gin framework for user management. The solution includes CRUD operations, search functionality, two exported types (User and Response), multiple handler functions, and basic validation with appropriate HTTP status codes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds a solution for the Gin challenge-1-basic-routing by implementing a basic User Management REST API with CRUD operations and search functionality. The solution demonstrates understanding of Gin routing, HTTP handlers, and JSON serialization, but contains several critical issues including race conditions, route ordering problems, and inconsistent API response formats.
Changes:
- Implemented all required REST API endpoints (GET, POST, PUT, DELETE for users, plus search functionality)
- Added helper functions for user validation and lookup
- Included Vietnamese language comments throughout the code documenting implementation decisions
Comments suppressed due to low confidence (13)
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:228
- The error message "invalid user" is not sufficiently descriptive. It doesn't explain what makes the user invalid. Consider providing more specific error messages such as "Name, Email, and Age are required fields" or "Age must be greater than 0" to help API consumers understand what went wrong.
if user.Name == "" || user.Email == "" || user.Age <= 0 {
return errors.New("invalid user")
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:206
- The response format is inconsistent with other successful responses. Line 206 uses
gin.H{"success": true, "data": matches}while other successful responses use the Response struct (e.g., lines 64-70, 91-96). For consistency, this should use Response{Success: true, Data: matches, Code: 200} to match the pattern used throughout the rest of the code.
c.JSON(200, gin.H{"success": true, "data": matches}) // ✅ Fix: luôn trả 200, kể cả khi không có kết quả
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:193
- The error format is inconsistent. Line 192 uses
gin.H{"error": "No search query"}while successful responses use the Response struct. For consistency, all error responses should use the same format. Consider using Response{Success: false, Error: "No search query", Code: 400} to match the pattern used in successful responses.
c.JSON(400, gin.H{"error": "No search query"})
return
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:88
- The error format is inconsistent. Line 87 uses
gin.H{"error": "User not found"}while lines 91-96 use the Response struct. For consistency, all error responses should use the same format. Consider using Response{Success: false, Error: "User not found", Code: 404} to match the pattern used in successful responses.
c.JSON(404, gin.H{"error": "User not found"})
return
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:147
- The error format is inconsistent. Line 146 uses
gin.H{"error": "User not found"}while successful responses use the Response struct. For consistency, all error responses should use the same format. Consider using Response{Success: false, Error: "User not found", Code: 404} to match the pattern used in successful responses.
c.JSON(404, gin.H{"error": "User not found"})
return
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:168
- The error format is inconsistent. Line 167 uses
gin.H{"error": "Invalid ID"}while successful responses use the Response struct. For consistency, all error responses should use the same format. Consider using Response{Success: false, Error: "Invalid ID", Code: 400} to match the pattern used in successful responses.
c.JSON(400, gin.H{"error": "Invalid ID"})
return
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:173
- The error format is inconsistent. Line 172 uses
gin.H{"error": "User not found"}while successful responses use the Response struct. For consistency, all error responses should use the same format. Consider using Response{Success: false, Error: "User not found", Code: 404} to match the pattern used in successful responses.
c.JSON(404, gin.H{"error": "User not found"})
return // ✅ Fix: phải return, không thì tiếp tục với i = -1 => panic
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:82
- The error format is inconsistent with other error responses in the file. Line 81 uses
gin.H{"error": "Invalid ID"}while lines 91-96 use the Response struct. For consistency, all error responses should use the same format. Consider using Response{Success: false, Error: "Invalid ID", Code: 400} to match the pattern used in successful responses.
c.JSON(400, gin.H{"error": "Invalid ID"})
return
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:108
- The error format is inconsistent. Line 107 uses
gin.H{"error": err.Error()}while line 118 uses the Response struct. For consistency, all error responses should use the same format. Consider using Response{Success: false, Error: err.Error(), Code: 400} to match the pattern used in successful responses.
c.JSON(400, gin.H{"error": err.Error()})
return
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:113
- The error format is inconsistent. Line 112 uses
gin.H{"error": err.Error()}while line 118 uses the Response struct. For consistency, all error responses should use the same format. Consider using Response{Success: false, Error: err.Error(), Code: 400} to match the pattern used in successful responses.
c.JSON(400, gin.H{"error": err.Error()})
return // ✅ Fix: phải return, không thì tiếp tục append user lỗi
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:138
- The error format is inconsistent. Line 137 uses
gin.H{"error": err.Error()}while successful responses use the Response struct. For consistency, all error responses should use the same format. Consider using Response{Success: false, Error: err.Error(), Code: 400} to match the pattern used in successful responses.
c.JSON(400, gin.H{"error": err.Error()})
return
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:142
- The error format is inconsistent. Line 141 uses
gin.H{"error": vali.Error()}while successful responses use the Response struct. For consistency, all error responses should use the same format. Consider using Response{Success: false, Error: vali.Error(), Code: 400} to match the pattern used in successful responses.
c.JSON(400, gin.H{"error": vali.Error()}) // ✅ Fix: dùng vali.Error() thay vì err.Error() (err ở đây là nil), thêm return
return
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go:132
- The error format is inconsistent. Line 131 uses
gin.H{"error": "Invalid ID"}while successful responses use the Response struct. For consistency, all error responses should use the same format. Consider using Response{Success: false, Error: "Invalid ID", Code: 400} to match the pattern used in successful responses.
c.JSON(400, gin.H{"error": "Invalid ID"})
return // ✅ Fix: return để dừng, không chạy tiếp với userID rác
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| r.GET("/users/:id", getUserByID) | ||
| r.POST("/users", createUser) | ||
| r.PUT("/users/:id", updateUser) | ||
| r.DELETE("/users/:id", deleteUser) | ||
| r.GET("/users/search", searchUsers) |
There was a problem hiding this comment.
Route order issue: The /users/search route is defined after /users/:id. In Gin, routes are matched in order, and /users/:id will match /users/search first, treating "search" as an ID parameter. The /users/search route should be defined before /users/:id to ensure the search endpoint works correctly. Move line 55 to before line 51.
| r.GET("/users/:id", getUserByID) | |
| r.POST("/users", createUser) | |
| r.PUT("/users/:id", updateUser) | |
| r.DELETE("/users/:id", deleteUser) | |
| r.GET("/users/search", searchUsers) | |
| r.GET("/users/search", searchUsers) | |
| r.GET("/users/:id", getUserByID) | |
| r.POST("/users", createUser) | |
| r.PUT("/users/:id", updateUser) | |
| r.DELETE("/users/:id", deleteUser) |
| var users = []User{ | ||
| {ID: 1, Name: "John Doe", Email: "john@example.com", Age: 30}, | ||
| {ID: 2, Name: "Jane Smith", Email: "jane@example.com", Age: 25}, | ||
| {ID: 3, Name: "Bob Wilson", Email: "bob@example.com", Age: 35}, | ||
| } | ||
| var nextID = 4 |
There was a problem hiding this comment.
The global variables users and nextID are accessed and modified concurrently by multiple HTTP handlers without any synchronization mechanism. This creates race conditions where concurrent requests could corrupt data or cause inconsistent state. Consider using a mutex (sync.RWMutex) to protect access to these shared variables, or use a thread-safe data structure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.go (3)
37-47: Clean up leftover TODO comments from the challenge template.All routes and handlers are implemented, yet the original TODO stubs (lines 37–47, 59, 63, 75–77, 101–104, 124–127, 161–163, 186–188, 212–213, 224–226) remain. These are noise in a final submission.
113-113: Replace Vietnamese inline fix-notes with English or remove them.Comments like
// ✅ Fix: phải return, không thì tiếp tục append user lỗi(and similar throughout) are Vietnamese development notes that should be translated to English or removed before submission to a shared repository.
70-70: Prefernet/httpstatus constants over raw integer literals.Using
http.StatusOK,http.StatusCreated,http.StatusBadRequest, andhttp.StatusNotFoundinstead of200,201,400,404throughout is the idiomatic Go style and makes intent explicit.
| var users = []User{ | ||
| {ID: 1, Name: "John Doe", Email: "john@example.com", Age: 30}, | ||
| {ID: 2, Name: "Jane Smith", Email: "jane@example.com", Age: 25}, | ||
| {ID: 3, Name: "Bob Wilson", Email: "bob@example.com", Age: 35}, | ||
| } | ||
| var nextID = 4 |
There was a problem hiding this comment.
Global mutable state is not protected against concurrent access.
users and nextID are mutated by multiple handlers without a mutex. Gin itself is thread-safe, but handlers must avoid mutating shared state without synchronization; improper use of shared maps or slices can lead to panics or data corruption. Concurrent POST, PUT, DELETE, and GET requests (all served in separate goroutines) will race on the slice and counter.
🔒 Proposed fix — add a RWMutex
+import "sync"
var users = []User{...}
var nextID = 4
+var mu sync.RWMutexThen guard writes with mu.Lock()/mu.Unlock() in createUser, updateUser, deleteUser, and reads with mu.RLock()/mu.RUnlock() in getAllUsers, getUserByID, searchUsers.
| r := gin.Default() | ||
| r.GET("/users", getAllUsers) | ||
| r.GET("/users/:id", getUserByID) | ||
| r.POST("/users", createUser) | ||
| r.PUT("/users/:id", updateUser) | ||
| r.DELETE("/users/:id", deleteUser) | ||
| r.GET("/users/search", searchUsers) | ||
| r.Run() // listens on 0.0.0.0:8080 by default |
There was a problem hiding this comment.
Route registration order causes a startup panic.
Registering GET /users/search (line 55) after GET /users/:id (line 51) will cause a panic: wildcard route conflicts with existing children at startup, because Gin's httprouter radix tree does not allow inserting a static child node once a wildcard has already been committed at the same path segment. The server will never start.
The wildcard route /users/:id conflicts with the static route /users/search because the router matches the prefixes one by one — not the complete route at once. The fix is to register the static route first:
🐛 Proposed fix — register /users/search before /users/:id
r := gin.Default()
r.GET("/users", getAllUsers)
-r.GET("/users/:id", getUserByID)
+r.GET("/users/search", searchUsers)
+r.GET("/users/:id", getUserByID)
r.POST("/users", createUser)
r.PUT("/users/:id", updateUser)
r.DELETE("/users/:id", deleteUser)
-r.GET("/users/search", searchUsers)
r.Run()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| r := gin.Default() | |
| r.GET("/users", getAllUsers) | |
| r.GET("/users/:id", getUserByID) | |
| r.POST("/users", createUser) | |
| r.PUT("/users/:id", updateUser) | |
| r.DELETE("/users/:id", deleteUser) | |
| r.GET("/users/search", searchUsers) | |
| r.Run() // listens on 0.0.0.0:8080 by default | |
| r := gin.Default() | |
| r.GET("/users", getAllUsers) | |
| r.GET("/users/search", searchUsers) | |
| r.GET("/users/:id", getUserByID) | |
| r.POST("/users", createUser) | |
| r.PUT("/users/:id", updateUser) | |
| r.DELETE("/users/:id", deleteUser) | |
| r.Run() // listens on 0.0.0.0:8080 by default |
| func getAllUsers(c *gin.Context) { | ||
| // TODO: Return all users | ||
| resp := Response{ | ||
| Success: true, | ||
| Data: users, | ||
| Message: "ssss", | ||
| Code: 200, | ||
| } | ||
| c.JSON(200, resp) | ||
| } |
There was a problem hiding this comment.
Remove the debug string "ssss" from the response.
Message: "ssss" (line 67) is a leftover debug artifact that will cause test assertion failures on response body checks and will be returned to real clients.
🐛 Proposed fix
resp := Response{
Success: true,
Data: users,
- Message: "ssss",
+ Message: "Get all users success",
Code: 200,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func getAllUsers(c *gin.Context) { | |
| // TODO: Return all users | |
| resp := Response{ | |
| Success: true, | |
| Data: users, | |
| Message: "ssss", | |
| Code: 200, | |
| } | |
| c.JSON(200, resp) | |
| } | |
| func getAllUsers(c *gin.Context) { | |
| // TODO: Return all users | |
| resp := Response{ | |
| Success: true, | |
| Data: users, | |
| Message: "Get all users success", | |
| Code: 200, | |
| } | |
| c.JSON(200, resp) | |
| } |
| // Helper function to validate user data | ||
| func validateUser(user User) error { | ||
| // TODO: Implement validation | ||
| // Check required fields: Name, Email | ||
| // Validate email format (basic check) | ||
| if user.Name == "" || user.Email == "" || user.Age <= 0 { | ||
| return errors.New("invalid user") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
validateUser doesn't actually validate email format despite the comment.
The TODO/comment states "Validate email format (basic check)" but only a non-empty check is performed — "notanemail" would pass. Add at least a minimal strings.Contains(user.Email, "@") guard.
🐛 Proposed fix
-if user.Name == "" || user.Email == "" || user.Age <= 0 {
- return errors.New("invalid user")
-}
+if user.Name == "" {
+ return errors.New("name is required")
+}
+if user.Email == "" || !strings.Contains(user.Email, "@") {
+ return errors.New("valid email is required")
+}
+if user.Age <= 0 {
+ return errors.New("age must be positive")
+}
gin challenge-1-basic-routing Solution
Submitted by: @nbploc08
Package: gin
Challenge: challenge-1-basic-routing
Description
This PR contains my solution for gin challenge-1-basic-routing.
Changes
packages/gin/challenge-1-basic-routing/submissions/nbploc08/solution.goTesting
Thank you for reviewing my submission! 🚀