From e9f813a1c945d630269c3cb240d7b5a99c80d7b2 Mon Sep 17 00:00:00 2001 From: Suyash Kumar Date: Sun, 16 Jun 2024 00:08:01 -0400 Subject: [PATCH] Improve some errors, rename interface{} to any --- dataset.go | 4 +- element.go | 54 +++++++-------- element_test.go | 5 +- pkg/debug/debug.go | 3 +- pkg/debug/default.go | 3 +- pkg/personname/groupInfo_test.go | 2 +- pkg/personname/info_test.go | 2 +- read.go | 112 ++++++++++++++----------------- read_test.go | 21 ++++-- 9 files changed, 106 insertions(+), 100 deletions(-) diff --git a/dataset.go b/dataset.go index f9fa3ffd..f9fab300 100644 --- a/dataset.go +++ b/dataset.go @@ -33,7 +33,7 @@ func (d *Dataset) FindElementByTag(tag tag.Tag) (*Element, error) { return e, nil } } - return nil, ErrorElementNotFound + return nil, fmt.Errorf("unable to find %v element: %w", tag, ErrorElementNotFound) } func (d *Dataset) transferSyntax() (binary.ByteOrder, bool, error) { @@ -57,7 +57,7 @@ func (d *Dataset) FindElementByTagNested(tag tag.Tag) (*Element, error) { return e, nil } } - return nil, ErrorElementNotFound + return nil, fmt.Errorf("unable to find %v element: %w", tag, ErrorElementNotFound) } // FlatIterator will be deprecated soon in favor of diff --git a/element.go b/element.go index f6671e00..d487017f 100644 --- a/element.go +++ b/element.go @@ -91,7 +91,7 @@ type Value interface { ValueType() ValueType // GetValue returns the underlying value that this Value holds. What type is returned here can be determined exactly // from the ValueType() of this Value (see the ValueType godoc). - GetValue() interface{} // TODO: rename to Get to read cleaner + GetValue() any // TODO: rename to Get to read cleaner String() string MarshalJSON() ([]byte, error) // Equals returns true if this value equals the input Value. @@ -107,7 +107,7 @@ type Value interface { // Acceptable types: []int, []string, []byte, []float64, PixelDataInfo, // [][]*Element (represents a sequence, which contains several // items which each contain several elements). -func NewValue(data interface{}) (Value, error) { +func NewValue(data any) (Value, error) { switch data.(type) { case []int: return &intsValue{value: data.([]int)}, nil @@ -127,11 +127,11 @@ func NewValue(data interface{}) (Value, error) { } return &sequencesValue{value: sequenceItems}, nil default: - return nil, ErrorUnexpectedDataType + return nil, fmt.Errorf("NewValue: unexpected data type %T: %w", data, ErrorUnexpectedDataType) } } -func mustNewValue(data interface{}) Value { +func mustNewValue(data any) Value { v, err := NewValue(data) if err != nil { panic(err) @@ -142,7 +142,7 @@ func mustNewValue(data interface{}) Value { // NewElement creates a new DICOM Element with the supplied tag and with a value // built from the provided data. The data can be one of the types that is // acceptable to NewValue. -func NewElement(t tag.Tag, data interface{}) (*Element, error) { +func NewElement(t tag.Tag, data any) (*Element, error) { tagInfo, err := tag.Find(t) if err != nil { return nil, err @@ -164,7 +164,7 @@ func NewElement(t tag.Tag, data interface{}) (*Element, error) { }, nil } -func mustNewElement(t tag.Tag, data interface{}) *Element { +func mustNewElement(t tag.Tag, data any) *Element { elem, err := NewElement(t, data) if err != nil { log.Panic(err) @@ -172,7 +172,7 @@ func mustNewElement(t tag.Tag, data interface{}) *Element { return elem } -func mustNewPrivateElement(t tag.Tag, rawVR string, data interface{}) *Element { +func mustNewPrivateElement(t tag.Tag, rawVR string, data any) *Element { value, err := NewValue(data) if err != nil { log.Panic(fmt.Errorf("error creating value: %w", err)) @@ -217,9 +217,9 @@ type bytesValue struct { value []byte } -func (b *bytesValue) isElementValue() {} -func (b *bytesValue) ValueType() ValueType { return Bytes } -func (b *bytesValue) GetValue() interface{} { return b.value } +func (b *bytesValue) isElementValue() {} +func (b *bytesValue) ValueType() ValueType { return Bytes } +func (b *bytesValue) GetValue() any { return b.value } func (b *bytesValue) String() string { return fmt.Sprintf("%v", b.value) } @@ -242,9 +242,9 @@ type stringsValue struct { value []string } -func (s *stringsValue) isElementValue() {} -func (s *stringsValue) ValueType() ValueType { return Strings } -func (s *stringsValue) GetValue() interface{} { return s.value } +func (s *stringsValue) isElementValue() {} +func (s *stringsValue) ValueType() ValueType { return Strings } +func (s *stringsValue) GetValue() any { return s.value } func (s *stringsValue) String() string { return fmt.Sprintf("%v", s.value) } @@ -273,9 +273,9 @@ type intsValue struct { value []int } -func (i *intsValue) isElementValue() {} -func (i *intsValue) ValueType() ValueType { return Ints } -func (i *intsValue) GetValue() interface{} { return i.value } +func (i *intsValue) isElementValue() {} +func (i *intsValue) ValueType() ValueType { return Ints } +func (i *intsValue) GetValue() any { return i.value } func (i *intsValue) String() string { return fmt.Sprintf("%v", i.value) } @@ -304,9 +304,9 @@ type floatsValue struct { value []float64 } -func (f *floatsValue) isElementValue() {} -func (f *floatsValue) ValueType() ValueType { return Floats } -func (f *floatsValue) GetValue() interface{} { return f.value } +func (f *floatsValue) isElementValue() {} +func (f *floatsValue) ValueType() ValueType { return Floats } +func (f *floatsValue) GetValue() any { return f.value } func (f *floatsValue) String() string { return fmt.Sprintf("%v", f.value) } @@ -345,7 +345,7 @@ func (s *SequenceItemValue) ValueType() ValueType { return SequenceItem } // GetValue returns the underlying value that this Value holds. What type is // returned here can be determined exactly from the ValueType() of this Value // (see the ValueType godoc). -func (s *SequenceItemValue) GetValue() interface{} { return s.elements } +func (s *SequenceItemValue) GetValue() any { return s.elements } // String is used to get a string representation of this struct. func (s *SequenceItemValue) String() string { @@ -379,9 +379,9 @@ type sequencesValue struct { value []*SequenceItemValue } -func (s *sequencesValue) isElementValue() {} -func (s *sequencesValue) ValueType() ValueType { return Sequences } -func (s *sequencesValue) GetValue() interface{} { return s.value } +func (s *sequencesValue) isElementValue() {} +func (s *sequencesValue) ValueType() ValueType { return Sequences } +func (s *sequencesValue) GetValue() any { return s.value } func (s *sequencesValue) String() string { // TODO: consider adding more sophisticated formatting return fmt.Sprintf("%+v", s.value) @@ -440,9 +440,9 @@ type pixelDataValue struct { PixelDataInfo } -func (p *pixelDataValue) isElementValue() {} -func (p *pixelDataValue) ValueType() ValueType { return PixelData } -func (p *pixelDataValue) GetValue() interface{} { return p.PixelDataInfo } +func (p *pixelDataValue) isElementValue() {} +func (p *pixelDataValue) ValueType() ValueType { return PixelData } +func (p *pixelDataValue) GetValue() any { return p.PixelDataInfo } func (p *pixelDataValue) String() string { if len(p.Frames) == 0 { return "empty pixel data" @@ -530,7 +530,7 @@ func MustGetPixelDataInfo(v Value) PixelDataInfo { // allValues is used for tests that need to pass in instances of the unexported // value structs to cmp.AllowUnexported. -var allValues = []interface{}{ +var allValues = []any{ floatsValue{}, intsValue{}, stringsValue{}, diff --git a/element_test.go b/element_test.go index 3d825a8c..f6ab5351 100644 --- a/element_test.go +++ b/element_test.go @@ -2,6 +2,7 @@ package dicom import ( "encoding/json" + "errors" "testing" "github.com/google/go-cmp/cmp" @@ -62,7 +63,7 @@ func TestElement_String(t *testing.T) { func TestNewValue(t *testing.T) { cases := []struct { name string - data interface{} + data any wantValue Value wantError error }{ @@ -141,7 +142,7 @@ func TestNewValue(t *testing.T) { func TestNewValue_UnexpectedType(t *testing.T) { data := 10 _, err := NewValue(data) - if err != ErrorUnexpectedDataType { + if !errors.Is(err, ErrorUnexpectedDataType) { t.Errorf("NewValue(%v) expected an error. got: %v, want: %v", data, err, ErrorUnexpectedDataType) } } diff --git a/pkg/debug/debug.go b/pkg/debug/debug.go index e9c1297f..63da8236 100644 --- a/pkg/debug/debug.go +++ b/pkg/debug/debug.go @@ -1,3 +1,4 @@ +//go:build debug // +build debug package debug @@ -10,6 +11,6 @@ func Log(data string) { } // Logf only logs with a formatted string when the debug build flag is present. -func Logf(format string, args ...interface{}) { +func Logf(format string, args ...any) { log.Printf(format, args...) } diff --git a/pkg/debug/default.go b/pkg/debug/default.go index 5b13f7bb..27d954a5 100644 --- a/pkg/debug/default.go +++ b/pkg/debug/default.go @@ -1,3 +1,4 @@ +//go:build !debug // +build !debug package debug @@ -6,4 +7,4 @@ package debug func Log(data string) {} // Logf only logs with a formatted string when the debug build flag is present. -func Logf(format string, args ...interface{}) {} +func Logf(format string, args ...any) {} diff --git a/pkg/personname/groupInfo_test.go b/pkg/personname/groupInfo_test.go index 4e71f468..055f6099 100644 --- a/pkg/personname/groupInfo_test.go +++ b/pkg/personname/groupInfo_test.go @@ -396,7 +396,7 @@ func TestGroupInfo_DCM_panic(t *testing.T) { TrailingNullLevel: 5, } - var recovered interface{} + var recovered any func() { defer func() { diff --git a/pkg/personname/info_test.go b/pkg/personname/info_test.go index 88ae685f..b99a6a09 100644 --- a/pkg/personname/info_test.go +++ b/pkg/personname/info_test.go @@ -493,7 +493,7 @@ func TestInfo_DCM_panic(t *testing.T) { TrailingNullLevel: 3, } - var recovered interface{} + var recovered any func() { defer func() { diff --git a/read.go b/read.go index bb4f6b2b..3aba364c 100644 --- a/read.go +++ b/read.go @@ -50,15 +50,8 @@ func (r *reader) readTag() (*tag.Tag, error) { if gerr == nil && eerr == nil { return &tag.Tag{Group: group, Element: element}, nil } - // If the error is an io.EOF, return the error directly instead of the compound error later. - // Once we target go1.20 we can use errors.Join: https://pkg.go.dev/errors#Join - if errors.Is(gerr, io.EOF) { - return nil, gerr - } - if errors.Is(eerr, io.EOF) { - return nil, eerr - } - return nil, fmt.Errorf("error reading tag: %v %v", gerr, eerr) + + return nil, fmt.Errorf("error reading tag: %w", errors.Join(gerr, eerr)) } // TODO: Parsed VR should be an enum. Will require refactors of tag pkg. @@ -166,7 +159,7 @@ func (r *reader) readHeader() ([]*Element, error) { // Check for Preamble (128 bytes) + magic word (4 bytes). data, err := r.rawReader.Peek(128 + 4) if err != nil { - return nil, err + return nil, fmt.Errorf("error reading DICOM header preamble: %w", err) } if string(data[128:]) != magicWord { // If magic word is not at byte offset 128 this is a non-standard @@ -186,7 +179,7 @@ func (r *reader) readHeader() ([]*Element, error) { // Read the length of the metadata elements: (0002,0000) MetaElementGroupLength maybeMetaLen, err := r.readElement(nil, nil) if err != nil { - return nil, err + return nil, fmt.Errorf("error reading DICOM header element: %w", err) } metaElems := []*Element{maybeMetaLen} // TODO: maybe set capacity to a reasonable initial size @@ -212,9 +205,8 @@ func (r *reader) readHeader() ([]*Element, error) { elem, err := r.readElement(nil, nil) if err != nil { // TODO: see if we can skip over malformed elements somehow - return nil, err + return nil, fmt.Errorf("error reading DICOM header element: %w", err) } - // log.Printf("Metadata Element: %s\n", elem) metaElems = append(metaElems, elem) } } else { @@ -222,12 +214,12 @@ func (r *reader) readHeader() ([]*Element, error) { debug.Log("Proceeding without metadata group length") for { // Lets peek into the tag field until we get to end-of-header - group_bytes, err := r.rawReader.Peek(2) + groupBytes, err := r.rawReader.Peek(2) if err != nil { return nil, ErrorMetaElementGroupLength } var group uint16 - buff := bytes.NewBuffer(group_bytes) + buff := bytes.NewBuffer(groupBytes) if err := binary.Read(buff, binary.LittleEndian, &group); err != nil { return nil, err } @@ -239,7 +231,7 @@ func (r *reader) readHeader() ([]*Element, error) { elem, err := r.readElement(nil, nil) if err != nil { // TODO: see if we can skip over malformed elements somehow - return nil, err + return nil, fmt.Errorf("error reading DICOM header element (with no meta element group length defined): %w", err) } metaElems = append(metaElems, elem) } @@ -256,7 +248,7 @@ func (r *reader) readPixelData(vl uint32, d *Dataset, fc chan<- *frame.Frame) (V // TODO: use basic offset table _, _, err := r.readRawItem(true /*shouldSkip*/) if err != nil { - return nil, err + return nil, fmt.Errorf("readPixelData: error skipping basic offset table: %w", err) } for !r.rawReader.IsLimitExhausted() { @@ -290,7 +282,7 @@ func (r *reader) readPixelData(vl uint32, d *Dataset, fc chan<- *frame.Frame) (V // If we're here, it means the VL isn't undefined length, so we should // be able to safely skip the native PixelData. if err := r.rawReader.Skip(int64(vl)); err != nil { - return nil, err + return nil, fmt.Errorf("readPixelData: skipPixelData option was set, and had error skipping PixelData: %w", err) } return &pixelDataValue{PixelDataInfo{IntentionallySkipped: true}}, nil } @@ -392,12 +384,12 @@ func (r *reader) readNativeFrames(parsedData *Dataset, fc chan<- *frame.Frame, v // Parse information from previously parsed attributes that are needed to parse NativeData Frames: rows, err := parsedData.FindElementByTag(tag.Rows) if err != nil { - return nil, 0, err + return nil, 0, fmt.Errorf("readNativeFrames: error finding Rows tag: %w", err) } cols, err := parsedData.FindElementByTag(tag.Columns) if err != nil { - return nil, 0, err + return nil, 0, fmt.Errorf("readNativeFrames: error finding Columns tag: %w", err) } nof, err := parsedData.FindElementByTag(tag.NumberOfFrames) @@ -406,7 +398,7 @@ func (r *reader) readNativeFrames(parsedData *Dataset, fc chan<- *frame.Frame, v // No error, so parse number of frames nFrames, err = strconv.Atoi(MustGetStrings(nof.Value)[0]) // odd that number of frames is encoded as a string... if err != nil { - return nil, 0, err + return nil, 0, fmt.Errorf("readNativeFrames: error converting NumberOfFrames from string to int: %w", err) } } else { // error fetching NumberOfFrames, so default to 1. TODO: revisit @@ -415,13 +407,13 @@ func (r *reader) readNativeFrames(parsedData *Dataset, fc chan<- *frame.Frame, v b, err := parsedData.FindElementByTag(tag.BitsAllocated) if err != nil { - return nil, 0, err + return nil, 0, fmt.Errorf("readNativeFrames: error finding BitsAllocated tag: %w", err) } bitsAllocated := MustGetInts(b.Value)[0] s, err := parsedData.FindElementByTag(tag.SamplesPerPixel) if err != nil { - return nil, 0, err + return nil, 0, fmt.Errorf("readNativeFrames: error finding SamplesPerPixel tag: %w", err) } samplesPerPixel := MustGetInts(s.Value)[0] @@ -441,15 +433,15 @@ func (r *reader) readNativeFrames(parsedData *Dataset, fc chan<- *frame.Frame, v case uint32(bytesToRead) == vl-1 && vl%2 == 0: skipFinalPaddingByte = true case uint32(bytesToRead) == vl-1 && vl%2 != 0: - return nil, 0, fmt.Errorf("vl=%d: %w", vl, ErrorExpectedEvenLength) + return nil, 0, fmt.Errorf("error when reading Native PixelData, got vl=%d, expected even VL: %w", vl, ErrorExpectedEvenLength) default: // calculated bytesToRead and actual VL mismatch if !r.opts.allowMismatchPixelDataLength { - return nil, 0, fmt.Errorf("expected_vl=%d actual_vl=%d %w", bytesToRead, vl, ErrorMismatchPixelDataLength) + return nil, 0, fmt.Errorf("error when reading Native PixelData: expected_vl=%d actual_vl=%d %w", bytesToRead, vl, ErrorMismatchPixelDataLength) } image, err := makeErrorPixelData(r.rawReader, vl, fc, ErrorMismatchPixelDataLength) if err != nil { - return nil, 0, err + return nil, 0, fmt.Errorf("readNativeFrames: error making error pixel data: %w", err) } return image, int(vl), nil } @@ -498,7 +490,7 @@ func (r *reader) readNativeFrames(parsedData *Dataset, fc chan<- *frame.Frame, v } else if bitsAllocated == 32 { buf[(pixel*samplesPerPixel)+value] = int(bo.Uint32(pixelBuf)) } else { - return nil, bytesToRead, fmt.Errorf("bitsAllocated=%d : %w", bitsAllocated, ErrorUnsupportedBitsAllocated) + return nil, bytesToRead, fmt.Errorf("error when reading Native PixelData, unsupported bitsAllocated, got bitsAllocated=%d : %w", bitsAllocated, ErrorUnsupportedBitsAllocated) } } currentFrame.NativeData.Data[pixel] = buf[pixel*samplesPerPixel : (pixel+1)*samplesPerPixel] @@ -512,7 +504,7 @@ func (r *reader) readNativeFrames(parsedData *Dataset, fc chan<- *frame.Frame, v if skipFinalPaddingByte { err := r.rawReader.Skip(1) if err != nil { - return nil, bytesToRead, fmt.Errorf("could not read padding byte: %w", err) + return nil, bytesToRead, fmt.Errorf("error when reading Native PixelData: could not read padding byte, when one needed to be skipped: %w", err) } bytesToRead++ } @@ -531,8 +523,7 @@ func (r *reader) readSequence(t tag.Tag, vr string, vl uint32, d *Dataset) (Valu subElement, err := r.readElement(seqElements, nil) if err != nil { // Stop reading due to error - log.Println("error reading subitem, ", err) - return nil, err + return nil, fmt.Errorf("readSequence: error reading subitem in a sequence: %w", err) } if subElement.Tag == tag.SequenceDelimitationItem { // Stop reading @@ -542,7 +533,7 @@ func (r *reader) readSequence(t tag.Tag, vr string, vl uint32, d *Dataset) (Valu // This is an error, should be an Item! // TODO: use error var log.Println("Tag is ", subElement.Tag) - return nil, fmt.Errorf("non item found in sequence") + return nil, fmt.Errorf("readSequence: error, non item element found in sequence. got: %v", subElement) } // Append the Item element's dataset of elements to this Sequence's sequencesValue. @@ -558,7 +549,7 @@ func (r *reader) readSequence(t tag.Tag, vr string, vl uint32, d *Dataset) (Valu subElement, err := r.readElement(seqElements, nil) if err != nil { // TODO: option to ignore errors parsing subelements? - return nil, err + return nil, fmt.Errorf("readSequence: error reading subitem in a sequence: %w", err) } // Append the Item element's dataset of elements to this Sequence's sequencesValue. @@ -575,7 +566,7 @@ func (r *reader) readSequence(t tag.Tag, vr string, vl uint32, d *Dataset) (Valu func (r *reader) readSequenceItem(t tag.Tag, vr string, vl uint32, d *Dataset) (Value, error) { var sequenceItem SequenceItemValue - // seqElements holds items read so farawReader. + // seqElements holds items read so far. // TODO: deduplicate with sequenceItem above seqElements := Dataset{} @@ -583,7 +574,7 @@ func (r *reader) readSequenceItem(t tag.Tag, vr string, vl uint32, d *Dataset) ( for { subElem, err := r.readElement(&seqElements, nil) if err != nil { - return nil, err + return nil, fmt.Errorf("readSequenceItem: error reading subitem in a sequence item: %w", err) } if subElem.Tag == tag.ItemDelimitationItem { break @@ -601,7 +592,7 @@ func (r *reader) readSequenceItem(t tag.Tag, vr string, vl uint32, d *Dataset) ( for !r.rawReader.IsLimitExhausted() { subElem, err := r.readElement(&seqElements, nil) if err != nil { - return nil, err + return nil, fmt.Errorf("readSequenceItem: error reading subitem in a sequence item: %w", err) } sequenceItem.elements = append(sequenceItem.elements, subElem) @@ -622,7 +613,7 @@ func (r *reader) readBytes(t tag.Tag, vr string, vl uint32) (Value, error) { } else if vr == vrraw.OtherWord { // OW -> stream of 16 bit words if vl%2 != 0 { - return nil, ErrorOWRequiresEvenVL + return nil, fmt.Errorf("error reading bytes element (%v) value: %w", t, ErrorOWRequiresEvenVL) } buf := bytes.NewBuffer(make([]byte, 0, vl)) @@ -630,7 +621,7 @@ func (r *reader) readBytes(t tag.Tag, vr string, vl uint32) (Value, error) { for i := 0; i < numWords; i++ { word, err := r.rawReader.ReadUInt16() if err != nil { - return nil, err + return nil, fmt.Errorf("error reading bytes element (%v) value: %w", t, err) } // TODO: support bytes.BigEndian byte ordering err = binary.Write(buf, binary.LittleEndian, word) @@ -641,11 +632,14 @@ func (r *reader) readBytes(t tag.Tag, vr string, vl uint32) (Value, error) { return &bytesValue{value: buf.Bytes()}, nil } - return nil, ErrorUnsupportedVR + return nil, fmt.Errorf("error reading bytes element (%v): %w", t, ErrorUnsupportedVR) } func (r *reader) readString(t tag.Tag, vr string, vl uint32) (Value, error) { str, err := r.rawReader.ReadString(vl) + if err != nil { + return nil, fmt.Errorf("error reading string element (%v) value: %w", t, err) + } onlySpaces := true for _, char := range str { if !unicode.IsSpace(char) { @@ -659,7 +653,7 @@ func (r *reader) readString(t tag.Tag, vr string, vl uint32) (Value, error) { // Split multiple strings strs := strings.Split(str, "\\") - return &stringsValue{value: strs}, err + return &stringsValue{value: strs}, nil } func (r *reader) readFloat(t tag.Tag, vr string, vl uint32) (Value, error) { @@ -673,26 +667,26 @@ func (r *reader) readFloat(t tag.Tag, vr string, vl uint32) (Value, error) { case vrraw.FloatingPointSingle: val, err := r.rawReader.ReadFloat32() if err != nil { - return nil, err + return nil, fmt.Errorf("error reading floating point element (%v) value: %w", t, err) } // TODO(suyashkumar): revisit this hack to prevent some internal representation issues upconverting from // float32 to float64. There is no loss of precision, but the value gets some additional significant digits // when using golang casting. This approach prevents those artifacts, but is less efficient. pval, err := strconv.ParseFloat(fmt.Sprint(val), 64) if err != nil { - return nil, err + return nil, fmt.Errorf("error reading floating point element (%v) value during strconv.ParseFloat: %w", t, err) } retVal.value = append(retVal.value, pval) break case vrraw.FloatingPointDouble: val, err := r.rawReader.ReadFloat64() if err != nil { - return nil, err + return nil, fmt.Errorf("error reading floating point element (%v) value: %w", t, err) } retVal.value = append(retVal.value, val) break default: - return nil, errorUnableToParseFloat + return nil, fmt.Errorf("error reading floating point element(%v) value: unsupported VR: %w", t, errorUnableToParseFloat) } } r.rawReader.PopLimit() @@ -702,7 +696,7 @@ func (r *reader) readFloat(t tag.Tag, vr string, vl uint32) (Value, error) { func (r *reader) readDate(t tag.Tag, vr string, vl uint32) (Value, error) { rawDate, err := r.rawReader.ReadString(vl) if err != nil { - return nil, err + return nil, fmt.Errorf("error reading date element (%v) value: %w", t, err) } date := strings.Trim(rawDate, " \000") @@ -722,33 +716,33 @@ func (r *reader) readInt(t tag.Tag, vr string, vl uint32) (Value, error) { case vrraw.UnsignedShort, vrraw.AttributeTag: val, err := r.rawReader.ReadUInt16() if err != nil { - return nil, err + return nil, fmt.Errorf("error reading int element (%v) value (ReadUInt16): %w", t, err) } retVal.value = append(retVal.value, int(val)) break case vrraw.UnsignedLong: val, err := r.rawReader.ReadUInt32() if err != nil { - return nil, err + return nil, fmt.Errorf("error reading int element (%v) value (ReadUInt32): %w", t, err) } retVal.value = append(retVal.value, int(val)) break case vrraw.SignedLong: val, err := r.rawReader.ReadInt32() if err != nil { - return nil, err + return nil, fmt.Errorf("error reading int element (%v) value (ReadInt32): %w", t, err) } retVal.value = append(retVal.value, int(val)) break case vrraw.SignedShort: val, err := r.rawReader.ReadInt16() if err != nil { - return nil, err + return nil, fmt.Errorf("error reading int element (%v) value (ReadInt16): %w", t, err) } retVal.value = append(retVal.value, int(val)) break default: - return nil, errors.New("unable to parse integer type") + return nil, fmt.Errorf("unable to parse integer type due to unknown VR %v", vr) } } r.rawReader.PopLimit() @@ -763,7 +757,7 @@ func (r *reader) readInt(t tag.Tag, vr string, vl uint32) (Value, error) { func (r *reader) readElement(d *Dataset, fc chan<- *frame.Frame) (*Element, error) { t, err := r.readTag() if err != nil { - return nil, err + return nil, fmt.Errorf("readElement: error when reading element tag: %w", err) } debug.Logf("readElement: tag: %s", t.String()) @@ -775,20 +769,19 @@ func (r *reader) readElement(d *Dataset, fc chan<- *frame.Frame) (*Element, erro vr, err := r.readVR(readImplicit, *t) if err != nil { - return nil, err + return nil, fmt.Errorf("readElement: error when reading VR for element %v: %w", t, err) } debug.Logf("readElement: vr: %s", vr) vl, err := r.readVL(readImplicit, *t, vr) if err != nil { - return nil, err + return nil, fmt.Errorf("readElement: error when reading VL for element %v: %w", t, err) } debug.Logf("readElement: vl: %d", vl) val, err := r.readValue(*t, vr, vl, readImplicit, d, fc) if err != nil { - log.Println("error reading value ", err) - return nil, err + return nil, fmt.Errorf("readElement: error when reading value for element %v: %w", t, err) } return &Element{Tag: *t, ValueRepresentation: tag.GetVRKind(*t, vr), RawValueRepresentation: vr, ValueLength: vl, Value: val}, nil @@ -801,16 +794,16 @@ func (r *reader) readElement(d *Dataset, fc chan<- *frame.Frame) (*Element, erro func (r *reader) readRawItem(shouldSkip bool) ([]byte, bool, error) { t, err := r.readTag() if err != nil { - return nil, true, err + return nil, true, fmt.Errorf("readRawItem: error when reading item tag: %w", err) } // Item is always encoded implicit. PS3.6 7.5 vr, err := r.readVR(true, *t) if err != nil { - return nil, true, err + return nil, true, fmt.Errorf("readRawItem: error when reading VR for item %v: %w", t, err) } vl, err := r.readVL(true, *t, vr) if err != nil { - return nil, true, err + return nil, true, fmt.Errorf("readRawItem: error when reading VL for item %v: %w", t, err) } if *t == tag.SequenceDelimitationItem { @@ -833,14 +826,13 @@ func (r *reader) readRawItem(shouldSkip bool) ([]byte, bool, error) { if shouldSkip { if err := r.rawReader.Skip(int64(vl)); err != nil { - return nil, false, err + return nil, false, fmt.Errorf("readRawItem: error when skipping item %v (vl=%d): %w", t, vl, err) } } else { data := make([]byte, vl) _, err = io.ReadFull(r.rawReader, data) if err != nil { - log.Println(err) - return nil, false, err + return nil, false, fmt.Errorf("readRawItem: error when reading item %v value: %w", t, err) } return data, false, nil } diff --git a/read_test.go b/read_test.go index 5b30afc2..2527f5de 100644 --- a/read_test.go +++ b/read_test.go @@ -5,6 +5,7 @@ import ( "bytes" "encoding/binary" "errors" + "io" "math/rand" "strconv" "testing" @@ -40,6 +41,16 @@ func TestReadTag(t *testing.T) { wantTag: tag.Tag{0x0011, 0x0010}, wantErr: nil, }, + { + name: "expected EOF on group read", + data: []byte{0x1}, + wantErr: io.EOF, + }, + { + name: "expected EOF on element read", + data: []byte{0x1, 0x2}, + wantErr: io.EOF, + }, } for _, tc := range cases { @@ -50,11 +61,11 @@ func TestReadTag(t *testing.T) { rawReader: dicomio.NewReader(bufio.NewReader(data), binary.LittleEndian, int64(data.Len())), } gotTag, err := r.readTag() - if err != tc.wantErr { + if !errors.Is(err, tc.wantErr) { t.Errorf("TestReadTag: unexpected err. got: %v, want: %v", err, tc.wantErr) } - if !gotTag.Equals(tc.wantTag) { + if gotTag != nil && !gotTag.Equals(tc.wantTag) { t.Errorf("TestReadTag: unexpected output. got: %v, want: %v", gotTag, tc.wantTag) } }) @@ -99,7 +110,7 @@ func TestReadFloat_float64(t *testing.T) { rawReader: dicomio.NewReader(bufio.NewReader(&data), binary.LittleEndian, int64(data.Len())), } got, err := r.readFloat(tag.Tag{}, tc.VR, uint32(data.Len())) - if err != tc.expectedErr { + if !errors.Is(err, tc.expectedErr) { t.Fatalf("readFloat(r, tg, %s, %d) got unexpected error: got: %v, want: %v", tc.VR, data.Len(), err, tc.expectedErr) } if diff := cmp.Diff(got, tc.want, cmp.AllowUnexported(floatsValue{})); diff != "" { @@ -146,7 +157,7 @@ func TestReadFloat_float32(t *testing.T) { rawReader: dicomio.NewReader(bufio.NewReader(&data), binary.LittleEndian, int64(data.Len())), } got, err := r.readFloat(tag.Tag{}, tc.VR, uint32(data.Len())) - if err != tc.expectedErr { + if !errors.Is(err, tc.expectedErr) { t.Fatalf("readFloat(r, tg, %s, %d) got unexpected error: got: %v, want: %v", tc.VR, data.Len(), err, tc.expectedErr) } if diff := cmp.Diff(got, tc.want, cmp.AllowUnexported(floatsValue{})); diff != "" { @@ -196,7 +207,7 @@ func TestReadOWBytes(t *testing.T) { r := &reader{rawReader: dicomio.NewReader(bufio.NewReader(&data), binary.LittleEndian, int64(data.Len()))} got, err := r.readBytes(tag.Tag{}, tc.VR, uint32(data.Len())) - if err != tc.expectedErr { + if !errors.Is(err, tc.expectedErr) { t.Fatalf("readBytes(r, tg, %s, %d) got unexpected error: got: %v, want: %v", tc.VR, data.Len(), err, tc.expectedErr) } if diff := cmp.Diff(got, tc.want, cmp.AllowUnexported(bytesValue{})); diff != "" {