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

fix issue 26 #27

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 10 additions & 9 deletions kaitai/stream.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,27 +55,28 @@ func (k *Stream) EOF() (bool, error) {
}

// Size returns the number of bytes of the stream.
func (k *Stream) Size() (int64, error) {
func (k *Stream) Size() (size int64, err error) {
// Go has no internal ReadSeeker function to get current ReadSeeker size,
// thus we use the following trick.
// Remember our current position
curPos, err := k.Pos()
if err != nil {
return 0, err
}
// Deferred Seek back to the current position
defer func() {
if _, serr := k.Seek(curPos, io.SeekStart); serr != nil {
err = fmt.Errorf("failed to seek to the initial position %v: %w", curPos, serr)
}
}()
// Seek to the end of the File object
_, err = k.Seek(0, io.SeekEnd)
if err != nil {
return 0, err
}
// Remember position, which is equal to the full length
fullSize, err := k.Pos()
if err != nil {
return fullSize, err
}
// Seek back to the current position
_, err = k.Seek(curPos, io.SeekStart)
return fullSize, err

// Return the current position, which is equal to the full length
return k.Pos()
}

// Pos returns the current position of the stream.
Expand Down
95 changes: 94 additions & 1 deletion kaitai/stream_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ func TestStream_Size(t *testing.T) {
want int64
wantErr bool
}{
{"Size", NewStream(bytes.NewReader([]byte("test"))), 4, false},
{"Zero size", NewStream(bytes.NewReader([]byte{})), 0, false},
{"Small size", NewStream(bytes.NewReader([]byte("test"))), 4, false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -74,6 +75,98 @@ func TestStream_Size(t *testing.T) {
}
}

type artificialError struct{}

func (e artificialError) Error() string {
return "artificial error when seeking with io.SeekCurrent after seeking to end"
}

type failingReader struct {
pos int64
mustFail func(fr failingReader, offset int64, whence int) bool
}

func (fr *failingReader) Read(p []byte) (n int, err error) { return 0, nil }
func (fr *failingReader) Seek(offset int64, whence int) (int64, error) {
if fr.mustFail(*fr, offset, whence) {
return 0, artificialError{}
}

switch {
case whence == io.SeekCurrent:
return fr.pos, nil
case whence == io.SeekStart:
fr.pos = offset
default: // whence == io.SeekEnd
fr.pos = -1
}

return fr.pos, nil
}

// No regression test for issue #26
func TestErrorHandlingInStream_Size(t *testing.T) {
tests := map[string]struct {
initialPos int64
failingCondition func(fr failingReader, offset int64, whence int) bool
errorCheck func(err error) bool
wantFinalPos int64
}{
"fails to get initial position": {
initialPos: 5,
failingCondition: func(fr failingReader, offset int64, whence int) bool {
return whence == io.SeekCurrent && offset == 0
},
errorCheck: func(err error) bool {
_, ok := err.(artificialError)
return ok
},
Comment on lines +120 to +123
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite get why this check function is declared for each case individually. All cases use failingReader:

fr := &failingReader{tt.initialPos, tt.failingCondition}
s := NewStream(fr)
_, err := s.Size()

which in turn always returns artificialError if failingCondition (also known as mustFail) is true:

if fr.mustFail(*fr, offset, whence) {
return 0, artificialError{}
}

So I'd rather replace stream_test.go:159-161 with the previous version:

_, isArtificialErr := err.(artificialError)
if !isArtificialErr {
t.Fatalf("Expected error of type %T, got one of type %T", artificialError{}, err)
}

Copy link
Author

Choose a reason for hiding this comment

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

Not all cases return artificialError: when the deferred seek fails it returns an artificialError wrapped into a fmt.wrapError.

A more explicit definition could be:

func TestErrorHandlingInStream_Size(t *testing.T) {
	isArtificialError := func(err error) bool {
		_, ok := err.(artificialError)
		return ok
	}

	isWrappedArtificialError := func(err error) bool {
		_, ok := errors.Unwrap(err).(artificialError)
		return ok
	}

	tests := map[string]struct {
		initialPos       int64
		failingCondition func(fr failingReader, offset int64, whence int) bool
		errorCheck       func(err error) bool
		wantFinalPos     int64
	}{
		"fails to get initial position": {
			initialPos: 5,
			failingCondition: func(fr failingReader, offset int64, whence int) bool {
				return whence == io.SeekCurrent && offset == 0
			},
			errorCheck:   isArtificialError,
			wantFinalPos: 5,
		},
		"seek to the end fails": {
			initialPos: 5,
			failingCondition: func(fr failingReader, offset int64, whence int) bool {
				return whence == io.SeekEnd
			},
			errorCheck:   isArtificialError,
			wantFinalPos: 5,
		},
		"deferred seek to the initial pos fails": {
			initialPos: 5,
			failingCondition: func(fr failingReader, offset int64, whence int) bool {
				return whence == io.SeekStart && fr.pos == -1
			},
			errorCheck:   isWrappedArtificialError,
			wantFinalPos: -1,
		},
	}

Anyway, you can go ahead, pick the version you like the most and modify the PR consequently.

Copy link
Member

@generalmimon generalmimon Jan 23, 2021

Choose a reason for hiding this comment

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

Not all cases return artificialError: when the deferred seek fails it returns an artificialError wrapped into a fmt.wrapError.

Oh, thanks, I didn't notice that. I didn't see this little exclamation mark in !ok here:

errorCheck: func(err error) bool {
_, ok := err.(artificialError)
return !ok
},

The above "check" is totally unacceptable because it only checks that the artificialError is either wrapped somewhere inside or we're dealing with a completely different error that does not have anything in common with artificialError at all. It is also deceptive because it looks so similar to return ok, which would make more sense.

But it turns out that the linter knows about the problem that wrapped errors cannot be simply type asserted, and it suggested a solution:

$ ./golangci-lint-1.35.2-windows-amd64/golangci-lint run kaitai --no-config -E errorlint
...
kaitai\stream_test.go:121:14: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
                                _, ok := err.(artificialError)
                                         ^

So I believe that this silver bullet errors.As should work for everything:

+	var wantErr artificialError
-	_, isArtificialErr := err.(artificialError)
+	_, isArtificialErr := errors.As(err, &wantErr)
 	if !isArtificialErr {
-		t.Fatalf("Expected error of type %T, got one of type %T", artificialError{}, err)
+		t.Fatalf("Expected error of type %T, got one of type %T", wantErr, err)
 	}

See https://play.golang.org/p/1FN6BVmqRWV for a demo.

wantFinalPos: 5,
},
"seek to the end fails": {
initialPos: 5,
failingCondition: func(fr failingReader, offset int64, whence int) bool {
return whence == io.SeekEnd
},
errorCheck: func(err error) bool {
_, ok := err.(artificialError)
return ok
},
wantFinalPos: 5,
},
"deferred seek to the initial pos fails": {
initialPos: 5,
failingCondition: func(fr failingReader, offset int64, whence int) bool {
return whence == io.SeekStart && fr.pos == -1
},
errorCheck: func(err error) bool {
_, ok := err.(artificialError)
return !ok
},
wantFinalPos: -1,
},
}
for name, tt := range tests {
t.Run(name, func(t *testing.T) {
fr := &failingReader{tt.initialPos, tt.failingCondition}
s := NewStream(fr)
_, err := s.Size()

if err == nil {
t.Fatal("Expected error, got nothing")
}

if !tt.errorCheck(err) {
t.Fatalf("Expected error of type %T, got one of type %T", artificialError{}, err)
}

if fr.pos != tt.wantFinalPos {
t.Fatalf("Expected position to be %v, got %v", tt.wantFinalPos, fr.pos)
}
})
}
}

func TestStream_Pos(t *testing.T) {
tests := []struct {
name string
Expand Down