🚧 API module Track #15
Conversation
|
Caution Review failedThe pull request is closed. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 buf (1.47.2)proto/git-core.protoConfig file YAML parsing error: yaml: unmarshal errors: WalkthroughThis pull request introduces a comprehensive set of enhancements across multiple Rust packages within the GitData project. The changes span database migrations, email configuration, repository management, authentication, and gRPC services. Key additions include SeaORM migration support, session management with Redis, email templating, new API endpoints for user and repository operations, and expanded gRPC interfaces for repository interactions. The modifications aim to improve system flexibility, security, and functionality across different components of the GitData ecosystem. Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIRouter
participant SessionMiddleware
participant AuthService
participant DatabaseService
Client->>APIRouter: Send Request
APIRouter->>SessionMiddleware: Validate/Create Session
SessionMiddleware->>AuthService: Authenticate User
AuthService->>DatabaseService: Verify Credentials
DatabaseService-->>AuthService: Authentication Result
AuthService-->>SessionMiddleware: Session Status
SessionMiddleware-->>APIRouter: Process Request
APIRouter-->>Client: Send Response
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
🪧 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 (
|
AI-generated insightsSummaryUpdated several dependencies in Cargo.lock, notably upgrading 'strsim' to version 0.11.1 and adding 'clap', 'darling', 'sea-orm-migration', and several related packages. ImprovementsConsider organizing dependencies in Cargo.toml to reduce clutter and improve readability, possibly grouping related dependencies together. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
🔭 Outside diff range comments (1)
gitdata-apis/Cargo.toml (1)
Line range hint
4-4: Fix invalid Rust edition.The specified edition "2024" is not valid. Valid Rust editions are "2015", "2018", and "2021".
-edition = "2024" +edition = "2021"
🧹 Nitpick comments (18)
gitdata-apis/src/service/users/apply.rs (2)
Line range hint
65-82: Simplify UUID generation logic.The current UUID generation loop can be simplified using a more functional approach.
Consider this alternative implementation:
use rand::Rng; use std::time::Duration; async fn generate_unique_uuid(&self) -> anyhow::Result<Uuid> { let mut attempts = 0; const MAX_ATTEMPTS: u32 = 10; while attempts < MAX_ATTEMPTS { let uid = Uuid::new_v4(); if !self.users_check_already_exists_uid(uid).await? { return Ok(uid); } attempts += 1; tokio::time::sleep(Duration::from_millis(50)).await; } Err(anyhow::anyhow!("Failed to generate unique UUID after {} attempts", MAX_ATTEMPTS)) } // Then replace the loop with: model.uid = Set(self.generate_unique_uuid().await?);
Line range hint
39-61: Improve error handling specificity.The error handling for username and email existence checks could be more specific to help with API responses.
Consider creating custom error types:
#[derive(thiserror::Error, Debug)] pub enum UserError { #[error("Username '{0}' already exists")] UsernameExists(String), #[error("Email '{0}' already exists")] EmailExists(String), #[error("Database error: {0}")] DatabaseError(#[from] sea_orm::DbErr), } // Then update the error handling: if self.users_check_already_exists_username(¶m.username).await? { return Err(UserError::UsernameExists(param.username).into()); } if self.users_check_already_exists_email(¶m.email).await? { return Err(UserError::EmailExists(param.email).into()); }gitdata-apis/src/jobs/email.rs (1)
51-52: Refactor Suggestion: Use a Templating Engine for Email ContentReplacing placeholders in the email body using
.replace("123456", &tx.code)is brittle and could lead to unintended replacements if "123456" appears elsewhere. Consider using a templating engine likeTeraorHandlebarsfor safer and more maintainable email templates.Example using
Tera:// Define your template with a placeholder let template = "Your verification code is {{ code }}"; // Render the template let mut context = tera::Context::new(); context.insert("code", &tx.code); let body = tera.render_str(template, &context)?;gitdata-apis/src/apis/app_router.rs (1)
55-59: Refactor Suggestion: Organize Routes for Improved ReadabilityThe routes under the
/userscope can be organized for better readability and maintenance by grouping related routes together and ensuring consistent formatting.Example:
.scope("/user") .service( scope("/setting") // Setting routes... ) .route("/apply", post().to(...)) .service( scope("/info") // Info routes... ) .service( scope("/ssh") // SSH key routes... ) .service( scope("/token") // Token routes... )libs/model/migrate/users.rs (3)
127-154: Review Nullability and Data Types inUsersTable Columns.Some columns in the
Userstable might benefit from adjusted nullability and data types:
- Consistency in Timestamps: Use a consistent data type for timestamp fields (
CreatedAt,UpdatedAt,LastUsed, etc.). Consider usingdate_time()instead ofbig_integer()for clarity.- String Lengths: Define maximum lengths for string fields to prevent excessively long inputs.
- Arrays: Ensure that the array types (
Social,Topic,Pinned,Member,Team) are supported and correctly mapped in your database.Example diff for timestamp fields:
.col(ColumnDef::new(Users::CreatedAt).big_integer().not_null()) .col(ColumnDef::new(Users::UpdatedAt).big_integer().not_null()) .col(ColumnDef::new(Users::LastUsed).big_integer().null()) +// Change to date_time() +.col(ColumnDef::new(Users::CreatedAt).date_time().not_null()) +.col(ColumnDef::new(Users::UpdatedAt).date_time().not_null()) +.col(ColumnDef::new(Users::LastUsed).date_time().null())
179-209: Ensure Tables are Dropped in Reverse Order of Creation for Foreign Key Dependencies.When dropping tables in the
downmethod, ensure that tables with foreign key constraints are dropped before the tables they reference to avoid dependency errors.Adjust the drop order accordingly.
127-154: Consider Indexing Frequently Queried Columns inUsersTable.Adding indexes on columns like
Username,Apply this diff to add indexes:
.col(ColumnDef::new(Users::Username).string().not_null()) +.index( + Index::create() + .name("idx-users-username") + .table(Users::Table) + .col(Users::Username) +)libs/rpc/core_git.rs (2)
136-159: Consider Handling Potential Errors increateMethod ofRepRepositoryClient.The
createmethod should handle potential errors more gracefully, possibly providing more context or retry mechanisms.Ensure that error handling aligns with your application's error management strategy.
196-240: Server Compression Settings Might Need Review.Review the compression settings for both accepting and sending compressed data to ensure they align with client capabilities and network efficiency.
Consider if default settings are sufficient or if adjustments are needed for your use case.
libs/rpc/git_core.rs (2)
252-274: Handle Client Readiness and Connection Errors inpathMethod.Ensure that the
pathmethod inRepRepositoryClientproperly handles situations where the service is not ready or the connection fails, possibly adding retry logic or more informative error messages.Review the error handling mechanism to improve robustness.
444-651: Optimize Match Statements in Server Implementation for Maintainability.The match statements used in the
callfunction can become unwieldy as more methods are added. Consider refactoring to improve readability and maintainability.You might use a macro or a routing table to handle method dispatching more elegantly.
libs/model/mod.rs (1)
20-20: Add documentation for the migrate moduleWhile adding the module is correct, consider adding documentation to explain its purpose and relationship to other modules.
Consider this improvement:
+/// Module containing database migration infrastructure and specific migrations +/// for setting up and maintaining the database schema. pub mod migrate;Also consider organizing related modules into submodules, for example:
- repository/
- issues/
- pr/
- labels/
- etc.
libs/model/users/token_key.rs (1)
Line range hint
24-24: Enhance access field documentation and validation.The access field comment could be more detailed. Consider:
- Defining constants for the access bits
- Adding validation in
new_tokenfor the access parameter- Documenting the behavior when invalid bits are set
Example improvement:
pub const TOKEN_ACCESS_NONE: i32 = 0; pub const TOKEN_ACCESS_READ: i32 = 1; pub const TOKEN_ACCESS_WRITE: i32 = 2; pub const TOKEN_ACCESS_OWNER: i32 = 4; pub const TOKEN_ACCESS_ALL: i32 = TOKEN_ACCESS_READ | TOKEN_ACCESS_WRITE | TOKEN_ACCESS_OWNER;gitdata-apis/src/service/emails/captcha.rs (1)
43-43: Consider internationalizing the email subject.The hardcoded Chinese subject "GitData 验证码" might not be suitable for international users. Consider using a localization system to support multiple languages.
libs/model/repository/repository.rs (1)
75-75: Consider handling default branch more explicitlyInstead of using an empty string as the default branch value, consider using a meaningful default like "main" or "master", or handle the None case more explicitly.
- default_branch: Set(default_branch.unwrap_or("".to_string())), + default_branch: Set(default_branch.unwrap_or("main".to_string())),gitdata-apis/src/service/mod.rs (1)
106-109: Improve database name comparisonThe current database name comparison is case-sensitive and could be more robust:
- Use case-insensitive comparison
- Consider using an enum for database types
- Add logging for migration operations
- if url.dbname.to_lowercase() != "jobs".to_string() { + if !url.dbname.eq_ignore_ascii_case("jobs") { + info!("Running migrations for database: {}", url.dbname); DatabaseMigrate::install(&db).await?; DatabaseMigrate::up(&db, None).await?; }Cargo.toml (1)
36-36: Specify more precise version constraintsUsing version "1" is too permissive as it allows any 1.x version. Consider:
- Using a more specific version range
- Ensuring compatibility with sea-orm version
-sea-orm-migration = { version = "1", features = ["runtime-tokio","sqlx-postgres", "cli"] } +sea-orm-migration = { version = "~1.0", features = ["runtime-tokio","sqlx-postgres", "cli"] }gitdata-apis/Cargo.toml (1)
Line range hint
1-50: Standardize version constraint style.The dependencies use inconsistent version constraint styles:
- Some use bare versions:
"1"- Some use tilde constraints:
"~0.16.0"- Some use exact versions:
"0.8.0"Consider standardizing to one approach, preferably using tilde constraints for minor version updates while preventing major version bumps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
Cargo.toml(1 hunks)gitdata-apis/Cargo.toml(1 hunks)gitdata-apis/src/apis/app_router.rs(2 hunks)gitdata-apis/src/apis/v1/auth/passwd.rs(2 hunks)gitdata-apis/src/apis/v1/user/ssh_keys.rs(1 hunks)gitdata-apis/src/jobs/email.rs(4 hunks)gitdata-apis/src/main.rs(2 hunks)gitdata-apis/src/public/captcha.html(1 hunks)gitdata-apis/src/public/mod.rs(1 hunks)gitdata-apis/src/public/users_forgetpasswd.html(1 hunks)gitdata-apis/src/service/auth/passwd.rs(1 hunks)gitdata-apis/src/service/emails/captcha.rs(1 hunks)gitdata-apis/src/service/mod.rs(3 hunks)gitdata-apis/src/service/repository/new_repo.rs(1 hunks)gitdata-apis/src/service/users/apply.rs(2 hunks)gitdata-health/Cargo.toml(1 hunks)gitdata-health/src/main.rs(1 hunks)gitdata-proxy/Cargo.toml(1 hunks)gitdata-proxy/src/main.rs(1 hunks)libs/config/email.rs(1 hunks)libs/config/mod.rs(1 hunks)libs/model/migrate/mod.rs(1 hunks)libs/model/migrate/users.rs(1 hunks)libs/model/mod.rs(1 hunks)libs/model/repository/repository.rs(1 hunks)libs/model/users/emails.rs(1 hunks)libs/model/users/follow.rs(1 hunks)libs/model/users/token_key.rs(1 hunks)libs/model/users/users.rs(1 hunks)libs/model/users/watch.rs(1 hunks)libs/rpc/core_git.rs(1 hunks)libs/rpc/git_core.rs(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- gitdata-health/src/main.rs
- gitdata-health/Cargo.toml
- libs/model/users/follow.rs
- gitdata-apis/src/public/captcha.html
- gitdata-apis/src/service/auth/passwd.rs
- gitdata-proxy/Cargo.toml
- gitdata-apis/src/public/users_forgetpasswd.html
🔇 Additional comments (12)
gitdata-apis/src/main.rs (1)
42-44: Verify Session TTL Extension PolicyUsing
TtlExtensionPolicy::OnEveryRequestextends the session TTL on each request, potentially keeping sessions alive indefinitely if the user is active. Verify that this policy aligns with your security requirements and consider whether a fixed session expiration might be more appropriate.gitdata-apis/src/apis/app_router.rs (2)
5-5: Verify Removal ofasyncKeyword fromAppRouterFunctionThe
AppRouterfunction has been changed frompub async fntopub fn. Ensure that none of the configurations within require the function to be asynchronous. If there are anyawaitpoints or if the function needs to perform asynchronous operations, theasynckeyword should be retained.
55-59: Consistency Issue: Route Path FormattingIn the scope definition for
"token", there is no leading slash, unlike other scopes like"/ssh"and"/info". While Actix allows both formats, it's important to maintain consistency to avoid confusion and potential routing issues.Consider adding a leading slash for consistency:
- scope("token") + scope("/token")libs/rpc/core_git.rs (2)
174-182: Implement All Required Methods inRepRepositoryTrait.Ensure that the
RepRepositorytrait includes all necessary methods that the server must implement according to your service requirements.Confirm that no required methods are missing by checking the service definitions.
31-34: Verify Enum String Representations Match Protobuf Definitions.Ensure that the
as_str_namemethod returns the correct string representations as defined in your Protobuf files. Verify that "LOCAL", "S3", and "NFS" are the exact names used.Please run the following script to verify the enum string names:
✅ Verification successful
String Representations Match Protobuf Definitions
The
as_str_namemethod correctly returns the exact string representations as defined in the protobuf files (proto/core-git.protoandproto/git-core.proto). The strings "S3" and "NFS" match the protobuf enum values perfectly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify enum string representations match Protobuf definitions. # Assuming the Protobuf file is available, extract enum names. grep -A 5 'enum RepositoryStoragePositionType' path/to/protobuf/file.protoLength of output: 137
Script:
#!/bin/bash # Find proto files and search for storage-related enums echo "=== Finding .proto files ===" fd -e proto echo -e "\n=== Searching for storage-related enum definitions ===" rg -i "enum.*storage.*type" -A 5 echo -e "\n=== Searching specifically for LOCAL, S3, NFS strings in .proto files ===" rg "(LOCAL|S3|NFS)" -A 2 -B 2 "\.proto$"Length of output: 2217
libs/rpc/git_core.rs (1)
145-148: Ensure Consistency in Enum String Representations.Verify that the
as_str_namemethod inRepositoryStoragePositionTypereturns the correct strings as per the Protobuf definitions.Please confirm that these strings match the Protobuf enum names.
gitdata-apis/src/service/repository/new_repo.rs (1)
18-20:⚠️ Potential issueImplementation is incomplete - needs to be completed before merging.
The
repository_newmethod contains only atodo!()macro, which will panic at runtime. This needs to be properly implemented before the PR can be merged.Additionally, consider:
- Adding input validation for the repository name format
- Implementing proper error handling using a Result type
- Adding documentation comments explaining the method's behavior
Would you like me to help design the implementation with proper validation and error handling?
libs/model/users/emails.rs (1)
15-15: Verify database migration for table rename.The table name change from "email" to "emails" will require a database migration. Please ensure:
- A migration is included for this table rename
- All existing queries have been updated to use the new table name
libs/model/users/token_key.rs (1)
16-16: Verify database migration for table rename.The table name change from "token" to "token_keys" will require a database migration. Please ensure a migration is included for this change.
libs/config/mod.rs (1)
10-10: LGTM!The addition of the email module follows the existing pattern of module declarations.
libs/model/users/users.rs (1)
39-39: Verify data migration strategy for repository_limit type changeChanging from
u64toi32:
- Could truncate existing values > i32::MAX
- Needs a migration strategy for existing data
- Consider if i16 would be sufficient (max value 32,767)
✅ Verification successful
Type change to i32 appears safe based on usage patterns
The codebase shows no evidence of values or operations that would be impacted by the type change:
- Only used with default value of 20
- No complex arithmetic or comparisons found
- No migration needed as current usage is well within i32 range
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing repository limits in the codebase rg -A 2 'repository_limit.*[0-9]+' # Look for any migration files related to this change fd -e rs -e sql migrationLength of output: 455
Script:
#!/bin/bash # Check for any arithmetic operations or comparisons with repository_limit rg -p 'repository_limit.*[<>=+\-*/]|[<>=+\-*/].*repository_limit' # Look for API endpoints or validation logic involving repository_limit ast-grep --pattern 'fn $_($_: $_) { $$$ repository_limit $$$ }'Length of output: 213
gitdata-apis/Cargo.toml (1)
Line range hint
1-50: Update potentially vulnerable dependencies.Several dependencies are using outdated versions that might have security vulnerabilities:
- serde 1.0.217
- serde_json 1.0.134
Let's check for known vulnerabilities and latest versions:
| @@ -4,7 +4,7 @@ use actix_web::HttpMessage; | |||
| use actix_web::HttpRequest; | |||
| use actix_web::Responder; | |||
| use actix_web::web; | |||
|
|
|||
| use sha256::Sha256Digest; | |||
There was a problem hiding this comment.
Consider using a more secure password hashing algorithm.
SHA-256 is a general-purpose hash function and not specifically designed for password hashing. Consider using algorithms like Argon2, bcrypt, or PBKDF2 that are specifically designed for password hashing with built-in salt support.
| @@ -23,6 +23,7 @@ pub async fn api_v1_auth_passwd( | |||
| return AppWrite::fail(err.to_string()); | |||
| } | |||
| }; | |||
| dbg!(inner.clone().password.digest()); | |||
There was a problem hiding this comment.
Remove debug logging of password hash.
Debug logging of password hashes, even if they're not plaintext, is a security risk as it could expose sensitive information in logs that might be used in rainbow table attacks.
If debugging is necessary during development, ensure it's behind a feature flag:
- dbg!(inner.clone().password.digest());
+ #[cfg(debug_assertions)]
+ tracing::debug!("Processing authentication request for 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.
| dbg!(inner.clone().password.digest()); | |
| #[cfg(debug_assertions)] | |
| tracing::debug!("Processing authentication request for user"); |
| @@ -4,7 +4,7 @@ use sea_orm::Set; | |||
| use sea_orm::prelude::Uuid; | |||
| use serde::Deserialize; | |||
| use serde::Serialize; | |||
|
|
|||
| use sha256::Sha256Digest; | |||
There was a problem hiding this comment.
Enhance password security and validation.
Several security improvements are needed:
- Replace SHA-256 with a proper password hashing algorithm (Argon2, bcrypt, or PBKDF2)
- Add password complexity validation
- Add rate limiting for user creation
Example implementation:
use argon2::{self, Config};
use password_validation::{PasswordValidation, ValidationCriteria};
// Add password validation
fn validate_password(password: &str) -> anyhow::Result<()> {
let criteria = ValidationCriteria::new()
.length(12, 128)
.numbers()
.symbols()
.uppercase_lowercase();
PasswordValidation::new(&criteria)
.validate(password)
.map_err(|e| anyhow::anyhow!("Password too weak: {}", e))
}
// In users_apply method:
validate_password(¶m.passwd)?;
// Use Argon2 instead of SHA-256
let salt = rand::random::<[u8; 32]>();
let config = Config::default();
let hash = argon2::hash_encoded(
param.passwd.as_bytes(),
&salt,
&config
)?;
let mut model = users::ActiveModel::new_users(param.username, param.email, hash);Also applies to: 64-64
| .session_ttl(Duration::days(30)) | ||
| .session_ttl_extension_policy(TtlExtensionPolicy::OnEveryRequest), | ||
| ) | ||
| .cookie_secure(false) |
There was a problem hiding this comment.
Security Issue: cookie_secure Should Be Enabled in Production
Setting .cookie_secure(false) allows the session cookie to be sent over unsecured connections, which can expose session data to attackers. It is recommended to set cookie_secure to true in production environments to ensure cookies are only transmitted over HTTPS.
Consider adjusting the setting based on the environment:
.cookie_secure(!cfg!(debug_assertions))| SessionMiddleware::builder(session.clone(), Key::from(&[0; 64])) | ||
| .cookie_name("SessionID".to_string()) | ||
| .cookie_path("/".to_string()) | ||
| .cookie_http_only(false) |
There was a problem hiding this comment.
Security Issue: cookie_http_only Should Be Set to true
Setting .cookie_http_only(false) allows client-side scripts to access the session cookie, increasing the risk of cross-site scripting (XSS) attacks. It is more secure to set cookie_http_only to true to prevent JavaScript access to the cookie.
Proposed fix:
- .cookie_http_only(false)
+ .cookie_http_only(true)📝 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.
| .cookie_http_only(false) | |
| .cookie_http_only(true) |
| pub fn check(&self) -> anyhow::Result<()> { | ||
| if self.smtp.is_empty() { | ||
| Self::default().save()?; | ||
| return Err(anyhow::anyhow!("smtp is empty")); | ||
| } | ||
| if self.port == 0 { | ||
| Self::default().save()?; | ||
| return Err(anyhow::anyhow!("port is empty")); | ||
| } | ||
| if self.username.is_empty() { | ||
| Self::default().save()?; | ||
| return Err(anyhow::anyhow!("username is empty")); | ||
| } | ||
| if self.password.is_empty() { | ||
| Self::default().save()?; | ||
| return Err(anyhow::anyhow!("password is empty")); | ||
| } | ||
| if self.from.is_empty() { | ||
| Self::default().save()?; | ||
| return Err(anyhow::anyhow!("from is empty")); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Prevent potential data loss in check method.
The current implementation saves default config on validation failure, which could lead to data loss. Instead:
- Separate validation from saving
- Don't overwrite existing config on validation failure
- Add validation during construction
pub fn check(&self) -> anyhow::Result<()> {
if self.smtp.is_empty() {
- Self::default().save()?;
return Err(anyhow::anyhow!("smtp is empty"));
}
// ... similar changes for other checks
Ok(())
}
+ impl TryFrom<EmailConfig> for EmailConfig {
+ type Error = anyhow::Error;
+ fn try_from(config: EmailConfig) -> Result<Self, Self::Error> {
+ if config.smtp.is_empty() {
+ return Err(anyhow::anyhow!("smtp is empty"));
+ }
+ // ... other validations
+ Ok(config)
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
libs/model/repository/repository.rs
Outdated
| ssh_path: Set(format!("[email protected]:{}/{}", owner_uid, name)), | ||
| http_path: Set(format!("https://gitdata.ai/{}/{}", owner_uid, name)), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract domain configuration to avoid hard-coding
The domain "gitdata.ai" is hard-coded in both SSH and HTTP paths. Consider extracting this to a configuration value for better maintainability and flexibility.
libs/model/repository/repository.rs
Outdated
| pub fn new( | ||
| name: String, | ||
| owner_uid: Uuid, | ||
| description: Option<String>, | ||
| visible: bool, | ||
| default_branch: Option<String>, | ||
| ) -> Self { |
There was a problem hiding this comment.
💡 Codebase verification
Add comprehensive repository name validation to prevent security issues
The codebase shows repository names are used in file paths but lack proper validation. This is particularly concerning as the code in gitdata-gits/src/http/ shows direct path construction using repository names.
Required validations:
- Prevent path traversal by disallowing
..,/, and similar characters - Enforce Git-standard repository naming (alphanumeric, hyphens, underscores)
- Set appropriate length limits
- Prevent reserved names (like .git, .., etc.)
🔗 Analysis chain
Add input validation for repository name
The constructor should validate the repository name format to prevent invalid characters and potential security issues. Consider adding checks for:
- Invalid characters in repository names
- Reserved names
- Length limits
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing name validation patterns in the codebase
rg -A 3 'fn validate.*name|name.*validation'
Length of output: 46
Script:
#!/bin/bash
# Search for repository-related validation or constraints
rg -i "repository.*valid|valid.*repository" -A 3
# Look for repository name patterns in tests
rg -i "test.*repository.*name" -A 3
# Search for any repository name related constants or patterns
rg "repo.*name|repository.*name" -A 3
# Check if there are any database schema files that might contain constraints
fd -e sql -e schema
Length of output: 1036
| let migrator = PostgresStorage::migrations(); | ||
| let mut job_txn = jobs_margin.get_postgres_connection_pool().acquire().await?; | ||
| migrator.run(&mut job_txn).await?; |
There was a problem hiding this comment.
Add explicit transaction handling for migrations
The transaction is acquired but not explicitly committed or rolled back. Consider wrapping in a try-catch block with proper transaction handling.
- let mut job_txn = jobs_margin.get_postgres_connection_pool().acquire().await?;
- migrator.run(&mut job_txn).await?;
+ let mut job_txn = jobs_margin.get_postgres_connection_pool().begin().await?;
+ if let Err(e) = migrator.run(&mut job_txn).await {
+ job_txn.rollback().await?;
+ return Err(e.into());
+ }
+ job_txn.commit().await?;📝 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.
| let migrator = PostgresStorage::migrations(); | |
| let mut job_txn = jobs_margin.get_postgres_connection_pool().acquire().await?; | |
| migrator.run(&mut job_txn).await?; | |
| let migrator = PostgresStorage::migrations(); | |
| let mut job_txn = jobs_margin.get_postgres_connection_pool().begin().await?; | |
| if let Err(e) = migrator.run(&mut job_txn).await { | |
| job_txn.rollback().await?; | |
| return Err(e.into()); | |
| } | |
| job_txn.commit().await?; |
| @@ -26,6 +26,7 @@ serde = { version = "1.0.217", features = ["derive"] } | |||
| serde_json = { version = "1.0.134", features = ["alloc"] } | |||
| tonic = { version = "0.12.3", features = ["default"] } | |||
| sea-orm = { version = "1", features = ["sqlx-all","runtime-tokio"] } | |||
| sea-orm-migration = { version = "1", features = ["runtime-tokio","sqlx-postgres","cli","default"] } | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify a more precise version constraint.
The version constraint "1" is too loose and could lead to unexpected breaking changes. Consider using a more specific version range like "1.0" or pin to an exact version.
-sea-orm-migration = { version = "1", features = ["runtime-tokio","sqlx-postgres","cli","default"] }
+sea-orm-migration = { version = "~1.0.0", features = ["runtime-tokio","sqlx-postgres","cli","default"] }📝 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.
| sea-orm-migration = { version = "1", features = ["runtime-tokio","sqlx-postgres","cli","default"] } | |
| sea-orm-migration = { version = "~1.0.0", features = ["runtime-tokio","sqlx-postgres","cli","default"] } |
There was a problem hiding this comment.
Actionable comments posted: 20
🔭 Outside diff range comments (1)
gitdata-gits/src/rpc/core_git.rs (1)
Line range hint
235-240: Validate file paths to prevent security vulnerabilitiesWhen constructing file paths using user-provided
file_pathandfile_name, there is a risk of directory traversal attacks if these inputs contain malicious paths like../. Ensure that the inputs are validated to prevent unauthorized access to the filesystem.Consider sanitizing the file paths:
use std::path::Path; let sanitized_file_path = Path::new(&file_path).components().filter(|c| matches!(c, std::path::Component::Normal(_))).collect::<PathBuf>(); let full_file_path = tmp.path().join(sanitized_file_path).join(&file_name);This approach filters out any parent directory components and constructs a safe path.
♻️ Duplicate comments (5)
gitdata-apis/src/service/users/apply.rs (1)
7-7:⚠️ Potential issueReplace SHA-256 with a secure password hashing algorithm.
SHA-256 is a general-purpose cryptographic hash function and is not suitable for password hashing. It lacks important security features like salting and adjustable work factors, making it vulnerable to rainbow table and brute force attacks.
libs/model/repository/repository.rs (1)
63-69:⚠️ Potential issueAdd input validation for repository name
The repository name is used in path construction without validation, which could lead to security vulnerabilities like path traversal attacks.
Add validation before using the name in path construction:
fn validate_repository_name(name: &str) -> Result<(), String> { if name.is_empty() || name.len() > 100 { return Err("Repository name must be between 1 and 100 characters".into()); } if name.contains("..") || name.contains('/') { return Err("Repository name contains invalid characters".into()); } if !name.chars().all(|c| c.is_alphanumeric() || c == '-' || c == '_') { return Err("Repository name must contain only alphanumeric characters, hyphens, or underscores".into()); } Ok(()) }Then update the constructor:
pub fn new( name: String, owner_uid: Uuid, description: Option<String>, visible: bool, default_branch: Option<String>, ) -> Self { + if let Err(e) = validate_repository_name(&name) { + panic!("Invalid repository name: {}", e); + } // ... rest of the implementationgitdata-apis/src/service/mod.rs (1)
56-58:⚠️ Potential issueAdd explicit transaction handling for migrations
The migration is still being executed without proper transaction handling, which could lead to partial migrations in case of failures.
Implement proper transaction handling:
- let migrator = PostgresStorage::migrations(); - let mut job_txn = jobs_margin.get_postgres_connection_pool().acquire().await?; - migrator.run(&mut job_txn).await?; + let migrator = PostgresStorage::migrations(); + let mut job_txn = jobs_margin.get_postgres_connection_pool().begin().await?; + if let Err(e) = migrator.run(&mut job_txn).await { + job_txn.rollback().await?; + return Err(e.into()); + } + job_txn.commit().await?;gitdata-apis/src/main.rs (2)
38-38:⚠️ Potential issueSecurity Issue: Use a Secure Key for SessionMiddleware
Using
Key::from(&[0; 64])provides a key with zero entropy, which is insecure.
41-41:⚠️ Potential issueSecurity Issues: Cookie Configuration Needs Hardening
Multiple security issues in cookie configuration:
cookie_http_only(false)allows client-side access to cookiescookie_secure(false)allows transmission over insecure connectionsAlso applies to: 48-48
🧹 Nitpick comments (13)
libs/model/repository/repository.rs (1)
95-96: Use the same timestamp for created_at and updated_atCreating separate timestamps could lead to microsecond-level differences. Generate once and reuse.
+ let now = chrono::Utc::now().timestamp(); - created_at: Set(chrono::Utc::now().timestamp()), - updated_at: Set(chrono::Utc::now().timestamp()), + created_at: Set(now), + updated_at: Set(now),libs/config/git.rs (1)
6-9: Consider adding URL validation for http and ssh fields.The struct fields should validate that they contain valid Git URLs. Consider implementing validation in the constructor or using a custom type that ensures URL correctness.
+use url::Url; + #[derive(Deserialize,Serialize,Clone,Debug, Default)] pub struct GitConfig { - pub http: String, - pub ssh: String, + http: String, + ssh: String, } + +impl GitConfig { + pub fn validate_urls(&self) -> anyhow::Result<()> { + Url::parse(&self.http)?; + // SSH validation logic here + Ok(()) + } + + pub fn http(&self) -> &str { + &self.http + } + + pub fn ssh(&self) -> &str { + &self.ssh + } +}gitdata-gits/src/main.rs (1)
Line range hint
44-50: Improve shutdown handling.The current shutdown logic using
while let Ok(_)could be simplified and made more robust:- while let Ok(_) = tokio::signal::ctrl_c().await { - http.abort(); - health.abort(); - info!("shutting down"); - break; - } + tokio::signal::ctrl_c().await?; + info!("initiating graceful shutdown"); + http.abort(); + health.abort(); + info!("shutdown complete");gitdata-gits/src/rpc/core_git.rs (2)
28-34: Reduce code duplication by extracting common logicThe logic for extracting
storage_positionfrom the request and handling theNonecase is duplicated in bothcreateandadd_filemethods. Consider extracting this logic into a helper function to improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle.Example of extracting common logic:
fn extract_storage_position(request: impl Into<RequestWithStoragePosition>) -> Result<RepositoryStoragePosition, Status> { let request = request.into().into_inner(); match request.storage_position { Some(storage_position) => Ok(storage_position), None => Err(Status::invalid_argument("storage_position is required")), } }You can then use this function in both methods to eliminate duplication.
Also applies to: 86-92
39-48: Refactor storage retrieval to eliminate duplicate codeThe pattern matching on
storage_position.r#typeand retrieval of storage instances is repeated in both methods. This code can be refactored into a separate function to enhance code readability and maintainability.Example refactored function:
fn get_storage(&self, storage_position: &RepositoryStoragePosition, node: &NodePath) -> Result<StorageSingleton, Status> { match storage_position.r#type { 0 => match self.storage.s3.get(&node.node) { Some(s3_storage) => Ok(StorageSingleton::S3(s3_storage.clone())), None => Err(Status::not_found("S3 storage node not found")), }, 1 => match self.storage.local.get(&node.node) { Some(local_storage) => Ok(StorageSingleton::Local(local_storage.clone())), None => Err(Status::not_found("Local storage node not found")), }, 2 => match self.storage.nfs.get(&node.node) { Some(nfs_storage) => Ok(StorageSingleton::Nfs(nfs_storage.clone())), None => Err(Status::not_found("NFS storage node not found")), }, _ => Err(Status::invalid_argument("storage_position.r#type is invalid")), } }Use this function in both
createandadd_filemethods to reduce duplication.Also applies to: 97-106
gitdata-gits/src/mount/s3.rs (1)
176-183: Simplify error handling increate_repositoryThe current error handling logic in
create_repositorycan be simplified for better readability.Simplify the method:
pub(crate) async fn create_repository(&self, path: String) -> anyhow::Result<()> { if std::fs::read_dir(&self.root.join(&path)).is_ok() { return Err(anyhow::anyhow!("Repository path already exists")); } git2::Repository::init_bare(&self.root.join(&path)) .with_context(|| format!("Failed to initialize repository at {}", path))?; Ok(()) }This removes unnecessary checks and leverages
with_contextfor better error messages.gitdata-gits/src/mount/local.rs (1)
176-183: Simplify error handling increate_repositoryThe error handling in the
create_repositorymethod can be streamlined for clarity.Refactored code:
pub(crate) async fn create_repository(&self, path: String) -> anyhow::Result<()> { if std::fs::read_dir(&self.root.join(&path)).is_ok() { return Err(anyhow::anyhow!("Repository path already exists")); } git2::Repository::init_bare(&self.root.join(&path)) .with_context(|| format!("Failed to initialize repository at {}", path))?; Ok(()) }This approach simplifies the logic and improves error messages.
gitdata-apis/src/service/core_git_rpc.rs (1)
Line range hint
8-28: Consider implementing retry logic inCoreGitRpcconnectionThe
connectmethod inCoreGitRpcattempts to establish a connection to the RPC server. If the connection fails, it returns an error immediately.Implementing a retry mechanism can improve the robustness of the service, especially in distributed systems where transient network issues may occur.
async fn connect() -> anyhow::Result<Self> { let rpc = RpcConfig::load()?; let mut attempts = 0; let core_git_rpc = loop { match RepRepositoryClient::connect(rpc.coregit_node()?.url()).await { Ok(client) => break client, Err(e) if attempts < MAX_RETRIES => { attempts += 1; tokio::time::sleep(RETRY_DELAY).await; continue; }, Err(e) => return Err(anyhow::anyhow!("Failed to connect to RPC server: {}", e)), } }; Ok(CoreGitRpc { client: core_git_rpc }) }Replace
MAX_RETRIESandRETRY_DELAYwith appropriate values as per your application's needs.gitdata-apis/src/service/repository/new_repo.rs (1)
69-69: Enhance README.md template content.The README content is very basic. Consider providing a more comprehensive template.
Example enhancement:
- let bytes = format!("### {}", param.name); + let bytes = format!(r#"# {} ## Description {} ## Getting Started ### Prerequisites ### Installation ### Usage ## Contributing ## License "#, param.name, param.description.as_deref().unwrap_or(""));gitdata-apis/src/apis/app_router.rs (4)
67-81: Resolve potential route conflicts in the SSH scope.The SSH scope defines two routes with an empty path
""but different HTTP methods (GET and POST). While this works, it's better to use explicit paths for clarity:scope("/ssh") .route( - "", + "/list", get().to(super::v1::user::ssh_keys::api_v1_user_ssh_key_list), ) .route( "/{ssh_key_uid}", delete() .to(super::v1::user::ssh_keys::api_v1_user_ssh_key_delete), ) .route( - "", + "/create", post() .to(super::v1::user::ssh_keys::api_v1_user_ssh_key_create), ),
84-97: Resolve potential route conflicts in the token scope.Similar to the SSH scope, the token scope has conflicting empty paths. Consider using explicit paths:
scope("token") .route( - "", + "/create", post().to(super::v1::user::token_key::api_v1_user_token_create), ) .route( "/{token_uid}", delete() .to(super::v1::user::token_key::api_v1_user_token_delete), ) .route( - "", + "/list", get().to(super::v1::user::token_key::api_v1_user_token_list), ),
50-51: Fix typo in route path.The route path contains a typo: "pined" should be "pinned".
- "/pined", + "/pinned",
34-98: Consider standardizing path parameter naming.The path parameters use inconsistent naming patterns:
{username}is domain-specific{ssh_key_uid}and{token_uid}use the_uidsuffixConsider standardizing to either all domain-specific names or all generic
uid-based names for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.gitignore(1 hunks)gitdata-apis/src/apis/app_router.rs(1 hunks)gitdata-apis/src/apis/v1/auth/passwd.rs(2 hunks)gitdata-apis/src/jobs/email.rs(4 hunks)gitdata-apis/src/main.rs(2 hunks)gitdata-apis/src/service/auth/passwd.rs(1 hunks)gitdata-apis/src/service/core_git_rpc.rs(2 hunks)gitdata-apis/src/service/emails/captcha.rs(1 hunks)gitdata-apis/src/service/mod.rs(3 hunks)gitdata-apis/src/service/repository/new_repo.rs(2 hunks)gitdata-apis/src/service/users/apply.rs(2 hunks)gitdata-gits/src/main.rs(2 hunks)gitdata-gits/src/mount/local.rs(3 hunks)gitdata-gits/src/mount/mod.rs(2 hunks)gitdata-gits/src/mount/nfs.rs(2 hunks)gitdata-gits/src/mount/s3.rs(2 hunks)gitdata-gits/src/rpc/core_git.rs(2 hunks)gitdata-gits/src/rpc/mod.rs(0 hunks)libs/config/git.rs(1 hunks)libs/model/repository/repository.rs(2 hunks)libs/rpc/core_git.rs(1 hunks)proto/core-git.proto(1 hunks)
💤 Files with no reviewable changes (1)
- gitdata-gits/src/rpc/mod.rs
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- gitdata-apis/src/apis/v1/auth/passwd.rs
- gitdata-apis/src/service/auth/passwd.rs
- gitdata-apis/src/service/emails/captcha.rs
- gitdata-apis/src/jobs/email.rs
🔇 Additional comments (16)
libs/model/repository/repository.rs (1)
14-14: LGTM!The import is properly scoped and follows Rust conventions.
libs/config/git.rs (1)
11-11: LGTM! Thread-safe singleton implementation.Good use of
OnceCellfor thread-safe global state management.gitdata-gits/src/main.rs (2)
5-5: LGTM!The import is correctly scoped and properly utilized in the code.
22-22: Verify storage path security.The storage path is created relative to the current working directory, which could be a security risk if the application is run from an unexpected location.
Run this check to analyze the storage path usage across the codebase:
✅ Verification successful
Storage path implementation is secure ✅
The current implementation follows Rust's security best practices by using the standard library's safe path manipulation functions. The storage is properly contained within the "./data" subdirectory of the current working directory, and all path operations use secure joining methods that prevent path traversal attacks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of storage path configuration rg -A 3 "current_dir|PathBuf::from|create_dir" # Look for path traversal prevention ast-grep --pattern 'Path$_->join($_)'Length of output: 5907
gitdata-apis/src/service/mod.rs (2)
13-13: LGTM! Imports are properly organized.The new imports for database migration functionality are correctly added and well-organized with other database-related imports.
Also applies to: 17-17
107-110: 🛠️ Refactor suggestionImprove database migration robustness
Several improvements are needed for the database migration logic:
- The database name comparison should be case-insensitive from the start
- The migrations should be wrapped in a transaction
- Consider if version control is needed for migrations
- if url.dbname.to_lowercase() != "jobs".to_string() { - DatabaseMigrate::install(&db).await?; - DatabaseMigrate::up(&db, None).await?; + let db_name = url.dbname.to_lowercase(); + if db_name != "jobs" { + let mut txn = db.begin().await?; + if let Err(e) = DatabaseMigrate::install(&txn).await { + txn.rollback().await?; + return Err(e.into()); + } + if let Err(e) = DatabaseMigrate::up(&txn, None).await { + txn.rollback().await?; + return Err(e.into()); + } + txn.commit().await?;Also, consider if version control is needed for migrations. The
Noneparameter inup()suggests migrations might need targeting specific versions.Let's verify the migration versioning implementation:
gitdata-gits/src/mount/s3.rs (1)
279-279: Handle authentication for pushing changesThe
pushoperation may fail if authentication is required and not provided. Ensure that theRemoteCallbackswith authentication support is set ingit2::PushOptions.Please check if authentication is handled properly. You can use the following script to verify:
gitdata-gits/src/mount/mod.rs (2)
18-20: LGTM! Appropriate visibility modifier.The change to
pub(crate)maintains proper encapsulation while allowing necessary access within the crate.
66-66: LGTM! Idiomatic Rust code.The simplified return statement follows Rust idioms and improves readability.
proto/core-git.proto (1)
35-37: LGTM! Well-structured message and RPC definitions.The response message and RPC method are well-defined and follow protobuf conventions.
Also applies to: 40-40
gitdata-apis/src/main.rs (1)
29-32: LGTM! Proper Redis session store initialization.The Redis session store is correctly initialized using the connection pool from the application state.
gitdata-apis/src/service/repository/new_repo.rs (2)
25-88: LGTM! Well-implemented transaction and error handling.The implementation properly handles transactions and includes comprehensive error handling with appropriate rollbacks.
78-78: Consider validating the default branch name.The default branch name should be validated against Git's branch naming rules.
gitdata-apis/src/apis/app_router.rs (3)
1-7: LGTM! Appropriate removal of async keyword.The removal of the
asynckeyword from theAppRouterfunction signature is correct since this function only performs route configuration and doesn't contain any asynchronous operations. The actual async operations are handled by the individual route handlers.
24-31: LGTM! Well-structured email captcha endpoints.The email routes are well-organized with a clear separation between captcha generation and verification endpoints. The use of POST method is appropriate for both operations.
13-20: Verify completeness of authentication routes.While the
/authscope includes routes for password management and logout, there appears to be no route for the login operation. Please verify if this is intentional or if the login functionality is handled elsewhere.
| let mut model = | ||
| users::ActiveModel::new_users(param.username, param.email, param.passwd.digest()); |
There was a problem hiding this comment.
Add input validation and rate limiting.
The user creation implementation needs additional security measures:
- Add input validation for username and email format
- Implement maximum length limits for all input fields
- Add rate limiting for the user creation endpoint
- Sanitize inputs to prevent injection attacks
Example implementation:
use validator::Validate;
#[derive(Deserialize, Serialize, Clone, Validate)]
pub struct UsersApplyParam {
#[validate(length(min = 3, max = 32), regex = "^[a-zA-Z0-9_-]+$")]
pub username: String,
#[validate(email, length(max = 254))]
pub email: String,
#[validate(length(min = 8, max = 128))]
pub passwd: String,
}
impl AppState {
pub async fn users_apply(&self, param: UsersApplyParam) -> anyhow::Result<()> {
// Validate input
param.validate()?;
// Rate limiting
let key = format!("rate_limit:user_creation:{}", self.get_client_ip()?);
if !self.rate_limiter.check_rate(&key, 3, 3600).await? {
return Err(anyhow::anyhow!("Too many attempts. Please try again later"));
}
// Rest of the implementation...
}
}| nums_issue: Set(0), | ||
| nums_pull: Set(0), | ||
| nums_commit: Set(0), | ||
| head: Set("".parse().unwrap()), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace unwrap with a safe default
Using unwrap() on parse() could panic. Since we're parsing an empty string literal, we can simply use to_string().
-head: Set("".parse().unwrap()),
+head: Set("".to_string()),📝 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.
| head: Set("".parse().unwrap()), | |
| head: Set("".to_string()), |
| owner_uid: Set(owner_uid), | ||
| description: Set(description), | ||
| visible: Set(visible), | ||
| default_branch: Set(default_branch.unwrap_or("".to_string())), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid empty default branch string
Using an empty string as the default branch value could cause issues with Git operations. Consider using a standard default like "main" or "master".
-default_branch: Set(default_branch.unwrap_or("".to_string())),
+default_branch: Set(default_branch.unwrap_or_else(|| "main".to_string())),📝 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.
| default_branch: Set(default_branch.unwrap_or("".to_string())), | |
| default_branch: Set(default_branch.unwrap_or_else(|| "main".to_string())), |
| pub fn save(&self) -> anyhow::Result<()> { | ||
| if std::fs::read_dir("./config").is_err(){ | ||
| std::fs::create_dir("./config")?; | ||
| } | ||
| std::fs::write("./config/git.toml", toml::to_string(self)?)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a configurable base path instead of hardcoded values.
Hardcoded paths can cause issues in different environments. Consider making the config directory path configurable.
+const CONFIG_DIR: &str = "./config";
+const CONFIG_FILE: &str = "git.toml";
+
pub fn save(&self) -> anyhow::Result<()> {
- if std::fs::read_dir("./config").is_err(){
- std::fs::create_dir("./config")?;
+ let path = std::path::Path::new(CONFIG_DIR);
+ if !path.exists() {
+ std::fs::create_dir_all(path)?;
}
- std::fs::write("./config/git.toml", toml::to_string(self)?)?;
+ let config_path = path.join(CONFIG_FILE);
+ std::fs::write(config_path, toml::to_string(self)?)?;
Ok(())
}📝 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.
| pub fn save(&self) -> anyhow::Result<()> { | |
| if std::fs::read_dir("./config").is_err(){ | |
| std::fs::create_dir("./config")?; | |
| } | |
| std::fs::write("./config/git.toml", toml::to_string(self)?)?; | |
| Ok(()) | |
| } | |
| const CONFIG_DIR: &str = "./config"; | |
| const CONFIG_FILE: &str = "git.toml"; | |
| pub fn save(&self) -> anyhow::Result<()> { | |
| let path = std::path::Path::new(CONFIG_DIR); | |
| if !path.exists() { | |
| std::fs::create_dir_all(path)?; | |
| } | |
| let config_path = path.join(CONFIG_FILE); | |
| std::fs::write(config_path, toml::to_string(self)?)?; | |
| Ok(()) | |
| } |
| pub fn load() -> anyhow::Result<Self> { | ||
| if std::fs::read_dir("./config").is_err(){ | ||
| std::fs::create_dir("./config")?; | ||
| Self::default().save()?; | ||
| warn!("git config not found, use default config."); | ||
| return Ok(Self::default()); | ||
| } | ||
| let config = match std::fs::read_to_string("./config/git.toml"){ | ||
| Ok(config) => config, | ||
| Err(_) => { | ||
| Self::default().save()?; | ||
| warn!("git config not found, use default config."); | ||
| return Ok(Self::default()); | ||
| } | ||
| }; | ||
| let config = match toml::from_str::<GitConfig>(&config){ | ||
| Ok(config) => config, | ||
| Err(_) => { | ||
| Self::default().save()?; | ||
| warn!("git config not found, use default config."); | ||
| return Ok(Self::default()); | ||
| } | ||
| }; | ||
| Ok(config) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate error handling logic.
The error handling code is duplicated multiple times. Consider extracting it into a separate function.
+ fn handle_config_error() -> anyhow::Result<Self> {
+ let config = Self::default();
+ config.save()?;
+ warn!("git config not found, use default config.");
+ Ok(config)
+ }
+
pub fn load() -> anyhow::Result<Self> {
- if std::fs::read_dir("./config").is_err(){
- std::fs::create_dir("./config")?;
- Self::default().save()?;
- warn!("git config not found, use default config.");
- return Ok(Self::default());
+ let path = std::path::Path::new(CONFIG_DIR);
+ if !path.exists() {
+ return Self::handle_config_error();
}
- let config = match std::fs::read_to_string("./config/git.toml"){
- Ok(config) => config,
- Err(_) => {
- Self::default().save()?;
- warn!("git config not found, use default config.");
- return Ok(Self::default());
- }
- };
- let config = match toml::from_str::<GitConfig>(&config){
- Ok(config) => config,
- Err(_) => {
- Self::default().save()?;
- warn!("git config not found, use default config.");
- return Ok(Self::default());
- }
- };
- Ok(config)
+
+ let config_path = path.join(CONFIG_FILE);
+ let content = std::fs::read_to_string(config_path)
+ .map_err(|_| Self::handle_config_error())?;
+
+ toml::from_str::<GitConfig>(&content)
+ .map_err(|_| Self::handle_config_error())?
}Committable suggestion skipped: line range outside the PR's diff.
gitdata-gits/src/mount/nfs.rs
Outdated
| pub(crate) async fn create_repository(&self, path : String) -> anyhow::Result<()> { | ||
| if std::fs::read_dir(&self.root.join(path.clone())).is_ok() { | ||
| return Err(anyhow::anyhow!("Repository Path already exists")); | ||
| } | ||
| let git = git2::Repository::init_bare(&self.root.join(path.clone())); | ||
| if git.is_ok() { | ||
| Ok(()) | ||
| } else if let Err(r) = git { | ||
| Err(anyhow::anyhow!("{}", r)) | ||
| } else { | ||
| Err(anyhow::anyhow!("Unknown Error")) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify error handling in create_repository function
The error handling logic in the create_repository function can be simplified. Currently, there's an unreachable else block because all possible cases of the Result git are already handled.
Apply this diff to streamline the function:
pub(crate) async fn create_repository(&self, path: String) -> anyhow::Result<()> {
if std::fs::read_dir(&self.root.join(path.clone())).is_ok() {
return Err(anyhow::anyhow!("Repository path already exists"));
}
let git = git2::Repository::init_bare(&self.root.join(path.clone()));
- if git.is_ok() {
- Ok(())
- } else if let Err(r) = git {
- Err(anyhow::anyhow!("{}", r))
- } else {
- Err(anyhow::anyhow!("Unknown Error"))
- }
+ match git {
+ Ok(_) => Ok(()),
+ Err(e) => Err(anyhow::anyhow!("Failed to initialize repository: {}", e)),
+ }
}📝 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.
| pub(crate) async fn create_repository(&self, path : String) -> anyhow::Result<()> { | |
| if std::fs::read_dir(&self.root.join(path.clone())).is_ok() { | |
| return Err(anyhow::anyhow!("Repository Path already exists")); | |
| } | |
| let git = git2::Repository::init_bare(&self.root.join(path.clone())); | |
| if git.is_ok() { | |
| Ok(()) | |
| } else if let Err(r) = git { | |
| Err(anyhow::anyhow!("{}", r)) | |
| } else { | |
| Err(anyhow::anyhow!("Unknown Error")) | |
| } | |
| } | |
| pub(crate) async fn create_repository(&self, path: String) -> anyhow::Result<()> { | |
| if std::fs::read_dir(&self.root.join(path.clone())).is_ok() { | |
| return Err(anyhow::anyhow!("Repository path already exists")); | |
| } | |
| let git = git2::Repository::init_bare(&self.root.join(path.clone())); | |
| match git { | |
| Ok(_) => Ok(()), | |
| Err(e) => Err(anyhow::anyhow!("Failed to initialize repository: {}", e)), | |
| } | |
| } |
gitdata-gits/src/mount/nfs.rs
Outdated
| pub(crate) async fn add_file( | ||
| &self, | ||
| path : String, | ||
| file_path : String, | ||
| bytes : Vec<u8>, | ||
| commit_email : String, | ||
| commit_users : String, | ||
| commit_message : String, | ||
| file_name : String, | ||
| branch_name : String, | ||
| ) -> anyhow::Result<()> { | ||
| use anyhow::Context; | ||
|
|
||
| let path = self.root.join(path); | ||
| let tmp = tempfile::tempdir().context("Failed to create temporary directory")?; | ||
| let clone_repository = git2::Repository::clone(path.to_str().unwrap(), tmp.path()) | ||
| .context("Failed to clone repository")?; | ||
|
|
||
| let branch = match clone_repository.find_branch(&branch_name, git2::BranchType::Local) { | ||
| Ok(branch) => branch, | ||
| Err(_) => { | ||
| let head_commit = clone_repository | ||
| .head() | ||
| .context("Failed to get HEAD")? | ||
| .peel_to_commit() | ||
| .context("Failed to peel HEAD to commit")?; | ||
| clone_repository | ||
| .branch(&branch_name, &head_commit, false) | ||
| .context("Failed to create branch")?; | ||
| clone_repository | ||
| .find_branch(&branch_name, git2::BranchType::Local) | ||
| .context("Failed to find branch after creation")? | ||
| } | ||
| }; | ||
|
|
||
| let branch_name = branch | ||
| .name() | ||
| .transpose() | ||
| .context("Failed to get branch name")? | ||
| .map_err(|_| anyhow::anyhow!("Branch name is empty"))?; | ||
|
|
||
| if !branch.is_head() { | ||
| clone_repository | ||
| .set_head(&branch_name) | ||
| .context("Failed to set HEAD to branch")?; | ||
| clone_repository | ||
| .checkout_head(Some(git2::build::CheckoutBuilder::default().force())) | ||
| .context("Failed to check out HEAD")?; | ||
| } | ||
|
|
||
| let full_file_path = tmp.path().join(&file_path).join(&file_name); | ||
| std::fs::create_dir_all( | ||
| full_file_path | ||
| .parent() | ||
| .context("Failed to get parent directory")?, | ||
| )?; | ||
| std::fs::write(&full_file_path, bytes).context("Failed to write file")?; | ||
|
|
||
| let relative_path = full_file_path | ||
| .strip_prefix(tmp.path()) | ||
| .context("Failed to strip prefix from file path")?; | ||
| let mut index = clone_repository | ||
| .index() | ||
| .context("Failed to get repository index")?; | ||
| index | ||
| .add_path(relative_path) | ||
| .context("Failed to add file to index")?; | ||
| index.write().context("Failed to write index")?; | ||
|
|
||
| let time = chrono::Utc::now().timestamp(); | ||
| let time = git2::Time::new(time, 0); | ||
| let user = Signature::new(&commit_users, &commit_email, &time) | ||
| .context("Failed to create commit signature")?; | ||
| let tree = clone_repository | ||
| .find_tree(index.write_tree().context("Failed to write tree")?) | ||
| .context("Failed to find tree")?; | ||
| let parent_commit = clone_repository | ||
| .find_commit( | ||
| branch | ||
| .get() | ||
| .target() | ||
| .context("Failed to get branch target")?, | ||
| ) | ||
| .context("Failed to find parent commit")?; | ||
| clone_repository | ||
| .commit(Some("HEAD"), &user, &user, &commit_message, &tree, &[ | ||
| &parent_commit, | ||
| ]) | ||
| .context("Failed to create commit")?; | ||
|
|
||
| let refspec = format!("refs/heads/{}:refs/heads/{}", branch_name, branch_name); | ||
| clone_repository | ||
| .find_remote("origin") | ||
| .context("Failed to find remote 'origin'")? | ||
| .push(&[refspec.as_str()], Some(&mut git2::PushOptions::new())) | ||
| .context("Failed to push changes to remote")?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness and error handling in add_file function
The add_file function is complex and would benefit from improved error handling and code safety:
-
Avoid
unwrap()onOptiontypesUsing
unwrap()on options likepath.to_str().unwrap()can lead to panics if theOptionisNone. It's safer to usecontext()to provide meaningful error messages. -
Ensure proper cleanup of temporary directories
The temporary directory created with
tempfile::tempdir()should be managed carefully to ensure it's cleaned up, even if an error occurs. -
Validate and sanitize file paths
Constructing file paths using user input (
file_path,file_name) without validation can lead to security vulnerabilities like path traversal attacks. -
Handle non-UTF8 paths
Since paths may not always be valid UTF-8, handling them as
OsStrinstead of converting to strings can prevent potential issues.
Consider applying these changes:
- Replace
unwrap()calls with error handling usingcontext(). - Use
let repo_path = path.to_str().context("Repository path is not valid UTF-8")?;. - Validate and sanitize
file_pathandfile_nameto prevent security issues. - Ensure that the temporary directory is properly cleaned up by using the
TempDir's lifetime or handling errors appropriately.
Here's a partial diff to illustrate some of these suggestions:
use anyhow::{Context, Result};
pub(crate) async fn add_file(
&self,
path: String,
file_path: String,
bytes: Vec<u8>,
commit_email: String,
commit_users: String,
commit_message: String,
file_name: String,
branch_name: String,
) -> Result<()> {
let path = self.root.join(&path);
- let tmp = tempfile::tempdir().context("Failed to create temporary directory")?;
- let clone_repository = git2::Repository::clone(path.to_str().unwrap(), tmp.path())
+ let repo_path = path.to_str().context("Repository path is not valid UTF-8")?;
+ let tmp_dir = tempfile::tempdir().context("Failed to create temporary directory")?;
+ let clone_repository = git2::Repository::clone(repo_path, tmp_dir.path())
.context("Failed to clone repository")?;
// Validate and sanitize file_path and file_name here
// Rest of the code...
}📝 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.
| pub(crate) async fn add_file( | |
| &self, | |
| path : String, | |
| file_path : String, | |
| bytes : Vec<u8>, | |
| commit_email : String, | |
| commit_users : String, | |
| commit_message : String, | |
| file_name : String, | |
| branch_name : String, | |
| ) -> anyhow::Result<()> { | |
| use anyhow::Context; | |
| let path = self.root.join(path); | |
| let tmp = tempfile::tempdir().context("Failed to create temporary directory")?; | |
| let clone_repository = git2::Repository::clone(path.to_str().unwrap(), tmp.path()) | |
| .context("Failed to clone repository")?; | |
| let branch = match clone_repository.find_branch(&branch_name, git2::BranchType::Local) { | |
| Ok(branch) => branch, | |
| Err(_) => { | |
| let head_commit = clone_repository | |
| .head() | |
| .context("Failed to get HEAD")? | |
| .peel_to_commit() | |
| .context("Failed to peel HEAD to commit")?; | |
| clone_repository | |
| .branch(&branch_name, &head_commit, false) | |
| .context("Failed to create branch")?; | |
| clone_repository | |
| .find_branch(&branch_name, git2::BranchType::Local) | |
| .context("Failed to find branch after creation")? | |
| } | |
| }; | |
| let branch_name = branch | |
| .name() | |
| .transpose() | |
| .context("Failed to get branch name")? | |
| .map_err(|_| anyhow::anyhow!("Branch name is empty"))?; | |
| if !branch.is_head() { | |
| clone_repository | |
| .set_head(&branch_name) | |
| .context("Failed to set HEAD to branch")?; | |
| clone_repository | |
| .checkout_head(Some(git2::build::CheckoutBuilder::default().force())) | |
| .context("Failed to check out HEAD")?; | |
| } | |
| let full_file_path = tmp.path().join(&file_path).join(&file_name); | |
| std::fs::create_dir_all( | |
| full_file_path | |
| .parent() | |
| .context("Failed to get parent directory")?, | |
| )?; | |
| std::fs::write(&full_file_path, bytes).context("Failed to write file")?; | |
| let relative_path = full_file_path | |
| .strip_prefix(tmp.path()) | |
| .context("Failed to strip prefix from file path")?; | |
| let mut index = clone_repository | |
| .index() | |
| .context("Failed to get repository index")?; | |
| index | |
| .add_path(relative_path) | |
| .context("Failed to add file to index")?; | |
| index.write().context("Failed to write index")?; | |
| let time = chrono::Utc::now().timestamp(); | |
| let time = git2::Time::new(time, 0); | |
| let user = Signature::new(&commit_users, &commit_email, &time) | |
| .context("Failed to create commit signature")?; | |
| let tree = clone_repository | |
| .find_tree(index.write_tree().context("Failed to write tree")?) | |
| .context("Failed to find tree")?; | |
| let parent_commit = clone_repository | |
| .find_commit( | |
| branch | |
| .get() | |
| .target() | |
| .context("Failed to get branch target")?, | |
| ) | |
| .context("Failed to find parent commit")?; | |
| clone_repository | |
| .commit(Some("HEAD"), &user, &user, &commit_message, &tree, &[ | |
| &parent_commit, | |
| ]) | |
| .context("Failed to create commit")?; | |
| let refspec = format!("refs/heads/{}:refs/heads/{}", branch_name, branch_name); | |
| clone_repository | |
| .find_remote("origin") | |
| .context("Failed to find remote 'origin'")? | |
| .push(&[refspec.as_str()], Some(&mut git2::PushOptions::new())) | |
| .context("Failed to push changes to remote")?; | |
| Ok(()) | |
| } | |
| pub(crate) async fn add_file( | |
| &self, | |
| path: String, | |
| file_path: String, | |
| bytes: Vec<u8>, | |
| commit_email: String, | |
| commit_users: String, | |
| commit_message: String, | |
| file_name: String, | |
| branch_name: String, | |
| ) -> Result<()> { | |
| let path = self.root.join(&path); | |
| let repo_path = path.to_str().context("Repository path is not valid UTF-8")?; | |
| let tmp_dir = tempfile::tempdir().context("Failed to create temporary directory")?; | |
| let clone_repository = git2::Repository::clone(repo_path, tmp_dir.path()) | |
| .context("Failed to clone repository")?; | |
| let branch = match clone_repository.find_branch(&branch_name, git2::BranchType::Local) { | |
| Ok(branch) => branch, | |
| Err(_) => { | |
| let head_commit = clone_repository | |
| .head() | |
| .context("Failed to get HEAD")? | |
| .peel_to_commit() | |
| .context("Failed to peel HEAD to commit")?; | |
| clone_repository | |
| .branch(&branch_name, &head_commit, false) | |
| .context("Failed to create branch")?; | |
| clone_repository | |
| .find_branch(&branch_name, git2::BranchType::Local) | |
| .context("Failed to find branch after creation")? | |
| } | |
| }; | |
| let branch_name = branch | |
| .name() | |
| .transpose() | |
| .context("Failed to get branch name")? | |
| .map_err(|_| anyhow::anyhow!("Branch name is empty"))?; | |
| if !branch.is_head() { | |
| clone_repository | |
| .set_head(&branch_name) | |
| .context("Failed to set HEAD to branch")?; | |
| clone_repository | |
| .checkout_head(Some(git2::build::CheckoutBuilder::default().force())) | |
| .context("Failed to check out HEAD")?; | |
| } | |
| let full_file_path = tmp_dir.path().join(&file_path).join(&file_name); | |
| std::fs::create_dir_all( | |
| full_file_path | |
| .parent() | |
| .context("Failed to get parent directory")?, | |
| )?; | |
| std::fs::write(&full_file_path, bytes).context("Failed to write file")?; | |
| let relative_path = full_file_path | |
| .strip_prefix(tmp_dir.path()) | |
| .context("Failed to strip prefix from file path")?; | |
| let mut index = clone_repository | |
| .index() | |
| .context("Failed to get repository index")?; | |
| index | |
| .add_path(relative_path) | |
| .context("Failed to add file to index")?; | |
| index.write().context("Failed to write index")?; | |
| let time = chrono::Utc::now().timestamp(); | |
| let time = git2::Time::new(time, 0); | |
| let user = Signature::new(&commit_users, &commit_email, &time) | |
| .context("Failed to create commit signature")?; | |
| let tree = clone_repository | |
| .find_tree(index.write_tree().context("Failed to write tree")?) | |
| .context("Failed to find tree")?; | |
| let parent_commit = clone_repository | |
| .find_commit( | |
| branch | |
| .get() | |
| .target() | |
| .context("Failed to get branch target")?, | |
| ) | |
| .context("Failed to find parent commit")?; | |
| clone_repository | |
| .commit(Some("HEAD"), &user, &user, &commit_message, &tree, &[ | |
| &parent_commit, | |
| ]) | |
| .context("Failed to create commit")?; | |
| let refspec = format!("refs/heads/{}:refs/heads/{}", branch_name, branch_name); | |
| clone_repository | |
| .find_remote("origin") | |
| .context("Failed to find remote 'origin'")? | |
| .push(&[refspec.as_str()], Some(&mut git2::PushOptions::new())) | |
| .context("Failed to push changes to remote")?; | |
| Ok(()) | |
| } |
libs/rpc/core_git.rs
Outdated
| let fut = async move { | ||
| let method = CreateSvc(inner); | ||
| let codec = tonic::codec::ProstCodec::default(); | ||
| let mut grpc = tonic::server::Grpc::new(codec) | ||
| .apply_compression_config( | ||
| accept_compression_encodings, | ||
| send_compression_encodings, | ||
| ) | ||
| .apply_max_message_size_config( | ||
| max_decoding_message_size, | ||
| max_encoding_message_size, | ||
| ); | ||
| let res = grpc.unary(method, req).await; | ||
| Ok(res) | ||
| }; | ||
| Box::pin(fut) | ||
| } | ||
| "/core_git.RepRepository/AddFile" => { | ||
| #[allow(non_camel_case_types)] | ||
| struct AddFileSvc<T: RepRepository>(pub Arc<T>); | ||
| impl< | ||
| T: RepRepository, | ||
| > tonic::server::UnaryService<super::RepositoryAddFileRequest> | ||
| for AddFileSvc<T> { | ||
| type Response = super::RepositoryAddFilesResponse; | ||
| type Future = BoxFuture< | ||
| tonic::Response<Self::Response>, | ||
| tonic::Status, | ||
| >; | ||
| fn call( | ||
| &mut self, | ||
| request: tonic::Request<super::RepositoryAddFileRequest>, | ||
| ) -> Self::Future { | ||
| let inner = Arc::clone(&self.0); | ||
| let fut = async move { | ||
| <T as RepRepository>::add_file(&inner, request).await | ||
| }; | ||
| Box::pin(fut) | ||
| } | ||
| } | ||
| let accept_compression_encodings = self.accept_compression_encodings; | ||
| let send_compression_encodings = self.send_compression_encodings; | ||
| let max_decoding_message_size = self.max_decoding_message_size; | ||
| let max_encoding_message_size = self.max_encoding_message_size; | ||
| let inner = self.inner.clone(); | ||
| let fut = async move { | ||
| let method = AddFileSvc(inner); | ||
| let codec = tonic::codec::ProstCodec::default(); | ||
| let mut grpc = tonic::server::Grpc::new(codec) | ||
| .apply_compression_config( | ||
| accept_compression_encodings, | ||
| send_compression_encodings, | ||
| ) | ||
| .apply_max_message_size_config( | ||
| max_decoding_message_size, | ||
| max_encoding_message_size, | ||
| ); | ||
| let res = grpc.unary(method, req).await; | ||
| Ok(res) | ||
| }; | ||
| Box::pin(fut) | ||
| } | ||
| _ => { | ||
| Box::pin(async move { | ||
| let mut response = http::Response::new(empty_body()); | ||
| let headers = response.headers_mut(); | ||
| headers | ||
| .insert( | ||
| tonic::Status::GRPC_STATUS, | ||
| (tonic::Code::Unimplemented as i32).into(), | ||
| ); | ||
| headers | ||
| .insert( | ||
| http::header::CONTENT_TYPE, | ||
| tonic::metadata::GRPC_CONTENT_TYPE, | ||
| ); | ||
| Ok(response) | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| impl<T> Clone for RepRepositoryServer<T> { | ||
| fn clone(&self) -> Self { | ||
| let inner = self.inner.clone(); | ||
| Self { | ||
| inner, | ||
| accept_compression_encodings: self.accept_compression_encodings, | ||
| send_compression_encodings: self.send_compression_encodings, | ||
| max_decoding_message_size: self.max_decoding_message_size, | ||
| max_encoding_message_size: self.max_encoding_message_size, | ||
| } | ||
| } | ||
| } | ||
| /// Generated gRPC service name | ||
| pub const SERVICE_NAME: &str = "core_git.RepRepository"; | ||
| impl<T> tonic::server::NamedService for RepRepositoryServer<T> { | ||
| const NAME: &'static str = SERVICE_NAME; | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid modifying generated code
The file core_git.rs is auto-generated by prost-build and should not be modified manually. Changes made directly to this file will be overwritten the next time the code is generated from the .proto definitions.
If you need to make changes to the gRPC services or messages, please modify the corresponding .proto files and regenerate the code using prost-build.
| #[tokio::test] | ||
| async fn test_rpc() { | ||
| let rpc = CoreGitRpc::get().await; | ||
| dbg!(rpc); | ||
| } |
There was a problem hiding this comment.
Handle errors in test_rpc function
In the test_rpc test function, the result of CoreGitRpc::get().await is a Result that should be properly handled. Ignoring potential errors may cause the test to pass unintentionally.
Apply this diff to handle the Result effectively:
#[tokio::test]
async fn test_rpc() {
- let rpc = CoreGitRpc::get().await;
+ let rpc = CoreGitRpc::get().await.expect("Failed to get CoreGitRpc");
dbg!(rpc);
}This change ensures that if CoreGitRpc::get() fails, the test will fail with a meaningful message.
📝 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.
| #[tokio::test] | |
| async fn test_rpc() { | |
| let rpc = CoreGitRpc::get().await; | |
| dbg!(rpc); | |
| } | |
| #[tokio::test] | |
| async fn test_rpc() { | |
| let rpc = CoreGitRpc::get().await.expect("Failed to get CoreGitRpc"); | |
| dbg!(rpc); | |
| } |
proto/core-git.proto
Outdated
| message RepositoryAddFileRequest { | ||
| RepositoryStoragePosition RepositoryStoragePosition = 1; | ||
| string path = 2; | ||
| bytes content = 3; | ||
| string email = 4; | ||
| string user = 5; | ||
| string message = 6; | ||
| string file_name = 7; | ||
| string branch = 8; | ||
| } |
There was a problem hiding this comment.
Fix field naming convention in protobuf message.
The field name RepositoryStoragePosition doesn't follow protobuf naming conventions. Field names should be snake_case.
Apply this diff:
- RepositoryStoragePosition RepositoryStoragePosition = 1;
+ RepositoryStoragePosition repository_storage_position = 1;📝 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.
| message RepositoryAddFileRequest { | |
| RepositoryStoragePosition RepositoryStoragePosition = 1; | |
| string path = 2; | |
| bytes content = 3; | |
| string email = 4; | |
| string user = 5; | |
| string message = 6; | |
| string file_name = 7; | |
| string branch = 8; | |
| } | |
| message RepositoryAddFileRequest { | |
| RepositoryStoragePosition repository_storage_position = 1; | |
| string path = 2; | |
| bytes content = 3; | |
| string email = 4; | |
| string user = 5; | |
| string message = 6; | |
| string file_name = 7; | |
| string branch = 8; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
proto/core-git.proto (4)
9-15: Consider reserving missing field numbers for backward compatibilityIn
RepositoryStoragePosition, field number1is missing. If this field was previously used and now removed, consider reserving it to maintain backward compatibility.Apply this diff:
message RepositoryStoragePosition { + reserved 1; string path = 2; string node = 3; }
66-72: Address missing field numbers inRepositoryTreeField number
3is missing in theRepositoryTreemessage. To maintain proper field numbering, consider reserving field number3if it was intentionally removed.Apply this diff:
message RepositoryTree { + reserved 3; string hash = 1; string path = 2; string size = 4; string name = 5; string mode = 6; }
74-81: Address missing field numbers inRepositoryFileResponseField number
3is missing in theRepositoryFileResponsemessage. Consider reserving field number3to maintain field numbering consistency.Apply this diff:
message RepositoryFileResponse { + reserved 3; string hash = 1; string path = 2; string size = 4; string name = 5; string mode = 6; bytes content = 7; }
93-100: Ensure consistent field numbering inRepositoryTagsField number
6is missing in theRepositoryTagsmessage. If this was intentional, consider reserving it to maintain consistency.Apply this diff:
message RepositoryTags { string name = 1; string hash = 2; string date = 3; string author = 4; string email = 5; + reserved 6; }gitdata-gits/src/http/refs.rs (2)
47-53: Simplify storage retrieval and error handling.You can streamline the retrieval of
storageand reduce nesting by using theok_or_elsemethod with the?operator.Apply this diff to simplify the code:
-let storage = match storage.node.get(&path.clone().node) { - Some(storage) => storage, - None => { - return HttpResponse::NotFound().body("Not found"); - } -}; +let storage = storage.node.get(&path.clone().node) + .ok_or_else(|| HttpResponse::NotFound().body("Not found"))?;This refactor improves readability and leverages Rust's
Resulthandling.
Line range hint
42-46: Avoid unnecessary cloning and string allocations.The handling of the
versionvariable involves redundant cloning and allocation.Optimize the code by simplifying the
versionextraction:-let version = req - .headers() - .get("Git-Protocol") - .unwrap_or(&HeaderValue::from_str("").unwrap()) - .to_str() - .map(|s| s.to_string()) - .unwrap_or("".to_string()); +let version = req + .headers() + .get("Git-Protocol") + .and_then(|hv| hv.to_str().ok()) + .unwrap_or("");This refactor reduces unnecessary string allocations and improves performance.
gitdata-gits/src/http/text.rs (1)
28-31: Consider adding error logging for storage access failures.While the error handling is functional, logging the specific reason why storage access failed would help with debugging production issues.
let storage = match storage.node.get(&nodepath.node) { Some(storage) => storage, - None => return HttpResponse::NotFound().finish(), + None => { + log::error!("Storage not found for node: {}", nodepath.node); + return HttpResponse::NotFound().finish(); + }, };gitdata-gits/src/mount/mod.rs (2)
8-10: Add documentation for the StoragePool struct.The StoragePool struct is a key component but lacks documentation explaining its purpose and usage.
+/// StoragePool manages a collection of node-based storage instances. +/// Each node is identified by a unique name and provides storage operations +/// through the NodeStorage implementation. #[derive(Clone)] pub struct StoragePool { pub(crate) node : HashMap<String, NodeStorage>, }
3-5: Document the relationship between modules.The git module's purpose and its relationship to StoragePool should be documented.
+/// Git-specific storage implementation providing core repository operations. use crate::mount::git::NodeStorage; +/// Module containing Git repository storage implementation. pub mod git;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
gitdata-apis/src/service/core_git_rpc.rs(2 hunks)gitdata-apis/src/service/repository/new_repo.rs(2 hunks)gitdata-gits/src/http/pack.rs(1 hunks)gitdata-gits/src/http/refs.rs(1 hunks)gitdata-gits/src/http/text.rs(1 hunks)gitdata-gits/src/main.rs(2 hunks)gitdata-gits/src/mount/git.rs(1 hunks)gitdata-gits/src/mount/mod.rs(2 hunks)gitdata-gits/src/mount/nfs.rs(0 hunks)gitdata-gits/src/mount/s3.rs(0 hunks)gitdata-gits/src/rpc/core_git.rs(2 hunks)gitdata-gits/src/rpc/git_core.rs(1 hunks)gitdata-gits/src/rpc/mod.rs(0 hunks)libs/rpc/core_git.rs(1 hunks)libs/rpc/git_core.rs(1 hunks)proto/core-git.proto(1 hunks)proto/git-core.proto(0 hunks)
💤 Files with no reviewable changes (4)
- gitdata-gits/src/rpc/mod.rs
- proto/git-core.proto
- gitdata-gits/src/mount/nfs.rs
- gitdata-gits/src/mount/s3.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- gitdata-apis/src/service/core_git_rpc.rs
- gitdata-gits/src/main.rs
- gitdata-apis/src/service/repository/new_repo.rs
🔇 Additional comments (8)
libs/rpc/core_git.rs (1)
1-815: Avoid modifying generated codeThe file
core_git.rsis auto-generated byprost-buildand should not be modified manually. Any changes made directly to this file will be overwritten the next time the code is generated from the.protodefinitions.If you need to make changes to the gRPC services or messages, please modify the corresponding
.protofiles and regenerate the code usingprost-build.gitdata-gits/src/rpc/git_core.rs (1)
17-27: Functionpathupdated correctlyThe
pathmethod has been updated to returnio::Result<NodePath>, and the implementation aligns with the new return type. Error handling is appropriately managed.proto/core-git.proto (1)
17-17: Fix field naming convention in protobuf messageThe field name
RepositoryStoragePositiondoesn't follow protobuf naming conventions. Field names should be insnake_case.Apply this diff:
message RepositoryAddFileRequest { - RepositoryStoragePosition RepositoryStoragePosition = 1; + RepositoryStoragePosition repository_storage_position = 1; string path = 2; bytes content = 3; string email = 4; string user = 5; string message = 6; string file_name = 7; string branch = 8; }libs/rpc/git_core.rs (1)
1-639: Generated code looks good.This is an auto-generated file by prost-build. The Protobuf message definitions and gRPC service implementations are well-structured. If changes are needed:
- Modify the
.protofiles instead of this generated code- Re-run the code generation
Run the following script to locate the source
.protofile:✅ Verification successful
Verified: Modify proto/git-core.proto for changes
The generated code in
libs/rpc/git_core.rsis produced fromproto/git-core.proto. This is confirmed by:
- Exact match of package name, messages, and service definitions
- Build configuration in
build.rsshowing the compilation setup🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find the source .proto file that generates this code # Search for .proto files containing the service name rg -l "service RepRepository"Length of output: 73
Script:
#!/bin/bash # Show content of both proto files and look for build configuration echo "=== Content of proto/git-core.proto ===" cat proto/git-core.proto echo -e "\n=== Content of proto/core-git.proto ===" cat proto/core-git.proto echo -e "\n=== Looking for build.rs files ===" fd build.rsLength of output: 4144
Script:
#!/bin/bash # Check build.rs for proto compilation configuration cat build.rsLength of output: 702
gitdata-gits/src/mount/git.rs (2)
266-267: Use correct time zone offset in commit timestamps.The
git2::Time::newfunction expects the time zone offset in minutes, but zero is passed, implying UTC. Ensure this is intentional.Please confirm that using a zero offset is appropriate for your application's time zone requirements.
481-505:⚠️ Potential issueHandle potential panics due to unwrapping options.
In the
tagefunction, usingunwrap()on potentiallyNonevalues can cause panics.Replace
unwrap()with safe handling:- let tag = tag.unwrap(); + let tag = match tag { + Some(tag) => tag, + None => continue, + };Similarly, handle other
unwrap()calls safely to prevent runtime panics.Likely invalid or redundant comment.
gitdata-gits/src/http/pack.rs (2)
68-85:⚠️ Potential issueAvoid potential deadlocks by minimizing lock granularity.
Holding a read lock on
StoragePoolduring async operations may lead to deadlocks.Consider cloning or extracting necessary data outside the lock:
-let storage = storage.read().unwrap(); -let storage = match storage.node.get(&path.node) { +let storage_node = { + let storage = storage.read().unwrap(); + storage.node.get(&path.node).cloned() +}; let storage = match storage_node { Some(storage) => storage, None => return actix_web::HttpResponse::NotFound().finish(), };This approach reduces the lock's scope and avoids locking across await points.
Likely invalid or redundant comment.
68-71:⚠️ Potential issueEnsure thread safety when accessing shared
StoragePool.The
StoragePoolis accessed under a read lock, but the underlyingNodeStoragemay not be thread-safe.Verify that
NodeStorageimplementsSyncandSendtraits. If not, consider wrappingNodeStoragein a thread-safe container likeArc<Mutex<T>>.Run the following script to check for
SyncandSendimplementations:
| return Err(Status::invalid_argument("node not found")); | ||
| } | ||
| }; | ||
| match storage.tage(request.path.clone()).await { |
There was a problem hiding this comment.
Fix typo in method name.
The method name tage appears to be a typo and should be tags.
Apply this diff to fix the typo:
- match storage.tage(request.path.clone()).await {
+ match storage.tags(request.path.clone()).await {📝 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.
| match storage.tage(request.path.clone()).await { | |
| match storage.tags(request.path.clone()).await { |
| name : PathBuf::from(request.path) | ||
| .file_name() | ||
| .unwrap_or("".as_ref()) | ||
| .to_str() | ||
| .unwrap_or("") | ||
| .to_string(), | ||
| mode : "0".to_string(), |
There was a problem hiding this comment.
Handle potential panics in file name extraction.
The code uses unwrap() on file_name() and to_str() which could panic if the path is invalid or contains non-UTF8 characters.
Apply this diff to safely handle the error cases:
- name : PathBuf::from(request.path)
- .file_name()
- .unwrap_or("".as_ref())
- .to_str()
- .unwrap_or("")
- .to_string(),
+ name : PathBuf::from(request.path.clone())
+ .file_name()
+ .and_then(|n| n.to_str())
+ .unwrap_or("")
+ .to_string(),📝 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.
| name : PathBuf::from(request.path) | |
| .file_name() | |
| .unwrap_or("".as_ref()) | |
| .to_str() | |
| .unwrap_or("") | |
| .to_string(), | |
| mode : "0".to_string(), | |
| name : PathBuf::from(request.path.clone()) | |
| .file_name() | |
| .and_then(|n| n.to_str()) | |
| .unwrap_or("") | |
| .to_string(), | |
| mode : "0".to_string(), |
| let data = match storage | ||
| .refs(&*path.path.clone(), service, Some(&version)) | ||
| .await | ||
| { | ||
| Ok(data) => data, | ||
| Err(_) => { | ||
| return HttpResponse::NotFound().body("Not found"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide specific error responses based on failure reasons.
Currently, all errors from storage.refs() result in a NotFound response, which may not accurately reflect the error. Differentiating error types enhances client feedback and aids debugging.
Consider handling different error cases:
let data = match storage
.refs(&*path.path.clone(), service, Some(&version))
.await
{
- Ok(data) => data,
- Err(_) => {
- return HttpResponse::NotFound().body("Not found");
- }
+ Ok(data) => data,
+ Err(e) => {
+ match e.kind() {
+ io::ErrorKind::NotFound => {
+ return HttpResponse::NotFound().body("Repository not found");
+ },
+ _ => {
+ return HttpResponse::InternalServerError().body("Internal server error");
+ }
+ }
+ }
}This change provides more accurate HTTP responses based on the error encountered.
Committable suggestion skipped: line range outside the PR's diff.
| let bytes = if gzip { | ||
| let mut decoder = GzDecoder::new(Cursor::new(payload)); | ||
| let mut decoded_data = Vec::new(); | ||
| if let Err(e) = io::copy(&mut decoder, &mut decoded_data) { | ||
| return Err(Error::new( | ||
| io::ErrorKind::Other, | ||
| format!("Error running command {}", e), | ||
| )); | ||
| } | ||
| decoded_data | ||
| } else { | ||
| payload.to_vec() | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential decompression errors gracefully.
When decompressing the payload, errors from io::copy could leave decoded_data in an inconsistent state.
Modify the error handling to ensure decoded_data is properly managed:
let bytes = if gzip {
let mut decoder = GzDecoder::new(Cursor::new(payload));
let mut decoded_data = Vec::new();
- if let Err(e) = io::copy(&mut decoder, &mut decoded_data) {
+ match io::copy(&mut decoder, &mut decoded_data) {
+ Ok(_) => {},
+ Err(e) => {
return Err(Error::new(
io::ErrorKind::Other,
format!("Error decompressing payload: {}", e),
));
+ }
}
decoded_data
} else {
payload.to_vec()
};This change ensures that any decompression errors are properly reported.
📝 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.
| let bytes = if gzip { | |
| let mut decoder = GzDecoder::new(Cursor::new(payload)); | |
| let mut decoded_data = Vec::new(); | |
| if let Err(e) = io::copy(&mut decoder, &mut decoded_data) { | |
| return Err(Error::new( | |
| io::ErrorKind::Other, | |
| format!("Error running command {}", e), | |
| )); | |
| } | |
| decoded_data | |
| } else { | |
| payload.to_vec() | |
| }; | |
| let bytes = if gzip { | |
| let mut decoder = GzDecoder::new(Cursor::new(payload)); | |
| let mut decoded_data = Vec::new(); | |
| match io::copy(&mut decoder, &mut decoded_data) { | |
| Ok(_) => {}, | |
| Err(e) => { | |
| return Err(Error::new( | |
| io::ErrorKind::Other, | |
| format!("Error decompressing payload: {}", e), | |
| )); | |
| } | |
| } | |
| decoded_data | |
| } else { | |
| payload.to_vec() | |
| }; |
| if !version.is_some() { | ||
| cmd.env("GIT_PROTOCOL", version.unwrap_or("")); | ||
| } |
There was a problem hiding this comment.
Correct the logical condition for setting the GIT_PROTOCOL environment variable.
The condition if !version.is_some() should be if version.is_some() to ensure the environment variable is set when a version is provided.
Apply this diff to fix the condition:
- if !version.is_some() {
+ if version.is_some() {
cmd.env("GIT_PROTOCOL", version.unwrap_or(""));
}This correction ensures that the GIT_PROTOCOL is set appropriately.
Committable suggestion skipped: line range outside the PR's diff.
| ) -> io::Result<impl Stream<Item = Result<Bytes, Error>> + use<>> { | ||
| let mut cmd = Command::new("git"); | ||
| cmd.arg(service.to_string()); | ||
| // cmd.arg("receive-pack"); | ||
| cmd.arg("--stateless-rpc"); | ||
| cmd.arg("."); | ||
| if !version.is_some() { | ||
| cmd.env("GIT_PROTOCOL", version.unwrap_or("".to_string())); | ||
| } | ||
| cmd.stderr(Stdio::piped()); | ||
| cmd.stdin(Stdio::piped()); | ||
| cmd.stdout(Stdio::piped()); | ||
| cmd.current_dir(self.root.join(path)); | ||
| let mut git_process = match cmd.spawn() { | ||
| Ok(process) => process, | ||
| Err(e) => { | ||
| return Err(Error::new( | ||
| io::ErrorKind::Other, | ||
| format!("Error running command {}", e), | ||
| )); | ||
| } | ||
| }; | ||
| let mut stdin = git_process.stdin.take().unwrap(); | ||
| let mut stdout = git_process.stdout.take().unwrap(); | ||
| let bytes = if gzip { | ||
| let mut decoder = GzDecoder::new(Cursor::new(payload)); | ||
| let mut decoded_data = Vec::new(); | ||
| if let Err(e) = io::copy(&mut decoder, &mut decoded_data) { | ||
| return Err(Error::new( | ||
| io::ErrorKind::Other, | ||
| format!("Error running command {}", e), | ||
| )); | ||
| } | ||
| decoded_data | ||
| } else { | ||
| payload.to_vec() | ||
| }; | ||
| if let Err(e) = stdin.write_all(&bytes) { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::Other, | ||
| format!("Error running command {}", e), | ||
| )); | ||
| } | ||
| drop(stdin); | ||
| Ok(fn_stream(move |emitter| async move { | ||
| let mut buffer = [0; 8192]; | ||
| loop { | ||
| match stdout.read(&mut buffer) { | ||
| Ok(0) => break, | ||
| Ok(n) => { | ||
| emitter | ||
| .emit(Ok(Bytes::copy_from_slice(&buffer[0..n]))) | ||
| .await; | ||
| } | ||
| Err(e) => { | ||
| emitter.emit(Err(Error::new(io::ErrorKind::Other, e))).await; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| })) | ||
| } |
There was a problem hiding this comment.
Specify the return type correctly for the pack function.
The return type impl Stream<Item = Result<Bytes, Error>> + use<> is invalid syntax.
Update the return type to correctly specify the stream:
- ) -> io::Result<impl Stream<Item = Result<Bytes, Error>> + use<>> {
+ ) -> io::Result<impl Stream<Item = Result<Bytes, Error>>> {This fix corrects the syntax and allows the code to compile.
📝 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.
| ) -> io::Result<impl Stream<Item = Result<Bytes, Error>> + use<>> { | |
| let mut cmd = Command::new("git"); | |
| cmd.arg(service.to_string()); | |
| // cmd.arg("receive-pack"); | |
| cmd.arg("--stateless-rpc"); | |
| cmd.arg("."); | |
| if !version.is_some() { | |
| cmd.env("GIT_PROTOCOL", version.unwrap_or("".to_string())); | |
| } | |
| cmd.stderr(Stdio::piped()); | |
| cmd.stdin(Stdio::piped()); | |
| cmd.stdout(Stdio::piped()); | |
| cmd.current_dir(self.root.join(path)); | |
| let mut git_process = match cmd.spawn() { | |
| Ok(process) => process, | |
| Err(e) => { | |
| return Err(Error::new( | |
| io::ErrorKind::Other, | |
| format!("Error running command {}", e), | |
| )); | |
| } | |
| }; | |
| let mut stdin = git_process.stdin.take().unwrap(); | |
| let mut stdout = git_process.stdout.take().unwrap(); | |
| let bytes = if gzip { | |
| let mut decoder = GzDecoder::new(Cursor::new(payload)); | |
| let mut decoded_data = Vec::new(); | |
| if let Err(e) = io::copy(&mut decoder, &mut decoded_data) { | |
| return Err(Error::new( | |
| io::ErrorKind::Other, | |
| format!("Error running command {}", e), | |
| )); | |
| } | |
| decoded_data | |
| } else { | |
| payload.to_vec() | |
| }; | |
| if let Err(e) = stdin.write_all(&bytes) { | |
| return Err(io::Error::new( | |
| io::ErrorKind::Other, | |
| format!("Error running command {}", e), | |
| )); | |
| } | |
| drop(stdin); | |
| Ok(fn_stream(move |emitter| async move { | |
| let mut buffer = [0; 8192]; | |
| loop { | |
| match stdout.read(&mut buffer) { | |
| Ok(0) => break, | |
| Ok(n) => { | |
| emitter | |
| .emit(Ok(Bytes::copy_from_slice(&buffer[0..n]))) | |
| .await; | |
| } | |
| Err(e) => { | |
| emitter.emit(Err(Error::new(io::ErrorKind::Other, e))).await; | |
| break; | |
| } | |
| } | |
| } | |
| })) | |
| } | |
| ) -> io::Result<impl Stream<Item = Result<Bytes, Error>>> { | |
| let mut cmd = Command::new("git"); | |
| cmd.arg(service.to_string()); | |
| // cmd.arg("receive-pack"); | |
| cmd.arg("--stateless-rpc"); | |
| cmd.arg("."); | |
| if !version.is_some() { | |
| cmd.env("GIT_PROTOCOL", version.unwrap_or("".to_string())); | |
| } | |
| cmd.stderr(Stdio::piped()); | |
| cmd.stdin(Stdio::piped()); | |
| cmd.stdout(Stdio::piped()); | |
| cmd.current_dir(self.root.join(path)); | |
| let mut git_process = match cmd.spawn() { | |
| Ok(process) => process, | |
| Err(e) => { | |
| return Err(Error::new( | |
| io::ErrorKind::Other, | |
| format!("Error running command {}", e), | |
| )); | |
| } | |
| }; | |
| let mut stdin = git_process.stdin.take().unwrap(); | |
| let mut stdout = git_process.stdout.take().unwrap(); | |
| let bytes = if gzip { | |
| let mut decoder = GzDecoder::new(Cursor::new(payload)); | |
| let mut decoded_data = Vec::new(); | |
| if let Err(e) = io::copy(&mut decoder, &mut decoded_data) { | |
| return Err(Error::new( | |
| io::ErrorKind::Other, | |
| format!("Error running command {}", e), | |
| )); | |
| } | |
| decoded_data | |
| } else { | |
| payload.to_vec() | |
| }; | |
| if let Err(e) = stdin.write_all(&bytes) { | |
| return Err(io::Error::new( | |
| io::ErrorKind::Other, | |
| format!("Error running command {}", e), | |
| )); | |
| } | |
| drop(stdin); | |
| Ok(fn_stream(move |emitter| async move { | |
| let mut buffer = [0; 8192]; | |
| loop { | |
| match stdout.read(&mut buffer) { | |
| Ok(0) => break, | |
| Ok(n) => { | |
| emitter | |
| .emit(Ok(Bytes::copy_from_slice(&buffer[0..n]))) | |
| .await; | |
| } | |
| Err(e) => { | |
| emitter.emit(Err(Error::new(io::ErrorKind::Other, e))).await; | |
| break; | |
| } | |
| } | |
| } | |
| })) | |
| } |
| let result = match storage | ||
| .pack( | ||
| path.path.clone(), | ||
| service, | ||
| Some(version.to_string()), | ||
| gzip, | ||
| bytes, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(response) => response, | ||
| Err(_) => return actix_web::HttpResponse::NotFound().finish(), | ||
| }; | ||
| response.streaming(result) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors explicitly and provide meaningful responses.
The error handling in the pack function returns NotFound for all errors, which may not be accurate.
Improve error handling by distinguishing error types:
let result = match storage
.pack(
path.path.clone(),
service,
Some(version.to_string()),
gzip,
bytes,
)
.await
{
- Ok(response) => response,
- Err(_) => return actix_web::HttpResponse::NotFound().finish(),
+ Ok(response) => response,
+ Err(e) => {
+ // Log the error e
+ return actix_web::HttpResponse::InternalServerError()
+ .body(format!("Failed to pack repository: {}", e));
+ }
};This change provides clients with more accurate error information.
📝 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.
| let result = match storage | |
| .pack( | |
| path.path.clone(), | |
| service, | |
| Some(version.to_string()), | |
| gzip, | |
| bytes, | |
| ) | |
| .await | |
| { | |
| Ok(response) => response, | |
| Err(_) => return actix_web::HttpResponse::NotFound().finish(), | |
| }; | |
| response.streaming(result) | |
| let result = match storage | |
| .pack( | |
| path.path.clone(), | |
| service, | |
| Some(version.to_string()), | |
| gzip, | |
| bytes, | |
| ) | |
| .await | |
| { | |
| Ok(response) => response, | |
| Err(e) => { | |
| // Log the error e | |
| return actix_web::HttpResponse::InternalServerError() | |
| .body(format!("Failed to pack repository: {}", e)); | |
| } | |
| }; | |
| response.streaming(result) |
| let namedfile = match storage.text(&*nodepath.path, &*file_path).await { | ||
| Ok(namedfile) => namedfile, | ||
| Err(_) => return HttpResponse::NotFound().finish(), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Differentiate between error types in the response.
The current implementation returns NotFound for any text retrieval error. Consider distinguishing between different error types (e.g., NotFound vs InternalServerError) based on the actual error.
let namedfile = match storage.text(&*nodepath.path, &*file_path).await {
Ok(namedfile) => namedfile,
- Err(_) => return HttpResponse::NotFound().finish(),
+ Err(e) => {
+ log::error!("Failed to retrieve text: {}", e);
+ return match e {
+ // Add specific error type matches based on NodeStorage error types
+ _ => HttpResponse::InternalServerError().finish(),
+ };
+ },
};Committable suggestion skipped: line range outside the PR's diff.
| pub fn add_node(&mut self, name : String, storage : NodeStorage) { | ||
| self.node.insert(name, storage); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and documentation for add_node method.
The add_node method should validate the input parameters and include documentation.
+ /// Adds a new node storage to the pool.
+ ///
+ /// # Arguments
+ /// * `name` - Unique identifier for the storage node
+ /// * `storage` - The NodeStorage implementation
+ ///
+ /// # Returns
+ /// Returns the previously associated NodeStorage if the name already existed
pub fn add_node(&mut self, name : String, storage : NodeStorage) {
+ if name.is_empty() {
+ log::warn!("Attempted to add storage with empty name");
+ return;
+ }
self.node.insert(name, storage);
}📝 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.
| pub fn add_node(&mut self, name : String, storage : NodeStorage) { | |
| self.node.insert(name, storage); | |
| } | |
| /// Adds a new node storage to the pool. | |
| /// | |
| /// # Arguments | |
| /// * `name` - Unique identifier for the storage node | |
| /// * `storage` - The NodeStorage implementation | |
| /// | |
| /// # Returns | |
| /// Returns the previously associated NodeStorage if the name already existed | |
| pub fn add_node(&mut self, name : String, storage : NodeStorage) { | |
| if name.is_empty() { | |
| log::warn!("Attempted to add storage with empty name"); | |
| return; | |
| } | |
| self.node.insert(name, storage); | |
| } |
Summary by CodeRabbit
Release Notes
New Features
Database Improvements
Security Enhancements
Infrastructure
gitdata-health,gitdata-proxy.Bug Fixes