From 508a839b39a3fb290a46822e366d145cc6fdefe3 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 20 Nov 2023 17:07:18 +0000 Subject: [PATCH 1/6] Return partial message when unpack errors --- errors.go | 4 +- field/numeric.go | 4 +- field/string.go | 4 +- message.go | 48 +++++++++---- message_test.go | 179 +++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 220 insertions(+), 19 deletions(-) diff --git a/errors.go b/errors.go index 3a63def..6aea3a3 100644 --- a/errors.go +++ b/errors.go @@ -4,6 +4,7 @@ package iso8583 // connection failed to unpack message type UnpackError struct { Err error + Fields []string RawMessage []byte } @@ -16,7 +17,8 @@ func (e *UnpackError) Unwrap() error { } type PackError struct { - Err error + Fields []string + Err error } func (e *PackError) Error() string { diff --git a/field/numeric.go b/field/numeric.go index 1c0f854..a3c485c 100644 --- a/field/numeric.go +++ b/field/numeric.go @@ -110,7 +110,7 @@ func (f *Numeric) Unpack(data []byte) (int, error) { raw, read, err := f.spec.Enc.Decode(data[prefBytes:], dataLen) if err != nil { - return 0, fmt.Errorf("failed to decode content: %w", err) + return dataLen, fmt.Errorf("failed to decode content: %w", err) } if f.spec.Pad != nil { @@ -118,7 +118,7 @@ func (f *Numeric) Unpack(data []byte) (int, error) { } if err := f.SetBytes(raw); err != nil { - return 0, fmt.Errorf("failed to set bytes: %w", err) + return dataLen, fmt.Errorf("failed to set bytes: %w", err) } return read + prefBytes, nil diff --git a/field/string.go b/field/string.go index c8ce11c..0819460 100644 --- a/field/string.go +++ b/field/string.go @@ -97,7 +97,7 @@ func (f *String) Unpack(data []byte) (int, error) { raw, read, err := f.spec.Enc.Decode(data[prefBytes:], dataLen) if err != nil { - return 0, fmt.Errorf("failed to decode content: %w", err) + return dataLen, fmt.Errorf("failed to decode content: %w", err) } if f.spec.Pad != nil { @@ -105,7 +105,7 @@ func (f *String) Unpack(data []byte) (int, error) { } if err := f.SetBytes(raw); err != nil { - return 0, fmt.Errorf("failed to set bytes: %w", err) + return dataLen, fmt.Errorf("failed to set bytes: %w", err) } return read + prefBytes, nil diff --git a/message.go b/message.go index d650342..6ec996a 100644 --- a/message.go +++ b/message.go @@ -7,6 +7,7 @@ import ( "reflect" "sort" "strconv" + "strings" "sync" "github.com/moov-io/iso8583/field" @@ -237,9 +238,10 @@ func (m *Message) Unpack(src []byte) error { // errors in a *UnpackError. It assumes that the mutex is already // locked by the caller. func (m *Message) wrappErrorUnpack(src []byte) error { - if err := m.unpack(src); err != nil { + if fields, err := m.unpack(src); err != nil { return &UnpackError{ Err: err, + Fields: fields, RawMessage: src, } } @@ -249,8 +251,10 @@ func (m *Message) wrappErrorUnpack(src []byte) error { // unpack contains the core logic for unpacking the message. This method does // not handle locking or error wrapping and should typically be used internally // after ensuring concurrency safety. -func (m *Message) unpack(src []byte) error { +func (m *Message) unpack(src []byte) ([]string, error) { var off int + fields := []string{} + var fieldError error // reset fields that were set m.fieldsMap = map[int]struct{}{} @@ -260,7 +264,7 @@ func (m *Message) unpack(src []byte) error { read, err := m.fields[mtiIdx].Unpack(src) if err != nil { - return fmt.Errorf("failed to unpack MTI: %w", err) + return nil, fmt.Errorf("failed to unpack MTI: %w", err) } m.fieldsMap[mtiIdx] = struct{}{} @@ -270,7 +274,7 @@ func (m *Message) unpack(src []byte) error { // unpack Bitmap read, err = m.fields[bitmapIdx].Unpack(src[off:]) if err != nil { - return fmt.Errorf("failed to unpack bitmap: %w", err) + return nil, fmt.Errorf("failed to unpack bitmap: %w", err) } off += read @@ -284,12 +288,18 @@ func (m *Message) unpack(src []byte) error { if m.bitmap().IsSet(i) { fl, ok := m.fields[i] if !ok { - return fmt.Errorf("failed to unpack field %d: no specification found", i) + fieldError = fmt.Errorf("failed to unpack field %d: no specification found", i) + fields = append(fields, strconv.Itoa(i)) + return fields, fieldError } read, err = fl.Unpack(src[off:]) if err != nil { - return fmt.Errorf("failed to unpack field %d (%s): %w", i, fl.Spec().Description, err) + fieldError = fmt.Errorf("failed to unpack field %d (%s): len %d, %w", i, fl.Spec().Description, read, err) + fields = append(fields, fl.Spec().Description) + if read == 0 { + return fields, fieldError + } } m.fieldsMap[i] = struct{}{} @@ -298,7 +308,7 @@ func (m *Message) unpack(src []byte) error { } } - return nil + return fields, fieldError } func (m *Message) MarshalJSON() ([]byte, error) { @@ -476,6 +486,9 @@ func (m *Message) Unmarshal(v interface{}) error { return errors.New("data is not a struct") } + var err error + fields := []string{} + // iterate over struct fields for i := 0; i < dataStruct.NumField(); i++ { indexTag := field.NewIndexTag(dataStruct.Type().Field(i)) @@ -500,17 +513,24 @@ func (m *Message) Unmarshal(v interface{}) error { if dataField.IsNil() && dataField.Kind() != reflect.Slice { dataField.Set(reflect.New(dataField.Type().Elem())) } - err := messageField.Unmarshal(dataField.Interface()) - if err != nil { - return fmt.Errorf("failed to get value from field %d: %w", indexTag.ID, err) + fieldErr := messageField.Unmarshal(dataField.Interface()) + if fieldErr != nil { + fields = append(fields, strconv.Itoa(indexTag.ID)) + err = fieldErr } default: // Native types - err := messageField.Unmarshal(dataField) - if err != nil { - return fmt.Errorf("failed to get value from field %d: %w", indexTag.ID, err) + fieldErr := messageField.Unmarshal(dataField) + if fieldErr != nil { + fields = append(fields, strconv.Itoa(indexTag.ID)) + err = fieldErr } } } - return nil + if len(fields) > 0 { + fieldStr := strings.Join(fields, ", ") + return fmt.Errorf("failed to get value from field %s: %w", fieldStr, err) + } + + return err } diff --git a/message_test.go b/message_test.go index 912c726..5c5ba4a 100644 --- a/message_test.go +++ b/message_test.go @@ -751,6 +751,185 @@ func TestPackUnpack(t *testing.T) { require.Equal(t, rawMsg, unpackError.RawMessage) }) + t.Run("Unpack data field error on final field returns partial message", func(t *testing.T) { + message := NewMessage(spec) + + rawMsg := []byte{0x30, 0x31, 0x30, 0x30, 0xf2, 0x3c, 0x24, 0x81, 0x28, 0xe0, 0x9a, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x31, 0x36, 0x34, 0x32, 0x37, 0x36, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x37, 0x37, 0x37, 0x30, 0x30, 0x30, 0x37, 0x30, 0x31, 0x31, 0x31, 0x31, 0x38, 0x34, 0x34, 0x30, 0x30, 0x30, 0x31, 0x32, 0x33, 0x31, 0x33, 0x31, 0x38, 0x34, 0x34, 0x30, 0x37, 0x30, 0x31, 0x31, 0x39, 0x30, 0x32, 0x6, 0x43, 0x39, 0x30, 0x31, 0x30, 0x32, 0x30, 0x36, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x33, 0x37, 0x34, 0x32, 0x37, 0x36, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x3d, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x30, 0x39, 0x38, 0x37, 0x36, 0x35, 0x34, 0x33, 0x32, 0x31, 0x30, 0x30, 0x31, 0x30, 0x30, 0x30, 0x30, 0x30, 0x33, 0x32, 0x31, 0x31, 0x32, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x33, 0x34, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x54, 0x65, 0x73, 0x74, 0x20, 0x74, 0x65, 0x78, 0x74, 0x64, 0x30, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x31, 0x32, 0x33, 0x34, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x32, 0x33, 0x9a, 0x6, 0x32, 0x31, 0x30, 0x37, 0x32, 0x30, 0x9f, 0x2, 0xc, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x35, 0x30, 0x31, 0x30, 0x31, 0x37, 0x41, 0x6e, 0x6f, 0x74, 0x68, 0x65, 0x72, 0x20, 0x74, 0x65, 0x73, 0x74, 0x20, 0x74, 0x65, 0x78} + + err := message.Unpack([]byte(rawMsg)) + + require.Error(t, err) + var unpackError *UnpackError + require.ErrorAs(t, err, &unpackError) + assert.ElementsMatch(t, unpackError.Fields, []string{"Field 120"}) + + 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, "000000", s) + + s, err = message.GetString(4) + require.NoError(t, err) + require.Equal(t, "77700", s) + + data := &TestISOData{} + require.NoError(t, message.Unmarshal(data)) + + assert.Equal(t, "4276555555555555", data.F2.Value()) + assert.Equal(t, "00", data.F3.F1.Value()) + assert.Equal(t, "00", data.F3.F2.Value()) + assert.Equal(t, "00", data.F3.F3.Value()) + assert.Equal(t, int64(77700), data.F4.Value()) + assert.Equal(t, int64(701111844), data.F7.Value()) + assert.Equal(t, int64(123), data.F11.Value()) + assert.Equal(t, int64(131844), data.F12.Value()) + assert.Equal(t, int64(701), data.F13.Value()) + assert.Equal(t, int64(1902), data.F14.Value()) + assert.Equal(t, int64(643), data.F19.Value()) + assert.Equal(t, int64(901), data.F22.Value()) + assert.Equal(t, int64(2), data.F25.Value()) + assert.Equal(t, int64(123456), data.F32.Value()) + assert.Equal(t, "4276555555555555=12345678901234567890", data.F35.Value()) + assert.Equal(t, "987654321001", data.F37.Value()) + assert.Equal(t, "00000321", data.F41.Value()) + assert.Equal(t, "120000000000034", data.F42.Value()) + assert.Equal(t, "Test text", data.F43.Value()) + assert.Equal(t, int64(643), data.F49.Value()) + assert.Nil(t, data.F50) + assert.Equal(t, string([]byte{1, 2, 3, 4, 5, 6, 7, 8}), data.F52.Value()) + assert.Equal(t, int64(1234000000000000), data.F53.Value()) + assert.Equal(t, "210720", data.F55.F9A.Value()) + assert.Equal(t, "000000000501", data.F55.F9F02.Value()) + assert.Empty(t, data.F120) + }) + + t.Run("Unpack data field error on middle field with fixed prefix returns partial message", func(t *testing.T) { + corruptSpecOut := &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.NewNumeric(&field.Spec{ + Length: 3, + Description: "Dodgy field", + Enc: encoding.ASCII, + Pref: prefix.ASCII.Fixed, + }), + 4: field.NewString(&field.Spec{ + Length: 6, + Description: "Anything", + Enc: encoding.ASCII, + Pref: prefix.ASCII.LL, + }), + }, + } + + corruptSpecIn := &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.NewString(&field.Spec{ + Length: 3, + Description: "Dodgy field", + Enc: encoding.ASCII, + Pref: prefix.ASCII.Fixed, + }), + 4: field.NewString(&field.Spec{ + Length: 6, + Description: "Anything", + Enc: encoding.ASCII, + Pref: prefix.ASCII.LL, + }), + }, + } + + type TestCorruptISODataIn struct { + F2 *field.String + F3 *field.String + F4 *field.String + } + + type TestCorruptISODataOut struct { + F2 *field.String + F3 *field.Numeric + F4 *field.String + } + + message := NewMessage(corruptSpecIn) + err := message.Marshal(&TestCorruptISODataIn{ + F2: field.NewStringValue("4276555555555555"), + F3: field.NewStringValue("ABC"), + F4: field.NewStringValue("123"), + }) + require.NoError(t, err) + + message.MTI("0100") + + rawMsg, err := message.Pack() + require.NoError(t, err) + + receivedMessage := NewMessage(corruptSpecOut) + + err = receivedMessage.Unpack([]byte(rawMsg)) + + require.Error(t, err) + var unpackErr *UnpackError + require.ErrorAs(t, err, &unpackErr) + require.Equal(t, []string{"Dodgy field"}, unpackErr.Fields) + + s, err := receivedMessage.GetString(2) + require.NoError(t, err) + require.Equal(t, "4276555555555555", s) + + s, err = receivedMessage.GetString(3) + require.NoError(t, err) + require.Equal(t, "0", s) + + s, err = receivedMessage.GetString(4) + require.NoError(t, err) + require.Equal(t, "123", s) + + data := &TestCorruptISODataOut{} + err = message.Unmarshal(data) + require.Error(t, err) + + assert.Equal(t, "4276555555555555", data.F2.Value()) + assert.Equal(t, int64(0), data.F3.Value()) + assert.Equal(t, "123", data.F4.Value()) + }) + // 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 3ec97069549827735bd011be007a71747ce92401 Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 21 Nov 2023 11:23:03 +0000 Subject: [PATCH 2/6] Stop parsing at error, return partial message --- errors.go | 5 +- field/numeric.go | 4 +- field/string.go | 4 +- message.go | 47 +++++------------ message_test.go | 128 +---------------------------------------------- 5 files changed, 21 insertions(+), 167 deletions(-) diff --git a/errors.go b/errors.go index 6aea3a3..f6a74fe 100644 --- a/errors.go +++ b/errors.go @@ -4,7 +4,7 @@ package iso8583 // connection failed to unpack message type UnpackError struct { Err error - Fields []string + Field string RawMessage []byte } @@ -17,8 +17,7 @@ func (e *UnpackError) Unwrap() error { } type PackError struct { - Fields []string - Err error + Err error } func (e *PackError) Error() string { diff --git a/field/numeric.go b/field/numeric.go index a3c485c..1c0f854 100644 --- a/field/numeric.go +++ b/field/numeric.go @@ -110,7 +110,7 @@ func (f *Numeric) Unpack(data []byte) (int, error) { raw, read, err := f.spec.Enc.Decode(data[prefBytes:], dataLen) if err != nil { - return dataLen, fmt.Errorf("failed to decode content: %w", err) + return 0, fmt.Errorf("failed to decode content: %w", err) } if f.spec.Pad != nil { @@ -118,7 +118,7 @@ func (f *Numeric) Unpack(data []byte) (int, error) { } if err := f.SetBytes(raw); err != nil { - return dataLen, fmt.Errorf("failed to set bytes: %w", err) + return 0, fmt.Errorf("failed to set bytes: %w", err) } return read + prefBytes, nil diff --git a/field/string.go b/field/string.go index 0819460..c8ce11c 100644 --- a/field/string.go +++ b/field/string.go @@ -97,7 +97,7 @@ func (f *String) Unpack(data []byte) (int, error) { raw, read, err := f.spec.Enc.Decode(data[prefBytes:], dataLen) if err != nil { - return dataLen, fmt.Errorf("failed to decode content: %w", err) + return 0, fmt.Errorf("failed to decode content: %w", err) } if f.spec.Pad != nil { @@ -105,7 +105,7 @@ func (f *String) Unpack(data []byte) (int, error) { } if err := f.SetBytes(raw); err != nil { - return dataLen, fmt.Errorf("failed to set bytes: %w", err) + return 0, fmt.Errorf("failed to set bytes: %w", err) } return read + prefBytes, nil diff --git a/message.go b/message.go index 6ec996a..329a074 100644 --- a/message.go +++ b/message.go @@ -7,7 +7,6 @@ import ( "reflect" "sort" "strconv" - "strings" "sync" "github.com/moov-io/iso8583/field" @@ -238,10 +237,10 @@ func (m *Message) Unpack(src []byte) error { // errors in a *UnpackError. It assumes that the mutex is already // locked by the caller. func (m *Message) wrappErrorUnpack(src []byte) error { - if fields, err := m.unpack(src); err != nil { + if field, err := m.unpack(src); err != nil { return &UnpackError{ Err: err, - Fields: fields, + Field: field, RawMessage: src, } } @@ -251,10 +250,8 @@ func (m *Message) wrappErrorUnpack(src []byte) error { // unpack contains the core logic for unpacking the message. This method does // not handle locking or error wrapping and should typically be used internally // after ensuring concurrency safety. -func (m *Message) unpack(src []byte) ([]string, error) { +func (m *Message) unpack(src []byte) (string, error) { var off int - fields := []string{} - var fieldError error // reset fields that were set m.fieldsMap = map[int]struct{}{} @@ -264,7 +261,7 @@ func (m *Message) unpack(src []byte) ([]string, error) { read, err := m.fields[mtiIdx].Unpack(src) if err != nil { - return nil, fmt.Errorf("failed to unpack MTI: %w", err) + return "", fmt.Errorf("failed to unpack MTI: %w", err) } m.fieldsMap[mtiIdx] = struct{}{} @@ -274,7 +271,7 @@ func (m *Message) unpack(src []byte) ([]string, error) { // unpack Bitmap read, err = m.fields[bitmapIdx].Unpack(src[off:]) if err != nil { - return nil, fmt.Errorf("failed to unpack bitmap: %w", err) + return "", fmt.Errorf("failed to unpack bitmap: %w", err) } off += read @@ -288,18 +285,12 @@ func (m *Message) unpack(src []byte) ([]string, error) { if m.bitmap().IsSet(i) { fl, ok := m.fields[i] if !ok { - fieldError = fmt.Errorf("failed to unpack field %d: no specification found", i) - fields = append(fields, strconv.Itoa(i)) - return fields, fieldError + return strconv.Itoa(i), fmt.Errorf("failed to unpack field %d: no specification found", i) } read, err = fl.Unpack(src[off:]) if err != nil { - fieldError = fmt.Errorf("failed to unpack field %d (%s): len %d, %w", i, fl.Spec().Description, read, err) - fields = append(fields, fl.Spec().Description) - if read == 0 { - return fields, fieldError - } + return strconv.Itoa(i), fmt.Errorf("failed to unpack field %d (%s): %w", i, fl.Spec().Description, err) } m.fieldsMap[i] = struct{}{} @@ -308,7 +299,7 @@ func (m *Message) unpack(src []byte) ([]string, error) { } } - return fields, fieldError + return "", nil } func (m *Message) MarshalJSON() ([]byte, error) { @@ -486,9 +477,6 @@ func (m *Message) Unmarshal(v interface{}) error { return errors.New("data is not a struct") } - var err error - fields := []string{} - // iterate over struct fields for i := 0; i < dataStruct.NumField(); i++ { indexTag := field.NewIndexTag(dataStruct.Type().Field(i)) @@ -513,24 +501,15 @@ func (m *Message) Unmarshal(v interface{}) error { if dataField.IsNil() && dataField.Kind() != reflect.Slice { dataField.Set(reflect.New(dataField.Type().Elem())) } - fieldErr := messageField.Unmarshal(dataField.Interface()) - if fieldErr != nil { - fields = append(fields, strconv.Itoa(indexTag.ID)) - err = fieldErr + if err := messageField.Unmarshal(dataField.Interface()); err != nil { + return fmt.Errorf("failed to get value from field %d: %w", indexTag.ID, err) } default: // Native types - fieldErr := messageField.Unmarshal(dataField) - if fieldErr != nil { - fields = append(fields, strconv.Itoa(indexTag.ID)) - err = fieldErr + if err := messageField.Unmarshal(dataField); err != nil { + return fmt.Errorf("failed to get value from field %d: %w", indexTag.ID, err) } } } - if len(fields) > 0 { - fieldStr := strings.Join(fields, ", ") - return fmt.Errorf("failed to get value from field %s: %w", fieldStr, err) - } - - return err + return nil } diff --git a/message_test.go b/message_test.go index 5c5ba4a..ad8b6d5 100644 --- a/message_test.go +++ b/message_test.go @@ -751,7 +751,7 @@ func TestPackUnpack(t *testing.T) { require.Equal(t, rawMsg, unpackError.RawMessage) }) - t.Run("Unpack data field error on final field returns partial message", func(t *testing.T) { + t.Run("Unpack data field error on field returns partial message", func(t *testing.T) { message := NewMessage(spec) rawMsg := []byte{0x30, 0x31, 0x30, 0x30, 0xf2, 0x3c, 0x24, 0x81, 0x28, 0xe0, 0x9a, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x31, 0x36, 0x34, 0x32, 0x37, 0x36, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x37, 0x37, 0x37, 0x30, 0x30, 0x30, 0x37, 0x30, 0x31, 0x31, 0x31, 0x31, 0x38, 0x34, 0x34, 0x30, 0x30, 0x30, 0x31, 0x32, 0x33, 0x31, 0x33, 0x31, 0x38, 0x34, 0x34, 0x30, 0x37, 0x30, 0x31, 0x31, 0x39, 0x30, 0x32, 0x6, 0x43, 0x39, 0x30, 0x31, 0x30, 0x32, 0x30, 0x36, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x33, 0x37, 0x34, 0x32, 0x37, 0x36, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x3d, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x30, 0x39, 0x38, 0x37, 0x36, 0x35, 0x34, 0x33, 0x32, 0x31, 0x30, 0x30, 0x31, 0x30, 0x30, 0x30, 0x30, 0x30, 0x33, 0x32, 0x31, 0x31, 0x32, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x33, 0x34, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x54, 0x65, 0x73, 0x74, 0x20, 0x74, 0x65, 0x78, 0x74, 0x64, 0x30, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x31, 0x32, 0x33, 0x34, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x32, 0x33, 0x9a, 0x6, 0x32, 0x31, 0x30, 0x37, 0x32, 0x30, 0x9f, 0x2, 0xc, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x35, 0x30, 0x31, 0x30, 0x31, 0x37, 0x41, 0x6e, 0x6f, 0x74, 0x68, 0x65, 0x72, 0x20, 0x74, 0x65, 0x73, 0x74, 0x20, 0x74, 0x65, 0x78} @@ -761,7 +761,7 @@ func TestPackUnpack(t *testing.T) { require.Error(t, err) var unpackError *UnpackError require.ErrorAs(t, err, &unpackError) - assert.ElementsMatch(t, unpackError.Fields, []string{"Field 120"}) + assert.Equal(t, unpackError.Field, "120") s, err := message.GetString(2) require.NoError(t, err) @@ -806,130 +806,6 @@ func TestPackUnpack(t *testing.T) { assert.Empty(t, data.F120) }) - t.Run("Unpack data field error on middle field with fixed prefix returns partial message", func(t *testing.T) { - corruptSpecOut := &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.NewNumeric(&field.Spec{ - Length: 3, - Description: "Dodgy field", - Enc: encoding.ASCII, - Pref: prefix.ASCII.Fixed, - }), - 4: field.NewString(&field.Spec{ - Length: 6, - Description: "Anything", - Enc: encoding.ASCII, - Pref: prefix.ASCII.LL, - }), - }, - } - - corruptSpecIn := &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.NewString(&field.Spec{ - Length: 3, - Description: "Dodgy field", - Enc: encoding.ASCII, - Pref: prefix.ASCII.Fixed, - }), - 4: field.NewString(&field.Spec{ - Length: 6, - Description: "Anything", - Enc: encoding.ASCII, - Pref: prefix.ASCII.LL, - }), - }, - } - - type TestCorruptISODataIn struct { - F2 *field.String - F3 *field.String - F4 *field.String - } - - type TestCorruptISODataOut struct { - F2 *field.String - F3 *field.Numeric - F4 *field.String - } - - message := NewMessage(corruptSpecIn) - err := message.Marshal(&TestCorruptISODataIn{ - F2: field.NewStringValue("4276555555555555"), - F3: field.NewStringValue("ABC"), - F4: field.NewStringValue("123"), - }) - require.NoError(t, err) - - message.MTI("0100") - - rawMsg, err := message.Pack() - require.NoError(t, err) - - receivedMessage := NewMessage(corruptSpecOut) - - err = receivedMessage.Unpack([]byte(rawMsg)) - - require.Error(t, err) - var unpackErr *UnpackError - require.ErrorAs(t, err, &unpackErr) - require.Equal(t, []string{"Dodgy field"}, unpackErr.Fields) - - s, err := receivedMessage.GetString(2) - require.NoError(t, err) - require.Equal(t, "4276555555555555", s) - - s, err = receivedMessage.GetString(3) - require.NoError(t, err) - require.Equal(t, "0", s) - - s, err = receivedMessage.GetString(4) - require.NoError(t, err) - require.Equal(t, "123", s) - - data := &TestCorruptISODataOut{} - err = message.Unmarshal(data) - require.Error(t, err) - - assert.Equal(t, "4276555555555555", data.F2.Value()) - assert.Equal(t, int64(0), data.F3.Value()) - assert.Equal(t, "123", data.F4.Value()) - }) - // 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 4b92ec4c289bd316eac61adaba40aaaed4caf68c Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 21 Nov 2023 15:56:20 +0000 Subject: [PATCH 3/6] Remove wrappErrorUnpack --- message.go | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/message.go b/message.go index 329a074..bc99009 100644 --- a/message.go +++ b/message.go @@ -230,27 +230,13 @@ func (m *Message) Unpack(src []byte) error { m.mu.Lock() defer m.mu.Unlock() - return m.wrappErrorUnpack(src) -} - -// wrappErrorUnpack calls the core unpacking logic and wraps any -// errors in a *UnpackError. It assumes that the mutex is already -// locked by the caller. -func (m *Message) wrappErrorUnpack(src []byte) error { - if field, err := m.unpack(src); err != nil { - return &UnpackError{ - Err: err, - Field: field, - RawMessage: src, - } - } - return nil + return m.unpack(src) } // unpack contains the core logic for unpacking the message. This method does // not handle locking or error wrapping and should typically be used internally // after ensuring concurrency safety. -func (m *Message) unpack(src []byte) (string, error) { +func (m *Message) unpack(src []byte) error { var off int // reset fields that were set @@ -261,7 +247,11 @@ func (m *Message) unpack(src []byte) (string, error) { read, err := m.fields[mtiIdx].Unpack(src) if err != nil { - return "", fmt.Errorf("failed to unpack MTI: %w", err) + return &UnpackError{ + Err: fmt.Errorf("failed to unpack MTI: %w", err), + Field: strconv.Itoa(mtiIdx), + RawMessage: src, + } } m.fieldsMap[mtiIdx] = struct{}{} @@ -271,7 +261,11 @@ func (m *Message) unpack(src []byte) (string, error) { // unpack Bitmap read, err = m.fields[bitmapIdx].Unpack(src[off:]) if err != nil { - return "", fmt.Errorf("failed to unpack bitmap: %w", err) + return &UnpackError{ + Err: fmt.Errorf("failed to unpack bitmap: %w", err), + Field: strconv.Itoa(bitmapIdx), + RawMessage: src, + } } off += read @@ -285,12 +279,20 @@ func (m *Message) unpack(src []byte) (string, error) { if m.bitmap().IsSet(i) { fl, ok := m.fields[i] if !ok { - return strconv.Itoa(i), fmt.Errorf("failed to unpack field %d: no specification found", i) + return &UnpackError{ + Err: fmt.Errorf("failed to unpack field %d: no specification found", i), + Field: strconv.Itoa(i), + RawMessage: src, + } } read, err = fl.Unpack(src[off:]) if err != nil { - return strconv.Itoa(i), fmt.Errorf("failed to unpack field %d (%s): %w", i, fl.Spec().Description, err) + return &UnpackError{ + Err: fmt.Errorf("failed to unpack field %d (%s): %w", i, fl.Spec().Description, err), + Field: strconv.Itoa(i), + RawMessage: src, + } } m.fieldsMap[i] = struct{}{} @@ -299,7 +301,7 @@ func (m *Message) unpack(src []byte) (string, error) { } } - return "", nil + return nil } func (m *Message) MarshalJSON() ([]byte, error) { From f0422889ddb272814810bc81f0a83f4f4c593b8e Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 21 Nov 2023 16:24:45 +0000 Subject: [PATCH 4/6] Revert improvement to Unmarshal(), add comment to test --- message.go | 6 ++++-- message_test.go | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/message.go b/message.go index bc99009..32d5667 100644 --- a/message.go +++ b/message.go @@ -503,11 +503,13 @@ func (m *Message) Unmarshal(v interface{}) error { if dataField.IsNil() && dataField.Kind() != reflect.Slice { dataField.Set(reflect.New(dataField.Type().Elem())) } - if err := messageField.Unmarshal(dataField.Interface()); err != nil { + err := messageField.Unmarshal(dataField.Interface()) + if err != nil { return fmt.Errorf("failed to get value from field %d: %w", indexTag.ID, err) } default: // Native types - if err := messageField.Unmarshal(dataField); err != nil { + err := messageField.Unmarshal(dataField) + if err != nil { return fmt.Errorf("failed to get value from field %d: %w", indexTag.ID, err) } } diff --git a/message_test.go b/message_test.go index ad8b6d5..7ada7d3 100644 --- a/message_test.go +++ b/message_test.go @@ -754,6 +754,7 @@ func TestPackUnpack(t *testing.T) { t.Run("Unpack data field error on field returns partial message", func(t *testing.T) { message := NewMessage(spec) + // One byte has been removed from field 120 which will make it fail the length check rawMsg := []byte{0x30, 0x31, 0x30, 0x30, 0xf2, 0x3c, 0x24, 0x81, 0x28, 0xe0, 0x9a, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x31, 0x36, 0x34, 0x32, 0x37, 0x36, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x37, 0x37, 0x37, 0x30, 0x30, 0x30, 0x37, 0x30, 0x31, 0x31, 0x31, 0x31, 0x38, 0x34, 0x34, 0x30, 0x30, 0x30, 0x31, 0x32, 0x33, 0x31, 0x33, 0x31, 0x38, 0x34, 0x34, 0x30, 0x37, 0x30, 0x31, 0x31, 0x39, 0x30, 0x32, 0x6, 0x43, 0x39, 0x30, 0x31, 0x30, 0x32, 0x30, 0x36, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x33, 0x37, 0x34, 0x32, 0x37, 0x36, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x35, 0x3d, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x30, 0x39, 0x38, 0x37, 0x36, 0x35, 0x34, 0x33, 0x32, 0x31, 0x30, 0x30, 0x31, 0x30, 0x30, 0x30, 0x30, 0x30, 0x33, 0x32, 0x31, 0x31, 0x32, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x33, 0x34, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x54, 0x65, 0x73, 0x74, 0x20, 0x74, 0x65, 0x78, 0x74, 0x64, 0x30, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x31, 0x32, 0x33, 0x34, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x32, 0x33, 0x9a, 0x6, 0x32, 0x31, 0x30, 0x37, 0x32, 0x30, 0x9f, 0x2, 0xc, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x35, 0x30, 0x31, 0x30, 0x31, 0x37, 0x41, 0x6e, 0x6f, 0x74, 0x68, 0x65, 0x72, 0x20, 0x74, 0x65, 0x73, 0x74, 0x20, 0x74, 0x65, 0x78} err := message.Unpack([]byte(rawMsg)) From d6c0ac974e54d0611824ca8e9bec8d8389f060bd Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 30 Nov 2023 11:38:55 +0000 Subject: [PATCH 5/6] Rename Field to FieldID --- errors.go | 2 +- message.go | 8 ++++---- message_test.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/errors.go b/errors.go index f6a74fe..b0643eb 100644 --- a/errors.go +++ b/errors.go @@ -4,7 +4,7 @@ package iso8583 // connection failed to unpack message type UnpackError struct { Err error - Field string + FieldID string RawMessage []byte } diff --git a/message.go b/message.go index 32d5667..25b78a9 100644 --- a/message.go +++ b/message.go @@ -249,7 +249,7 @@ func (m *Message) unpack(src []byte) error { if err != nil { return &UnpackError{ Err: fmt.Errorf("failed to unpack MTI: %w", err), - Field: strconv.Itoa(mtiIdx), + FieldID: strconv.Itoa(mtiIdx), RawMessage: src, } } @@ -263,7 +263,7 @@ func (m *Message) unpack(src []byte) error { if err != nil { return &UnpackError{ Err: fmt.Errorf("failed to unpack bitmap: %w", err), - Field: strconv.Itoa(bitmapIdx), + FieldID: strconv.Itoa(bitmapIdx), RawMessage: src, } } @@ -281,7 +281,7 @@ func (m *Message) unpack(src []byte) error { if !ok { return &UnpackError{ Err: fmt.Errorf("failed to unpack field %d: no specification found", i), - Field: strconv.Itoa(i), + FieldID: strconv.Itoa(i), RawMessage: src, } } @@ -290,7 +290,7 @@ func (m *Message) unpack(src []byte) error { if err != nil { return &UnpackError{ Err: fmt.Errorf("failed to unpack field %d (%s): %w", i, fl.Spec().Description, err), - Field: strconv.Itoa(i), + FieldID: strconv.Itoa(i), RawMessage: src, } } diff --git a/message_test.go b/message_test.go index 7ada7d3..9e7170b 100644 --- a/message_test.go +++ b/message_test.go @@ -762,7 +762,7 @@ func TestPackUnpack(t *testing.T) { require.Error(t, err) var unpackError *UnpackError require.ErrorAs(t, err, &unpackError) - assert.Equal(t, unpackError.Field, "120") + assert.Equal(t, unpackError.FieldID, "120") s, err := message.GetString(2) require.NoError(t, err) From 03ff0304983ab9f8d178f119a1863380c9bc3fd8 Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 30 Nov 2023 13:41:16 +0000 Subject: [PATCH 6/6] Put error wrapping function back --- message.go | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/message.go b/message.go index 25b78a9..d9411ab 100644 --- a/message.go +++ b/message.go @@ -230,13 +230,27 @@ func (m *Message) Unpack(src []byte) error { m.mu.Lock() defer m.mu.Unlock() - return m.unpack(src) + return m.wrapErrorUnpack(src) +} + +// 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 (m *Message) wrapErrorUnpack(src []byte) error { + if fieldID, err := m.unpack(src); err != nil { + return &UnpackError{ + Err: err, + FieldID: fieldID, + RawMessage: src, + } + } + return nil } // unpack contains the core logic for unpacking the message. This method does // not handle locking or error wrapping and should typically be used internally // after ensuring concurrency safety. -func (m *Message) unpack(src []byte) error { +func (m *Message) unpack(src []byte) (string, error) { var off int // reset fields that were set @@ -247,11 +261,7 @@ func (m *Message) unpack(src []byte) error { read, err := m.fields[mtiIdx].Unpack(src) if err != nil { - return &UnpackError{ - Err: fmt.Errorf("failed to unpack MTI: %w", err), - FieldID: strconv.Itoa(mtiIdx), - RawMessage: src, - } + return strconv.Itoa(mtiIdx), fmt.Errorf("failed to unpack MTI: %w", err) } m.fieldsMap[mtiIdx] = struct{}{} @@ -261,11 +271,7 @@ func (m *Message) unpack(src []byte) error { // unpack Bitmap read, err = m.fields[bitmapIdx].Unpack(src[off:]) if err != nil { - return &UnpackError{ - Err: fmt.Errorf("failed to unpack bitmap: %w", err), - FieldID: strconv.Itoa(bitmapIdx), - RawMessage: src, - } + return strconv.Itoa(bitmapIdx), fmt.Errorf("failed to unpack bitmap: %w", err) } off += read @@ -279,20 +285,12 @@ func (m *Message) unpack(src []byte) error { if m.bitmap().IsSet(i) { fl, ok := m.fields[i] if !ok { - return &UnpackError{ - Err: fmt.Errorf("failed to unpack field %d: no specification found", i), - FieldID: strconv.Itoa(i), - RawMessage: src, - } + return strconv.Itoa(i), fmt.Errorf("failed to unpack field %d: no specification found", i) } read, err = fl.Unpack(src[off:]) if err != nil { - return &UnpackError{ - Err: fmt.Errorf("failed to unpack field %d (%s): %w", i, fl.Spec().Description, err), - FieldID: strconv.Itoa(i), - RawMessage: src, - } + return strconv.Itoa(i), fmt.Errorf("failed to unpack field %d (%s): %w", i, fl.Spec().Description, err) } m.fieldsMap[i] = struct{}{} @@ -301,7 +299,7 @@ func (m *Message) unpack(src []byte) error { } } - return nil + return "", nil } func (m *Message) MarshalJSON() ([]byte, error) {