Skip to content

throw exception when creating an entry name that already exists in ZipArchive #60973

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 1, 2021
Merged

throw exception when creating an entry name that already exists in ZipArchive #60973

merged 5 commits into from
Dec 1, 2021

Conversation

pedrobsaila
Copy link
Contributor

Fixes #51051

@ghost ghost added area-System.IO.Compression community-contribution Indicates that the PR has been added by a community member labels Oct 28, 2021
@ghost
Copy link

ghost commented Oct 28, 2021

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

Issue Details

Fixes #51051

Author: pedrobsaila
Assignees: -
Labels:

area-System.IO.Compression
Milestone: -

@pedrobsaila pedrobsaila mentioned this pull request Oct 28, 2021
Closed
danmoseley
danmoseley reviewed Oct 29, 2021
View reviewed changes
src/libraries/System.IO.Compression/src/Resources/Strings.resx Outdated
@@ -308,4 +308,7 @@
<data name="Zip64EOCDNotWhereExpected" xml:space="preserve">
<value>Zip 64 End of Central Directory Record not where indicated.</value>
</data>
<data name="EntryNameAlreadyExists" xml:space="preserve">
<value>The entry name already exists in the archive.</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the entryname a reasonable length? if so I suggest to include in the message ie

Suggested change
<value>The entry name already exists in the archive.</value>
<value>The entry name '{0}' already exists in the archive.</value>

whenever an exception refers to a named thing (like a path, etc) it can be helpful to include what the thing is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you did this, you'd use AssertExtensions.ThrowsContains to verify it's present.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the entry name would have reasonable length because it's just a file name

carlossanlop
carlossanlop reviewed Nov 29, 2021
View reviewed changes
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, @pedrobsaila , and thank you for your patience.

I left some suggestions for you to consider.

@carlossanlop carlossanlop added this to the .NET 7.0 milestone Nov 29, 2021
@karelz karelz modified the milestones: .NET 7.0, 7.0.0 Nov 30, 2021
@runfoapp runfoapp bot mentioned this pull request Nov 30, 2021
Closed
carlossanlop
carlossanlop approved these changes Dec 1, 2021
View reviewed changes
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, @pedrobsaila. Thanks for your contribution!

@carlossanlop carlossanlop merged commit 7a62468 into dotnet:main Dec 1, 2021
@pedrobsaila pedrobsaila deleted the 51051 branch December 1, 2021 21:02
@ghost ghost locked as resolved and limited conversation to collaborators Jan 1, 2022
jonathanpeppers
jonathanpeppers reviewed Apr 29, 2022
View reviewed changes
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs
Comment on lines +391 to +394
if (_entriesDictionary.ContainsKey(entryName))
{
throw new InvalidOperationException(string.Format(SR.EntryNameAlreadyExists, entryName));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on getting various MAUI workloads running on .NET 7. One of our tests hit an issue due to this change.

An MSBuild task inside a NuGet package has the following call:

https://github.com/xamarin/XamarinComponents/blob/895182cc16d88b0e092578d8a6d27bb68d0d459a/Util/Xamarin.Build.Download/source/Xamarin.Build.Download/AndroidAarFixups.cs#L60

Previously the code was creating a new entry then removing the old one afterward. (Code is old, it may indeed be incorrect)

Is the new exception intentional? Is it something that needs to be on a "breaking changes" list somewhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed an issue; it seems I can reproduce in a smaller example: #68734

@danmoseley danmoseley added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Apr 29, 2022
@jozkee jozkee removed breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels May 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonathanpeppers jonathanpeppers jonathanpeppers left review comments

@danmoseley danmoseley danmoseley left review comments

@carlossanlop carlossanlop carlossanlop approved these changes

Assignees

@carlossanlop carlossanlop

Labels
area-System.IO.Compression community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

ZipArchive.GetEntry() doesn't work properly after adding 2 entries with the same path, and removing one of them
6 participants
@pedrobsaila @jonathanpeppers @carlossanlop @danmoseley @karelz @jozkee