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

fix : noneprefixer did not return correct fixed len #304

Closed T-eli closed 5 months ago

T-eli commented 5 months ago

this pull request contains 2 fixes:

  1. noneprefixer fixed was returning len(data) instead of fixed value
  2. text edit: numeric field parses int64 not int

while i am here i have a question if the someone can answer: -> Aren't Numeric Fields usually encoded in BCD?

Nemric field pack method: data := []byte(strconv.FormatInt(f.value, 10))...

should be : data, _ := hex.DecodeString(strconv.FormatInt(f.value, 10))

I know that you can set the Encoder in spec to BCD. but I dont see the deference between using a String Field and Numeric Field.

codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (27feb74) 74.98% compared to head (87a4dcb) 74.98%.

:exclamation: Current head 87a4dcb differs from pull request most recent head 09e2fc7. Consider uploading reports for the commit 09e2fc7 to get more accurate results

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #304 +/- ## ======================================= Coverage 74.98% 74.98% ======================================= Files 44 44 Lines 2495 2495 ======================================= Hits 1871 1871 Misses 407 407 Partials 217 217 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alovak commented 5 months ago

Hey! Thanks for the PR and the fixes!

while i am here i have a question if the someone can answer: -> Aren't Numeric Fields usually encoded in BCD?

Nemric field pack method: data := []byte(strconv.FormatInt(f.value, 10))...

should be : data, _ := hex.DecodeString(strconv.FormatInt(f.value, 10))

I know that you can set the Encoder in spec to BCD. but I dont see the deference between using a String Field and Numeric Field.

I saw different encodings used for the numeric fields. It depends on the card brand / processor.

The difference between String and Numeric is in the type of the underlying value string vs int64:

str := field.NewStringValue("100")
str.Value() // "100"

i := field.NewNumericValue(100)
i.Value() // 100

There will be an error if you try to unpack a message with a non-numeric field value for a field specified as field.Numeric.

alovak commented 5 months ago

We should revert the change to the None prefixer before we can merge the fixes.

T-eli commented 5 months ago

hello @alovak ,

thank you for answering my question.

i was confused by the none.fixed which i interpreted as no prefix + use fixed length

anyways i cherry picked to other changes. if you want to merge this PR

alovak commented 5 months ago

@T-eli if you have any other questions, feel free to ask them on either https://github.com/moov-io/iso8583/issues or joining Moov Slack community (channel #iso8583)

T-eli commented 5 months ago

thanks @alovak.