Skip to content

Commit

Permalink
Stop parsing at error, return partial message
Browse files Browse the repository at this point in the history
  • Loading branch information
@meparle
meparle committed Nov 21, 2023
1 parent 508a839 commit 3ec9706
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 167 deletions.
5 changes: 2 additions & 3 deletions errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ package iso8583
// connection failed to unpack message
type UnpackError struct {
Err error
Fields []string
Field string
RawMessage []byte
}

Expand All @@ -17,8 +17,7 @@ func (e *UnpackError) Unwrap() error {
}

type PackError struct {
Fields []string
Err error
Err error
}

func (e *PackError) Error() string {
Expand Down
4 changes: 2 additions & 2 deletions field/numeric.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,15 @@ 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 {
raw = f.spec.Pad.Unpad(raw)
}

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
Expand Down
4 changes: 2 additions & 2 deletions field/string.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,15 +97,15 @@ 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 {
raw = f.spec.Pad.Unpad(raw)
}

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
Expand Down
47 changes: 13 additions & 34 deletions message.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"reflect"
"sort"
"strconv"
"strings"
"sync"

"github.com/moov-io/iso8583/field"
Expand Down Expand Up @@ -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,
}
}
Expand All @@ -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{}{}
Expand All @@ -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{}{}
Expand All @@ -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
Expand All @@ -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{}{}
Expand All @@ -308,7 +299,7 @@ func (m *Message) unpack(src []byte) ([]string, error) {
}
}

return fields, fieldError
return "", nil
}

func (m *Message) MarshalJSON() ([]byte, error) {
Expand Down Expand Up @@ -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))
Expand All @@ -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
}
128 changes: 2 additions & 126 deletions message_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 3ec9706

Please sign in to comment.