Skip to content

Change some exception types thrown in Tar APIs - #74845

Merged
carlossanlop merged 17 commits into
dotnet:mainfrom
carlossanlop:TarException
Sep 2, 2022
Merged

Change some exception types thrown in Tar APIs#74845
carlossanlop merged 17 commits into
dotnet:mainfrom
carlossanlop:TarException

Conversation

@carlossanlop

@carlossanlopcarlossanlop commented Aug 30, 2022

Copy link
Copy Markdown
Contributor

As discussed here: #74786 (comment)
@bartonjs@jozkee

@danmoseley would you approve this exception type change for RC2? Edit: Dan said yes :)

@ghost

Copy link
Copy Markdown

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

Issue Details

As discussed here: #74786 (comment)
@bartonjs@jozkee

@danmoseley would you approve this exception type change for RC2?

Author:carlossanlop
Assignees:carlossanlop
Labels:

area-System.IO

Milestone:-

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs Outdated
@danmoseley

Copy link
Copy Markdown
Contributor

@danmoseley would you approve this exception type change for RC2

Yes as getting the API right is important. We do have upstack dependencies (the new docker tooling) but presumably not relying on this error path.

@carlossanlopcarlossanlop changed the title Throw ArgumentException on unsupported tar entry typeChange some exception types thrown in Tar APisAug 31, 2022
@carlossanlopcarlossanlop changed the title Change some exception types thrown in Tar APisChange some exception types thrown in Tar APIsAug 31, 2022
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/PaxTarEntry.cs Outdated
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs Outdated
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs Outdated
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs Outdated
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarFile.cs Outdated
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarFile.cs Outdated
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarFile.cs Outdated
@bartonjs
bartonjs self-requested a review August 31, 2022 17:14
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.cs Outdated
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs Outdated
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.cs Outdated
Comment threadsrc/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs Outdated
Comment threadsrc/libraries/System.Formats.Tar/tests/TarTestsBase.cs
@jozkee

jozkee commented Aug 31, 2022

Copy link
Copy Markdown
Member

TarEntryType.SparseFile=>thrownewNotSupportedException(string.Format(SR.TarEntryTypeNotSupported,header._typeFlag)),

@bartonjs, this is thrown when TarReader.GetNextEntry() steps into a sparse file, but AFAIK, NotSupportedException is for members that you choose not to support in your type. Do you know if a better exception could be thrown here?

@bartonjs

Copy link
Copy Markdown
Member

There's certainly prior art for throwing NotSupportedException for "there's a protocol feature that I don't support", and I can't think of a better one to use. (Plus, with some indirection, this would just be the implementation of something like TarSparseFile::Read, where it wouldn't look out of place at all.) (There's also prior art for using it when InvalidOperationException should be used, so make of that what you will.)

FWIW: I feel like I saw places that were using FormatException (an error with string.Format or related APIs) that probably meant InvalidDataException ("The exception that is thrown when a data stream is in an invalid format"). Don't know if those already got tackled.

@carlossanlop

Copy link
Copy Markdown
ContributorAuthor

@bartonjs not tackled yet but I can change them if that exception type is preferred.

Comment threadsrc/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs Outdated

@jozkeejozkee left a comment

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.

Otherwise, LGTM.

@carlossanlop

Copy link
Copy Markdown
ContributorAuthor

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2967038898

@carlossanlop

Copy link
Copy Markdown
ContributorAuthor

CI failures are unrelated infra problems.
Everything has been resolved, and it's signed off. Merging now.

@carlossanlop
carlossanlop merged commit b609e53 into dotnet:mainSep 2, 2022
@carlossanlop
carlossanlop deleted the TarException branch September 2, 2022 00:07
@ghostghost locked as resolved and limited conversation to collaborators Oct 2, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@carlossanlop@danmoseley@jozkee@bartonjs@jeffhandley