Skip to content

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

Merged
carlossanlop merged 5 commits into
dotnet:mainfrom
pedrobsaila:51051
Dec 1, 2021
Merged

throw exception when creating an entry name that already exists in ZipArchive#60973
carlossanlop merged 5 commits into
dotnet:mainfrom
pedrobsaila:51051

Conversation

@pedrobsaila

@pedrobsailapedrobsaila commented Oct 28, 2021

Copy link
Copy Markdown
Contributor

Fixes#51051

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

ghost commented Oct 28, 2021

Copy link
Copy Markdown

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:-

@pedrobsailapedrobsaila mentioned this pull request Oct 28, 2021
Closed
danmoseley
danmoseley reviewed Oct 29, 2021
View reviewed changes
Comment threadsrc/libraries/System.IO.Compression/src/Resources/Strings.resx Outdated
<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>

@danmoseleydanmoseleyOct 29, 2021

Copy link
Copy Markdown
Contributor

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.

@danmoseleydanmoseleyOct 29, 2021

Copy link
Copy Markdown
Contributor

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.

@pedrobsailapedrobsailaOct 29, 2021

Copy link
Copy Markdown
ContributorAuthor

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

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs Outdated
Comment threadsrc/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs Outdated
Comment threadsrc/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs Outdated
Comment threadsrc/libraries/System.IO.Compression/src/Resources/Strings.resx Outdated
@carlossanlopcarlossanlop added this to the .NET 7.0 milestone Nov 29, 2021
@karelzkarelz modified the milestones: .NET 7.0, 7.0.0Nov 30, 2021
@runfoapprunfoappBot mentioned this pull request Nov 30, 2021
Closed
carlossanlop
carlossanlop approved these changes Dec 1, 2021
View reviewed changes

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

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:mainDec 1, 2021
@pedrobsaila
pedrobsaila deleted the 51051 branch December 1, 2021 21:02
@ghostghost locked as resolved and limited conversation to collaborators Jan 1, 2022
jonathanpeppers
jonathanpeppers reviewed Apr 29, 2022
View reviewed changes
Comment threadsrc/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));
}

@jonathanpeppersjonathanpeppersApr 29, 2022

Copy link
Copy Markdown
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?

@jonathanpeppersjonathanpeppersApr 30, 2022

Copy link
Copy Markdown
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

@danmoseleydanmoseley added breaking-change Issue or PR that represents a breaking API or functional change over a previous release. 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
@jozkeejozkee removed breaking-change Issue or PR that represents a breaking API or functional change over a previous release. 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jonathanpeppersjonathanpeppersjonathanpeppers left review comments
+2 more reviewers
@danmoseleydanmoseleydanmoseley left review comments
@carlossanlopcarlossanlopcarlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlopcarlossanlop

Labels

area-System.IO.Compressioncommunity-contributionIndicates 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

7 participants

@pedrobsaila@jonathanpeppers@carlossanlop@danmoseley@karelz@adamsitnik@jozkee