moov-io / iso8583

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

Make bitmap of arbitrary size #211

Closed alovak closed 1 year ago

alovak commented 1 year ago

What

Example of how to create fixed bitmap:

    bitmap := NewBitmap(&Spec{
        Length:            2, // 2 bytes - 16 bits
        Description:       "Bitmap",
        Enc:               encoding.BytesToASCIIHex,
        Pref:              prefix.Hex.Fixed,
        DisableAutoExpand: true,
    })

    // when setting bits inside of bitmap range
    bitmap.Set(1)
    bitmap.Set(16)
        bitmap.Set(17) // is ignored as 17 is out of range

Why

P.S. Note, for most card brands, the default single bitmap size is 8 bytes (64 bits). Because of the previous implementation, the Length parameter was ignored and instead underlying storage for the 3 (max allowed) bitmaps was used. For all operations, it calculated the number of bitmaps based on the bits set. So, if for auto-expanding bitmaps you use 16 or something else, then bitmap will be expanded by 16 bytes for new bitmaps. That's not what you want. Just change from 16 to 8.

adamdecaf commented 1 year ago

It sounds like AutoExpand: false is the current behavior so that should be the default. Changing this into an auto-expanding bitmap (by default) will break existing code.

mfdeveloper508 commented 1 year ago

LGTM the approach is good

my advice is

alovak commented 1 year ago

@adamdecaf our current behaviour is AutoExpand: true that's why I had to introduce option that is false by default (negative naming).

alovak commented 1 year ago

@mfdeveloper508 thanks for the suggestions

I tried to not break anything. Now integration will be broken only if Length is different from 8. If it's not, then new version of the package will not require any changes from you.

I like your proposal, but I'm afraid as it's a not backward compatible change, I would vote for DisableAutoExpand. ,

remove DisableAutoExpand option AutoExpand is active if there isn't any length setting AutoExpand is inactive if there is any length setting ^^ this will force people to make updates to their systems as the default use case is auto expand and I believe most of the time Lenght is specified.

I'm also not aware of cases with auto-expanding bitmaps of length different than 8. But maybe there are such cases?

adamdecaf commented 1 year ago

Thanks for making it backwards compatible.