-
Notifications
You must be signed in to change notification settings - Fork 520
Fix archive extraction to preserve directory structure when options is null #1191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5437d9f
4084b34
d5cab81
2a40813
29197f2
c4a28e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,9 @@ CancellationToken cancellationToken | |
| var bytesRead = 0L; | ||
| var seenDirectories = new HashSet<string>(); | ||
|
|
||
| // When extracting an entire archive, default to extracting with full paths | ||
| options ??= new ExtractionOptions { ExtractFullPath = true, Overwrite = true }; | ||
|
|
||
|
Comment on lines
+61
to
+63
|
||
| await foreach (var entry in archive.EntriesAsync.WithCancellation(cancellationToken)) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -718,6 +718,47 @@ await entry.WriteToDirectoryAsync( | |||||
| VerifyFiles(); | ||||||
| } | ||||||
|
|
||||||
| /// <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() | ||||||
| { | ||||||
|
Comment on lines
+721
to
+727
|
||||||
| var testFile = "Rar.issue1050.rar"; | ||||||
| using var fileStream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, testFile)); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Using
Suggested change
|
||||||
| await using var archive = RarArchive.OpenAsyncArchive(fileStream); | ||||||
|
|
||||||
| // Extract using archive.WriteToDirectoryAsync without explicit options | ||||||
| await archive.WriteToDirectoryAsync(SCRATCH_FILES_PATH); | ||||||
|
|
||||||
| // Verify files are in their subdirectories, not at the root | ||||||
| Assert.True( | ||||||
| File.Exists(Path.Combine(SCRATCH_FILES_PATH, "PhysicsBraid", "263825.tr11dtp")), | ||||||
| "File should be in PhysicsBraid subdirectory" | ||||||
| ); | ||||||
| Assert.True( | ||||||
| File.Exists(Path.Combine(SCRATCH_FILES_PATH, "Animations", "15441.tr11anim")), | ||||||
| "File should be in Animations subdirectory" | ||||||
| ); | ||||||
| Assert.True( | ||||||
| File.Exists(Path.Combine(SCRATCH_FILES_PATH, "Braid", "766728.tr11dtp")), | ||||||
| "File should be in Braid subdirectory" | ||||||
| ); | ||||||
| Assert.True( | ||||||
| File.Exists(Path.Combine(SCRATCH_FILES_PATH, "Braid", "766832.tr11dtp")), | ||||||
| "File should be in Braid subdirectory" | ||||||
| ); | ||||||
| Assert.True( | ||||||
| File.Exists(Path.Combine(SCRATCH_FILES_PATH, "HeadBraid", "321353.tr11modeldata")), | ||||||
| "File should be in HeadBraid subdirectory" | ||||||
| ); | ||||||
|
|
||||||
| // NOTE: The file size check is omitted because there's a separate pre-existing bug | ||||||
| // in the async RAR stream implementation that causes incorrect file sizes. | ||||||
| // This test only verifies the directory structure fix. | ||||||
| } | ||||||
|
|
||||||
| private async ValueTask ArchiveOpenStreamReadAsync( | ||||||
| ReaderOptions? readerOptions, | ||||||
| params string[] testArchives | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -722,6 +722,47 @@ public void Rar_StreamValidation_ThrowsOnTruncatedStream() | |||||
| Assert.Contains("unpacked file size does not match header", exception.Message); | ||||||
| } | ||||||
|
|
||||||
| /// <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() | ||||||
| { | ||||||
|
Comment on lines
+725
to
+731
|
||||||
| var testFile = "Rar.issue1050.rar"; | ||||||
| using var fileStream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, testFile)); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Using
Suggested change
|
||||||
| using var archive = RarArchive.OpenArchive(fileStream); | ||||||
|
|
||||||
| // Extract using archive.WriteToDirectory without explicit options | ||||||
| archive.WriteToDirectory(SCRATCH_FILES_PATH); | ||||||
|
|
||||||
| // Verify files are in their subdirectories, not at the root | ||||||
| Assert.True( | ||||||
| File.Exists(Path.Combine(SCRATCH_FILES_PATH, "PhysicsBraid", "263825.tr11dtp")), | ||||||
| "File should be in PhysicsBraid subdirectory" | ||||||
| ); | ||||||
| Assert.True( | ||||||
| File.Exists(Path.Combine(SCRATCH_FILES_PATH, "Animations", "15441.tr11anim")), | ||||||
| "File should be in Animations subdirectory" | ||||||
| ); | ||||||
| Assert.True( | ||||||
| File.Exists(Path.Combine(SCRATCH_FILES_PATH, "Braid", "766728.tr11dtp")), | ||||||
| "File should be in Braid subdirectory" | ||||||
| ); | ||||||
| Assert.True( | ||||||
| File.Exists(Path.Combine(SCRATCH_FILES_PATH, "Braid", "766832.tr11dtp")), | ||||||
| "File should be in Braid subdirectory" | ||||||
| ); | ||||||
| Assert.True( | ||||||
| File.Exists(Path.Combine(SCRATCH_FILES_PATH, "HeadBraid", "321353.tr11modeldata")), | ||||||
| "File should be in HeadBraid subdirectory" | ||||||
| ); | ||||||
|
|
||||||
| // 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 | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
| } | ||||||
|
Comment on lines
+761
to
+764
|
||||||
|
|
||||||
| /// <summary> | ||||||
| /// Test case for malformed RAR archives that previously caused infinite loops. | ||||||
| /// This test verifies that attempting to read entries from a potentially malformed | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new default
options ??= ...ExtractFullPath=true...is only applied inWriteToDirectoryInternal, which is skipped for solid archives and SevenZip (those go throughExtractAllEntries()+reader.WriteAllToDirectory(...)). As a result, callingarchive.WriteToDirectory(dest)without options can still flatten paths for solid/7z archives. Consider applying the same defaulting in the publicWriteToDirectory(...)method (before the solid/7z branch), so behavior is consistent across archive types.