feat: add FileStore exception types#353
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #353 +/- ##
==========================================
+ Coverage 91.76% 91.90% +0.14%
==========================================
Files 64 69 +5
Lines 2755 2803 +48
Branches 364 364
==========================================
+ Hits 2528 2576 +48
Misses 138 138
Partials 89 89 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces 5 new exception types specifically for FileStore operations as part of the larger File Store implementation effort (issue 3 of 10, related to PR #328). The exceptions cover various error scenarios: duplicate names, closed store operations, missing names, disallowed overwrites, and path traversal attempts. Each exception includes comprehensive constructor tests (25 test methods total) covering all constructor variations.
Changes:
- Added 5 new exception types for FileStore operations with appropriate base classes (IOException, ArgumentException, InvalidOperationException)
- Implemented comprehensive constructor tests validating default messages, custom messages, null handling, and inner exceptions
- Used a default message pattern with null-coalescing operators in all exception constructors
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/OrasProject.Oras/Content/File/Exceptions/DuplicateNameException.cs | New IOException for when a name already exists in FileStore; uses default message pattern |
| src/OrasProject.Oras/Content/File/Exceptions/FileStoreClosedException.cs | New InvalidOperationException for operations on closed FileStore; uses default message pattern |
| src/OrasProject.Oras/Content/File/Exceptions/MissingNameException.cs | New ArgumentException for missing required names; uses default message pattern |
| src/OrasProject.Oras/Content/File/Exceptions/OverwriteDisallowedException.cs | New IOException for disallowed file overwrites; uses default message pattern |
| src/OrasProject.Oras/Content/File/Exceptions/PathTraversalDisallowedException.cs | New IOException for disallowed path traversal attempts; uses default message pattern |
| tests/OrasProject.Oras.Tests/Content/File/Exceptions/ExceptionTest.cs | Comprehensive tests for all 5 exception constructors with message and inner exception validation |
Remove _defaultMessage constant and null-coalescing pattern from all FileStore exceptions to match existing codebase convention (e.g., NotFoundException). Simplify tests to use Assert.ThrowsAsync pattern matching ExceptionTest.cs. Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
6fddf4c to
29da813
Compare
…llowedException.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
…ption.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
…ception.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
…Exception.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
…Test.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
akashsinghal
left a comment
akashsinghal
left a comment
There was a problem hiding this comment.
Caution
This review was generated by Copilot using Claude Opus 4.6, mimicking the review style of shizhMSFT. This is not the real shizhMSFT.
Clean exception type definitions, consistent with the established pattern. A few observations below.
- Change PathTraversalDisallowedException base class from IOException to UnauthorizedAccessException (security boundary violation) - Add paramName constructor to MissingNameException to follow the ArgumentException contract - Replace async Assert.ThrowsAsync with direct instantiation and property verification in all exception constructor tests (no async operations involved) - Apply same test pattern fix to Content, Exceptions, Registry, and Registry/Remote exception tests for consistency Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
You can also share your feedback on Copilot code review. Take the survey.
|
Caution This review was generated by Copilot using Claude Opus 4.6, mimicking the review style of shizhMSFT. This is not the real shizhMSFT. Second-round review of PR #353 All 19 prior threads addressed — Two observations: 1. Scope creep into existing exception tests (4 additional test files modified) This PR originally adds 5 FileStore exception types, but now also rewrites all existing exception tests across 2. nit: The class declaration splits the base type onto a separate line: Otherwise SGTM — all prior feedback addressed cleanly. |
akashsinghal
left a comment
akashsinghal
left a comment
There was a problem hiding this comment.
LGTM. there is one small non blocking comment on line breaks
What this PR does / why we need it
Adds 5 exception types for FileStore operations:
DuplicateFileNameException— thrown when a name already exists in the FileStoreFileStoreClosedException— thrown when an operation is attempted on a closed FileStoreMissingNameException— thrown when a name is required but not providedOverwriteDisallowedException— thrown when overwriting is not allowedPathTraversalDisallowedException— thrown when a path traversal attempt is detectedWhich issue(s) this PR resolves / fixes
Part of the File Store implementation split (issue 3 of 10).
Related: #328, #37
Please check the following list