feat: add group permission storage models#1956
Conversation
|
Claude finished @AllureCurtain's task in 2m 32s —— View job Code Review: feat: add group permission storage models
Overall the PR is clean and follows the established SeaORM patterns in the codebase. The structure is sound. Below are my findings organized by severity. Summary
Correctness1. List query runs outside transaction in
txn.commit().await?;
self.list_resource_permissions(resource_type, resource_id).await // outside txnThe final Consider either accepting that the return is a best-effort snapshot, documenting it clearly, or collecting the written records in-memory and returning them directly. Fix this → Design Concerns2.
Both If relying on cascade, the 3. No validation on input strings —
if payload.name.trim().is_empty() {
return Err(MegaError::with_message("Group name cannot be empty", 400));
}4.
This is fine for now, but adding new variants to a PostgreSQL 5. No
Performance6.
let group_ids = self.find_group_ids_by_username(username).await?; // query 1
// ...
mega_group::Entity::find()
.filter(mega_group::Column::Id.is_in(group_ids)) // query 2
.all(self.get_connection()).await?This could be a single query using a subquery or JOIN. With large numbers of groups, the 7.
The Missing Test Coverage8. No tests for The PR only adds 9. No semantic relations defined between entities All three entity Minor / Style10.
.filter_map(|username| {
if seen.insert(username.to_string()) { // allocation 1
Some(username.to_string()) // allocation 2
} else {
None
}
})Both .filter_map(|username| {
let s = username.to_string();
seen.insert(s.clone()).then_some(s)
})11. Migration module doc comment is not updated
12.
What's Done Well
|
|
@codex Review |
There was a problem hiding this comment.
Pull request overview
This PR implements the foundational database schema and storage layer for a group-based permission system, addressing issue #1935. The changes introduce three new database tables (mega_group, mega_group_member, mega_resource_permission) with their corresponding SeaORM entity models and a comprehensive storage layer for managing groups, group memberships, and resource permissions.
Changes:
- Added database migration creating three tables for groups, group members, and resource permissions with proper foreign key constraints and indexes
- Created SeaORM entity models and enum types for
PermissionEnum(read/write/admin) andResourceTypeEnum(note) - Implemented
GroupStoragewith methods for CRUD operations on groups, group membership management, and resource permission binding
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| jupiter/src/migration/m20260216_013852_create_group_permission_tables.rs | Migration creating mega_group, mega_group_member, and mega_resource_permission tables with indexes and foreign keys |
| jupiter/callisto/src/mega_group.rs | SeaORM entity model for groups table |
| jupiter/callisto/src/mega_group_member.rs | SeaORM entity model for group membership table |
| jupiter/callisto/src/mega_resource_permission.rs | SeaORM entity model for resource permissions table |
| jupiter/callisto/src/sea_orm_active_enums.rs | Added PermissionEnum and ResourceTypeEnum enums |
| jupiter/callisto/src/prelude.rs | Exported new entity types in prelude |
| jupiter/callisto/src/mod.rs | Added module declarations for new entities |
| jupiter/src/storage/group_storage.rs | Storage layer implementation with group and permission management operations |
| jupiter/src/model/group_dto.rs | DTO definitions for group operations |
| jupiter/src/model/mod.rs | Added group_dto module declaration |
| jupiter/src/storage/mod.rs | Integrated GroupStorage into Storage and AppService structs |
| jupiter/src/migration/mod.rs | Registered new migration in migrator |
| jupiter/src/tests.rs | Added GroupStorage initialization in test helper |
| .await?) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The new GroupStorage functionality lacks test coverage. Other storage implementations in the codebase (e.g., build_trigger_storage.rs, dynamic_sidebar_storage.rs) include comprehensive unit tests for their core operations. Consider adding tests for critical operations like create_group, add_group_members (with duplicates), replace_resource_permissions, and the normalization functions.
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| #[test] | |
| fn normalize_usernames_trims_and_deduplicates() { | |
| let input = vec![ | |
| " alice ".to_string(), | |
| "".to_string(), | |
| "bob".to_string(), | |
| "alice".to_string(), | |
| " ".to_string(), | |
| "ALICE".to_string(), | |
| ]; | |
| let result = normalize_usernames(&input); | |
| // Should trim whitespace and drop empty / whitespace-only entries. | |
| // Deduplication is exact (case-sensitive) and preserves the first occurrence order. | |
| assert_eq!( | |
| result, | |
| vec![ | |
| "alice".to_string(), | |
| "bob".to_string(), | |
| "ALICE".to_string(), | |
| ] | |
| ); | |
| } | |
| #[test] | |
| fn normalize_usernames_handles_empty_input() { | |
| let input: Vec<String> = Vec::new(); | |
| let result = normalize_usernames(&input); | |
| assert!(result.is_empty()); | |
| } | |
| } |
| manager | ||
| .create_index( | ||
| Index::create() | ||
| .if_not_exists() | ||
| .name("idx_resource_permission_group_id") | ||
| .table(MegaResourcePermission::Table) | ||
| .col(MegaResourcePermission::GroupId) | ||
| .to_owned(), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Consider adding a composite index on (resource_type, resource_id) for mega_resource_permission table. The list_resource_permissions method at lines 216-227 queries by these two columns, and while a full table scan or the unique index idx_resource_permission_unique can be used, a dedicated composite index would optimize this common query pattern more efficiently.
| storage::base_storage::{BaseStorage, StorageConnector}, | ||
| }; | ||
|
|
||
| #[derive(Clone)] |
There was a problem hiding this comment.
The GroupStorage struct is missing the Debug derive that other storage structs have (e.g., UserStorage at user_storage.rs:12). This is inconsistent with the codebase convention and could make debugging more difficult.
| #[derive(Clone)] | |
| #[derive(Clone, Debug)] |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Signed-off-by: allure <1550220889@qq.com>
|
Claude finished @AllureCurtain's task in 2m 9s —— View job Code Review: feat: add group permission storage models
Overall the PR is well-structured and follows established SeaORM patterns in the codebase. I noticed some items have already been addressed compared to the previous review (e.g., Correctness1.
match mega_resource_permission::Entity::insert_many(models)
.on_conflict(on_conflict)
.exec(self.get_connection()) // no transaction
.await { ... }
self.list_resource_permissions(resource_type, resource_id).await // second connection, no transactionUnlike Design Concerns2. No validation on group name in
An empty if payload.name.trim().is_empty() {
return Err(MegaError::with_message("Group name cannot be empty", 400));
}3. Empty SeaORM
The FK constraints exist at the DB level but SeaORM relation traversal ( 4.
The explicit pre-deletion of members and permissions is redundant for PostgreSQL/MySQL given the 5. No The storage layer has create, read, and delete for groups, but no method to update Performance6.
let group_ids = self.find_group_ids_by_username(username).await?; // query 1
mega_group::Entity::find()
.filter(mega_group::Column::Id.is_in(group_ids)) // query 2This could be a single query using a JOIN or subquery. With large group counts, the 7. Unnecessary
.filter(mega_resource_permission::Column::GroupId.is_in(group_ids.to_vec()))
8. Missing
Missing Test Coverage9. No tests for The PR only registers Minor / Style10. Migration module doc comment not updated
11.
What's Done Well
|
link #1935