feat: implement File Store#328
Conversation
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #328 +/- ##
==========================================
- Coverage 91.45% 90.22% -1.23%
==========================================
Files 61 68 +7
Lines 2586 3305 +719
Branches 345 443 +98
==========================================
+ Hits 2365 2982 +617
- Misses 137 218 +81
- Partials 84 105 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR implements a File Store feature for the ORAS .NET library, providing file-system-based content storage that implements the ITarget interface for OCI artifact operations.
Changes:
- Adds a comprehensive File Store implementation with support for named file storage, tar/gzip directory archives, and fallback memory storage
- Implements five custom exceptions for file store operations
- Adds OCI standard annotations class and file store-specific annotations
- Includes tar/gzip utilities for directory handling
- Provides comprehensive test coverage with 2,239 lines of tests across 45+ test cases
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 63 comments.
Show a summary per file
| File | Description |
|---|---|
| src/OrasProject.Oras/Content/File/Store.cs | Core File Store implementation with ITarget, IPredecessorFindable, and disposal interfaces |
| src/OrasProject.Oras/Content/File/TarUtilities.cs | Utilities for creating and extracting tar.gz archives with checksums |
| src/OrasProject.Oras/Content/File/Annotations.cs | File store-specific annotation constants |
| src/OrasProject.Oras/Oci/Annotations.cs | OCI standard annotation key constants |
| src/OrasProject.Oras/Content/File/Exceptions/*.cs | Five custom exception classes for file store operations |
| tests/OrasProject.Oras.Tests/Content/File/StoreTest.cs | Comprehensive test suite with 45+ test cases covering all file store functionality |
| tests/OrasProject.Oras.Tests/Content/File/Exceptions/ExceptionTest.cs | Tests for all exception constructors and messages |
| tests/OrasProject.Oras.Tests/Registry/Remote/RepositoryTest.cs | Whitespace cleanup |
| src/OrasProject.Oras/Registry/Remote/BlobStore.cs | Whitespace cleanup |
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com> Update src/OrasProject.Oras/Content/File/Store.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com> Update tests/OrasProject.Oras.Tests/Content/File/StoreTest.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com> Update tests/OrasProject.Oras.Tests/Content/File/StoreTest.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com> Update src/OrasProject.Oras/Content/File/Store.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com> Update tests/OrasProject.Oras.Tests/Content/File/StoreTest.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com> Update tests/OrasProject.Oras.Tests/Content/File/StoreTest.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com> Update src/OrasProject.Oras/Content/File/TarUtilities.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
1ccaefa to
2ea33d7
Compare
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
Suggested PR Split & Design Doc Request@wangxiaoxuan273 — This PR is quite large (+3,988 lines across 13 files). To make review more tractable, we'd like to request two things: 1. Design DocBefore submitting code PRs, please create a brief design document covering:
2. Suggested Issue Breakdown (~10 issues, each ≤200-300 lines)Issues 1-4 can be worked on in parallel. The rest form a sequential chain.
Dependency graph: Additional notes:
|
shizhMSFT
left a comment
There was a problem hiding this comment.
Caution
This review is generated by GitHub Copilot using Claude Opus 4.6, mimicking shizhMSFT's review style. This is not the real shizhMSFT. Take the feedback with that context in mind.
Substantial effort here a full FileStore implementation with ~1200 lines of production code and ~2500 lines of tests. The overall structure aligns well with oras-go semantics.
That said, I have a few security and correctness concerns that should be addressed before merging.
| using var hash = IncrementalHash.CreateHash(HashAlgorithmName.SHA256); | ||
|
|
||
| // Read the entire decompressed content for hashing | ||
| var bufferedStream = new MemoryStream(); |
There was a problem hiding this comment.
The entire decompressed content is buffered into a MemoryStream when a checksum is present. There is no size limit.
This is vulnerable to a zip bomb attack a malicious gzip archive of a few KB can decompress to gigabytes and blow up the memory of the process.
Consider streaming through a HashingStream (you already have one in Store.cs) as a pass-through while extracting, rather than buffering the entire decompressed content. This way you can verify the checksum at the end without holding everything in memory.
| var computedDigest = $"sha256:{Convert.ToHexString(computedHash).ToLowerInvariant()}"; | ||
| if (!string.Equals(computedDigest, checksum, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| throw new InvalidOperationException($"Content digest mismatch: expected {checksum}, got {computedDigest}"); |
There was a problem hiding this comment.
InvalidOperationException is thrown here for a digest mismatch, but Store.cs uses MismatchedDigestException (from OrasProject.Oras.Content.Exceptions) for the same scenario in VerifyAndCopyAsync. Should be consistent use MismatchedDigestException here as well.
The repo copilot-instructions.md also says: "Prefer existing types in OrasProject.Oras.Exceptions."
| string targetDirectory, | ||
| string directoryName, | ||
| Stream tarStream, | ||
| bool preservePermissions, |
There was a problem hiding this comment.
preservePermissions is accepted as a parameter but never used inside ExtractTarDirectoryAsync. Extracted files are always created with default permissions.
This is dead code. Either implement it (apply entry.Mode via File.SetUnixFileMode on .NET 8+) or remove the parameter until the feature is ready. Shipping a parameter that silently does nothing is misleading to callers.
| { | ||
| var fileStream = System.IO.File.Create(fullPath); | ||
| await using var _fs = fileStream.ConfigureAwait(false); | ||
| await entry.DataStream.CopyToAsync(fileStream, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
There is no limit on entry.DataStream. A malicious tar archive could contain a single entry claiming to be a regular file with an extremely large size. The CopyToAsync call would write unbounded data to disk.
Consider checking entry.Length against a configurable max size before copying, or at minimum, counting bytes during copy and aborting if the total exceeds a reasonable threshold.
| } | ||
| break; | ||
|
|
||
| case TarEntryType.HardLink: |
There was a problem hiding this comment.
Hard links are silently dropped. This can cause data loss when extracting archives that rely on hard links.
The comment says ".NET 8 does not have File.CreateHardLink", but you can P/Invoke CreateHardLink on Windows or use link() on Unix. At minimum, fall back to copying the linked file content instead of silently skipping. If intentionally unsupported, consider throwing or logging a warning so the caller knows data was lost.
|
|
||
| // Check if the content exists in the store | ||
| if (_digestToPath.ContainsKey(target.Digest)) | ||
| { |
There was a problem hiding this comment.
ExistsAsync returns true if the digest is in _digestToPath, without checking whether the file still exists on disk. But FetchAsync (line 175) does check System.IO.File.Exists(path) and throws NotFoundException if the file is gone.
This inconsistency means ExistsAsync can return true and then an immediate FetchAsync throws. Should we add the same file-existence check here?
| /// <summary> | ||
| /// Creates a temporary file. | ||
| /// </summary> | ||
| private Task<(FileStream, string)> CreateTempFileAsync(CancellationToken cancellationToken) |
There was a problem hiding this comment.
nit: CreateTempFileAsync is not actually async it returns Task.FromResult(...). Consider making it a synchronous method (CreateTempFile) to avoid misleading callers and the unnecessary Task allocation.
|
|
||
| foreach (var tempPath in _tmpFiles.Keys) | ||
| { | ||
| try |
There was a problem hiding this comment.
nit: DisposeAsync just calls the synchronous Dispose(). If temp file cleanup could involve many files, consider making this truly async. At minimum, add GC.SuppressFinalize(this) in Dispose() per the standard dispose pattern.
| /// Tests pushing with duplicate name fails. | ||
| /// </summary> | ||
| [Fact] | ||
| public async Task FileStore_File_Push_DuplicateName() |
There was a problem hiding this comment.
The CopyAsync / CopyNodeAsync helpers reimplement content-graph traversal and copy logic manually. Should these tests be exercising the library own copy API (e.g., Extensions.CopyGraphAsync) instead? Reimplementing the logic in tests risks testing the test helper rather than the actual library behavior.
What this PR does / why we need it
Which issue(s) this PR resolves / fixes
Resolves / Fixes #<issue_id>
Please check the following list