Fix archive extraction to preserve directory structure when options is null#1191
Fix archive extraction to preserve directory structure when options is null#11916 commits merged intomasterfrom
Conversation
- Set default ExtractFullPath=true in WriteToDirectoryInternal methods - Add test case with sample RAR archive containing subdirectories - Tests verify files are extracted to correct subdirectories, not root Co-authored-by: adamhathcock <527620+adamhathcock@users.noreply.github.com>
Co-authored-by: adamhathcock <527620+adamhathcock@users.noreply.github.com>
Co-authored-by: adamhathcock <527620+adamhathcock@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes IArchive.WriteToDirectory() / IAsyncArchive.WriteToDirectoryAsync() default extraction behavior so omitting ExtractionOptions preserves the archive’s internal directory structure instead of flattening files into the destination root.
Changes:
- Default
ExtractionOptionstoExtractFullPath = true(andOverwrite = true) whenoptionsis null in sync/async archive extraction internals. - Add sync + async RAR regression tests ensuring files are extracted into subdirectories when no options are provided.
- Update
packages.lock.json(includesMicrosoft.NET.ILLink.Tasksversion changes).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/SharpCompress/Archives/IArchiveExtensions.cs |
Sets default extraction options when options is null during non-solid/non-7z archive extraction. |
src/SharpCompress/Archives/IAsyncArchiveExtensions.cs |
Async equivalent defaulting of extraction options during non-solid/non-7z extraction. |
tests/SharpCompress.Test/Rar/RarArchiveTests.cs |
Adds regression test for WriteToDirectory() path preservation (and a sync size assertion). |
tests/SharpCompress.Test/Rar/RarArchiveAsyncTests.cs |
Adds regression test for WriteToDirectoryAsync() path preservation. |
src/SharpCompress/packages.lock.json |
Lockfile changes affecting Microsoft.NET.ILLink.Tasks versions for net8.0/net10.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // When extracting an entire archive, default to extracting with full paths | ||
| options ??= new ExtractionOptions { ExtractFullPath = true, Overwrite = true }; | ||
|
|
There was a problem hiding this comment.
The new default options ??= ...ExtractFullPath=true... is only applied in WriteToDirectoryInternal, which is skipped for solid archives and SevenZip (those go through ExtractAllEntries() + reader.WriteAllToDirectory(...)). As a result, calling archive.WriteToDirectory(dest) without options can still flatten paths for solid/7z archives. Consider applying the same defaulting in the public WriteToDirectory(...) method (before the solid/7z branch), so behavior is consistent across archive types.
| // When extracting an entire archive, default to extracting with full paths | ||
| options ??= new ExtractionOptions { ExtractFullPath = true, Overwrite = true }; | ||
|
|
There was a problem hiding this comment.
Same as the sync extension: the default ExtractFullPath=true is only applied in WriteToDirectoryAsyncInternal, but solid archives and SevenZip use ExtractAllEntriesAsync() + reader.WriteAllToDirectoryAsync(...) and still receive options=null. To fully fix path flattening when callers omit options, apply the same defaulting in WriteToDirectoryAsync(...) before choosing the solid/7z path.
| /// <summary> | ||
| /// Tests for Issue #1050 - RAR extraction with WriteToDirectory creates folders | ||
| /// but places all files at the top level instead of in their subdirectories. | ||
| /// </summary> | ||
| [Fact] | ||
| public void Rar_Issue1050_WriteToDirectory_ExtractsToSubdirectories() | ||
| { |
There was a problem hiding this comment.
The PR metadata references sharpcompress#1190, but this new test’s XML doc + method name refer to “Issue #1050”. This makes it hard to trace why the test exists. Update the issue number (and potentially the test name/archive filename) to match the actual tracked issue for this change.
| // Verify the exact file size of 766832.tr11dtp matches the archive entry size | ||
| var fileInfo = new FileInfo(Path.Combine(SCRATCH_FILES_PATH, "Braid", "766832.tr11dtp")); | ||
| Assert.Equal(4867620, fileInfo.Length); // Expected: 4,867,620 bytes | ||
| } |
There was a problem hiding this comment.
The comment says the extracted file size is being validated against the archive entry size, but the test currently asserts a hard-coded byte count. This is brittle if the test archive is ever regenerated. Prefer reading the expected size from the corresponding archive entry (or otherwise deriving it from the test data) and asserting equality against that value.
| /// <summary> | ||
| /// Tests for Issue #1050 - RAR extraction with WriteToDirectoryAsync creates folders | ||
| /// but places all files at the top level instead of in their subdirectories. | ||
| /// </summary> | ||
| [Fact] | ||
| public async ValueTask Rar_Issue1050_WriteToDirectoryAsync_ExtractsToSubdirectories() | ||
| { |
There was a problem hiding this comment.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| public void Rar_Issue1050_WriteToDirectory_ExtractsToSubdirectories() | ||
| { | ||
| var testFile = "Rar.issue1050.rar"; | ||
| using var fileStream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, testFile)); |
There was a problem hiding this comment.
WARNING: Using File.OpenRead() instead of File.Open(..., FileMode.Open) avoids the unnecessary read/write permissions. The current implementation opens the file with read/write access which is unnecessary for reading an archive.
| using var fileStream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, testFile)); | |
| using var fileStream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, testFile)); |
| public async ValueTask Rar_Issue1050_WriteToDirectoryAsync_ExtractsToSubdirectories() | ||
| { | ||
| var testFile = "Rar.issue1050.rar"; | ||
| using var fileStream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, testFile)); |
There was a problem hiding this comment.
WARNING: Using File.OpenRead() instead of File.Open(..., FileMode.Open) avoids the unnecessary read/write permissions. The current implementation opens the file with read/write access which is unnecessary for reading an archive.
| using var fileStream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, testFile)); | |
| using var fileStream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, testFile)); |
|
|
||
| // Verify the exact file size of 766832.tr11dtp matches the archive entry size | ||
| var fileInfo = new FileInfo(Path.Combine(SCRATCH_FILES_PATH, "Braid", "766832.tr11dtp")); | ||
| Assert.Equal(4867620, fileInfo.Length); // Expected: 4,867,620 bytes |
There was a problem hiding this comment.
SUGGESTION: The comment says the extracted file size is being validated, but the assertion doesn't match the comment's stated purpose. The comment says it validates "exact file size...matches the archive entry size" but actually hardcodes 4867620. Consider also verifying against entry.Size to ensure the test validates what the comment describes.
| /// </summary> | ||
| [Fact] | ||
| public async ValueTask Rar_Issue1050_WriteToDirectoryAsync_ExtractsToSubdirectories() | ||
| { |
There was a problem hiding this comment.
SUGGESTION: The PR metadata references sharpcompress#1190, but this new test references Issue #1050. Consider clarifying in the test comment whether this is testing the same issue or a different one, as the mismatch may confuse future maintainers.
| /// </summary> | ||
| [Fact] | ||
| public void Rar_Issue1050_WriteToDirectory_ExtractsToSubdirectories() | ||
| { |
There was a problem hiding this comment.
SUGGESTION: The PR metadata references sharpcompress#1190, but this new test references Issue #1050. Consider clarifying in the test comment whether this is testing the same issue or a different one, as the mismatch may confuse future maintainers.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
The fix only applies to Files Reviewed (6 files)
|
WriteToDirectory()andWriteToDirectoryAsync()flatten directory structures when called without explicit options, extracting all files to the root instead of their subdirectories.Changes
ExtractFullPath = trueby default inWriteToDirectoryInternalandWriteToDirectoryAsyncInternalExample
Note
The async file size issue mentioned in the original report is a separate bug in RAR async stream reading, not the extraction logic. Sync extraction produces correct sizes.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.