feat(oauth,user)finish the logout and users functions #6
Conversation
WalkthroughThe changes introduce new request types, controller methods, and services to handle user updates and role management. Updates were made to authentication endpoints by adding logout and user info update functionalities. Additionally, a new user management controller and routes were created to support operations like retrieving and updating users. The data layer has been enhanced with new models, DAO methods, and migrations. Auxiliary packages like ginx and the dependency injection configuration have been updated for better error handling and integration of the new user features. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant AC as AuthController
participant AS as AuthService
participant R as RedisJWTHandler
C->>AC: Logout Request
AC->>AS: Logout(ctx)
AS->>R: ClearToken()
R-->>AS: Confirmation/Error
AS-->>AC: Logout Result
AC-->>C: Logout Response
sequenceDiagram
participant C as Client
participant UC as UserController
participant US as UserService
participant DAO as UserDAO
C->>UC: GetUsers Request
UC->>US: GetUsers(ctx, id)
US->>DAO: FindByProjectID(ctx, id)
DAO-->>US: Users List
US->>DAO: GetResponse(users)
DAO-->>US: Formatted Data
US-->>UC: User Data
UC-->>C: Response
Note over C,UC: Alternatively, for updating user roles:
C->>UC: UpdateUsers Request
UC->>US: UpdateUserRole(ctx, userId, projectPermit)
US->>DAO: ChangeProjectRole(ctx, userId, projectPermit)
DAO-->>US: Update Result
US-->>UC: Confirmation
UC-->>C: Response
Poem
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
router/User.go (2)
22-22: Consider using GET method for retrieving users.The
/getUsersendpoint uses POST method but since it's retrieving data, consider using GET method to align with REST principles.Apply this diff to use GET method:
- UserGroup.POST("/getUsers", authMiddleware, ginx.WrapReq(c.GetUsers)) + UserGroup.GET("/users", authMiddleware, ginx.WrapReq(c.GetUsers))
23-23: Consider renaming updateUser endpoint for consistency.For consistency with REST principles and the previous suggestion, consider renaming the endpoint.
Apply this diff to rename the endpoint:
- UserGroup.POST("/updateUser", authMiddleware, ginx.WrapReq(c.UpdateUsers)) + UserGroup.PUT("/users", authMiddleware, ginx.WrapReq(c.UpdateUsers))router/Auth.go (1)
23-23: Consider keeping the route group as/authfor authentication-related endpoints.The route group has been changed from
/authto/user, but this seems inconsistent since this is in the authentication controller and handles authentication-related operations like login, logout, and registration.- authGroup := s.Group("/user") + authGroup := s.Group("/auth")service/User.go (1)
30-36: Simplify error handling in UpdateUserRole.The error handling can be simplified by directly returning the error from
ChangeProjectRole.func (userService *UserService) UpdateUserRole(ctx context.Context, userId int, projectPermit []model.ProjectPermit) error { - err := userService.userDAO.ChangeProjectRole(ctx, userId, projectPermit) - if err != nil { - return err - } - return nil + return userService.userDAO.ChangeProjectRole(ctx, userId, projectPermit) }repository/model/model.go (1)
23-34: Add documentation for new structs.Consider adding documentation comments for
ProjectPermitandUserResponsestructs to explain their purpose and usage.+// ProjectPermit represents the permissions a user has for a specific project type ProjectPermit struct { ProjectID uint `json:"project_id"` ProjectName string `json:"project_name"` ProjectRole int `json:"project_role"` } +// UserResponse represents the API response structure for user data type UserResponse struct { Name string `json:"name"` UserID uint `json:"user_id"` Avatar string `json:"avatar"` ProjectPermit []ProjectPermit `json:"project_permit"` Role int `json:"role"` }service/Auth.go (1)
55-61: Consider cleaning up user session data during logout.The logout implementation only clears the token. Consider cleaning up any additional session data.
controller/Auth.go (1)
95-117: Improve error messages and reduce code duplication.The Logout method has empty error messages and duplicated error response structure.
func (c *AuthController) Logout(ctx *gin.Context) (response.Response, error) { _, err := ginx.GetClaims[jwt.UserClaims](ctx) if err != nil { return response.Response{ - Msg: "", + Msg: "Invalid or expired token", Code: 40001, Data: nil, }, err } err = c.service.Logout(ctx) if err != nil { return response.Response{ - Msg: "", + Msg: "Failed to logout", Code: 40001, Data: nil, }, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
api/request/request.go(1 hunks)controller/Auth.go(3 hunks)controller/controller.go(1 hunks)pkg/ginx/ginx.go(5 hunks)repository/dao/dao.go(1 hunks)repository/dao/user.go(1 hunks)repository/model/model.go(1 hunks)router/Auth.go(2 hunks)router/User.go(1 hunks)router/router.go(2 hunks)service/Auth.go(2 hunks)service/User.go(1 hunks)wire.go(1 hunks)wire_gen.go(1 hunks)
🔇 Additional comments (14)
repository/dao/dao.go (1)
9-9: LGTM! Verify model dependencies.The migration logic is correct. However, let's verify that all required model dependencies are properly defined.
✅ Verification successful
Model dependencies verified successfully.
- The definitions for
User,Project, andUserProjectare present inrepository/model/model.goand include the expected fields and relationships.- The many-to-many relationship in
Project(viaUsers []User gorm:"many2many:user_projects;") confirms proper dependency mapping without circular references.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify model dependencies and their relationships. # Test: Check model definitions and their relationships. ast-grep --pattern $'type (User|Project|UserProject) struct { $$$ }' # Test: Check for any circular dependencies between models. rg -A 5 $'type (User|Project|UserProject) struct'Length of output: 1132
api/request/request.go (1)
21-25: Verify the ProjectPermit type definition.The struct uses
model.ProjectPermittype. Let's verify its definition and usage.✅ Verification successful
ProjectPermit type definition and usage are confirmed to be correct.
- The
ProjectPermitstruct is defined inrepository/model/model.gowith fieldsProjectID,ProjectName, andProjectRole.- Its usage across the codebase (e.g., in
api/request/request.go, controller, service, and DAO layers) is consistent with the definition.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ProjectPermit type definition and its usage. # Test: Check ProjectPermit type definition ast-grep --pattern $'type ProjectPermit struct { $$$ }' # Test: Check for any other usages of ProjectPermit rg -A 5 $'ProjectPermit'Length of output: 3349
router/router.go (1)
12-12: LGTM!The integration of UserController and user routes is clean and well-structured.
Also applies to: 31-31
router/Auth.go (1)
12-14: LGTM! New authentication endpoints are well-structured.The new methods and routes for registration, logout, and user info updates are properly implemented with appropriate middleware protection where needed.
Also applies to: 25-27
service/User.go (1)
19-29: Verify the GetUsers implementation logic.The method name
GetUserssuggests returning multiple users, but it's usingFindByIDwhich typically returns a single user. This seems inconsistent.Consider either:
- Renaming the method to
GetUserif it's meant to return a single user- Modifying the implementation to fetch multiple users if that's the intended behavior
-func (userService *UserService) GetUsers(ctx context.Context, id int) ([]model.UserResponse, error) { +func (userService *UserService) GetUser(ctx context.Context, id int) (*model.UserResponse, error) {repository/model/model.go (1)
5-12: LGTM! Well-structured model definitions.The model definitions are well-organized with appropriate GORM tags and relationships.
Also applies to: 13-17, 18-22
wire.go (1)
40-42: LGTM! Proper dependency injection setup.The new user service and controller are correctly integrated into the wire dependency injection setup.
wire_gen.go (1)
39-41: LGTM! Dependencies are properly wired.The dependency injection setup for userService and userController looks correct.
Also applies to: 47-47
pkg/ginx/ginx.go (6)
16-20: LGTM!Good addition of middleware error checking to prevent further processing when errors exist.
23-23: LGTM!Correct fix to pass request by reference for proper binding.
51-54: LGTM!Good improvements:
- Added middleware error checking.
- Fixed request binding by reference.
Also applies to: 57-57
78-81: LGTM!Good addition of middleware error checking.
96-99: LGTM!Good addition of middleware error checking.
122-124: LGTM!Good improvement to use appropriate binding methods based on the request method:
- ShouldBindQuery for GET requests.
- ShouldBind for other methods.
api/request/request.go
Outdated
| type GetUserReq struct { | ||
| Role int `json:"role"` | ||
| Projecet_id int `json:"project_id"` | ||
| } |
There was a problem hiding this comment.
Fix typo in field name Projecet_id.
The field name contains a typo and should be corrected to maintain consistency.
Apply this diff to fix the typo:
type GetUserReq struct {
Role int `json:"role"`
- Projecet_id int `json:"project_id"`
+ Project_id int `json:"project_id"`
}📝 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.
| type GetUserReq struct { | |
| Role int `json:"role"` | |
| Projecet_id int `json:"project_id"` | |
| } | |
| type GetUserReq struct { | |
| Role int `json:"role"` | |
| Project_id int `json:"project_id"` | |
| } |
controller/controller.go
Outdated
| if req.Role != 2 { | ||
| return response.Response{ | ||
| Msg: "无权限", | ||
| Code: 40001, | ||
| Data: nil, | ||
| }, nil | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded role check with constants.
The role check is hardcoded to value 2. This makes the code less maintainable and prone to errors.
Consider creating an enum or constants for roles:
+const (
+ ADMIN_ROLE = 2
+)
- if req.Role != 2 {
+ if req.Role != ADMIN_ROLE {Also applies to: 46-52
controller/controller.go
Outdated
| }, nil | ||
|
|
||
| } | ||
| userResponse, err := uc.service.GetUsers(ctx, req.Projecet_id) |
There was a problem hiding this comment.
Fix typo in field name.
The field name Projecet_id contains a typo.
- userResponse, err := uc.service.GetUsers(ctx, req.Projecet_id)
+ userResponse, err := uc.service.GetUsers(ctx, req.Project_id)📝 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.
| userResponse, err := uc.service.GetUsers(ctx, req.Projecet_id) | |
| userResponse, err := uc.service.GetUsers(ctx, req.Project_id) |
service/Auth.go
Outdated
| func (s *AuthService) UpdateMyInfo(ctx context.Context, req request.UpdateUserReq) error { | ||
| var user model.User | ||
| user.Email = req.Email | ||
| user.Name = req.Name | ||
| err := s.userDAO.Update(ctx, &user) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Add input validation and user existence check.
The UpdateMyInfo method should validate input data and check if the user exists before updating.
func (s *AuthService) UpdateMyInfo(ctx context.Context, req request.UpdateUserReq) error {
+ // Validate input
+ if req.Email == "" || req.Name == "" {
+ return errors.New("email and name are required")
+ }
+
+ // Check if user exists
+ existingUser, err := s.userDAO.FindByEmail(ctx, req.Email)
+ if err != nil {
+ return err
+ }
+ if existingUser == nil {
+ return errors.New("user not found")
+ }
+
var user model.User
+ user.ID = existingUser.ID
user.Email = req.Email
user.Name = req.Name
err := s.userDAO.Update(ctx, &user)📝 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 (s *AuthService) UpdateMyInfo(ctx context.Context, req request.UpdateUserReq) error { | |
| var user model.User | |
| user.Email = req.Email | |
| user.Name = req.Name | |
| err := s.userDAO.Update(ctx, &user) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| } | |
| func (s *AuthService) UpdateMyInfo(ctx context.Context, req request.UpdateUserReq) error { | |
| // Validate input | |
| if req.Email == "" || req.Name == "" { | |
| return errors.New("email and name are required") | |
| } | |
| // Check if user exists | |
| existingUser, err := s.userDAO.FindByEmail(ctx, req.Email) | |
| if err != nil { | |
| return err | |
| } | |
| if existingUser == nil { | |
| return errors.New("user not found") | |
| } | |
| var user model.User | |
| user.ID = existingUser.ID | |
| user.Email = req.Email | |
| user.Name = req.Name | |
| err := s.userDAO.Update(ctx, &user) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| } |
controller/Auth.go
Outdated
| func (c *AuthController) UpdateMyInfo(ctx *gin.Context, req request.UpdateUserReq) (response.Response, error) { | ||
| err := c.service.UpdateMyInfo(ctx, req) | ||
| if err != nil { | ||
| return response.Response{}, err | ||
| } | ||
| return response.Response{ | ||
| Msg: "更新用户信息成功", | ||
| Code: 200, | ||
| Data: nil, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Add claims validation for UpdateMyInfo.
The UpdateMyInfo method should validate user claims before updating information.
func (c *AuthController) UpdateMyInfo(ctx *gin.Context, req request.UpdateUserReq) (response.Response, error) {
+ claims, err := ginx.GetClaims[jwt.UserClaims](ctx)
+ if err != nil {
+ return response.Response{
+ Msg: "Invalid or expired token",
+ Code: 40001,
+ Data: nil,
+ }, err
+ }
+
err := c.service.UpdateMyInfo(ctx, req)
if err != nil {
- return response.Response{}, err
+ return response.Response{
+ Msg: "Failed to update user info",
+ Code: 40001,
+ Data: nil,
+ }, err
}📝 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 (c *AuthController) UpdateMyInfo(ctx *gin.Context, req request.UpdateUserReq) (response.Response, error) { | |
| err := c.service.UpdateMyInfo(ctx, req) | |
| if err != nil { | |
| return response.Response{}, err | |
| } | |
| return response.Response{ | |
| Msg: "更新用户信息成功", | |
| Code: 200, | |
| Data: nil, | |
| }, nil | |
| } | |
| func (c *AuthController) UpdateMyInfo(ctx *gin.Context, req request.UpdateUserReq) (response.Response, error) { | |
| claims, err := ginx.GetClaims[jwt.UserClaims](ctx) | |
| if err != nil { | |
| return response.Response{ | |
| Msg: "Invalid or expired token", | |
| Code: 40001, | |
| Data: nil, | |
| }, err | |
| } | |
| err = c.service.UpdateMyInfo(ctx, req) | |
| if err != nil { | |
| return response.Response{ | |
| Msg: "Failed to update user info", | |
| Code: 40001, | |
| Data: nil, | |
| }, err | |
| } | |
| return response.Response{ | |
| Msg: "更新用户信息成功", | |
| Code: 200, | |
| Data: nil, | |
| }, nil | |
| } |
repository/dao/user.go
Outdated
| func (d *UserDAO) FindByID(ctx context.Context, id int) ([]model.User, error) { | ||
| var users []model.User | ||
| err := d.DB.WithContext(ctx).Preload("Projects").Joins("JOIN user_projects ON user_projects.user_id = users.id"). | ||
| Where("user_projects.project_id = ? AND user_projects.role = ?", id, 1).Find(&users).Error | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return users, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance flexibility and error handling.
The function has the following issues:
- The role value is hardcoded to 1, limiting reusability.
- Missing error handling for gorm.ErrRecordNotFound.
Apply this diff to improve the function:
-func (d *UserDAO) FindByID(ctx context.Context, id int) ([]model.User, error) {
+func (d *UserDAO) FindByID(ctx context.Context, id int, role int) ([]model.User, error) {
var users []model.User
err := d.DB.WithContext(ctx).Preload("Projects").Joins("JOIN user_projects ON user_projects.user_id = users.id").
- Where("user_projects.project_id = ? AND user_projects.role = ?", id, 1).Find(&users).Error
+ Where("user_projects.project_id = ? AND user_projects.role = ?", id, role).Find(&users).Error
if err != nil {
+ if errors.Is(err, gorm.ErrRecordNotFound) {
+ return nil, nil
+ }
return nil, err
}
return users, nil
}📝 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 (d *UserDAO) FindByID(ctx context.Context, id int) ([]model.User, error) { | |
| var users []model.User | |
| err := d.DB.WithContext(ctx).Preload("Projects").Joins("JOIN user_projects ON user_projects.user_id = users.id"). | |
| Where("user_projects.project_id = ? AND user_projects.role = ?", id, 1).Find(&users).Error | |
| if err != nil { | |
| return nil, err | |
| } | |
| return users, nil | |
| } | |
| func (d *UserDAO) FindByID(ctx context.Context, id int, role int) ([]model.User, error) { | |
| var users []model.User | |
| err := d.DB.WithContext(ctx).Preload("Projects"). | |
| Joins("JOIN user_projects ON user_projects.user_id = users.id"). | |
| Where("user_projects.project_id = ? AND user_projects.role = ?", id, role). | |
| Find(&users).Error | |
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| return nil, nil | |
| } | |
| return nil, err | |
| } | |
| return users, nil | |
| } |
repository/dao/user.go
Outdated
| func (d *UserDAO) ChangeProjectRole(ctx context.Context, userId int, projectPermit []model.ProjectPermit) error { | ||
| var user model.User | ||
| err := d.DB.WithContext(ctx).Preload("Projects").Where("user_id = ?", userId).First(&user).Error | ||
| if err != nil { | ||
| return errors.New("未找到该用户") | ||
| } | ||
| var userProject model.UserProject | ||
| for _, project := range projectPermit { | ||
| userProject.Role = project.ProjectRole | ||
| userProject.UserID = user.ID | ||
| userProject.ProjectID = project.ProjectID | ||
| d.DB.WithContext(ctx).Save(&userProject) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Fix data consistency and error handling issues.
The function has several critical issues:
- Missing transaction for batch updates.
- Missing error handling for DB queries.
- Incorrect user lookup using "user_id" field.
Apply this diff to improve the function:
func (d *UserDAO) ChangeProjectRole(ctx context.Context, userId int, projectPermit []model.ProjectPermit) error {
var user model.User
- err := d.DB.WithContext(ctx).Preload("Projects").Where("user_id = ?", userId).First(&user).Error
+ err := d.DB.WithContext(ctx).Preload("Projects").Where("id = ?", userId).First(&user).Error
if err != nil {
+ if errors.Is(err, gorm.ErrRecordNotFound) {
+ return errors.New("未找到该用户")
+ }
return errors.New("未找到该用户")
}
- var userProject model.UserProject
- for _, project := range projectPermit {
- userProject.Role = project.ProjectRole
- userProject.UserID = user.ID
- userProject.ProjectID = project.ProjectID
- d.DB.WithContext(ctx).Save(&userProject)
+
+ // Start transaction
+ tx := d.DB.WithContext(ctx).Begin()
+ if tx.Error != nil {
+ return tx.Error
+ }
+ defer func() {
+ if r := recover(); r != nil {
+ tx.Rollback()
+ }
+ }()
+
+ for _, project := range projectPermit {
+ var userProject model.UserProject
+ err := tx.Where("user_id = ? AND project_id = ?", user.ID, project.ProjectID).
+ FirstOrCreate(&userProject).Error
+ if err != nil {
+ tx.Rollback()
+ return err
+ }
+
+ userProject.Role = project.ProjectRole
+ if err := tx.Save(&userProject).Error; err != nil {
+ tx.Rollback()
+ return err
+ }
}
+
+ if err := tx.Commit().Error; err != nil {
+ return err
+ }
+
return nil
}📝 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 (d *UserDAO) ChangeProjectRole(ctx context.Context, userId int, projectPermit []model.ProjectPermit) error { | |
| var user model.User | |
| err := d.DB.WithContext(ctx).Preload("Projects").Where("user_id = ?", userId).First(&user).Error | |
| if err != nil { | |
| return errors.New("未找到该用户") | |
| } | |
| var userProject model.UserProject | |
| for _, project := range projectPermit { | |
| userProject.Role = project.ProjectRole | |
| userProject.UserID = user.ID | |
| userProject.ProjectID = project.ProjectID | |
| d.DB.WithContext(ctx).Save(&userProject) | |
| } | |
| return nil | |
| } | |
| func (d *UserDAO) ChangeProjectRole(ctx context.Context, userId int, projectPermit []model.ProjectPermit) error { | |
| var user model.User | |
| err := d.DB.WithContext(ctx).Preload("Projects").Where("id = ?", userId).First(&user).Error | |
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| return errors.New("未找到该用户") | |
| } | |
| return errors.New("未找到该用户") | |
| } | |
| // Start transaction | |
| tx := d.DB.WithContext(ctx).Begin() | |
| if tx.Error != nil { | |
| return tx.Error | |
| } | |
| defer func() { | |
| if r := recover(); r != nil { | |
| tx.Rollback() | |
| } | |
| }() | |
| for _, project := range projectPermit { | |
| var userProject model.UserProject | |
| err := tx.Where("user_id = ? AND project_id = ?", user.ID, project.ProjectID). | |
| FirstOrCreate(&userProject).Error | |
| if err != nil { | |
| tx.Rollback() | |
| return err | |
| } | |
| userProject.Role = project.ProjectRole | |
| if err := tx.Save(&userProject).Error; err != nil { | |
| tx.Rollback() | |
| return err | |
| } | |
| } | |
| if err := tx.Commit().Error; err != nil { | |
| return err | |
| } | |
| return nil | |
| } |
repository/dao/user.go
Outdated
| func (d *UserDAO) GetResponse(ctx context.Context, users []model.User) ([]model.UserResponse, error) { | ||
| var userResponses []model.UserResponse | ||
| for _, user := range users { | ||
| var projectPermits []model.ProjectPermit | ||
| for _, project := range user.Projects { | ||
| var userProject model.UserProject | ||
| d.DB.Where("user_id = ? AND project_id = ?", user.ID, project.ID).First(&userProject) | ||
|
|
||
| projectPermits = append(projectPermits, model.ProjectPermit{ | ||
| ProjectID: project.ID, | ||
| ProjectName: project.ProjectName, | ||
| ProjectRole: userProject.Role, | ||
| }) | ||
| } | ||
|
|
||
| userResponses = append(userResponses, model.UserResponse{ | ||
| Name: user.Name, | ||
| UserID: user.ID, | ||
| Avatar: user.Avatar, | ||
| ProjectPermit: projectPermits, | ||
| Role: user.UserRole, | ||
| }) | ||
| } | ||
|
|
||
| return userResponses, nil | ||
| } |
There was a problem hiding this comment.
Fix N+1 query problem and add error handling.
The function has performance and reliability issues:
- N+1 query problem: DB query inside loop.
- Missing error handling for DB queries.
- Missing context in DB queries.
Apply this diff to improve the function:
func (d *UserDAO) GetResponse(ctx context.Context, users []model.User) ([]model.UserResponse, error) {
var userResponses []model.UserResponse
+ // Get all user IDs and project IDs for batch query
+ var userIDs []uint
+ var projectIDs []uint
+ for _, user := range users {
+ userIDs = append(userIDs, user.ID)
+ for _, project := range user.Projects {
+ projectIDs = append(projectIDs, project.ID)
+ }
+ }
+
+ // Batch query user_projects
+ var userProjects []model.UserProject
+ if err := d.DB.WithContext(ctx).Where("user_id IN ? AND project_id IN ?", userIDs, projectIDs).Find(&userProjects).Error; err != nil {
+ return nil, err
+ }
+
+ // Create a map for quick lookup
+ userProjectMap := make(map[string]model.UserProject)
+ for _, up := range userProjects {
+ key := fmt.Sprintf("%d-%d", up.UserID, up.ProjectID)
+ userProjectMap[key] = up
+ }
+
for _, user := range users {
var projectPermits []model.ProjectPermit
for _, project := range user.Projects {
- var userProject model.UserProject
- d.DB.Where("user_id = ? AND project_id = ?", user.ID, project.ID).First(&userProject)
+ key := fmt.Sprintf("%d-%d", user.ID, project.ID)
+ userProject, exists := userProjectMap[key]
+ if !exists {
+ continue
+ }
projectPermits = append(projectPermits, model.ProjectPermit{
ProjectID: project.ID,📝 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 (d *UserDAO) GetResponse(ctx context.Context, users []model.User) ([]model.UserResponse, error) { | |
| var userResponses []model.UserResponse | |
| for _, user := range users { | |
| var projectPermits []model.ProjectPermit | |
| for _, project := range user.Projects { | |
| var userProject model.UserProject | |
| d.DB.Where("user_id = ? AND project_id = ?", user.ID, project.ID).First(&userProject) | |
| projectPermits = append(projectPermits, model.ProjectPermit{ | |
| ProjectID: project.ID, | |
| ProjectName: project.ProjectName, | |
| ProjectRole: userProject.Role, | |
| }) | |
| } | |
| userResponses = append(userResponses, model.UserResponse{ | |
| Name: user.Name, | |
| UserID: user.ID, | |
| Avatar: user.Avatar, | |
| ProjectPermit: projectPermits, | |
| Role: user.UserRole, | |
| }) | |
| } | |
| return userResponses, nil | |
| } | |
| func (d *UserDAO) GetResponse(ctx context.Context, users []model.User) ([]model.UserResponse, error) { | |
| var userResponses []model.UserResponse | |
| // Get all user IDs and project IDs for batch query | |
| var userIDs []uint | |
| var projectIDs []uint | |
| for _, user := range users { | |
| userIDs = append(userIDs, user.ID) | |
| for _, project := range user.Projects { | |
| projectIDs = append(projectIDs, project.ID) | |
| } | |
| } | |
| // Batch query user_projects | |
| var userProjects []model.UserProject | |
| if err := d.DB.WithContext(ctx).Where("user_id IN ? AND project_id IN ?", userIDs, projectIDs).Find(&userProjects).Error; err != nil { | |
| return nil, err | |
| } | |
| // Create a map for quick lookup | |
| userProjectMap := make(map[string]model.UserProject) | |
| for _, up := range userProjects { | |
| key := fmt.Sprintf("%d-%d", up.UserID, up.ProjectID) | |
| userProjectMap[key] = up | |
| } | |
| for _, user := range users { | |
| var projectPermits []model.ProjectPermit | |
| for _, project := range user.Projects { | |
| key := fmt.Sprintf("%d-%d", user.ID, project.ID) | |
| userProject, exists := userProjectMap[key] | |
| if !exists { | |
| continue | |
| } | |
| projectPermits = append(projectPermits, model.ProjectPermit{ | |
| ProjectID: project.ID, | |
| ProjectName: project.ProjectName, | |
| ProjectRole: userProject.Role, | |
| }) | |
| } | |
| userResponses = append(userResponses, model.UserResponse{ | |
| Name: user.Name, | |
| UserID: user.ID, | |
| Avatar: user.Avatar, | |
| ProjectPermit: projectPermits, | |
| Role: user.UserRole, | |
| }) | |
| } | |
| return userResponses, nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (18)
pkg/jwt/jwt.go (1)
68-70: Improve field naming consistency and comments.Consider these improvements:
- Rename
UserRuletoUserRoleto match the parameter name inSetJWTToken.- Make comment style consistent by adding periods to all comments.
Apply this diff:
- Uid uint // 用户 ID - Email string //用户名称 - UserRule int + Uid uint // 用户 ID. + Email string // 用户名称. + UserRole int // 用户角色.controller/User.go (3)
14-20: Use consistent naming and ensure a clear interface contract.
Your struct field is named "service UserService" while your constructor accepts a "*service.UserService". This might be confusing. Also validate that all methods in "service.UserService" match the "UserService" interface methods.
22-26: Consider dependency injection improvements.
The constructor ties the controller directly to the implementation in "service.UserService". If you want to keep it more flexible, consider depending on the interface instead of the concrete struct to improve testability.
47-68: Rename "UpdateUsers" to reflect single-user role updates.
The method updates one user's role, so "UpdateUser" or "UpdateUserRole" might be more precise. This improves clarity and consistency with "UpdateUserRoleReq".api/response/response.go (1)
13-19: Fix spelling of "TotleNumber".
"TotleNumber" is likely a typo. Change it to "TotalNumber" for clarity and correctness.Apply this diff to correct the field name:
- TotleNumber int `json:"totle_number"` + TotalNumber int `json:"total_number"`router/Project.go (1)
16-26: Consider adding input validation middleware.While the routes are properly protected with authentication middleware, consider adding input validation middleware to ensure request data meets requirements before reaching the controller.
pkg/apikey/apikey.go (1)
12-27: Make token expiration configurable.The token expiration time is hardcoded to 24 hours. Consider making this configurable through environment variables or configuration files for different environments.
router/router.go (1)
10-18: Improve parameter ordering for better readability.Consider grouping related parameters together. Move Project controller next to User controller:
func NewRouter( OAuth *controller.AuthController, User *controller.UserController, + Project *controller.ProjectController, AuthMiddleware *middleware.AuthMiddleware, corsMiddleware *middleware.CorsMiddleware, loggerMiddleware *middleware.LoggerMiddleware, - Project *controller.ProjectController, ) *gin.Engine {service/User.go (2)
19-29: Consider adding input validation.While the error handling is good, consider validating the project ID before making the DAO call.
func (userService *UserService) GetUsers(ctx context.Context, id int) ([]model.UserResponse, error) { + if id <= 0 { + return nil, fmt.Errorf("invalid project ID: %d", id) + } users, err := userService.userDAO.FindByProjectID(ctx, id)
30-36: Add role validation and logging.The method should validate the role value and consider adding logging for audit purposes.
func (userService *UserService) UpdateUserRole(ctx context.Context, userId int, projectPermit []model.ProjectPermit, role int) error { + if userId <= 0 { + return fmt.Errorf("invalid user ID: %d", userId) + } + if role < 0 { + return fmt.Errorf("invalid role value: %d", role) + } + if len(projectPermit) == 0 { + return fmt.Errorf("project permits cannot be empty") + } err := userService.userDAO.ChangeProjectRole(ctx, userId, projectPermit, role)service/service.go (1)
41-56: Consider implementing caching for frequently accessed data.Both GetProjectList and Detail methods could benefit from caching to improve performance, especially if these endpoints are frequently accessed.
Consider implementing a caching layer using the existing Redis infrastructure:
- Cache project lists with appropriate TTL
- Implement cache invalidation on project updates
- Use cache-aside pattern for project details
repository/model/model.go (1)
7-12: Add indexes for frequently queried fields.Consider adding indexes to improve query performance on frequently accessed fields.
- Name string `gorm:"column:name;unique;not null"` + Name string `gorm:"column:name;unique;not null;index"` - Email string `gorm:"column:email;not null"` + Email string `gorm:"column:email;not null;index"`controller/Project.go (2)
28-42: Improve error handling and response consistency.Consider the following improvements:
- Use consistent error messages in English.
- Maintain consistent response structure between success and error cases.
Apply this diff:
func (ctrl *ProjectController) GetProjectList(ctx *gin.Context) (response.Response, error) { list, err := ctrl.service.GetProjectList(ctx) if err != nil { return response.Response{ Code: 400, - Msg: "获取列表失败", + Msg: "Failed to retrieve project list", Data: nil, }, err } return response.Response{ Data: list, Code: 200, - Msg: "获取列表成功", + Msg: "Successfully retrieved project list", }, nil }
43-70: Extract role constant and improve error messages.The hardcoded role value reduces maintainability. Consider extracting it as a constant and improving error messages.
Apply this diff:
+const ( + ADMIN_ROLE = 2 +) + func (ctrl *ProjectController) Create(ctx *gin.Context, req request.CreateProject) (response.Response, error) { token, err := ginx.GetClaims[jwt.UserClaims](ctx) if err != nil { return response.Response{ - Msg: "", + Msg: "Invalid or expired token", Code: 40001, Data: nil, }, err } - if token.UserRule != 2 { + if token.UserRule != ADMIN_ROLE { return response.Response{ Code: 400, - Msg: "无权限", + Msg: "Insufficient permissions: admin role required", }, nil }controller/Auth.go (2)
95-117: Add descriptive error messages.The error messages are empty, which makes debugging harder.
Apply this diff:
func (c *AuthController) Logout(ctx *gin.Context) (response.Response, error) { _, err := ginx.GetClaims[jwt.UserClaims](ctx) if err != nil { return response.Response{ - Msg: "", + Msg: "Invalid or expired token", Code: 40001, Data: nil, }, err } err = c.service.Logout(ctx) if err != nil { return response.Response{ - Msg: "", + Msg: "Failed to logout user", Code: 40001, Data: nil, }, err } return response.Response{ - Msg: "成功登出", + Msg: "Successfully logged out", Code: 200, Data: nil, }, nil }
118-136: Add error message for update failure.The error response is empty, which makes debugging harder.
Apply this diff:
err = c.service.UpdateMyInfo(ctx, req, token.Uid) if err != nil { - return response.Response{}, err + return response.Response{ + Msg: "Failed to update user information", + Code: 40001, + Data: nil, + }, err } return response.Response{ - Msg: "更新用户信息成功", + Msg: "Successfully updated user information", Code: 200, Data: nil, }, nilrepository/dao/user.go (2)
131-144: Optimize performance and improve error handling.Consider the following improvements:
- Use English error messages for better internationalization.
- Avoid unnecessary intermediate slice creation.
Apply this diff:
func (d *UserDAO) GetProjectList(ctx context.Context) ([]model.ProjectList, error) { - var projects []model.Project - if err := d.DB.WithContext(ctx).Find(&projects).Error; err != nil { - return nil, errors.New("查询数据库错误") + var projectList []model.ProjectList + if err := d.DB.WithContext(ctx).Model(&model.Project).Select("id as project_id, project_name").Find(&projectList).Error; err != nil { + return nil, errors.New("failed to query database") } - var projectlist []model.ProjectList - for _, project := range projects { - projectlist = append(projectlist, model.ProjectList{ - ProjectId: project.ID, - ProjectName: project.ProjectName, - }) - } - return projectlist, nil + return projectList, nil }
145-159: Improve error messages and add explicit transaction.Consider the following improvements:
- Use English error messages for better internationalization.
- Add explicit transaction for atomic operations.
Apply this diff:
func (d *UserDAO) CreateProject(ctx context.Context, project *model.Project) error { - if err := d.DB.WithContext(ctx).Create(project).Error; err != nil { - return errors.New("创建项目失败") + tx := d.DB.WithContext(ctx).Begin() + if tx.Error != nil { + return errors.New("failed to begin transaction") } + defer func() { + if r := recover(); r != nil { + tx.Rollback() + } + }() + + if err := tx.Create(project).Error; err != nil { + tx.Rollback() + return errors.New("failed to create project") } + key, err := apikey.GenerateAPIKey(strconv.Itoa(int(project.ID))) if err != nil { - return errors.New("生成apikey失败") + tx.Rollback() + return errors.New("failed to generate API key") } project.Apikey = key - if err := d.DB.WithContext(ctx).Save(project).Error; err != nil { + if err := tx.Save(project).Error; err != nil { + tx.Rollback() return err } + + if err := tx.Commit().Error; err != nil { + return errors.New("failed to commit transaction") + } + return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
api/request/request.go(1 hunks)api/response/response.go(2 hunks)controller/Auth.go(3 hunks)controller/Project.go(1 hunks)controller/User.go(1 hunks)pkg/apikey/apikey.go(1 hunks)pkg/jwt/jwt.go(2 hunks)pkg/jwt/redis_handler.go(1 hunks)repository/dao/dao.go(1 hunks)repository/dao/user.go(2 hunks)repository/model/model.go(1 hunks)router/Project.go(1 hunks)router/router.go(2 hunks)service/Auth.go(3 hunks)service/User.go(1 hunks)service/service.go(1 hunks)wire.go(1 hunks)wire_gen.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wire.go
- service/Auth.go
🔇 Additional comments (14)
pkg/jwt/jwt.go (1)
47-56: LGTM! Clean integration of user role information.The changes correctly integrate the user role information into the JWT claims while maintaining backward compatibility.
pkg/jwt/redis_handler.go (1)
66-68: LGTM! Clean propagation of user role information.The changes correctly integrate with the updated JWT package while maintaining proper error handling.
repository/dao/dao.go (1)
9-9: Check for related constraints and dependencies.
Auto-migrating multiple models is convenient but make sure any necessary foreign key constraints or indexes are correctly created. Consider a more controlled migration flow if your schema evolves frequently.api/response/response.go (1)
3-4: Import from “model” confirmed.
The import appears to be necessary for referencing "model.UserResponse" and is implemented correctly.router/Project.go (1)
10-14: LGTM! Well-defined interface with consistent method signatures.The ProjectController interface is well-structured with consistent method signatures, following the pattern of taking gin.Context and returning response.Response, error.
pkg/apikey/apikey.go (1)
28-37: LGTM! Proper signing method validation.The ParseAPIKey function correctly validates the signing method before returning the token.
service/User.go (1)
10-17: LGTM! Well-structured service with proper dependency injection.The UserService struct is well-designed with clear dependencies and a clean constructor function.
service/service.go (1)
11-18: Remove unused Count struct.The Count struct appears to be unused in the codebase. Consider removing it if it's not needed.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for usages of the Count struct rg "Count\s*{|Count\s*struct" -A 2Length of output: 152
🏁 Script executed:
#!/bin/bash # First, search for the Count struct declaration (definition) in the codebase. rg "type Count\s*\{" -A 2 # Next, search for any references to the Count identifier to see if it is used elsewhere. # This search includes any usage of the word 'Count' excluding comments might appear. rg "\bCount\b"Length of output: 83
Unused Count Struct Detected
Our search results show that the only occurrence of the Count struct is its declaration in service/service.go with no additional references elsewhere. It appears to be unused in the codebase and can be safely removed.wire_gen.go (1)
39-49: LGTM! Proper dependency initialization order.The initialization order is correct, with dependencies being created before they are needed. The router initialization properly includes all required controllers.
controller/Project.go (1)
14-21: LGTM! Well-structured interface design.The interface design follows good separation of concerns and has clear method signatures.
controller/Auth.go (1)
21-26: LGTM! Clear interface definition.The interface is well-defined with clear method signatures for authentication operations.
repository/dao/user.go (3)
71-79: Enhance flexibility and error handling.The function has the following issues:
- The role value is hardcoded to 1, limiting reusability.
- Missing error handling for gorm.ErrRecordNotFound.
89-114: Fix N+1 query problem and add error handling.The function has performance and reliability issues:
- N+1 query problem: DB query inside loop.
- Missing error handling for DB queries.
115-130: Fix data consistency and error handling issues.The function has several critical issues:
- Missing transaction for batch updates.
- Missing error handling for DB queries.
- Incorrect user lookup using "user_id" field.
| func (uc *UserController) GetUsers(ctx *gin.Context, req request.GetUserReq) (response.Response, error) { | ||
| if req.Role != 2 { | ||
| return response.Response{ | ||
| Msg: "无权限", | ||
| Code: 40001, | ||
| Data: nil, | ||
| }, nil | ||
|
|
||
| } | ||
| userResponse, err := uc.service.GetUsers(ctx, req.Project_id) | ||
| if err != nil { | ||
| return response.Response{}, err | ||
| } | ||
|
|
||
| return response.Response{ | ||
| Msg: "获取成功", | ||
| Code: 200, | ||
| Data: userResponse, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate role checks and messages.
"req.Role != 2 → 无权限" is a hard-coded comparison. Consider using configurable role constants or enumerations to avoid magic numbers. Also review the response message to see if a localized or more descriptive error would be better.
| "github.com/dgrijalva/jwt-go" | ||
| ) | ||
|
|
||
| var SecretKey = []byte("Muxi-Team-Auditor-Backend") |
There was a problem hiding this comment.
Move secret key to configuration.
Hardcoding sensitive values like secret keys is a security risk. Consider moving this to environment variables or a secure configuration management system.
| type GetUserReq struct { | ||
| Role int `json:"role"` | ||
| Project_id int `json:"project_id"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent field naming convention.
The Project_id field uses snake_case while other fields use camelCase. Consider using consistent camelCase for all JSON field names:
type GetUserReq struct {
Role int `json:"role"`
- Project_id int `json:"project_id"`
+ ProjectId int `json:"projectId"`
}📝 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.
| type GetUserReq struct { | |
| Role int `json:"role"` | |
| Project_id int `json:"project_id"` | |
| } | |
| type GetUserReq struct { | |
| Role int `json:"role"` | |
| ProjectId int `json:"projectId"` | |
| } |
| type UpdateUserReq struct { | ||
| Name string `json:"name"` | ||
| Email string `json:"email"` | ||
| Avatar string `json:"avatar"` | ||
| } | ||
| type GetUserReq struct { | ||
| Role int `json:"role"` | ||
| Project_id int `json:"project_id"` | ||
| } | ||
| type UpdateUserRoleReq struct { | ||
| Role int `json:"role"` | ||
| UserId int `json:"user_id"` | ||
| ProjectPermit []model.ProjectPermit `json:"project_permit"` | ||
| } | ||
| type CreateProject struct { | ||
| Name string `json:"name"` | ||
| Logo string `json:"logo"` | ||
| AudioRule string `json:"audio_rule"` | ||
| UserIds []uint `json:"user_ids"` | ||
| } | ||
| type GetProjectDetail struct { | ||
| ProjectId uint `json:"project_id"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation tags for request fields.
Consider adding validation tags to ensure request data meets requirements. For example:
- Email format validation
- Required fields
- Min/max length for strings
- Range validation for numeric fields
Example:
type UpdateUserReq struct {
- Name string `json:"name"`
- Email string `json:"email"`
- Avatar string `json:"avatar"`
+ Name string `json:"name" binding:"required,min=1,max=100"`
+ Email string `json:"email" binding:"required,email"`
+ Avatar string `json:"avatar" binding:"url"`
}📝 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.
| type UpdateUserReq struct { | |
| Name string `json:"name"` | |
| Email string `json:"email"` | |
| Avatar string `json:"avatar"` | |
| } | |
| type GetUserReq struct { | |
| Role int `json:"role"` | |
| Project_id int `json:"project_id"` | |
| } | |
| type UpdateUserRoleReq struct { | |
| Role int `json:"role"` | |
| UserId int `json:"user_id"` | |
| ProjectPermit []model.ProjectPermit `json:"project_permit"` | |
| } | |
| type CreateProject struct { | |
| Name string `json:"name"` | |
| Logo string `json:"logo"` | |
| AudioRule string `json:"audio_rule"` | |
| UserIds []uint `json:"user_ids"` | |
| } | |
| type GetProjectDetail struct { | |
| ProjectId uint `json:"project_id"` | |
| } | |
| type UpdateUserReq struct { | |
| Name string `json:"name" binding:"required,min=1,max=100"` | |
| Email string `json:"email" binding:"required,email"` | |
| Avatar string `json:"avatar" binding:"url"` | |
| } | |
| type GetUserReq struct { | |
| Role int `json:"role"` | |
| Project_id int `json:"project_id"` | |
| } | |
| type UpdateUserRoleReq struct { | |
| Role int `json:"role"` | |
| UserId int `json:"user_id"` | |
| ProjectPermit []model.ProjectPermit `json:"project_permit"` | |
| } | |
| type CreateProject struct { | |
| Name string `json:"name"` | |
| Logo string `json:"logo"` | |
| AudioRule string `json:"audio_rule"` | |
| UserIds []uint `json:"user_ids"` | |
| } | |
| type GetProjectDetail struct { | |
| ProjectId uint `json:"project_id"` | |
| } |
| func (s *ProjectService) Create(ctx context.Context, name string, logo string, audioRule string, ids []uint) error { | ||
|
|
||
| users, err := s.userDAO.FindByUserIDs(ctx, ids) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| project := model.Project{ | ||
| ProjectName: name, | ||
| Logo: logo, | ||
| AudioRule: audioRule, | ||
| Users: users, | ||
| } | ||
| err = s.userDAO.CreateProject(ctx, &project) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction handling and input validation.
The Create method should use a transaction to ensure atomic operations, and validate input parameters.
func (s *ProjectService) Create(ctx context.Context, name string, logo string, audioRule string, ids []uint) error {
+ if name == "" {
+ return fmt.Errorf("project name cannot be empty")
+ }
+ if len(ids) == 0 {
+ return fmt.Errorf("user IDs cannot be empty")
+ }
+
+ tx := s.userDAO.DB.Begin()
+ defer func() {
+ if r := recover(); r != nil {
+ tx.Rollback()
+ }
+ }()
users, err := s.userDAO.FindByUserIDs(ctx, ids)
if err != nil {
+ tx.Rollback()
return err
}
project := model.Project{
ProjectName: name,
Logo: logo,
AudioRule: audioRule,
Users: users,
}
- err = s.userDAO.CreateProject(ctx, &project)
+ err = s.userDAO.CreateProject(ctx, tx, &project)
if err != nil {
+ tx.Rollback()
return err
}
+ return tx.Commit().Error
- return nil
}📝 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 (s *ProjectService) Create(ctx context.Context, name string, logo string, audioRule string, ids []uint) error { | |
| users, err := s.userDAO.FindByUserIDs(ctx, ids) | |
| if err != nil { | |
| return err | |
| } | |
| project := model.Project{ | |
| ProjectName: name, | |
| Logo: logo, | |
| AudioRule: audioRule, | |
| Users: users, | |
| } | |
| err = s.userDAO.CreateProject(ctx, &project) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| } | |
| func (s *ProjectService) Create(ctx context.Context, name string, logo string, audioRule string, ids []uint) error { | |
| if name == "" { | |
| return fmt.Errorf("project name cannot be empty") | |
| } | |
| if len(ids) == 0 { | |
| return fmt.Errorf("user IDs cannot be empty") | |
| } | |
| tx := s.userDAO.DB.Begin() | |
| defer func() { | |
| if r := recover(); r != nil { | |
| tx.Rollback() | |
| } | |
| }() | |
| users, err := s.userDAO.FindByUserIDs(ctx, ids) | |
| if err != nil { | |
| tx.Rollback() | |
| return err | |
| } | |
| project := model.Project{ | |
| ProjectName: name, | |
| Logo: logo, | |
| AudioRule: audioRule, | |
| Users: users, | |
| } | |
| err = s.userDAO.CreateProject(ctx, tx, &project) | |
| if err != nil { | |
| tx.Rollback() | |
| return err | |
| } | |
| return tx.Commit().Error | |
| } |
| type Item struct { | ||
| gorm.Model | ||
| Status int `gorm:"column:status;not null"` //0:未审核1：通过2：否决 | ||
| ProjectId uint `gorm:"column:project_id;not null"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add enum validation for status field.
The status field in Item model should have proper validation for enum values.
type Item struct {
gorm.Model
- Status int `gorm:"column:status;not null"` //0:未审核1：通过2：否决
+ Status int `gorm:"column:status;not null;check:status IN (0,1,2)"` //0:未审核1：通过2：否决
ProjectId uint `gorm:"column:project_id;not null"`
}📝 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.
| type Item struct { | |
| gorm.Model | |
| Status int `gorm:"column:status;not null"` //0:未审核1：通过2：否决 | |
| ProjectId uint `gorm:"column:project_id;not null"` | |
| } | |
| type Item struct { | |
| gorm.Model | |
| Status int `gorm:"column:status;not null;check:status IN (0,1,2)"` //0:未审核1：通过2：否决 | |
| ProjectId uint `gorm:"column:project_id;not null"` | |
| } |
| type Project struct { | ||
| gorm.Model | ||
| ProjectName string `gorm:"column:project_name;not null"` | ||
| Logo string `gorm:"column:logo;not null"` | ||
| AudioRule string `gorm:"column:audio_rule;not null"` | ||
| Users []User `gorm:"many2many:user_projects;"` | ||
| Items []Item | ||
| Apikey string `gorm:"column:apikey;not null"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and improve database schema.
The Project model could benefit from additional constraints and indexes.
type Project struct {
gorm.Model
- ProjectName string `gorm:"column:project_name;not null"`
+ ProjectName string `gorm:"column:project_name;not null;index"`
Logo string `gorm:"column:logo;not null"`
AudioRule string `gorm:"column:audio_rule;not null"`
Users []User `gorm:"many2many:user_projects;"`
Items []Item
- Apikey string `gorm:"column:apikey;not null"`
+ Apikey string `gorm:"column:apikey;not null;uniqueIndex"`
}📝 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.
| type Project struct { | |
| gorm.Model | |
| ProjectName string `gorm:"column:project_name;not null"` | |
| Logo string `gorm:"column:logo;not null"` | |
| AudioRule string `gorm:"column:audio_rule;not null"` | |
| Users []User `gorm:"many2many:user_projects;"` | |
| Items []Item | |
| Apikey string `gorm:"column:apikey;not null"` | |
| } | |
| type Project struct { | |
| gorm.Model | |
| ProjectName string `gorm:"column:project_name;not null;index"` | |
| Logo string `gorm:"column:logo;not null"` | |
| AudioRule string `gorm:"column:audio_rule;not null"` | |
| Users []User `gorm:"many2many:user_projects;"` | |
| Items []Item | |
| Apikey string `gorm:"column:apikey;not null;uniqueIndex"` | |
| } |
| func (ctrl *ProjectController) Detail(ctx *gin.Context, req request.GetProjectDetail) (response.Response, error) { | ||
| _, err := ginx.GetClaims[jwt.UserClaims](ctx) | ||
| if err != nil { | ||
| return response.Response{ | ||
| Msg: "", | ||
| Code: 40001, | ||
| Data: nil, | ||
| }, err | ||
| } | ||
| detail, err := ctrl.service.Detail(ctx, req.ProjectId) | ||
| if err != nil { | ||
| return response.Response{ | ||
| Msg: "", | ||
| Code: 40001, | ||
| Data: nil, | ||
| }, err | ||
| } | ||
|
|
||
| return response.Response{ | ||
| Code: 200, | ||
| Msg: "获取成功", | ||
| Data: detail, | ||
| }, nil | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and utilize claims.
The claims validation is performed but not utilized, and error messages are empty.
Apply this diff:
func (ctrl *ProjectController) Detail(ctx *gin.Context, req request.GetProjectDetail) (response.Response, error) {
- _, err := ginx.GetClaims[jwt.UserClaims](ctx)
+ claims, err := ginx.GetClaims[jwt.UserClaims](ctx)
if err != nil {
return response.Response{
- Msg: "",
+ Msg: "Invalid or expired token",
Code: 40001,
Data: nil,
}, err
}
+
+ // Log access for audit trail
+ log.Printf("User %d accessed project details for project %d", claims.Uid, req.ProjectId)
+
detail, err := ctrl.service.Detail(ctx, req.ProjectId)
if err != nil {
return response.Response{
- Msg: "",
+ Msg: "Failed to retrieve project details",
Code: 40001,
Data: nil,
}, err
}
return response.Response{
Code: 200,
- Msg: "获取成功",
+ Msg: "Successfully retrieved project details",
Data: detail,
}, nil
}📝 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 (ctrl *ProjectController) Detail(ctx *gin.Context, req request.GetProjectDetail) (response.Response, error) { | |
| _, err := ginx.GetClaims[jwt.UserClaims](ctx) | |
| if err != nil { | |
| return response.Response{ | |
| Msg: "", | |
| Code: 40001, | |
| Data: nil, | |
| }, err | |
| } | |
| detail, err := ctrl.service.Detail(ctx, req.ProjectId) | |
| if err != nil { | |
| return response.Response{ | |
| Msg: "", | |
| Code: 40001, | |
| Data: nil, | |
| }, err | |
| } | |
| return response.Response{ | |
| Code: 200, | |
| Msg: "获取成功", | |
| Data: detail, | |
| }, nil | |
| } | |
| func (ctrl *ProjectController) Detail(ctx *gin.Context, req request.GetProjectDetail) (response.Response, error) { | |
| claims, err := ginx.GetClaims[jwt.UserClaims](ctx) | |
| if err != nil { | |
| return response.Response{ | |
| Msg: "Invalid or expired token", | |
| Code: 40001, | |
| Data: nil, | |
| }, err | |
| } | |
| // Log access for audit trail | |
| log.Printf("User %d accessed project details for project %d", claims.Uid, req.ProjectId) | |
| detail, err := ctrl.service.Detail(ctx, req.ProjectId) | |
| if err != nil { | |
| return response.Response{ | |
| Msg: "Failed to retrieve project details", | |
| Code: 40001, | |
| Data: nil, | |
| }, err | |
| } | |
| return response.Response{ | |
| Code: 200, | |
| Msg: "Successfully retrieved project details", | |
| Data: detail, | |
| }, nil | |
| } |
| func (d *UserDAO) GetProjectDetails(ctx context.Context, id uint) (response.GetDetailResp, error) { | ||
| var project model.Project | ||
| err := d.DB.WithContext(ctx).Preload("Items").Preload("Users").First(&project, id).Error | ||
| if err != nil { | ||
| return response.GetDetailResp{}, err | ||
| } | ||
| countMap := map[int]int{ | ||
| 0: 0, | ||
| 1: 0, | ||
| 2: 0, | ||
| } | ||
| for _, item := range project.Items { | ||
| countMap[item.Status]++ | ||
| } | ||
| var users []model.UserResponse | ||
| for _, user := range project.Users { | ||
| users = append(users, model.UserResponse{ | ||
| Name: user.Name, | ||
| UserID: user.ID, | ||
| Avatar: user.Avatar, | ||
| }) | ||
| } | ||
|
|
||
| re := response.GetDetailResp{ | ||
| TotleNumber: countMap[0] + countMap[1] + countMap[2], | ||
| CurrentNumber: countMap[0], | ||
| Apikey: project.Apikey, | ||
| AuditRule: project.AudioRule, | ||
| Members: users, | ||
| } | ||
| return re, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix typo and improve error handling.
The function has the following issues:
- Typo in field name "TotleNumber".
- Missing error handling for gorm.ErrRecordNotFound.
Apply this diff:
func (d *UserDAO) GetProjectDetails(ctx context.Context, id uint) (response.GetDetailResp, error) {
var project model.Project
err := d.DB.WithContext(ctx).Preload("Items").Preload("Users").First(&project, id).Error
if err != nil {
+ if errors.Is(err, gorm.ErrRecordNotFound) {
+ return response.GetDetailResp{}, errors.New("project not found")
+ }
return response.GetDetailResp{}, err
}
countMap := map[int]int{
@@ -183,7 +186,7 @@
re := response.GetDetailResp{
- TotleNumber: countMap[0] + countMap[1] + countMap[2],
+ TotalNumber: countMap[0] + countMap[1] + countMap[2],
CurrentNumber: countMap[0],
Apikey: project.Apikey,
AuditRule: project.AudioRule,📝 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 (d *UserDAO) GetProjectDetails(ctx context.Context, id uint) (response.GetDetailResp, error) { | |
| var project model.Project | |
| err := d.DB.WithContext(ctx).Preload("Items").Preload("Users").First(&project, id).Error | |
| if err != nil { | |
| return response.GetDetailResp{}, err | |
| } | |
| countMap := map[int]int{ | |
| 0: 0, | |
| 1: 0, | |
| 2: 0, | |
| } | |
| for _, item := range project.Items { | |
| countMap[item.Status]++ | |
| } | |
| var users []model.UserResponse | |
| for _, user := range project.Users { | |
| users = append(users, model.UserResponse{ | |
| Name: user.Name, | |
| UserID: user.ID, | |
| Avatar: user.Avatar, | |
| }) | |
| } | |
| re := response.GetDetailResp{ | |
| TotleNumber: countMap[0] + countMap[1] + countMap[2], | |
| CurrentNumber: countMap[0], | |
| Apikey: project.Apikey, | |
| AuditRule: project.AudioRule, | |
| Members: users, | |
| } | |
| return re, nil | |
| } | |
| func (d *UserDAO) GetProjectDetails(ctx context.Context, id uint) (response.GetDetailResp, error) { | |
| var project model.Project | |
| err := d.DB.WithContext(ctx).Preload("Items").Preload("Users").First(&project, id).Error | |
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| return response.GetDetailResp{}, errors.New("project not found") | |
| } | |
| return response.GetDetailResp{}, err | |
| } | |
| countMap := map[int]int{ | |
| 0: 0, | |
| 1: 0, | |
| 2: 0, | |
| } | |
| for _, item := range project.Items { | |
| countMap[item.Status]++ | |
| } | |
| var users []model.UserResponse | |
| for _, user := range project.Users { | |
| users = append(users, model.UserResponse{ | |
| Name: user.Name, | |
| UserID: user.ID, | |
| Avatar: user.Avatar, | |
| }) | |
| } | |
| re := response.GetDetailResp{ | |
| TotalNumber: countMap[0] + countMap[1] + countMap[2], | |
| CurrentNumber: countMap[0], | |
| Apikey: project.Apikey, | |
| AuditRule: project.AudioRule, | |
| Members: users, | |
| } | |
| return re, nil | |
| } |
|
关于project的功能实现 |
| Data: userResponse, | ||
| }, nil | ||
| } | ||
| func (uc *UserController) UpdateUsers(ctx *gin.Context, req request.UpdateUserRoleReq) (response.Response, error) { |
There was a problem hiding this comment.
有中间件已经解决了这个userClaims的问题
There was a problem hiding this comment.
直接用ginx.WrapClaimsAndReq泛型函数应该就可以实现的
There was a problem hiding this comment.
你去看看ginx里面那几个函数的说明应该就能明白
|
类似这样的做法：func (h *GradeHandler) RegisterRoutes(s *gin.RouterGroup, authMiddleware gin.HandlerFunc) { } // GradeByTerm 查询按学年和学期的成绩 } |
Summary by CodeRabbit
New Features
Improvements