feat: az-blob-storage#66
Conversation
…zurite Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 25 minutes and 41 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an Azure Blob Storage upload provider ( ChangesAzure Blob Storage Provider
Backup Upload Failure Notification
Sequence Diagram(s)sequenceDiagram
participant BackupService
participant AzureBlobProvider
participant helpers as upload_stream_to_azure
participant AzureBlob as Azure Blob Service
participant Server as Portabase Server
BackupService->>AzureBlobProvider: upload(result, storage, encrypt)
AzureBlobProvider->>AzureBlobProvider: resolve AzureBlobProviderConfig → ResolvedAzure
AzureBlobProvider->>helpers: upload_stream_to_azure(resolved, container, blob, stream)
loop stage BLOCK_SIZE chunks
helpers->>AzureBlob: PUT block (SAS-authenticated)
AzureBlob-->>helpers: 201 Created
end
helpers->>AzureBlob: commit_block_list
AzureBlob-->>helpers: 201 Created
helpers-->>AzureBlobProvider: Ok(())
AzureBlobProvider-->>BackupService: UploadResult { success: true }
alt upload fails
AzureBlobProvider-->>BackupService: UploadResult { success: false }
BackupService->>Server: PATCH backup_upload_status (status: failed, path: "", size: 0)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/tests/services/backup_uploader_tests.rs (1)
49-53: ⚡ Quick winAssert the full failed-status payload contract
Line 51 only validates
status. Please also assertpathandsizeso this regression test protects the exact failure payload behavior.Proposed test matcher hardening
Mock::given(method("PATCH")) .and(path("/agent/agent-1/backup/upload/status")) - .and(body_partial_json(json!({ "status": "failed" }))) + .and(body_partial_json(json!({ + "status": "failed", + "path": "", + "size": 0 + }))) .respond_with(ResponseTemplate::new(200).set_body_json(json!({})))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/services/backup_uploader_tests.rs` around lines 49 - 53, The mock assertion using body_partial_json in the test only validates the status field in the request payload. Expand the json matcher to also assert the path and size fields are present and correct in the failed-status payload. This ensures the regression test fully protects the contract of the failure payload by validating all required fields that are sent when a backup upload fails, not just the status field.src/services/storage/providers/azure_blob/mod.rs (1)
70-71: ⚡ Quick winUnnecessary clone of entire
storagestruct.
storage.clone().config.try_into()clones the entireDatabaseStoragejust to accessconfig. Consider cloning only the config field.♻️ Proposed fix
- let config: AzureBlobProviderConfig = match storage.clone().config.try_into() { + let config: AzureBlobProviderConfig = match storage.config.clone().try_into() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/storage/providers/azure_blob/mod.rs` around lines 70 - 71, In the AzureBlobProviderConfig initialization, the code is cloning the entire `storage` struct and then accessing its `config` field unnecessarily. Instead of calling `storage.clone().config.try_into()`, change it to clone only the config field by calling `storage.config.clone().try_into()`. This avoids the expensive clone of the entire DatabaseStorage struct and only clones the specific config field that is needed for the try_into() conversion.src/services/storage/providers/azure_blob/helpers.rs (2)
127-127: 💤 Low valuePotential unnecessary allocation with
block.to_vec().
blockis alreadyBytes, and.to_vec()creates a copy. IfRequestContent::from()acceptsBytesorVec<u8>by move, you could avoid the copy by usingblock.into()or checking if there's a more direct conversion path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/storage/providers/azure_blob/helpers.rs` at line 127, The `stage_block` method call is creating an unnecessary copy of data by calling `block.to_vec()` on a `Bytes` value. Since `block` is already `Bytes`, replace the `.to_vec()` call with a direct conversion like `.into()` or check the signature of `RequestContent::from()` to see if it accepts `Bytes` directly without requiring a vector conversion. This will avoid the allocation overhead and let the `Bytes` value be moved into `RequestContent` more efficiently.
59-62: 💤 Low valueConsider making SAS expiry and protocol configurable.
The SAS token expires in 1 hour (line 60-61), which could be insufficient for very large uploads over slow connections. Additionally,
signed_protocol = "https,http"(line 62) allows HTTP for Azurite compatibility, but this weakens security in production.Consider making these configurable or environment-dependent, or at minimum document the 1-hour upload window constraint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/storage/providers/azure_blob/helpers.rs` around lines 59 - 62, The hardcoded SAS token expiry duration (1 hour in the signed_expiry calculation) and the signed_protocol value ("https,http") should be made configurable or environment-dependent. Extract these values from the function body into configuration parameters that can be set based on the deployment environment (production should only allow "https" while local/Azurite environments can allow both "https,http"), and ensure the expiry duration is adjustable to accommodate large uploads over slow connections. At minimum, document the 1-hour constraint if these remain hardcoded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/storage/providers/azure_blob/models.rs`:
- Around line 5-14: The AzureBlobProviderConfig struct currently derives
Serialize which poses a security risk as the account_key field containing
sensitive credentials could be accidentally exposed during serialization in logs
or error messages. Remove the Serialize derive from the struct definition, or
alternatively keep Serialize but add the #[serde(skip_serializing)] attribute
specifically to the account_key field to prevent that sensitive field from being
serialized while keeping the rest of the struct serializable.
---
Nitpick comments:
In `@src/services/storage/providers/azure_blob/helpers.rs`:
- Line 127: The `stage_block` method call is creating an unnecessary copy of
data by calling `block.to_vec()` on a `Bytes` value. Since `block` is already
`Bytes`, replace the `.to_vec()` call with a direct conversion like `.into()` or
check the signature of `RequestContent::from()` to see if it accepts `Bytes`
directly without requiring a vector conversion. This will avoid the allocation
overhead and let the `Bytes` value be moved into `RequestContent` more
efficiently.
- Around line 59-62: The hardcoded SAS token expiry duration (1 hour in the
signed_expiry calculation) and the signed_protocol value ("https,http") should
be made configurable or environment-dependent. Extract these values from the
function body into configuration parameters that can be set based on the
deployment environment (production should only allow "https" while local/Azurite
environments can allow both "https,http"), and ensure the expiry duration is
adjustable to accommodate large uploads over slow connections. At minimum,
document the 1-hour constraint if these remain hardcoded.
In `@src/services/storage/providers/azure_blob/mod.rs`:
- Around line 70-71: In the AzureBlobProviderConfig initialization, the code is
cloning the entire `storage` struct and then accessing its `config` field
unnecessarily. Instead of calling `storage.clone().config.try_into()`, change it
to clone only the config field by calling `storage.config.clone().try_into()`.
This avoids the expensive clone of the entire DatabaseStorage struct and only
clones the specific config field that is needed for the try_into() conversion.
In `@src/tests/services/backup_uploader_tests.rs`:
- Around line 49-53: The mock assertion using body_partial_json in the test only
validates the status field in the request payload. Expand the json matcher to
also assert the path and size fields are present and correct in the
failed-status payload. This ensures the regression test fully protects the
contract of the failure payload by validating all required fields that are sent
when a backup upload fails, not just the status field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e0a39e8-0967-47bc-b193-e7ec36828605
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlsrc/services/backup/uploader.rssrc/services/mod.rssrc/services/storage/mod.rssrc/services/storage/providers/azure_blob/helpers.rssrc/services/storage/providers/azure_blob/mod.rssrc/services/storage/providers/azure_blob/models.rssrc/services/storage/providers/mod.rssrc/tests/mod.rssrc/tests/services/backup_uploader_tests.rssrc/tests/services/mod.rssrc/tests/storage/azure_blob.rssrc/tests/storage/mod.rs
| #[derive(Debug, Deserialize, Serialize)] | ||
| pub struct AzureBlobProviderConfig { | ||
| pub account_name: String, | ||
| pub account_key: String, | ||
| pub container_name: String, | ||
| #[serde(default)] | ||
| pub connection_string: String, | ||
| #[serde(default)] | ||
| pub endpoint_url: Option<String>, | ||
| } |
There was a problem hiding this comment.
Avoid deriving Serialize on config containing secrets.
AzureBlobProviderConfig contains account_key, and deriving Serialize could lead to accidental exposure in logs, debug output, or error messages if the struct is ever serialized. Consider removing Serialize or using #[serde(skip_serializing)] on account_key.
🛡️ Proposed fix
#[derive(Debug, Deserialize, Serialize)]
pub struct AzureBlobProviderConfig {
pub account_name: String,
+ #[serde(skip_serializing)]
pub account_key: String,
pub container_name: String,
#[serde(default)]
+ #[serde(skip_serializing)]
pub connection_string: 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.
| #[derive(Debug, Deserialize, Serialize)] | |
| pub struct AzureBlobProviderConfig { | |
| pub account_name: String, | |
| pub account_key: String, | |
| pub container_name: String, | |
| #[serde(default)] | |
| pub connection_string: String, | |
| #[serde(default)] | |
| pub endpoint_url: Option<String>, | |
| } | |
| #[derive(Debug, Deserialize, Serialize)] | |
| pub struct AzureBlobProviderConfig { | |
| pub account_name: String, | |
| #[serde(skip_serializing)] | |
| pub account_key: String, | |
| pub container_name: String, | |
| #[serde(default)] | |
| #[serde(skip_serializing)] | |
| pub connection_string: String, | |
| #[serde(default)] | |
| pub endpoint_url: Option<String>, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/storage/providers/azure_blob/models.rs` around lines 5 - 14, The
AzureBlobProviderConfig struct currently derives Serialize which poses a
security risk as the account_key field containing sensitive credentials could be
accidentally exposed during serialization in logs or error messages. Remove the
Serialize derive from the struct definition, or alternatively keep Serialize but
add the #[serde(skip_serializing)] attribute specifically to the account_key
field to prevent that sensitive field from being serialized while keeping the
rest of the struct serializable.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes