From c2c2f93ff080fd4a2894c483d51a6661c4fc941e Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 8 Jul 2024 14:26:56 +0100 Subject: [PATCH 1/5] Return subfield IDs upon unpack error --- errors.go => errors/errors.go | 3 +- field/composite.go | 60 ++++++--- field/composite_test.go | 6 + message.go | 11 +- message_test.go | 246 +++++++++++++++++++++++++++++++++- 5 files changed, 300 insertions(+), 26 deletions(-) rename errors.go => errors/errors.go (92%) diff --git a/errors.go b/errors/errors.go similarity index 92% rename from errors.go rename to errors/errors.go index b0643eb8..fd459e18 100644 --- a/errors.go +++ b/errors/errors.go @@ -1,10 +1,11 @@ -package iso8583 +package errors // UnpackError returns error with possibility to access RawMessage when // connection failed to unpack message type UnpackError struct { Err error FieldID string + FieldIDs []string RawMessage []byte } diff --git a/field/composite.go b/field/composite.go index 0db1399f..e933d206 100644 --- a/field/composite.go +++ b/field/composite.go @@ -10,6 +10,7 @@ import ( "sync" "github.com/moov-io/iso8583/encoding" + mooverrors "github.com/moov-io/iso8583/errors" "github.com/moov-io/iso8583/prefix" "github.com/moov-io/iso8583/sort" "github.com/moov-io/iso8583/utils" @@ -312,7 +313,7 @@ func (f *Composite) Unpack(data []byte) (int, error) { // data is stripped of the prefix before it is provided to unpack(). // Therefore, it is unaware of when to stop parsing unless we bound the // length of the slice by the data length. - read, err := f.unpack(data[offset:offset+dataLen], isVariableLength) + read, err := f.wrapErrorUnpack(data[offset:offset+dataLen], isVariableLength) if err != nil { return 0, err } @@ -331,7 +332,7 @@ func (f *Composite) SetBytes(data []byte) error { f.mu.Lock() defer f.mu.Unlock() - _, err := f.unpack(data, false) + _, err := f.wrapErrorUnpack(data, false) return err } @@ -515,7 +516,28 @@ func (f *Composite) packByTag() ([]byte, error) { return packed, nil } -func (f *Composite) unpack(data []byte, isVariableLength bool) (int, error) { +// wrapErrorUnpack calls the core unpacking logic and wraps any +// errors in a *UnpackError. It assumes that the mutex is already +// locked by the caller. +func (f *Composite) wrapErrorUnpack(src []byte, isVariableLength bool) (int, error) { + offset, tagID, err := f.unpack(src, isVariableLength) + subfields := []string{} + var unpackErr *mooverrors.UnpackError + if errors.As(err, &unpackErr) { + subfields = unpackErr.FieldIDs + } + if err != nil { + return offset, &mooverrors.UnpackError{ + Err: err, + FieldID: tagID, + FieldIDs: append([]string{tagID}, subfields...), + RawMessage: src, + } + } + return offset, nil +} + +func (f *Composite) unpack(data []byte, isVariableLength bool) (int, string, error) { if f.bitmap() != nil { return f.unpackSubfieldsByBitmap(data) } @@ -525,7 +547,7 @@ func (f *Composite) unpack(data []byte, isVariableLength bool) (int, error) { return f.unpackSubfields(data, isVariableLength) } -func (f *Composite) unpackSubfields(data []byte, isVariableLength bool) (int, error) { +func (f *Composite) unpackSubfields(data []byte, isVariableLength bool) (int, string, error) { offset := 0 for _, tag := range f.orderedSpecFieldTags { field, ok := f.subfields[tag] @@ -535,7 +557,7 @@ func (f *Composite) unpackSubfields(data []byte, isVariableLength bool) (int, er read, err := field.Unpack(data[offset:]) if err != nil { - return 0, fmt.Errorf("failed to unpack subfield %v: %w", tag, err) + return 0, tag, fmt.Errorf("failed to unpack subfield %v: %w", tag, err) } f.setSubfields[tag] = struct{}{} @@ -547,10 +569,10 @@ func (f *Composite) unpackSubfields(data []byte, isVariableLength bool) (int, er } } - return offset, nil + return offset, "", nil } -func (f *Composite) unpackSubfieldsByBitmap(data []byte) (int, error) { +func (f *Composite) unpackSubfieldsByBitmap(data []byte) (int, string, error) { var off int // Reset fields that were set. @@ -560,7 +582,7 @@ func (f *Composite) unpackSubfieldsByBitmap(data []byte) (int, error) { read, err := f.bitmap().Unpack(data[off:]) if err != nil { - return 0, fmt.Errorf("failed to unpack bitmap: %w", err) + return 0, "", fmt.Errorf("failed to unpack bitmap: %w", err) } off += read @@ -571,12 +593,12 @@ func (f *Composite) unpackSubfieldsByBitmap(data []byte) (int, error) { fl, ok := f.subfields[iStr] if !ok { - return 0, fmt.Errorf("failed to unpack subfield %s: no specification found", iStr) + return 0, iStr, fmt.Errorf("failed to unpack subfield %s: no specification found", iStr) } read, err = fl.Unpack(data[off:]) if err != nil { - return 0, fmt.Errorf("failed to unpack subfield %s (%s): %w", iStr, fl.Spec().Description, err) + return 0, iStr, fmt.Errorf("failed to unpack subfield %s (%s): %w", iStr, fl.Spec().Description, err) } f.setSubfields[iStr] = struct{}{} @@ -585,7 +607,7 @@ func (f *Composite) unpackSubfieldsByBitmap(data []byte) (int, error) { } } - return off, nil + return off, "", nil } const ( @@ -596,12 +618,12 @@ const ( maxLenOfUnknownTag = math.MaxInt ) -func (f *Composite) unpackSubfieldsByTag(data []byte) (int, error) { +func (f *Composite) unpackSubfieldsByTag(data []byte) (int, string, error) { offset := 0 for offset < len(data) { tagBytes, read, err := f.spec.Tag.Enc.Decode(data[offset:], f.spec.Tag.Length) if err != nil { - return 0, fmt.Errorf("failed to unpack subfield Tag: %w", err) + return 0, "", fmt.Errorf("failed to unpack subfield Tag: %w", err) } offset += read @@ -623,15 +645,15 @@ func (f *Composite) unpackSubfieldsByTag(data []byte) (int, error) { maxLen = maxLenOfUnknownTag } - fieldLength, readed, err := pref.DecodeLength(maxLen, data[offset:]) + fieldLength, read, err := pref.DecodeLength(maxLen, data[offset:]) if err != nil { - return 0, err + return 0, "", err } - offset += fieldLength + readed + offset += fieldLength + read continue } - return 0, fmt.Errorf("failed to unpack subfield %v: field not defined in Spec", tag) + return 0, tag, fmt.Errorf("failed to unpack subfield %v: field not defined in Spec", tag) } field, ok := f.subfields[tag] @@ -641,14 +663,14 @@ func (f *Composite) unpackSubfieldsByTag(data []byte) (int, error) { read, err = field.Unpack(data[offset:]) if err != nil { - return 0, fmt.Errorf("failed to unpack subfield %v: %w", tag, err) + return 0, tag, fmt.Errorf("failed to unpack subfield %v: %w", tag, err) } f.setSubfields[tag] = struct{}{} offset += read } - return offset, nil + return offset, "", nil } func (f *Composite) skipUnknownTLVTags() bool { diff --git a/field/composite_test.go b/field/composite_test.go index adef77fb..53dc52e6 100644 --- a/field/composite_test.go +++ b/field/composite_test.go @@ -8,9 +8,11 @@ import ( "testing" "github.com/moov-io/iso8583/encoding" + mooverrors "github.com/moov-io/iso8583/errors" "github.com/moov-io/iso8583/padding" "github.com/moov-io/iso8583/prefix" "github.com/moov-io/iso8583/sort" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -799,6 +801,10 @@ func TestCompositePacking(t *testing.T) { read, err := composite.Unpack([]byte("ABCDEF")) require.Equal(t, 0, read) require.Error(t, err) + var unpackError *mooverrors.UnpackError + require.ErrorAs(t, err, &unpackError) + assert.Equal(t, "3", unpackError.FieldID) + assert.Equal(t, []string{"3"}, unpackError.FieldIDs) require.EqualError(t, err, "failed to unpack subfield 3: failed to set bytes: failed to convert into number") require.ErrorIs(t, err, strconv.ErrSyntax) }) diff --git a/message.go b/message.go index d9411ab0..7e4bee5b 100644 --- a/message.go +++ b/message.go @@ -9,6 +9,7 @@ import ( "strconv" "sync" + mooverrors "github.com/moov-io/iso8583/errors" "github.com/moov-io/iso8583/field" "github.com/moov-io/iso8583/utils" ) @@ -175,7 +176,7 @@ func (m *Message) Pack() ([]byte, error) { func (m *Message) wrapErrorPack() ([]byte, error) { data, err := m.pack() if err != nil { - return nil, &PackError{Err: err} + return nil, &mooverrors.PackError{Err: err} } return data, nil @@ -238,9 +239,15 @@ func (m *Message) Unpack(src []byte) error { // locked by the caller. func (m *Message) wrapErrorUnpack(src []byte) error { if fieldID, err := m.unpack(src); err != nil { - return &UnpackError{ + subfields := []string{} + var unpackErr *mooverrors.UnpackError + if errors.As(err, &unpackErr) { + subfields = unpackErr.FieldIDs + } + return &mooverrors.UnpackError{ Err: err, FieldID: fieldID, + FieldIDs: append([]string{fieldID}, subfields...), RawMessage: src, } } diff --git a/message_test.go b/message_test.go index 9e7170ba..dcfb24cd 100644 --- a/message_test.go +++ b/message_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/moov-io/iso8583/encoding" + mooverrors "github.com/moov-io/iso8583/errors" "github.com/moov-io/iso8583/field" "github.com/moov-io/iso8583/padding" "github.com/moov-io/iso8583/prefix" @@ -722,7 +723,7 @@ func TestPackUnpack(t *testing.T) { _, err := message.Pack() require.Error(t, err) - var packErr *PackError + var packErr *mooverrors.PackError require.ErrorAs(t, err, &packErr) }) @@ -733,7 +734,7 @@ func TestPackUnpack(t *testing.T) { require.Error(t, err) - var unpackError *UnpackError + var unpackError *mooverrors.UnpackError require.ErrorAs(t, err, &unpackError) }) @@ -746,7 +747,7 @@ func TestPackUnpack(t *testing.T) { require.Error(t, err) - var unpackError *UnpackError + var unpackError *mooverrors.UnpackError require.ErrorAs(t, err, &unpackError) require.Equal(t, rawMsg, unpackError.RawMessage) }) @@ -760,7 +761,7 @@ func TestPackUnpack(t *testing.T) { err := message.Unpack([]byte(rawMsg)) require.Error(t, err) - var unpackError *UnpackError + var unpackError *mooverrors.UnpackError require.ErrorAs(t, err, &unpackError) assert.Equal(t, unpackError.FieldID, "120") @@ -807,6 +808,243 @@ func TestPackUnpack(t *testing.T) { assert.Empty(t, data.F120) }) + t.Run("Unpack data field error on subfield returns both numbers and partial message", func(t *testing.T) { + type TestISOF3MixedData struct { + F1 *field.String + F2 *field.Numeric + F3 *field.String + } + + type TestISOShortData struct { + F2 *field.String + F3 *TestISOF3MixedData + } + + shortSpec := &MessageSpec{ + Fields: map[int]field.Field{ + 0: field.NewString(&field.Spec{ + Length: 4, + Description: "Message Type Indicator", + Enc: encoding.ASCII, + Pref: prefix.ASCII.Fixed, + }), + 1: field.NewBitmap(&field.Spec{ + Description: "Bitmap", + Enc: encoding.Binary, + Pref: prefix.ASCII.Fixed, + }), + 2: field.NewString(&field.Spec{ + Length: 19, + Description: "Primary Account Number", + Enc: encoding.ASCII, + Pref: prefix.ASCII.LL, + }), + 3: field.NewComposite(&field.Spec{ + Length: 6, + Description: "Processing Code", + Pref: prefix.ASCII.Fixed, + Tag: &field.TagSpec{ + Sort: sort.StringsByInt, + }, + Subfields: map[string]field.Field{ + "1": field.NewString(&field.Spec{ + Length: 2, + Description: "Transaction Type", + Enc: encoding.ASCII, + Pref: prefix.ASCII.Fixed, + }), + "2": field.NewNumeric(&field.Spec{ + Length: 2, + Description: "From Account", + Enc: encoding.ASCII, + Pref: prefix.ASCII.Fixed, + Pad: padding.Left('0'), + }), + "3": field.NewString(&field.Spec{ + Length: 2, + Description: "To Account", + Enc: encoding.ASCII, + Pref: prefix.ASCII.Fixed, + }), + }, + }), + }, + } + message := NewMessage(shortSpec) + + message.MTI("0100") + + got, err := message.Pack() + require.NotNil(t, got) + require.NoError(t, err) + + // Field F3SF2 has a letter in, which will make it fail + rawMsg := []byte{0x30, 0x31, 0x30, 0x30, // MTI F0 + 0x60, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // Bitmap F1 + 0x31, 0x36, // Tag for F2 + 0x34, 0x32, 0x37, 0x36, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, //F2 + 0x30, 0x30, 0x30, 0x51, 0x30, 0x30, //F3 + } + + // assert.Equal(t, rawMsg, got) + + err = message.Unpack([]byte(rawMsg)) + + require.Error(t, err) + var unpackError *mooverrors.UnpackError + require.ErrorAs(t, err, &unpackError) + assert.Equal(t, "3", unpackError.FieldID) + assert.Equal(t, []string{"3", "2"}, unpackError.FieldIDs) + + s, err := message.GetString(2) + require.NoError(t, err) + require.Equal(t, "4276555555555555", s) + + s, err = message.GetString(3) + require.NoError(t, err) + require.Equal(t, "00", s) + + data := &TestISOShortData{} + require.NoError(t, message.Unmarshal(data)) + + assert.Equal(t, "4276555555555555", data.F2.Value()) + }) + + t.Run("Unpack data field error on subfield in nested spec returns both numbers and partial message", func(t *testing.T) { + type TestDateAndTimeData struct { + F1 *field.String + F2 *field.Numeric + } + + type TestISOF3MixedData struct { + F1 *field.String + F2 *TestDateAndTimeData + F3 *field.String + } + + type TestISOShortData struct { + F2 *field.String + F3 *TestISOF3MixedData + } + + shortSpec := &MessageSpec{ + Fields: map[int]field.Field{ + 0: field.NewString(&field.Spec{ + Length: 4, + Description: "Message Type Indicator", + Enc: encoding.ASCII, + Pref: prefix.ASCII.Fixed, + }), + 1: field.NewBitmap(&field.Spec{ + Description: "Bitmap", + Enc: encoding.Binary, + Pref: prefix.ASCII.Fixed, + }), + 2: field.NewString(&field.Spec{ + Length: 19, + Description: "Primary Account Number", + Enc: encoding.ASCII, + Pref: prefix.ASCII.LL, + }), + 3: field.NewComposite(&field.Spec{ + Length: 14, + Description: "Processing Code", + Pref: prefix.ASCII.LL, + Tag: &field.TagSpec{ + Sort: sort.StringsByInt, + }, + Subfields: map[string]field.Field{ + "1": field.NewString(&field.Spec{ + Length: 2, + Description: "Transaction Type", + Enc: encoding.ASCII, + Pref: prefix.ASCII.Fixed, + }), + "2": field.NewComposite(&field.Spec{ + Length: 10, + Description: "Authorization System Advice Date and Time", + Pref: prefix.ASCII.Fixed, + Tag: &field.TagSpec{ + Sort: sort.StringsByInt, + }, + Subfields: map[string]field.Field{ + "1": field.NewString(&field.Spec{ + Length: 4, + Description: "Date", + Enc: encoding.ASCII, + Pref: prefix.ASCII.Fixed, + }), + "2": field.NewNumeric(&field.Spec{ + Length: 6, + Description: "Time", + Enc: encoding.ASCII, + Pref: prefix.ASCII.Fixed, + }), + }, + }), + "3": field.NewString(&field.Spec{ + Length: 2, + Description: "To Account", + Enc: encoding.ASCII, + Pref: prefix.ASCII.Fixed, + }), + }, + }), + }, + } + message := NewMessage(shortSpec) + err := message.Marshal(&TestISOShortData{ + F2: field.NewStringValue("4276555555555555"), + F3: &TestISOF3MixedData{ + F1: field.NewStringValue("00"), + F2: &TestDateAndTimeData{ + F1: field.NewStringValue("1111"), + F2: field.NewNumericValue(123456), + }, + F3: field.NewStringValue("00"), + }, + }) + require.NoError(t, err) + + message.MTI("0100") + + got, err := message.Pack() + require.NotNil(t, got) + require.NoError(t, err) + + // Field F3SF2SF2 is numeric type but has a letter in, which will make unpack error + rawMsg := []byte{0x30, 0x31, 0x30, 0x30, // MTI F0 + 0x60, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // Bitmap F1 + 0x31, 0x36, // Tag for F2 + 0x34, 0x32, 0x37, 0x36, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, //F2 + 0x31, 0x34, 0x30, 0x30, 0x31, 0x31, 0x31, //F3 SF1 + 0x31, 0x31, 0x32, 0x33, 0x34, 0x35, 0x51, //F3 SF2 + 0x30, 0x30, //F3 SF3 + } + + err = message.Unpack([]byte(rawMsg)) + + require.Error(t, err) + var unpackError *mooverrors.UnpackError + require.ErrorAs(t, err, &unpackError) + assert.Equal(t, "3", unpackError.FieldID) + assert.Equal(t, []string{"3", "2", "2"}, unpackError.FieldIDs) + + s, err := message.GetString(2) + require.NoError(t, err) + require.Equal(t, "4276555555555555", s) + + s, err = message.GetString(3) + require.NoError(t, err) + require.Equal(t, "00111112345600", s) + + data := &TestISOShortData{} + require.NoError(t, message.Unmarshal(data)) + + assert.Equal(t, "4276555555555555", data.F2.Value()) + require.Nil(t, data.F3) + }) + // this test should check that BCD fields are packed and // unpacked correctly it's a confirmation that issue // https://github.com/moov-io/iso8583/issues/220 is fixed From cbb601e2e28f28ae9a6b4693b4faed522595ffe6 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 8 Jul 2024 14:36:25 +0100 Subject: [PATCH 2/5] Tidy test --- message_test.go | 32 ++++---------------------------- 1 file changed, 4 insertions(+), 28 deletions(-) diff --git a/message_test.go b/message_test.go index dcfb24cd..4d434e3d 100644 --- a/message_test.go +++ b/message_test.go @@ -871,13 +871,8 @@ func TestPackUnpack(t *testing.T) { }, } message := NewMessage(shortSpec) - message.MTI("0100") - got, err := message.Pack() - require.NotNil(t, got) - require.NoError(t, err) - // Field F3SF2 has a letter in, which will make it fail rawMsg := []byte{0x30, 0x31, 0x30, 0x30, // MTI F0 0x60, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // Bitmap F1 @@ -886,9 +881,7 @@ func TestPackUnpack(t *testing.T) { 0x30, 0x30, 0x30, 0x51, 0x30, 0x30, //F3 } - // assert.Equal(t, rawMsg, got) - - err = message.Unpack([]byte(rawMsg)) + err := message.Unpack([]byte(rawMsg)) require.Error(t, err) var unpackError *mooverrors.UnpackError @@ -993,26 +986,9 @@ func TestPackUnpack(t *testing.T) { }, } message := NewMessage(shortSpec) - err := message.Marshal(&TestISOShortData{ - F2: field.NewStringValue("4276555555555555"), - F3: &TestISOF3MixedData{ - F1: field.NewStringValue("00"), - F2: &TestDateAndTimeData{ - F1: field.NewStringValue("1111"), - F2: field.NewNumericValue(123456), - }, - F3: field.NewStringValue("00"), - }, - }) - require.NoError(t, err) - message.MTI("0100") - got, err := message.Pack() - require.NotNil(t, got) - require.NoError(t, err) - - // Field F3SF2SF2 is numeric type but has a letter in, which will make unpack error + // Field F3SF2SF2 is numeric type but has a letter in, which will cause an unpack error rawMsg := []byte{0x30, 0x31, 0x30, 0x30, // MTI F0 0x60, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // Bitmap F1 0x31, 0x36, // Tag for F2 @@ -1022,7 +998,7 @@ func TestPackUnpack(t *testing.T) { 0x30, 0x30, //F3 SF3 } - err = message.Unpack([]byte(rawMsg)) + err := message.Unpack([]byte(rawMsg)) require.Error(t, err) var unpackError *mooverrors.UnpackError @@ -1036,7 +1012,7 @@ func TestPackUnpack(t *testing.T) { s, err = message.GetString(3) require.NoError(t, err) - require.Equal(t, "00111112345600", s) + require.Equal(t, "00", s) data := &TestISOShortData{} require.NoError(t, message.Unmarshal(data)) From 97b6a9719e0f31b512ae25b03e48173f0cfac617 Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 12 Jul 2024 10:57:42 +0100 Subject: [PATCH 3/5] Comments --- errors/errors.go | 10 ++++++---- field/composite.go | 10 +++++----- message.go | 8 ++++---- message_test.go | 10 ++++++---- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/errors/errors.go b/errors/errors.go index fd459e18..15840fc8 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -1,10 +1,12 @@ package errors -// UnpackError returns error with possibility to access RawMessage when -// connection failed to unpack message +// UnpackError returns an error with the possibility to access the RawMessage when +// the connection failed to unpack the message type UnpackError struct { - Err error - FieldID string + Err error + // the field ID of the field on which unpacking errored + FieldID string + // the field ID and subfield IDs (if any) that errored ordered from outermost inwards FieldIDs []string RawMessage []byte } diff --git a/field/composite.go b/field/composite.go index e933d206..850b1010 100644 --- a/field/composite.go +++ b/field/composite.go @@ -521,12 +521,12 @@ func (f *Composite) packByTag() ([]byte, error) { // locked by the caller. func (f *Composite) wrapErrorUnpack(src []byte, isVariableLength bool) (int, error) { offset, tagID, err := f.unpack(src, isVariableLength) - subfields := []string{} - var unpackErr *mooverrors.UnpackError - if errors.As(err, &unpackErr) { - subfields = unpackErr.FieldIDs - } if err != nil { + subfields := []string{} + var unpackErr *mooverrors.UnpackError + if errors.As(err, &unpackErr) { + subfields = unpackErr.FieldIDs + } return offset, &mooverrors.UnpackError{ Err: err, FieldID: tagID, diff --git a/message.go b/message.go index 7e4bee5b..aca03ea5 100644 --- a/message.go +++ b/message.go @@ -9,7 +9,7 @@ import ( "strconv" "sync" - mooverrors "github.com/moov-io/iso8583/errors" + iso8583errors "github.com/moov-io/iso8583/errors" "github.com/moov-io/iso8583/field" "github.com/moov-io/iso8583/utils" ) @@ -176,7 +176,7 @@ func (m *Message) Pack() ([]byte, error) { func (m *Message) wrapErrorPack() ([]byte, error) { data, err := m.pack() if err != nil { - return nil, &mooverrors.PackError{Err: err} + return nil, &iso8583errors.PackError{Err: err} } return data, nil @@ -240,11 +240,11 @@ func (m *Message) Unpack(src []byte) error { func (m *Message) wrapErrorUnpack(src []byte) error { if fieldID, err := m.unpack(src); err != nil { subfields := []string{} - var unpackErr *mooverrors.UnpackError + var unpackErr *iso8583errors.UnpackError if errors.As(err, &unpackErr) { subfields = unpackErr.FieldIDs } - return &mooverrors.UnpackError{ + return &iso8583errors.UnpackError{ Err: err, FieldID: fieldID, FieldIDs: append([]string{fieldID}, subfields...), diff --git a/message_test.go b/message_test.go index 4d434e3d..fc2d01e8 100644 --- a/message_test.go +++ b/message_test.go @@ -988,14 +988,16 @@ func TestPackUnpack(t *testing.T) { message := NewMessage(shortSpec) message.MTI("0100") - // Field F3SF2SF2 is numeric type but has a letter in, which will cause an unpack error + // Field F3SF2SF2 is a numeric type but has a letter in, which will cause an unpack error rawMsg := []byte{0x30, 0x31, 0x30, 0x30, // MTI F0 0x60, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // Bitmap F1 0x31, 0x36, // Tag for F2 0x34, 0x32, 0x37, 0x36, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, //F2 - 0x31, 0x34, 0x30, 0x30, 0x31, 0x31, 0x31, //F3 SF1 - 0x31, 0x31, 0x32, 0x33, 0x34, 0x35, 0x51, //F3 SF2 - 0x30, 0x30, //F3 SF3 + 0x31, 0x34, // Tag for F3 + 0x30, 0x30, // F3 SF1 + 0x31, 0x31, 0x31, 0x31, //F3 SF2 SF1 + 0x31, 0x32, 0x33, 0x34, 0x35, 0x51, //F3 SF2 SF2 + 0x30, 0x30, // F3 SF3 } err := message.Unpack([]byte(rawMsg)) From 41e97e56f185caad0a40db051e210ba7b3464e9a Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 12 Jul 2024 11:05:56 +0100 Subject: [PATCH 4/5] Make linter happy --- message_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/message_test.go b/message_test.go index fc2d01e8..78ccc756 100644 --- a/message_test.go +++ b/message_test.go @@ -995,8 +995,8 @@ func TestPackUnpack(t *testing.T) { 0x34, 0x32, 0x37, 0x36, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, //F2 0x31, 0x34, // Tag for F3 0x30, 0x30, // F3 SF1 - 0x31, 0x31, 0x31, 0x31, //F3 SF2 SF1 - 0x31, 0x32, 0x33, 0x34, 0x35, 0x51, //F3 SF2 SF2 + 0x31, 0x31, 0x31, 0x31, //F3 SF2 Subfield 1 + 0x31, 0x32, 0x33, 0x34, 0x35, 0x51, //F3 SF2 Subfield 2 0x30, 0x30, // F3 SF3 } From cd9ca5785f62152c1d793032f3f2fdbbdfe151bf Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 15 Jul 2024 16:52:50 +0100 Subject: [PATCH 5/5] Remove FieldIDs --- errors/errors.go | 28 +++++++++++++++++++++++----- field/composite.go | 10 ++-------- field/composite_test.go | 6 +++--- message.go | 6 ------ message_test.go | 18 +++++++++--------- 5 files changed, 37 insertions(+), 31 deletions(-) diff --git a/errors/errors.go b/errors/errors.go index 15840fc8..ec2f9060 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -1,13 +1,14 @@ package errors +import ( + "errors" +) + // UnpackError returns an error with the possibility to access the RawMessage when // the connection failed to unpack the message type UnpackError struct { - Err error - // the field ID of the field on which unpacking errored - FieldID string - // the field ID and subfield IDs (if any) that errored ordered from outermost inwards - FieldIDs []string + Err error + FieldID string RawMessage []byte } @@ -19,6 +20,23 @@ func (e *UnpackError) Unwrap() error { return e.Err } +// FieldIDs returns the list of field and subfield IDs (if any) that errored from outermost inwards +func (e *UnpackError) FieldIDs() []string { + fieldIDs := []string{e.FieldID} + err := e.Err + var unpackError *UnpackError + for { + if errors.As(err, &unpackError) { + fieldIDs = append(fieldIDs, unpackError.FieldID) + err = unpackError.Unwrap() + } else { + break + } + } + + return fieldIDs +} + type PackError struct { Err error } diff --git a/field/composite.go b/field/composite.go index 850b1010..b119b120 100644 --- a/field/composite.go +++ b/field/composite.go @@ -10,7 +10,7 @@ import ( "sync" "github.com/moov-io/iso8583/encoding" - mooverrors "github.com/moov-io/iso8583/errors" + iso8583errors "github.com/moov-io/iso8583/errors" "github.com/moov-io/iso8583/prefix" "github.com/moov-io/iso8583/sort" "github.com/moov-io/iso8583/utils" @@ -522,15 +522,9 @@ func (f *Composite) packByTag() ([]byte, error) { func (f *Composite) wrapErrorUnpack(src []byte, isVariableLength bool) (int, error) { offset, tagID, err := f.unpack(src, isVariableLength) if err != nil { - subfields := []string{} - var unpackErr *mooverrors.UnpackError - if errors.As(err, &unpackErr) { - subfields = unpackErr.FieldIDs - } - return offset, &mooverrors.UnpackError{ + return offset, &iso8583errors.UnpackError{ Err: err, FieldID: tagID, - FieldIDs: append([]string{tagID}, subfields...), RawMessage: src, } } diff --git a/field/composite_test.go b/field/composite_test.go index 53dc52e6..7651b88e 100644 --- a/field/composite_test.go +++ b/field/composite_test.go @@ -8,7 +8,7 @@ import ( "testing" "github.com/moov-io/iso8583/encoding" - mooverrors "github.com/moov-io/iso8583/errors" + iso8583errors "github.com/moov-io/iso8583/errors" "github.com/moov-io/iso8583/padding" "github.com/moov-io/iso8583/prefix" "github.com/moov-io/iso8583/sort" @@ -801,10 +801,10 @@ func TestCompositePacking(t *testing.T) { read, err := composite.Unpack([]byte("ABCDEF")) require.Equal(t, 0, read) require.Error(t, err) - var unpackError *mooverrors.UnpackError + var unpackError *iso8583errors.UnpackError require.ErrorAs(t, err, &unpackError) assert.Equal(t, "3", unpackError.FieldID) - assert.Equal(t, []string{"3"}, unpackError.FieldIDs) + assert.Equal(t, []string{"3"}, unpackError.FieldIDs()) require.EqualError(t, err, "failed to unpack subfield 3: failed to set bytes: failed to convert into number") require.ErrorIs(t, err, strconv.ErrSyntax) }) diff --git a/message.go b/message.go index aca03ea5..1a6640a7 100644 --- a/message.go +++ b/message.go @@ -239,15 +239,9 @@ func (m *Message) Unpack(src []byte) error { // locked by the caller. func (m *Message) wrapErrorUnpack(src []byte) error { if fieldID, err := m.unpack(src); err != nil { - subfields := []string{} - var unpackErr *iso8583errors.UnpackError - if errors.As(err, &unpackErr) { - subfields = unpackErr.FieldIDs - } return &iso8583errors.UnpackError{ Err: err, FieldID: fieldID, - FieldIDs: append([]string{fieldID}, subfields...), RawMessage: src, } } diff --git a/message_test.go b/message_test.go index 78ccc756..09fd1851 100644 --- a/message_test.go +++ b/message_test.go @@ -10,7 +10,7 @@ import ( "time" "github.com/moov-io/iso8583/encoding" - mooverrors "github.com/moov-io/iso8583/errors" + iso8583errors "github.com/moov-io/iso8583/errors" "github.com/moov-io/iso8583/field" "github.com/moov-io/iso8583/padding" "github.com/moov-io/iso8583/prefix" @@ -723,7 +723,7 @@ func TestPackUnpack(t *testing.T) { _, err := message.Pack() require.Error(t, err) - var packErr *mooverrors.PackError + var packErr *iso8583errors.PackError require.ErrorAs(t, err, &packErr) }) @@ -734,7 +734,7 @@ func TestPackUnpack(t *testing.T) { require.Error(t, err) - var unpackError *mooverrors.UnpackError + var unpackError *iso8583errors.UnpackError require.ErrorAs(t, err, &unpackError) }) @@ -747,7 +747,7 @@ func TestPackUnpack(t *testing.T) { require.Error(t, err) - var unpackError *mooverrors.UnpackError + var unpackError *iso8583errors.UnpackError require.ErrorAs(t, err, &unpackError) require.Equal(t, rawMsg, unpackError.RawMessage) }) @@ -761,7 +761,7 @@ func TestPackUnpack(t *testing.T) { err := message.Unpack([]byte(rawMsg)) require.Error(t, err) - var unpackError *mooverrors.UnpackError + var unpackError *iso8583errors.UnpackError require.ErrorAs(t, err, &unpackError) assert.Equal(t, unpackError.FieldID, "120") @@ -884,10 +884,10 @@ func TestPackUnpack(t *testing.T) { err := message.Unpack([]byte(rawMsg)) require.Error(t, err) - var unpackError *mooverrors.UnpackError + var unpackError *iso8583errors.UnpackError require.ErrorAs(t, err, &unpackError) assert.Equal(t, "3", unpackError.FieldID) - assert.Equal(t, []string{"3", "2"}, unpackError.FieldIDs) + assert.Equal(t, []string{"3", "2"}, unpackError.FieldIDs()) s, err := message.GetString(2) require.NoError(t, err) @@ -1003,10 +1003,10 @@ func TestPackUnpack(t *testing.T) { err := message.Unpack([]byte(rawMsg)) require.Error(t, err) - var unpackError *mooverrors.UnpackError + var unpackError *iso8583errors.UnpackError require.ErrorAs(t, err, &unpackError) assert.Equal(t, "3", unpackError.FieldID) - assert.Equal(t, []string{"3", "2", "2"}, unpackError.FieldIDs) + assert.Equal(t, []string{"3", "2", "2"}, unpackError.FieldIDs()) s, err := message.GetString(2) require.NoError(t, err)