Chore: Use UUID instead of using String #7699
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request migrates the codebase to use UUIDs instead of strings for identifying key entities like workspaces, views, and chats. This change improves data integrity and consistency across the application. Updated class diagram for FolderManagerclassDiagram
class FolderManager {
-user: FolderUser
-collab_builder: CollabBuilder
-cloud_service: dyn FolderCloudService
-operation_handlers: HashMap<ViewLayout, FolderOperationHandler>
-mutex_folder: Arc<Mutex<Folder>>
-folder_indexer: FolderIndexer
-store_preferences: KVStorePreferences
}
class FolderUser {
<<interface>>
+user_id() i64
+workspace_id() Uuid
+collab_db(uid: i64) Weak<CollabKVDB>
+is_folder_exist_on_disk(uid: i64, workspace_id: Uuid) FlowyResult<bool>
}
FolderManager -- FolderUser : depends on
note for FolderUser "workspace_id() now returns Uuid instead of String"
note for FolderUser "is_folder_exist_on_disk() now takes Uuid instead of String"
Updated class diagram for DatabaseManagerclassDiagram
class DatabaseManager {
-user: DatabaseUser
-cloud_service: dyn DatabaseCloudService
-ai_service: dyn DatabaseAIService
-workspace_database: Arc<RwLock<WorkspaceDatabaseCollabServiceImpl>>
-database_editors: DatabaseEditorMap
-store_preferences: KVStorePreferences
}
class DatabaseUser {
<<interface>>
+user_id() i64
+collab_db(uid: i64) Weak<CollabKVDB>
+workspace_id() Uuid
+workspace_database_object_id() Uuid
}
DatabaseManager -- DatabaseUser : depends on
note for DatabaseUser "workspace_id() now returns Uuid instead of String"
note for DatabaseUser "workspace_database_object_id() now returns Uuid instead of String"
Updated class diagram for AIUserServiceclassDiagram
class AIUserService {
<<interface>>
+user_id() i64
+device_id() String
+workspace_id() Uuid
+sqlite_connection(uid: i64) DBConnection
+application_root_dir() PathBuf
}
note for AIUserService "workspace_id() now returns Uuid instead of String"
Updated class diagram for AIExternalServiceclassDiagram
class AIExternalService {
<<interface>>
+query_chat_rag_ids(parent_view_id: Uuid, chat_id: Uuid) Result<Vec<Uuid>, FlowyError>
+sync_rag_documents(workspace_id: Uuid, rag_ids: Vec<Uuid>, rag_metadata_map: HashMap<Uuid, AFCollabMetadata>) Result<Vec<AFCollabMetadata>, FlowyError>
+notify_did_send_message(chat_id: Uuid, message: String) Result<(), FlowyError>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @appflowy - I've reviewed your changes - here's some feedback:
Overall Comments:
- The change from String to UUID is good, but you should also validate the UUIDs when they come from external sources, like deserialization or user input.
- Consider adding a migration strategy for existing data that uses strings for IDs.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 3 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| .load::<AFCollabMetadata>(&mut conn)? | ||
| .into_iter() | ||
| .map(|m| (m.object_id.clone(), m)) | ||
| .flat_map(|m| Uuid::from_str(&m.object_id).map(|v| (v, m))) |
There was a problem hiding this comment.
suggestion (bug_risk): Consider explicit handling of Uuid conversion failures.
Using flat_map with .ok() silently drops conversion failures. If a failure indicates data inconsistency, logging or handling the error explicitly might help with debugging.
Suggested implementation:
.into_iter()
.map(|m| {
Uuid::from_str(&m.object_id)
.map(|v| (v, m))
.map_err(|e| {
log::error!("Failed to convert object_id '{}' to Uuid: {}", m.object_id, e);
e
})
})
.collect::<Result<_, _>>()?;Ensure that the log crate is imported and initialized in your project if it is not already. You may also need to adjust the error handling to integrate with your application's error type.
| @@ -44,16 +44,18 @@ use flowy_sqlite::kv::KVStorePreferences; | |||
| use futures::future; | |||
| use std::collections::HashMap; | |||
| use std::fmt::{Display, Formatter}; | |||
There was a problem hiding this comment.
issue (complexity): Consider updating APIs to use Uuid directly for workspace IDs instead of converting to and from String to reduce conversion overhead and simplify code flow, especially in methods like get_workspace_info and related callsites, including notifications, index removals, and view movements, to improve code clarity and efficiency..
Consider updating the APIs for workspace IDs so they use `Uuid` directly, rather than relying on converting to and from `String`. For example, instead of writing:
folder.get_workspace_info(&workspace_id.to_string())
update both the API and its callers so that the signature becomes:
fn get_workspace_info(&self, workspace_id: &Uuid) -> Option<Workspace>
Then adjust call sites accordingly:
folder.get_workspace_info(workspace_id)
This removes repeated `.to_string()` and `Uuid::from_str(...)` invocations. Audit similar cases (e.g., in notifications, index removals, view movements) and update them to work directly with `Uuid`. This will reduce conversion clutter and simplify the control flow.| @@ -24,15 +24,17 @@ use lib_infra::box_any::BoxAny; | |||
| use lib_infra::isolate_stream::{IsolateSink, SinkExt}; | |||
| use lib_infra::util::timestamp; | |||
| use std::path::{Path, PathBuf}; | |||
| use std::str::FromStr; | |||
| use std::sync::atomic::AtomicBool; | |||
| use std::sync::Arc; | |||
There was a problem hiding this comment.
issue (complexity): Consider updating lower-layer functions to accept Uuid directly for workspace_id instead of converting it to a string, to improve consistency and reduce conversion overhead throughout the codebase.
It looks like the change now brings workspace_id into domain type Uuid, but then many higher‐layer callers continue to convert it to and from strings. Instead of sprinkling conversions, consider updating the lower layer (e.g. database and cloud APIs) to work with Uuid directly. This minimizes conversion overhead and makes the intent clearer.
For example, if you have a function like:
fn delete_upload_file_by_file_id(conn: DBConnection, workspace_id: &String, parent_dir: &str, file_id: &str) -> Result<Option<File>, FlowyError> { ... }change its signature to:
fn delete_upload_file_by_file_id(conn: DBConnection, workspace_id: &Uuid, parent_dir: &str, file_id: &str) -> Result<Option<File>, FlowyError> { ... }Then in the caller, you won’t need to call
&workspace_id.to_string()—you can pass &workspace_id directly. Similarly update all functions that expect a string workspace id. This makes the API consistent and reduces cognitive load.
| @@ -46,9 +48,10 @@ pub(crate) fn subscribe_folder_view_changed( | |||
| ChildViewChangeReason::Create, | |||
| ); | |||
| let folder = lock.read().await; | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring the domain to use Uuid for the parent_view_id field to avoid repeated conversions and error handling in the observer logic, reducing nesting and complexity while keeping all functionality intact.
Consider refactoring the domain to use Uuid for the parent_view_id field instead of a String. This would eliminate the repeated inline calls to Uuid::from_str and the associated error handling. For example, update the view struct so the conversion happens once, at creation time:
// Before
pub struct View {
pub parent_view_id: String,
// ...
}
// After
use uuid::Uuid;
pub struct View {
pub parent_view_id: Uuid,
// ...
}Then ensure that when you create or deserialize a View, the conversion occurs right there:
let view = View {
parent_view_id: Uuid::parse_str(&raw_parent_view_id).expect("Invalid UUID format"),
// ...
};With the field stored as a Uuid, you can remove the additional error handling in your observer logic, thus reducing nesting and complexity while keeping all functionality intact.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Feature Preview
PR Checklist
Summary by Sourcery
Migrate workspace and view identifiers from string to UUID across the AppFlowy codebase
Enhancements:
Chores: