Unstable json unmarshaler

[zhumanrakhat]

Problem

Example snippet which unmarshals json to iso8583 format from the readme is unstable and does not work locally. There is an error occurred when parsing and produces randomly packed message. To reproduce issue, please copy snippet from demo section and try to launch several time. It should outputs different message each time...

Demo

var spec = &iso8583.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.BytesToASCIIHex,
            Pref:        prefix.Hex.Fixed,
        }),

        // Message fields:
        2: field.NewString(&field.Spec{
            Length:      19,
            Description: "Primary Account Number",
            Enc:         encoding.ASCII,
            Pref:        prefix.ASCII.LL,
        }),
        3: field.NewNumeric(&field.Spec{
            Length:      6,
            Description: "Processing Code",
            Enc:         encoding.ASCII,
            Pref:        prefix.ASCII.Fixed,
            Pad:         padding.Left('0'),
        }),
        4: field.NewString(&field.Spec{
            Length:      12,
            Description: "Transaction Amount",
            Enc:         encoding.ASCII,
            Pref:        prefix.ASCII.Fixed,
            Pad:         padding.Left('0'),
        }),
    },
}

func main() {
    data := `{"0":"0100","1":"70000000000000000000000000000000","2":"4242424242424242","3":123456,"4":"100"}`

    message := iso8583.NewMessage(spec)
    if err := json.Unmarshal([]byte(data), message); err != nil {
        log.Printf("[error occurred] - %+v", err)
    }

    packed, err := message.Pack()
    if err != nil {
        log.Printf("[error occurred] - %+v", err)
    }

    log.Println(string(packed))
}

>go run main.go
2023/06/27 11:48:42 [error occurred] - failed to unmarshal field 1
2023/06/27 11:48:42 01003000000000000000123456000000000100

>go run main.go
2023/06/27 11:48:43 [error occurred] - failed to unmarshal field 1
2023/06/27 11:48:43 0000000000000000

>go run main.go
2023/06/27 11:48:45 [error occurred] - failed to unmarshal field 1
2023/06/27 11:48:45 01000000000000000000

>go run main.go
2023/06/27 11:48:46 [error occurred] - failed to unmarshal field 1
2023/06/27 11:48:46 01003000000000000000123456000000000100

>go run main.go
2023/06/27 11:48:48 [error occurred] - failed to unmarshal field 1
2023/06/27 11:48:48 01000000000000000000

[alovak]

@zhumanrakhat thanks for reporting the issue! I'll check it.

[alovak]

I can explain the issue you observe.

First, when the following code is executed:

    message := iso8583.NewMessage(spec)
    if err := json.Unmarshal([]byte(data), message); err != nil {
        log.Printf("Unmarshal error [error occurred] - %+v", err)
    }

the following code inside the iso8583 package is executed:

func (m *Message) UnmarshalJSON(b []byte) error {
    var data map[string]json.RawMessage
    if err := json.Unmarshal(b, &data); err != nil {
        return err
    }

    for id, rawMsg := range data { // < --- this is the root cause of random results
        // ...

as you may see, we iterate over a map, which does not guarantee the order of keys to be the same every time. It means, that sometimes it can unmarshal some fields, but when it gets the field 1 it fails to unmarshal it and returns the error.

But because you ignore the error in your example and use the message that contains only some (each time it's a random fields, until field 1 is attempted to be unmarshal) unmarshalled fields to pack, you get different results each time.

It's certainly not an issue per se, but I'm considering whether we should sort the keys of the map to ensure UnmarshalJSON operates deterministically. @adamdecaf what do you think? We sort keys when we MarshalJSON (just to make it... readable?) but here I'm leaning towards keeping it as is as it will negatively impact the performance of the method for no real benefit.

[alovak]

@zhumanrakhat in addition, I have found that when we marshal message to JSON, we don't include the bitmap, but when we unmarshal - we don't ignore it.

We can start a new thread for the discussion about including bitmap into the message JSON or not.

Bitmap was removed from JSON because of p.3 here

[alovak]

Readme is fixed by https://github.com/moov-io/iso8583/pull/244

[zhumanrakhat]

@alovak thank you for detailed answer