feat: Add local filesystem cloud storage adapter#4525
feat: Add local filesystem cloud storage adapter#4525meylis1998 wants to merge 7 commits intoserverpod:mainfrom
Conversation
Adds a new `serverpod_cloud_storage_local` package that allows storing files directly on the server's local filesystem instead of using cloud storage providers like Amazon S3 or Google Cloud Storage. Features: - Store files on local filesystem with configurable storage path - Support for public and private storage configurations - Public URL generation for serving files through Serverpod's existing cloud storage endpoint - Path sanitization to prevent directory traversal attacks - File verification status tracking via metadata files Limitations: - Direct file uploads (createDirectFileUploadDescription) are not supported as they require database access for tracking upload entries. Users should use storeFile() through their endpoints instead. This is useful for: - Development and testing environments - Self-hosted deployments - Scenarios where external cloud dependencies are not desired Closes serverpod#1817
…storage - Add storeFileStream/retrieveFileStream for memory-efficient large file handling - Add expiration cleanup scheduler with startCleanupScheduler/stopCleanupScheduler - Add getFileSize method for file size queries without loading content - Improve path sanitization to prevent directory traversal attacks - Add comprehensive test suite (51 tests) - Update README with new features documentation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Hey @marcelomendoncasoares can you please review this PR |
|
Hi @meylis1998! Thanks for this contribution! We usually have CodeRabbit review community PRs before manual review. So I'll trigger it on this PR and ask you to please address its first comments. When done, ping @nielsenko that he is going to take the review process onward. |
|
@coderabbitai Full review. |
|
@marcelomendoncasoares: I'll perform a full review of this PR now. 🧠 Learnings used✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis pull request introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Session
participant LocalCloudStorage as LocalCloudStorage<br/>(Adapter)
participant Filesystem
participant Metadata as Metadata<br/>File (.meta)
Note over Session,Metadata: File Storage with Verification & Expiration
Session->>LocalCloudStorage: storeFile(path, byteData,<br/>expiration, verified)
LocalCloudStorage->>LocalCloudStorage: _getFullPath(path)<br/>[sanitize, prevent traversal]
LocalCloudStorage->>Filesystem: create parent directories
LocalCloudStorage->>Filesystem: write data file
alt has expiration or needs verification
LocalCloudStorage->>Metadata: write .meta file<br/>[verified, expiration]
end
Note over Session,Metadata: File Retrieval with Verification
Session->>LocalCloudStorage: retrieveFile(path)
LocalCloudStorage->>LocalCloudStorage: _getFullPath(path)
LocalCloudStorage->>Filesystem: check file exists
alt metadata file exists
LocalCloudStorage->>Metadata: read verification status
LocalCloudStorage->>LocalCloudStorage: _isFileVerified(metadata)
alt not verified
LocalCloudStorage-->>Session: null (unverified)
end
end
LocalCloudStorage->>Filesystem: read file content
LocalCloudStorage-->>Session: ByteData
sequenceDiagram
participant Scheduler
participant LocalCloudStorage as LocalCloudStorage<br/>(Adapter)
participant Filesystem
participant Metadata as Metadata Files
Note over Scheduler,Metadata: Expiration Cleanup Cycle
Scheduler->>LocalCloudStorage: cleanupExpiredFiles()
LocalCloudStorage->>Filesystem: scan storage directory
loop for each file
LocalCloudStorage->>Metadata: read .meta file
LocalCloudStorage->>LocalCloudStorage: parse expiration timestamp
alt file expired
LocalCloudStorage->>Filesystem: delete data file
LocalCloudStorage->>Metadata: delete .meta file
LocalCloudStorage->>LocalCloudStorage: increment cleanup count
end
end
LocalCloudStorage-->>Scheduler: count of cleaned files
rect rgba(100, 150, 200, 0.2)
Note over Scheduler,Metadata: Scheduler automatically repeats<br/>at configurable interval
end
sequenceDiagram
participant Session
participant LocalCloudStorage as LocalCloudStorage<br/>(Adapter)
participant Filesystem
participant Metadata as Metadata Files
Note over Session,Metadata: Stream-based File Storage
Session->>LocalCloudStorage: storeFileStream(path, stream,<br/>expectedLength, expiration)
LocalCloudStorage->>LocalCloudStorage: _getFullPath(path)
LocalCloudStorage->>Filesystem: create parent directories
LocalCloudStorage->>Filesystem: open file sink
loop for each chunk from stream
LocalCloudStorage->>Filesystem: write chunk to sink
end
LocalCloudStorage->>Filesystem: close file sink
alt has expiration or needs verification
LocalCloudStorage->>Metadata: write .meta file<br/>[verified, expiration]
end
Note over Session,Metadata: Stream-based File Retrieval
Session->>LocalCloudStorage: retrieveFileStream(path)
LocalCloudStorage->>LocalCloudStorage: _getFullPath(path)
LocalCloudStorage->>Filesystem: check file exists & verified
LocalCloudStorage->>Filesystem: open file for reading
LocalCloudStorage-->>Session: Stream<List<int>><br/>[chunked delivery]
loop client consumes chunks
Session->>Filesystem: read next chunk
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Pre-merge checks✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In
@integrations/serverpod_cloud_storage_local/lib/src/cloud_storage/local_cloud_storage.dart:
- Around line 77-104: Add unit tests that directly exercise
LocalCloudStorage._getFullPath to validate sanitization for edge cases: create a
LocalCloudStorage with a known storagePath, call _getFullPath with inputs
"foo/../../../../../../etc/passwd", "foo\\..\\..\\bar", "foo%2F..%2Fbar",
"foo\\0bar", and "foo:bar" and assert each returned path is inside the
configured storage directory (startsWith storagePath), contains no ".."
segments, uses platform separator (no mixed separators), and that encoded or
null-byte characters are handled/removed or mapped to a safe filename; also add
a test for an input that becomes empty (e.g., ".." or "" ) and assert it returns
storagePath joined with "file". Ensure test names reflect each case (e.g.,
returnsSafePath_forMultipleParentSegments, mixedSeparatorsSanitized,
encodedTraversalSanitized, specialCharsHandled, emptyBecomesFile).
In @integrations/serverpod_cloud_storage_local/pubspec.yaml:
- Line 13: Relax the exact serverpod dependency in pubspec.yaml by replacing the
fixed version "serverpod: 3.1.1" with a caret range so it follows semantic
versioning (e.g., change the serverpod entry to use ^3.1.1) to allow compatible
patch/minor updates while staying on the same major version.
🧹 Nitpick comments (2)
integrations/serverpod_cloud_storage_local/lib/src/cloud_storage/local_cloud_storage.dart (2)
68-75: Consider adding error handling and validation for storage directory initialization.The constructor creates the storage directory synchronously without error handling. If directory creation fails (e.g., due to permissions), or if
storagePathpoints to an existing file rather than a directory, subsequent operations will fail with unclear errors.💡 Suggested enhancement for robustness
}) : _serverpod = serverpod, super(storageId) { - // Ensure the storage directory exists - final dir = Directory(storagePath); - if (!dir.existsSync()) { - dir.createSync(recursive: true); - } + // Ensure the storage directory exists and is valid + final dir = Directory(storagePath); + + // Check if path exists as a file (not a directory) + if (File(storagePath).existsSync()) { + throw ArgumentError( + 'Storage path "$storagePath" exists as a file, not a directory', + ); + } + + // Create directory if it doesn't exist + if (!dir.existsSync()) { + try { + dir.createSync(recursive: true); + } catch (e) { + throw CloudStorageException( + 'Failed to create storage directory "$storagePath": $e', + ); + } + } }
361-406: Cleanup implementation is correct; consider scalability for large deployments.The method properly:
- Walks the storage directory recursively
- Identifies expired files via
.metatimestamps- Deletes both data and metadata files
- Handles errors gracefully by continuing with remaining files
For large storage volumes (millions of files), the recursive scan could be slow. Consider documenting this limitation or adding incremental cleanup in future iterations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
integrations/serverpod_cloud_storage_local/.gitignoreintegrations/serverpod_cloud_storage_local/CHANGELOG.mdintegrations/serverpod_cloud_storage_local/LICENSEintegrations/serverpod_cloud_storage_local/README.mdintegrations/serverpod_cloud_storage_local/analysis_options.yamlintegrations/serverpod_cloud_storage_local/lib/serverpod_cloud_storage_local.dartintegrations/serverpod_cloud_storage_local/lib/src/cloud_storage/local_cloud_storage.dartintegrations/serverpod_cloud_storage_local/pubspec.yamlintegrations/serverpod_cloud_storage_local/test/local_cloud_storage_test.dart
🧰 Additional context used
📓 Path-based instructions (4)
**/README*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never update README files unless explicitly instructed by the user
Files:
integrations/serverpod_cloud_storage_local/README.md
**/CHANGELOG*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never update CHANGELOG files unless explicitly instructed by the user; CHANGELOG files are manually updated during release process
Files:
integrations/serverpod_cloud_storage_local/CHANGELOG.md
**/*.dart
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.dart: Follow Dart formatting conventions with dart format command
Run 'dart format .' to format all code before committing
Run 'melos lint_strict' or 'util/run_tests_analyze' to check code quality before committing, ensuring compliance with CI strict mode
Files:
integrations/serverpod_cloud_storage_local/lib/serverpod_cloud_storage_local.dartintegrations/serverpod_cloud_storage_local/test/local_cloud_storage_test.dartintegrations/serverpod_cloud_storage_local/lib/src/cloud_storage/local_cloud_storage.dart
**/*test.dart
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*test.dart: When using group() with test(), ensure each statement ('Given', 'When', 'Then') appears only once in the combined description to avoid duplication
Place passing/success test cases at the top of test files before error test cases for the same functionality
Group related test cases using group() where appropriate
Files:
integrations/serverpod_cloud_storage_local/test/local_cloud_storage_test.dart
🧠 Learnings (21)
📓 Common learnings
Learnt from: nielsenko
Repo: serverpod/serverpod PR: 3625
File: packages/serverpod/lib/src/cloud_storage/public_endpoint.dart:82-93
Timestamp: 2025-07-03T09:16:08.642Z
Learning: In Serverpod's CloudStoragePublicEndpoint, the _readBinaryBody method must buffer the entire request body in memory because the storage.storeFile interface requires a ByteData object. Implementing true streaming uploads would require reworking Serverpod's serialization handling of binary arguments. The current implementation uses BytesBuilder for efficient byte accumulation instead of list concatenation.
Learnt from: tp
Repo: serverpod/serverpod PR: 3651
File: tests/serverpod_new_auth_test/serverpod_new_auth_test_server/test/integration/email_account_endpoint_test.dart:1-1
Timestamp: 2025-06-19T11:00:17.085Z
Learning: In Serverpod applications, methods that rely on public asset storage (like setUserImage/removeUserImage) should be tested in E2E tests rather than withServerpod integration tests, as they require a full server environment with asset storage infrastructure to properly test image upload/download functionality.
Learnt from: tp
Repo: serverpod/serverpod PR: 3747
File: tests/serverpod_new_auth_test/serverpod_new_auth_test_server/pubspec.yaml:37-38
Timestamp: 2025-07-02T16:09:27.702Z
Learning: In Serverpod migration testing, it's appropriate for test servers like serverpod_new_auth_test_server to include dependencies on both legacy packages (e.g., serverpod_auth_server) and new modular packages to enable comprehensive testing of migration paths from old to new authentication systems.
📚 Learning: 2025-06-17T11:34:13.289Z
Learnt from: tp
Repo: serverpod/serverpod PR: 3666
File: modules/new_serverpod_auth/serverpod_auth_migration/serverpod_auth_migration_server/README.md:3-8
Timestamp: 2025-06-17T11:34:13.289Z
Learning: The README.md files in the serverpod_auth_migration module directories are auto-generated and get overwritten by utility scripts, so manual updates should be avoided until proper documentation is added during feature rollout.
Applied to files:
integrations/serverpod_cloud_storage_local/LICENSEintegrations/serverpod_cloud_storage_local/README.md
📚 Learning: 2025-07-14T07:47:16.177Z
Learnt from: tp
Repo: serverpod/serverpod PR: 3783
File: modules/new_serverpod_auth/serverpod_auth_apple/serverpod_auth_apple_server/docker-compose.yaml:4-7
Timestamp: 2025-07-14T07:47:16.177Z
Learning: In Serverpod authentication modules, Docker compose files for different auth modules (like serverpod_auth_apple_server and serverpod_auth_apple_account_server) use the same ports (9090 for Postgres, 9091 for Redis) by design, since only one container/Docker compose setup is intended to run at a time.
Applied to files:
integrations/serverpod_cloud_storage_local/LICENSE
📚 Learning: 2025-08-25T19:46:58.073Z
Learnt from: tp
Repo: serverpod/serverpod PR: 3917
File: modules/new_serverpod_auth/serverpod_auth_idp/serverpod_auth_idp_server/analysis_options.yaml:3-6
Timestamp: 2025-08-25T19:46:58.073Z
Learning: In Serverpod, test tool paths like `test/integration/test_tools/serverpod_test_tools.dart` are configured per project in the "generator.yaml" file and managed by a generator system, so they don't need to be updated manually across all files at once.
Applied to files:
integrations/serverpod_cloud_storage_local/.gitignoreintegrations/serverpod_cloud_storage_local/analysis_options.yamlintegrations/serverpod_cloud_storage_local/pubspec.yamlintegrations/serverpod_cloud_storage_local/README.mdintegrations/serverpod_cloud_storage_local/lib/serverpod_cloud_storage_local.dartintegrations/serverpod_cloud_storage_local/test/local_cloud_storage_test.dart
📚 Learning: 2025-06-17T11:44:37.288Z
Learnt from: tp
Repo: serverpod/serverpod PR: 3666
File: modules/new_serverpod_auth/serverpod_auth_migration/serverpod_auth_migration_client/CHANGELOG.md:1-7
Timestamp: 2025-06-17T11:44:37.288Z
Learning: Changes to README.md and CHANGELOG.md files made by the util/update_pubspecs script can be safely ignored during code review, as these files are auto-generated by copying from a single source rather than being manually authored.
Applied to files:
integrations/serverpod_cloud_storage_local/.gitignore
📚 Learning: 2025-06-17T11:44:37.288Z
Learnt from: tp
Repo: serverpod/serverpod PR: 3666
File: modules/new_serverpod_auth/serverpod_auth_migration/serverpod_auth_migration_client/CHANGELOG.md:1-7
Timestamp: 2025-06-17T11:44:37.288Z
Learning: The util/update_pubspecs script automatically copies the root CHANGELOG.md to all packages/modules and README_subpackage.md to all modules as README.md. These file changes should be ignored during code review as they are auto-generated distribution of shared content, not manual authoring that requires review.
Applied to files:
integrations/serverpod_cloud_storage_local/.gitignore
📚 Learning: 2025-07-02T16:07:50.947Z
Learnt from: tp
Repo: serverpod/serverpod PR: 3747
File: modules/new_serverpod_auth/serverpod_auth_migration/serverpod_auth_migration_server/lib/src/business/migrating_email_endpoint_base.dart:4-4
Timestamp: 2025-07-02T16:07:50.947Z
Learning: In Serverpod, tp prefers importing the full package (e.g., 'package:serverpod_auth_migration_server/serverpod_auth_migration_server.dart') as a baseline rather than using smaller local imports, even within the same package where it might be considered a "self-import".
Applied to files:
integrations/serverpod_cloud_storage_local/.gitignoreintegrations/serverpod_cloud_storage_local/pubspec.yamlintegrations/serverpod_cloud_storage_local/README.mdintegrations/serverpod_cloud_storage_local/lib/serverpod_cloud_storage_local.dartintegrations/serverpod_cloud_storage_local/test/local_cloud_storage_test.dartintegrations/serverpod_cloud_storage_local/lib/src/cloud_storage/local_cloud_storage.dart
📚 Learning: 2025-06-27T08:39:36.440Z
Learnt from: tp
Repo: serverpod/serverpod PR: 3721
File: util/pub_get_all:0-0
Timestamp: 2025-06-27T08:39:36.440Z
Learning: In the Serverpod project, `dart pub get` commands work fine on Flutter packages both locally and in CI environments, even when the package has `flutter: sdk: flutter` dependencies in pubspec.yaml. The technical constraint of Flutter SDK dependencies causing `dart pub` failures may not apply in all environments or recent Dart versions.
Applied to files:
integrations/serverpod_cloud_storage_local/.gitignoreintegrations/serverpod_cloud_storage_local/pubspec.yamlintegrations/serverpod_cloud_storage_local/README.md
📚 Learning: 2025-07-01T13:01:27.054Z
Learnt from: exaby73
Repo: serverpod/serverpod PR: 3741
File: modules/serverpod_auth/serverpod_auth_server/analysis_options.yaml:14-16
Timestamp: 2025-07-01T13:01:27.054Z
Learning: In Serverpod, first-party packages like serverpod_auth_server can appropriately use blanket `invalid_use_of_internal_member: ignore` directives in their analysis_options.yaml since they use internal Database methods internally without exposing them in their public APIs to end users.
Applied to files:
integrations/serverpod_cloud_storage_local/.gitignoreintegrations/serverpod_cloud_storage_local/pubspec.yaml
📚 Learning: 2025-12-11T03:56:54.151Z
Learnt from: CR
Repo: serverpod/serverpod PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-11T03:56:54.151Z
Learning: Applies to **/*.dart : Run 'dart format .' to format all code before committing
Applied to files:
integrations/serverpod_cloud_storage_local/.gitignore
📚 Learning: 2025-12-11T03:56:54.151Z
Learnt from: CR
Repo: serverpod/serverpod PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-11T03:56:54.151Z
Learning: Applies to **/*.dart : Follow Dart formatting conventions with dart format command
Applied to files:
integrations/serverpod_cloud_storage_local/.gitignoreintegrations/serverpod_cloud_storage_local/analysis_options.yaml
📚 Learning: 2025-12-11T03:56:54.151Z
Learnt from: CR
Repo: serverpod/serverpod PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-11T03:56:54.151Z
Learning: Applies to **/*.dart : Run 'melos lint_strict' or 'util/run_tests_analyze' to check code quality before committing, ensuring compliance with CI strict mode
Applied to files:
integrations/serverpod_cloud_storage_local/analysis_options.yaml
📚 Learning: 2025-12-11T03:56:54.151Z
Learnt from: CR
Repo: serverpod/serverpod PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-11T03:56:54.151Z
Learning: Activate serverpod_cli from source using 'dart pub global activate --source path tools/serverpod_cli' after installation for development
Applied to files:
integrations/serverpod_cloud_storage_local/pubspec.yamlintegrations/serverpod_cloud_storage_local/lib/serverpod_cloud_storage_local.dart
📚 Learning: 2025-07-02T16:09:27.702Z
Learnt from: tp
Repo: serverpod/serverpod PR: 3747
File: tests/serverpod_new_auth_test/serverpod_new_auth_test_server/pubspec.yaml:37-38
Timestamp: 2025-07-02T16:09:27.702Z
Learning: In Serverpod migration testing, it's appropriate for test servers like serverpod_new_auth_test_server to include dependencies on both legacy packages (e.g., serverpod_auth_server) and new modular packages to enable comprehensive testing of migration paths from old to new authentication systems.
Applied to files:
integrations/serverpod_cloud_storage_local/pubspec.yaml
📚 Learning: 2025-11-03T15:40:41.827Z
Learnt from: SandPod
Repo: serverpod/serverpod PR: 4144
File: modules/new_serverpod_auth/serverpod_auth_idp/serverpod_auth_idp_server/lib/src/providers/passkey/endpoints/passkey_idp_base_endpoint.dart:0-0
Timestamp: 2025-11-03T15:40:41.827Z
Learning: In Serverpod authentication IDP endpoint files (modules/new_serverpod_auth/serverpod_auth_idp/serverpod_auth_idp_server), endpoint base classes should import `serverpod_auth_idp_server/core.dart` to access the Session type, rather than importing `package:serverpod_auth_core_server/session.dart` directly. This pattern is consistently used in Apple, Email, and Google endpoint files.
Applied to files:
integrations/serverpod_cloud_storage_local/pubspec.yaml
📚 Learning: 2025-07-03T09:16:08.642Z
Learnt from: nielsenko
Repo: serverpod/serverpod PR: 3625
File: packages/serverpod/lib/src/cloud_storage/public_endpoint.dart:82-93
Timestamp: 2025-07-03T09:16:08.642Z
Learning: In Serverpod's CloudStoragePublicEndpoint, the _readBinaryBody method must buffer the entire request body in memory because the storage.storeFile interface requires a ByteData object. Implementing true streaming uploads would require reworking Serverpod's serialization handling of binary arguments. The current implementation uses BytesBuilder for efficient byte accumulation instead of list concatenation.
Applied to files:
integrations/serverpod_cloud_storage_local/pubspec.yamlintegrations/serverpod_cloud_storage_local/README.mdintegrations/serverpod_cloud_storage_local/lib/serverpod_cloud_storage_local.dartintegrations/serverpod_cloud_storage_local/lib/src/cloud_storage/local_cloud_storage.dart
📚 Learning: 2025-06-19T11:00:17.085Z
Learnt from: tp
Repo: serverpod/serverpod PR: 3651
File: tests/serverpod_new_auth_test/serverpod_new_auth_test_server/test/integration/email_account_endpoint_test.dart:1-1
Timestamp: 2025-06-19T11:00:17.085Z
Learning: In Serverpod applications, methods that rely on public asset storage (like setUserImage/removeUserImage) should be tested in E2E tests rather than withServerpod integration tests, as they require a full server environment with asset storage infrastructure to properly test image upload/download functionality.
Applied to files:
integrations/serverpod_cloud_storage_local/pubspec.yamlintegrations/serverpod_cloud_storage_local/README.mdintegrations/serverpod_cloud_storage_local/test/local_cloud_storage_test.dart
📚 Learning: 2025-12-11T03:56:54.151Z
Learnt from: CR
Repo: serverpod/serverpod PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-11T03:56:54.151Z
Learning: Applies to **/*test.dart : Group related test cases using group() where appropriate
Applied to files:
integrations/serverpod_cloud_storage_local/test/local_cloud_storage_test.dart
📚 Learning: 2025-12-11T03:56:54.151Z
Learnt from: CR
Repo: serverpod/serverpod PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-11T03:56:54.151Z
Learning: Verify example projects still work before submitting PR by running example servers with 'dart bin/main.dart --apply-migrations'
Applied to files:
integrations/serverpod_cloud_storage_local/test/local_cloud_storage_test.dart
📚 Learning: 2025-12-11T03:56:54.151Z
Learnt from: CR
Repo: serverpod/serverpod PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-11T03:56:54.151Z
Learning: Applies to **/*test.dart : Place passing/success test cases at the top of test files before error test cases for the same functionality
Applied to files:
integrations/serverpod_cloud_storage_local/test/local_cloud_storage_test.dart
📚 Learning: 2025-06-20T07:18:29.965Z
Learnt from: nielsenko
Repo: serverpod/serverpod PR: 3695
File: packages/serverpod_client/lib/src/file_uploader.dart:57-63
Timestamp: 2025-06-20T07:18:29.965Z
Learning: MultipartFile uploads in the Serverpod FileUploader require knowing the content length due to the inherent nature of the multipart protocol. When length is unknown, falling back to `await stream.toBytes()` is necessary. The main streaming benefit of the FileUploader refactor is realized in binary uploads (`_UploadType.binary`) where `StreamedRequest` allows immediate streaming without reading the full content into memory first.
Applied to files:
integrations/serverpod_cloud_storage_local/lib/src/cloud_storage/local_cloud_storage.dart
🪛 GitHub Actions: Serverpod CI
integrations/serverpod_cloud_storage_local/pubspec.yaml
[warning] 1-1: Package validation found 4 warnings and 2 hints during publishing, including overly tight version constraints for serverpod-related dependencies.
[warning] 1-1: Flutter constraint in pubspec.yaml has an upper bound (^3.32.0); consider replacing with a lower-bound constraint (>=3.32.0) to avoid deprecation issues.
🔇 Additional comments (17)
integrations/serverpod_cloud_storage_local/lib/src/cloud_storage/local_cloud_storage.dart (14)
106-144: LGTM - File storage implementation is correct.The method properly:
- Sanitizes the path via
_getFullPath- Creates parent directories recursively
- Writes ByteData with correct buffer handling (offset and length)
- Stores metadata for expiration and verification tracking
- Wraps errors in
CloudStorageExceptionNote: No locking mechanism for concurrent writes to the same path. If multiple threads write simultaneously, the filesystem handles atomicity at the write level, but the last write wins. This is acceptable for typical cloud storage semantics.
146-170: LGTM - File retrieval correctly handles verification.The method appropriately returns
nullfor missing or unverified files, maintaining consistency with the CloudStorage contract.
196-214: LGTM - File existence check includes verification.Correctly returns
falsefor both missing and unverified files, consistent with the storage contract.
216-237: LGTM - File deletion handles both data and metadata.The method correctly removes both the file and its associated
.metafile, ensuring complete cleanup.
239-258: Direct uploads intentionally unsupported - well documented.Returning
nullcorrectly signals that direct upload descriptions cannot be created. The design rationale is clear: avoiding database dependencies keeps the implementation simple for development and self-hosted scenarios.As documented in the PR description, users should upload via
storeFile()through endpoints instead.
260-285: Verification mechanism works but has limited use case.The method marks files as verified by deleting their
.metafile (since_isFileVerifiedreturnstruewhen no metadata exists). Given thatcreateDirectFileUploadDescriptionreturnsnull(direct uploads unsupported), this method would primarily be useful for manually placed files with unverified metadata.The implementation is functionally correct.
287-305: LGTM - Metadata format is adequate for current needs.The simple
key=valueformat works well for the current metadata (boolean verified flag and ISO8601 expiration timestamp). More complex escaping isn't needed for these specific value types.
307-333: LGTM - Verification check has sensible defaults.The method correctly defaults to
true(verified) when:
- No metadata file exists
- Metadata parsing fails
This defensive behavior prevents valid files from becoming inaccessible due to metadata corruption.
335-359: LGTM - Cleanup scheduler lifecycle is well-managed.The implementation correctly:
- Prevents multiple concurrent timers via
stopCleanupScheduler()before starting- Provides manual control over scheduler lifecycle
- Uses a reasonable default interval (1 hour)
Note: The scheduler must be started explicitly by users; it's not automatic. This is documented in the method's example.
408-423: LGTM - Expiration parsing is robust.The method safely handles metadata parsing errors by returning
null, which prevents accidental deletion of files with corrupted metadata.
425-488: LGTM - Streaming upload is memory-efficient and properly implemented.The method correctly:
- Streams data directly to disk without loading into memory
- Creates parent directories as needed
- Ensures the IOSink is closed via
finallyblock- Stores metadata for expiration/verification tracking
The
expectedLengthparameter is unused but provides a useful API signature for future progress tracking or validation enhancements.Based on learnings, this addresses the limitation that Serverpod's public endpoints must buffer entire uploads (since
storeFilerequiresByteData). Users can create custom endpoints utilizingstoreFileStreamfor large file uploads.
490-533: LGTM - Streaming retrieval works correctly.The method properly checks verification before returning the file stream. The
chunkSizeparameter is currently unused (File.openRead() uses its own buffering), but provides a reasonable API signature for potential future tuning.
535-564: LGTM - File size retrieval is efficient and correct.The method efficiently gets file size via
file.length()without loading the file into memory, properly respecting verification status.
172-194: No changes needed—path configuration is correct.The path
/serverpod_cloud_storagematches Serverpod's cloud storage endpoint configuration. The endpoint is registered in Serverpod's server and the implementation correctly uses the same path with the appropriatemethod=filequery parameter, consistent with the defaultDatabaseCloudStorageimplementation and verified by existing tests.integrations/serverpod_cloud_storage_local/lib/serverpod_cloud_storage_local.dart (1)
1-37: Excellent library documentation and structure!The comprehensive doc comments effectively communicate the package's purpose, use cases, and provide a clear usage example. The structure follows Dart conventions properly.
integrations/serverpod_cloud_storage_local/README.md (1)
1-158: Comprehensive and well-structured documentation!The README effectively documents all key aspects: features, installation, usage patterns, configuration options, limitations, and security considerations. The examples are practical and the limitation regarding direct uploads is clearly communicated.
integrations/serverpod_cloud_storage_local/test/local_cloud_storage_test.dart (1)
15-1111: Excellent test coverage and organization!The test suite is comprehensive with 51 well-organized tests covering all critical functionality:
- Security (path sanitization and traversal protection)
- File operations (CRUD, nested directories, edge cases)
- Metadata handling (expiration, verification status)
- Concurrent operations (thread safety)
- Public URL generation
- Streaming support with proper edge case coverage
- Cleanup scheduler functionality
The tests follow coding guidelines: appropriate use of
group(), success cases before error cases, proper setUp/tearDown, and clear test descriptions. The helper functions for ByteData generation and verification ensure test reproducibility.
integrations/serverpod_cloud_storage_local/lib/src/cloud_storage/local_cloud_storage.dart
Show resolved
Hide resolved
- Relax serverpod version constraint to ^3.1.1 for semantic versioning - Add path sanitization edge case tests for security coverage: - Multiple consecutive parent directory segments - Mixed separator traversal attempts - URL encoded traversal attempts - Special characters handling - Empty path default filename fallback
|
I've addressed the CodeRabbit review feedback:
All 57 tests pass. @marcelomendoncasoares Ready for review! |
Change serverpod dependency from ^3.1.1 to 3.1.1 to match all other packages in the monorepo.
nielsenko
left a comment
There was a problem hiding this comment.
Could you elaborate a bit on flow regarding the .meta file?
| ## 3.1.1 | ||
|
|
||
| - Initial release of the local filesystem cloud storage adapter. | ||
| - Supports storing files on the local filesystem. | ||
| - Supports public and private storage configurations. | ||
| - Upload files through endpoints using storeFile() (direct uploads not supported). | ||
| - Includes path sanitization to prevent directory traversal attacks. | ||
| - Streaming support for memory-efficient large file handling. | ||
| - Automatic expiration cleanup scheduler. |
There was a problem hiding this comment.
CHANGELOG should be copied from main CHANGELOG during release processing. You should add you entries to the root changelog. I suggest rebasing on (or merging) from main first to avoid conflicts.
Make sure to update util/update_pubspecs to do the copying.
| @@ -0,0 +1,21 @@ | |||
| Copyright 2020 The Serverpod authors | |||
There was a problem hiding this comment.
Should the year be updated?
|  | ||
|
|
||
| # Serverpod Local Cloud Storage | ||
|
|
||
| A local filesystem storage adapter for Serverpod that allows you to store files directly on your server without requiring external cloud storage services like Amazon S3 or Google Cloud Storage. | ||
|
|
||
| ## Features | ||
|
|
There was a problem hiding this comment.
This entire file should be copied from README_subpackage.md
Make sure to update util/update_pubspec script to do so.
The extra information should be added to serverpod_docs repo. Maybe mention next to s3 and gcp storage in main repo (just a very short entry).
| while (sanitized.startsWith('/') || sanitized.startsWith('\\')) { | ||
| sanitized = sanitized.substring(1); | ||
| } |
There was a problem hiding this comment.
| while (sanitized.startsWith('/') || sanitized.startsWith('\\')) { | |
| sanitized = sanitized.substring(1); | |
| } | |
| final root = p.rootPrefix(sanitized); | |
| if (!root.isEmpty) sanitized = p.relative(sanitized, from: root); |
| // Remove any .. segments that might remain after normalization | ||
| final segments = sanitized.split(RegExp(r'[/\\]')); | ||
| final cleanSegments = segments.where((s) => s != '..' && s.isNotEmpty); | ||
| sanitized = cleanSegments.join(p.separator); |
There was a problem hiding this comment.
| // Remove any .. segments that might remain after normalization | |
| final segments = sanitized.split(RegExp(r'[/\\]')); | |
| final cleanSegments = segments.where((s) => s != '..' && s.isNotEmpty); | |
| sanitized = cleanSegments.join(p.separator); |
normalize already handled ..
| final fullPath = _getFullPath(path); | ||
| final file = File(fullPath); | ||
|
|
||
| if (file.existsSync()) { |
There was a problem hiding this comment.
| if (file.existsSync()) { | |
| if (await file.exists()) { |
|
|
||
| // Also delete metadata file if it exists | ||
| final metadataFile = File('$fullPath.meta'); | ||
| if (metadataFile.existsSync()) { |
There was a problem hiding this comment.
This repeats. Why not use async version when in an async function?
| final fullPath = _getFullPath(path); | ||
| final file = File(fullPath); | ||
|
|
||
| if (!file.existsSync()) { |
|
|
||
| // Remove any unverified metadata if present | ||
| final metadataFile = File('$fullPath.meta'); | ||
| if (metadataFile.existsSync()) { |
| final metadataFile = File('$fullPath.meta'); | ||
|
|
||
| // If no metadata file exists, the file is considered verified | ||
| if (!metadataFile.existsSync()) { |
There was a problem hiding this comment.
| if (!metadataFile.existsSync()) { | |
| if (!await metadataFile.exists()) { |
- Move CHANGELOG entries to root, update util/update_pubspecs - Use README_subpackage.md template - Refactor path sanitization using path package utilities - Use Uint8List.sublistView for ByteData conversion - Replace sync existsSync() with async exists()
|
might i recommend a better way for storing metadata in the case of |
Closes #1817
Summary
serverpod_cloud_storage_localpackage for storing files directly on the server filesystemFeatures
Limitations
createDirectFileUploadDescription) returnnullstoreFile()to upload files through endpoints insteadUsage
Test plan
dart test)Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.