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.

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

fix: update error returned when oci.NewFromTar is used with a non-tar file #793

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: update error returned when oci.NewFromTar is used with a non-tar file #793

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
995e97f
fix: updating error handling for non tar files
ADorigi Jul 20, 2024
7fc9e05
fix: non-tar file check now done while indexing entries
ADorigi Jul 23, 2024
b5c7ebd
fix: wrapped error to include file path
ADorigi Jul 23, 2024
275a44a
fix: condensed the tests and added friendly message
ADorigi Jul 24, 2024
51ec1d4
fix: separating test for tarfiles without extension
ADorigi Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions content/oci/readonlyoci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"oras.land/oras-go/v2"
"oras.land/oras-go/v2/content"
"oras.land/oras-go/v2/content/memory"
"oras.land/oras-go/v2/errdef"
"oras.land/oras-go/v2/internal/docker"
"oras.land/oras-go/v2/internal/spec"
"oras.land/oras-go/v2/registry"
Expand Down Expand Up @@ -836,3 +837,15 @@ func Test_deleteAnnotationRefName(t *testing.T) {
})
}
}

func TestNewFromTar_NonTarFile(t *testing.T) {

emptyfile := "testdata/emptyfile"
Wwwsylvia marked this conversation as resolved.
Show resolved Hide resolved

_, err := NewFromTar(context.Background(), emptyfile)

if want := errdef.ErrNotTarFile; !errors.Is(err, want) {
t.Errorf("OCI.NewFromTar(%s) error = %v, wantErr %v", emptyfile, err, want)
}

}
Empty file added content/oci/testdata/dummyfile
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions errdef/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,5 @@ var (
ErrSizeExceedsLimit = errors.New("size exceeds limit")
ErrUnsupported = errors.New("unsupported")
ErrUnsupportedVersion = errors.New("unsupported version")
ErrNotTarFile = errors.New("tar file type expected")
)
7 changes: 7 additions & 0 deletions internal/fs/tarfs/tarfs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ func New(path string) (*TarFS, error) {
if err != nil {
return nil, fmt.Errorf("failed to resolve absolute path for %s: %w", path, err)
}

extension := filepath.Ext(pathAbs)

if extension != ".tar" {
Copy link
Member

Choose a reason for hiding this comment

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

Are compressed formats supported here like tgz?

Copy link
Author

Choose a reason for hiding this comment

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

No, I believe they will not be.
Is there a list of extensions we need to support?

Copy link
Author

Choose a reason for hiding this comment

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

I can add them to the check and add corresponding tests for them too.

Copy link
Member

Choose a reason for hiding this comment

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

I imagine only tar, but I'm not sure.

Copy link
Author

Choose a reason for hiding this comment

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

The current tests work with the files hello-world.tar and test.tar. From my understanding, .tars are the only ones supported as of now.

Copy link
Member

Choose a reason for hiding this comment

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

What if the target file/directory name ends with .tar, but it is actually not a tar type? Or what if the file is a tar file but it does not have a .tar suffix in its name?

Copy link
Member

Choose a reason for hiding this comment

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

In tfs.indexEntries(), maybe we can return a friendly error if tr.Next() fails?

Copy link
Author

Choose a reason for hiding this comment

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

I think tfs.indexEntries would indeed be a better place for this check. We can check if EOF is reached before any entries are added to tfs. This would infer it is an empty file. While a non-tar file will give us an io.ErrUnexpectedEOF while reading it.

We can then give the user our custom exported error.

return nil, errdef.ErrNotTarFile
}

tarfs := &TarFS{
path: pathAbs,
entries: make(map[string]*entry),
Expand Down
12 changes: 12 additions & 0 deletions internal/fs/tarfs/tarfs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,3 +268,15 @@ func TestTarFS_Stat_Unsupported(t *testing.T) {
}
}
}

func TestTarFS_New_NonTarFile(t *testing.T) {

emptyfile := "testdata/emptyfile"

_, err := New(emptyfile)

Wwwsylvia marked this conversation as resolved.
Show resolved Hide resolved
if want := errdef.ErrNotTarFile; !errors.Is(err, want) {
t.Errorf("TarFS.New(%s) error = %v, wantErr %v", emptyfile, err, want)
}

}
Empty file added internal/fs/tarfs/testdata/emptyfile
View file
Open in desktop
Empty file.