moov-io / iso8583

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

Create message copy (or modify message) #118

Closed alovak closed 2 years ago

alovak commented 3 years ago

On the issuer side, it's common to reply to the acquirer with the message that has most fields the same as the original message. We need to find a convenient and safe way to create a message copy where we can replace some of the existing fields. Fields removal or message copying should work correctly with both typed and untyped APIs.

Options

Option 1 - Copy message except for some fields (suggested by @vtolstov)

func CopyMessage(src *Message, fields_num_to_exclude []int) *Message

Example:

responseMessage := iso8583.CopyMessage(requestMessage, []int{2, 8, 10})

// set new fields
responseMessage.Field(2, "4242424242424242")

Option 2 - Modify existing message (by @shamigor)

This option does not seem safe as we always work with the point to the message. Removing fields from the message may have some side effects. This option is partially implemented in #113

Example:

requestMessage.RemoveFields([]int{2, 8, 10})

// set new fields
requestMessage.Field(2, "4242424242424242")

Option 3 - ???

alovak commented 3 years ago

@krishishah @shamigor @vtolstov I would love to see what you guys think about it.

vtolstov commented 3 years ago

i'm prefer to have copy, in this case i can use message after response to client =) also creating copy is more idiomatic but i want to have not func CopyMessage(src Message, fields_num_to_exclude []int) Message but func CopyMessage(src Message, fields_num_to_exclude ...int) Message

in this case i don't need to create []int slice but pass field numbers with comma (2, 8, 20)

shamigor commented 3 years ago

``> @krishishah @shamigor @vtolstov I would love to see what you guys think about it.

In my view, if a user has opportunities to change a message(right now they have - add and modify fields), they'll do it (no matter the original or copy)

Maybe we can add a new func that return a new immutable msg struct or interface with read-only and for copy methods:

func NewMessageFromSrc(spec *MessageSpec, src []byte) *ReadOnlyMessage {
    ...
    return &ReadOnlyMessage{
    ...
    }
}
alovak commented 3 years ago

You are right, that current API allows to set (and basically modify) fields. Would you use a CopyMessage method that returns message without some fields, so you can set them?

krishishah commented 3 years ago

As a point of reference, whenever I think about what the API of this library should be, I think of the protobuf library: https://pkg.go.dev/github.com/golang/protobuf/proto

The high level semantics are not too dissimilar from what we're doing here!

I'd prefer we name the copy function to mirror theirs: iso8583.Clone(...) or iso8583.CloneMessage(...) would be nice.

On the issuer side (where we receive a request, modify a few fields and send an almost identical response), the safest option would be to make a message copy to then mutate for the response path.

I think the nicest workflow for this would be as follows:

  1. Clone message (takes in no arguments other than the message)
  2. Empty fields (can be on pointer receiver)
  3. Set new partial fields (can be on pointer receiver)

I suppose if we use the typed API, 2 and 3 may be converge-able into 1 step?

It'd also be great to have an API to bulk set fields (on the pointer receiver of a message is fine). This would be great if it could be done via the typed API.

e.g.

msg.SetPartialData(
    &Data{
        F5: ...,
        F55: ...,
    }
)

I haven't thought through how the typed API would work in great depth but it would mutate the internal data struct of the Message if not nil whilst also calling SetData on each of the set fields

Dakinola892 commented 2 years ago

I've made a PR to help resolve this issue: https://github.com/moov-io/iso8583/pull/150

alovak commented 2 years ago

Resolved in #150