moov-io / iso8583

A golang implementation to marshal and unmarshal iso8583 message.
https://moov.io
Apache License 2.0
353 stars 105 forks source link

Proposal: use normal field names and field tags for Data structs #160

Closed alovak closed 2 years ago

alovak commented 2 years ago

Personally, I struggle from working with structs like this:

type AuthData struct {
    // F2 contains the Primary Account Number (PAN)
    F2 *field.String
    // F3 is the ProcessingCode and is mandatory for processing this message.
    // It will be preserved and returned in the response message without alteration.
    F3 *field.Numeric
    // F4 contains the total transaction amount (including tax), in the
    // currency designated by the Currency Code Transaction.
    //
    // Ex: $100.00 is represented as 000000010000
    F4 *field.String
        // ...
}

so, I suggest using field tags to be able to use normal field names and still be able to specify field index/tag like this:

type AuthData struct {
    PrimaryAccountNumber *field.String `field:"2"`
    ProcessingCode *field.Numeric `field:"3"`
    TransactionAmount *field.String `field:"4"`
}

Please, let me know your thoughts!

atonks2 commented 2 years ago

@alovak it's like you can read my mind 😂

I've been wondering why we don't implement this using tags like you would for json or yaml. I'm all for this.

parsnips commented 2 years ago

There's two things to consider here:

  1. some fields that have network specific intricacies i.e. Visa DPS vs First Data etc. What to do there?
  2. as proposed, this would break backwards compatibility. Is there a way to do this without breaking BC? (as a consumer of this lib, if I need to make a breaking change, the value needs to be high).
alovak commented 2 years ago

for 2 - this will not break existing integrations. We will support both ways to define fields.

Can you share more information on 1st?

parsnips commented 2 years ago

Yeah for example field 44 is "Additional Response Data" ... Idea here to just be agonostic like:

AdditionalResponseData *field.String `field:"44"`
adamdecaf commented 2 years ago

Initially we were worried how much drift there is with various vendors so we didn't want to hardcode too many names into structs. We've only used a few of the messages and haven't ran into much drift, so having struct tags seems useful.

I like this change, but would worry how much trouble we'll get into as we complete all the message specs.

alovak commented 2 years ago

Implemented in #163 and in #162