[ResourceApp][Backend][Feat] Implement Resource-Permission Endpoints#76
[ResourceApp][Backend][Feat] Implement Resource-Permission Endpoints#76saknarajapakshe wants to merge 4 commits intoLSFLK:mainfrom
Conversation
…chema(user_groups)
… operations and database schema
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's authorization capabilities by introducing a dedicated permission management system. It establishes a clear, DDD-aligned structure for handling resource permissions within groups, allowing for granular control over access. The changes include new API endpoints for creating, updating, deleting, and querying permissions, alongside robust validation and error handling to ensure data integrity and a consistent user experience. Furthermore, it expands existing group functionalities to manage user memberships effectively. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively implements the permission domain, adding DDD-style flows for creating, updating, deleting, and listing resource permissions. The separation into handlers, services, and repositories is well-executed. My review focuses on a few areas for improvement, primarily within the repository logic and database schema design. I've identified a potential design issue with the uniqueness of permissions that should be clarified, an incorrect handling of no-op updates, an opportunity to remove a redundant database query, and a couple of minor error message inconsistencies. Overall, this is a solid contribution that completes the permission domain.
| if existing.PermissionType == permissionType { | ||
| return nil, ErrPermissionConflict | ||
| } |
There was a problem hiding this comment.
Returning a conflict error when attempting to update a permission to the type it already has is not ideal. This action should be treated as a no-op (a successful operation that results in no change) rather than an error. API consumers may not always know the current state and shouldn't be penalized for it. A better approach is to return the existing permission object with a success status.
| if existing.PermissionType == permissionType { | |
| return nil, ErrPermissionConflict | |
| } | |
| if existing.PermissionType == permissionType { | |
| return &existing, nil | |
| } |
|
|
||
| INDEX `idx_resource_permissions_resource` (`resource_id`), | ||
| INDEX `idx_resource_permissions_group` (`group_id`), | ||
| UNIQUE KEY `uk_resource_group_permission` (`resource_id`, `group_id`, `permission_type`), |
There was a problem hiding this comment.
The unique key includes permission_type, which allows a group to have multiple permission types (e.g., both REQUEST and APPROVE) for the same resource. This might be unintentional and could lead to confusing behavior. Typically, a group has a single, highest permission level for a resource (e.g., APPROVE should imply REQUEST).
If the intent is to enforce one permission per group-resource pair, the unique key should be (resource_id, group_id). This would also align with the current ErrPermissionConflict message ("permission already exists for this group and resource").
Please clarify if the current design is intended. If so, consider updating the error message and documenting this behavior. If not, the unique constraint should be changed.
| case errors.Is(err, ErrGroupNotFound), errors.Is(err, ErrUserNotFound): | ||
| c.JSON(http.StatusNotFound, gin.H{"error": err.Error()}) | ||
| default: | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to add users to group: "}) |
There was a problem hiding this comment.
The error message string has a trailing space. This is likely a typo and can cause issues for API clients that parse error messages.
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to add users to group: "}) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to add users to group"}) |
|
|
||
| var ( | ||
| ErrPermissionNotFound = errors.New("permission not found") | ||
| ErrPermissionConflict = errors.New("permission already exists for this group and resource") |
There was a problem hiding this comment.
This error message is misleading given the database schema. The unique constraint is on (resource_id, group_id, permission_type), meaning a conflict occurs only when the same permission type is duplicated for a group and resource. The message should be more specific to reflect this.
| ErrPermissionConflict = errors.New("permission already exists for this group and resource") | |
| ErrPermissionConflict = errors.New("permission type already exists for this group and resource") |
| var updated ResourcePermission | ||
| if err := r.db.First(&updated, "id = ?", id).Error; err != nil { | ||
| if err == gorm.ErrRecordNotFound { | ||
| return nil, ErrPermissionNotFound | ||
| } | ||
| return nil, err | ||
| } | ||
|
|
||
| return &updated, nil |
There was a problem hiding this comment.
This second database query to fetch the updated permission is redundant and inefficient. You have already fetched the permission into the existing variable at the beginning of the function. After a successful database update, you can simply update the PermissionType field on the existing object in memory and return it. This avoids an unnecessary database roundtrip.
existing.PermissionType = permissionType
return &existing, nil
saknarajapakshe
changed the title
1 participant
Overview
This PR completes the Permission domain implementation for the Resource bounded context. It adds DDD-style flows for permission endpoints and enforces consistent domain rules (unique group-resource-permission, permission type validation) with repository/service separation.
/api/resource-permissions/api/resource-permissions/:id/api/resource-permissions/:id/api/groups/:id/permissionsWhat added
cmd/server/main.go: route registration for permission endpointsinternal/permission/handlers.go: handlers + input validation + status code mappinginternal/permission/service.go: permission business rules (create/update/delete/list)internal/permission/repository.go: repository methods with DB checks and conflict handlinginternal/permission/models.go: domain model and request/response modelsinternal/permission/constants.go: permission type constants and domain errors (REQUEST,APPROVE,ErrPermissionConflict, etc.)internal/db/db.go: auto-migratepermission.ResourcePermissionincludedHow to test locally
resource_app)go test ./...5. test endpoints:
GET /api/groups/:groupId/permissionsPOST /api/resource-permissionsPATCH /api/resource-permissions/:idDELETE /api/resource-permissions/:idBehavior expectations
409 Conflict("permission already exists for this group and resource")400 Bad Request404 Not Found201/200with expected payloadrelated to #75