Skip to content
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

ZipFile.ExtractToDirectory doc comments wrongly claim that the directory must not exist #95501

Closed
cn-byn opened this issue Dec 1, 2023 · 6 comments
Labels
area-System.IO.Compression documentation Documentation bug or enhancement, does not impact product or test code good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@cn-byn
Copy link

cn-byn commented Dec 1, 2023

Description

The documentation comments of the ZipFile.ExtractToDirectory methods say:

Extracts all of the files in the specified archive to a directory on the file system. The specified directory must not exist.

and

<param name="destinationDirectoryName">The path to the directory on the file system. The directory specified must not exist, but the directory that it is contained in must exist.

Additionally, ZipFileExtensions.ExtractToDirectory contradictorily claims:

Extracts all of the files in the archive to a directory on the file system. The specified directory may already exist.

and

<param name="destinationDirectoryName">The path to the directory on the file system. The directory specified must not exist.

Looking through the source code, most of the statements don't seem to be true and it seems totally fine to provide a directory path that either exists or doesn't.

That's because those methods call ZipFileExtensions.ExtractRelativeToDirectory and there the directory is created (with a comment):

// in ZipFileExtensions.ZipArchiveEntry.Extract.cs:
// Note that this will give us a good DirectoryInfo even if destinationDirectoryName exists:
DirectoryInfo di = Directory.CreateDirectory(destinationDirectoryName);

Reproduction Steps

// case 1: specified path does not exist
string dir1 = @"C:\temp\some1\path1";
ZipFile.ExtractToDirectory(@"C:\temp\some.zip", dir1); // works, but according to the docs should not (because parent dir does not exist)

// case 2: specified path is created before calling ExtractToDirectory
string dir2 = @"C:\temp\some2\path2";
Directory.CreateDirectory(dir2);
ZipFile.ExtractToDirectory(@"C:\temp\some.zip", dir2); // works, but according to the docs should not (because dir does exist)

Expected behavior

The docs should match the behavior of the code and don't make contradictory statements.

Actual behavior

Calling ExtractToDirectory works in all cases. It doesn't matter if the specified directory exists or doesn't, but the docs say otherwise.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 1, 2023
@ghost
Copy link

ghost commented Dec 1, 2023

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

The documentation comments of the ZipFile.ExtractToDirectory methods say:

Extracts all of the files in the specified archive to a directory on the file system. The specified directory must not exist.

and

<param name="destinationDirectoryName">The path to the directory on the file system. The directory specified must not exist, but the directory that it is contained in must exist.

Additionally, ZipFileExtensions.ExtractToDirectory contradictorily claims:

Extracts all of the files in the archive to a directory on the file system. The specified directory may already exist.

and

<param name="destinationDirectoryName">The path to the directory on the file system. The directory specified must not exist.

Looking through the source code, most of the statements don't seem to be true and it seems totally fine to provide a directory path that either exists or doesn't.

That's because those methods call ZipFileExtensions.ExtractRelativeToDirectory and there the directory is created (with a comment):

// in ZipFileExtensions.ZipArchiveEntry.Extract.cs:
// Note that this will give us a good DirectoryInfo even if destinationDirectoryName exists:
DirectoryInfo di = Directory.CreateDirectory(destinationDirectoryName);

Reproduction Steps

// case 1: specified path does not exist
string dir1 = @"C:\temp\some1\path1";
ZipFile.ExtractToDirectory(@"C:\temp\some.zip", dir1); // works, but according to the docs should not (because parent dir does not exist)

// case 2: specified path is created before calling ExtractToDirectory
string dir2 = @"C:\temp\some2\path2";
Directory.CreateDirectory(dir2);
ZipFile.ExtractToDirectory(@"C:\temp\some.zip", dir2); // works, but according to the docs should not (because dir does exist)

### Expected behavior

The docs should match the behavior of the code and don't make contradictory statements.

### Actual behavior

Calling `ExtractToDirectory` works in all cases. It doesn't matter if the specified directory exists or doesn't, but the docs say otherwise.

### Regression?

_No response_

### Known Workarounds

_No response_

### Configuration

_No response_

### Other information

_No response_

<table>
  <tr>
    <th align="left">Author:</th>
    <td>cn-byn</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>-</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`area-System.IO`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>-</td>
  </tr>
</table>
</details>

@ghost
Copy link

ghost commented Dec 1, 2023

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

The documentation comments of the ZipFile.ExtractToDirectory methods say:

Extracts all of the files in the specified archive to a directory on the file system. The specified directory must not exist.

and

<param name="destinationDirectoryName">The path to the directory on the file system. The directory specified must not exist, but the directory that it is contained in must exist.

Additionally, ZipFileExtensions.ExtractToDirectory contradictorily claims:

Extracts all of the files in the archive to a directory on the file system. The specified directory may already exist.

and

<param name="destinationDirectoryName">The path to the directory on the file system. The directory specified must not exist.

Looking through the source code, most of the statements don't seem to be true and it seems totally fine to provide a directory path that either exists or doesn't.

That's because those methods call ZipFileExtensions.ExtractRelativeToDirectory and there the directory is created (with a comment):

// in ZipFileExtensions.ZipArchiveEntry.Extract.cs:
// Note that this will give us a good DirectoryInfo even if destinationDirectoryName exists:
DirectoryInfo di = Directory.CreateDirectory(destinationDirectoryName);

Reproduction Steps

// case 1: specified path does not exist
string dir1 = @"C:\temp\some1\path1";
ZipFile.ExtractToDirectory(@"C:\temp\some.zip", dir1); // works, but according to the docs should not (because parent dir does not exist)

// case 2: specified path is created before calling ExtractToDirectory
string dir2 = @"C:\temp\some2\path2";
Directory.CreateDirectory(dir2);
ZipFile.ExtractToDirectory(@"C:\temp\some.zip", dir2); // works, but according to the docs should not (because dir does exist)

Expected behavior

The docs should match the behavior of the code and don't make contradictory statements.

Actual behavior

Calling ExtractToDirectory works in all cases. It doesn't matter if the specified directory exists or doesn't, but the docs say otherwise.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: cn-byn
Assignees: -
Labels:

area-System.IO.Compression, untriaged

Milestone: -

@danmoseley
Copy link
Member

Thanks @cn-byn - do you want to offer a PR?

@carlossanlop carlossanlop added documentation Documentation bug or enhancement, does not impact product or test code and removed untriaged New issue has not been triaged by the area owner labels Dec 6, 2023
@carlossanlop carlossanlop added this to the 9.0.0 milestone Dec 6, 2023
@danmoseley danmoseley added good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors labels Mar 22, 2024
@rextor92
Copy link

I can work on this.

@msafwankarim
Copy link
Contributor

msafwankarim commented Mar 27, 2024

If a file that is being extracted already exists then it throws IOException stating that file already exists. Although exception message can be improved but claim from docs can be considered true. Should we change it?

PS: I have synchronised the descriptions of destinationDirectoryName parameter for all methods in PR #100330. Please review

cc: @danmoseley

@directhex
Copy link
Contributor

Resolved in #100330

@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO.Compression documentation Documentation bug or enhancement, does not impact product or test code good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

7 participants