refactor: rm user table && fix tag pointing to wrong tree hash #1594
Conversation
Signed-off-by: Ruizhi Huang <[email protected]> cargo fmt Signed-off-by: Ruizhi Huang <[email protected]>
231220075
force-pushed
the
refactor/user_data
branch
from
f8a3dee to
c61faa1
Compare
genedna
requested review from
benjamin-747 and
Copilot
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the user management architecture by removing the local user table and simplifying commit binding to use only usernames. The key changes include removing email-based user lookups, eliminating the UserAuthExtractor trait, and fixing tag creation to point to the correct tree hash instead of ZERO_ID. Additionally, it improves the get-latest-commit logic for single files by implementing a proper commit history traversal algorithm.
Reviewed Changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
mono/src/api/router/preview_router.rs |
Simplified commit binding to use username-only model with anonymous fallback |
mono/src/api/router/commit_router.rs |
Removed user validation and simplified binding update to username-only |
mono/src/api/oauth/model.rs |
Removed dependency on user::Model and updated conversions to use LoginUser |
mono/src/api/oauth/mod.rs |
Removed user persistence, directly converting GitHub user to session |
jupiter/src/storage/user_storage.rs |
Removed user lookup methods (find by email/name, save user) |
jupiter/src/storage/mono_storage.rs |
Simplified commit binding to accept authenticated username directly |
jupiter/src/storage/commit_binding_storage.rs |
Removed author_email parameter from upsert_binding |
jupiter/src/service/blame_service.rs |
Updated to username-only model, removed email extraction logic |
jupiter/src/model/blame_dto.rs |
Removed user verification fields from CommitBindingInfo |
jupiter/src/migration/mod.rs |
Added migration to drop user table |
jupiter/src/migration/m20251026_065433_drop_user_table.rs |
New migration that drops user table and author_email column |
jupiter/callisto/src/*.rs |
Updated SeaORM codegen version to 1.1.17 |
jupiter/callisto/src/user.rs |
Removed entire user entity model |
jupiter/callisto/src/commit_auths.rs |
Removed author_email field from model |
ceres/src/protocol/smart.rs |
Simplified commit binding to username-only, removed email extraction |
ceres/src/protocol/mod.rs |
Moved PushUserInfo to local definition with username only |
ceres/src/model/git.rs |
Removed user verification fields from CommitBindingInfo |
ceres/src/model/commit.rs |
Simplified response to contain only username |
ceres/src/lib.rs |
Removed auth module |
ceres/src/auth/mod.rs |
Deleted entire auth module |
ceres/src/api_service/mono_api_service.rs |
Fixed tag creation to resolve tree hash from commit; improved get-latest-commit filtering |
ceres/src/api_service/mod.rs |
Implemented precise commit history traversal for file paths |
| let mut steps: u32 = 0; | ||
| const MAX_STEPS: u32 = 10_000; |
There was a problem hiding this comment.
The constant MAX_STEPS is defined inside the loop function. Consider moving it to a module-level constant or as an associated constant to improve maintainability and avoid re-defining it on each call.
| let final_username = json.author_username.as_deref().and_then(|u| { | ||
| let t = u.trim(); | ||
| if t.is_empty() || t.eq_ignore_ascii_case("anonymous") { | ||
| None | ||
| } else { | ||
| Some(t.to_string()) | ||
| } | ||
| }); |
There was a problem hiding this comment.
This username normalization logic is duplicated in both create_entry and save_edit functions. Consider extracting it into a helper function to reduce code duplication and improve maintainability.
Signed-off-by: Ruizhi Huang <[email protected]>
| async fn down(&self, manager: &SchemaManager) -> Result<(), DbErr> { | ||
| // No-op: we don't recreate the legacy users table | ||
| let _ = manager; // silence unused warning in some compilers |
There was a problem hiding this comment.
Rather than silencing the unused parameter warning with let _ = manager;, mark the parameter as unused with an underscore prefix _manager in the function signature. This is more idiomatic Rust.
| async fn down(&self, manager: &SchemaManager) -> Result<(), DbErr> { | |
| // No-op: we don't recreate the legacy users table | |
| let _ = manager; // silence unused warning in some compilers | |
| async fn down(&self, _manager: &SchemaManager) -> Result<(), DbErr> { | |
| // No-op: we don't recreate the legacy users table |
|
|
||
| let mut curr_commit = head_commit.clone(); | ||
| // Safety guard to avoid pathological loops on very deep histories | ||
| let mut steps: u32 = 0; |
There was a problem hiding this comment.
The magic number 10,000 for MAX_STEPS should be documented. Consider adding a comment explaining why this limit was chosen and what scenarios it protects against (e.g., repository size, expected history depth).
| let mut steps: u32 = 0; | |
| let mut steps: u32 = 0; | |
| // Limit the number of commit history traversal steps to prevent excessive resource usage | |
| // in pathological cases (e.g., extremely deep or cyclic histories in very large monorepos). | |
| // The value 10,000 was chosen as a conservative upper bound, which should be far greater | |
| // than the expected depth of commit history for any normal repository, even at Mega scale. | |
| // If this limit is reached, the function falls back to returning the HEAD commit to avoid | |
| // timeouts or runaway resource consumption. Adjust this value if repository characteristics change. |
| let Some(mut curr_blob) = current_blob else { | ||
| return Ok(None); | ||
| }; | ||
|
|
||
| let mut curr_commit = head_commit.clone(); |
There was a problem hiding this comment.
[nitpick] Using let-else with mut for curr_blob is correct, but consider whether mutability is needed from the start. The variable is reassigned in the loop body (line 344), so the mut is justified, but worth reviewing if the algorithm could be restructured to reduce mutability.
| let Some(mut curr_blob) = current_blob else { | |
| return Ok(None); | |
| }; | |
| let mut curr_commit = head_commit.clone(); | |
| let Some(curr_blob) = current_blob else { | |
| return Ok(None); | |
| }; | |
| let mut curr_commit = head_commit.clone(); | |
| let mut curr_blob = curr_blob; |
| if let Ok(Some(binding)) = self.build_commit_binding_info(&commit.id.to_string()).await | ||
| && !binding.is_anonymous | ||
| && binding.matched_username.is_some() | ||
| { | ||
| let username = binding.matched_username.unwrap(); |
There was a problem hiding this comment.
The pattern of checking binding.matched_username.is_some() followed by unwrap() appears twice (lines 115-120 and 146-153). Consider using if let Some(username) = binding.matched_username pattern instead to make the code more idiomatic and avoid the redundant check.
7827e69
|
link(#1562 ) |
2 participants
rm user table;
fix tag pointing to wrong tree hash;
adjust get latest commit on single file；
fix get latest commit API bug with specific file paths.