Return early when packing zero length fields #149
Conversation
field/composite_test.go
Outdated
| err := composite.SetData(data) | ||
| require.NoError(t, err) | ||
|
|
||
| // Subfield 4 & 5 falls outside of the bounds of the 3 byte limit imposed |
There was a problem hiding this comment.
Also, I'm not sure I understand this bit of the PR. How does the early return help with fields that fall outside of the bounds? Also, where 3 byte limit prefix is? Is it because of ebcdic1047? Sorry for the dumb questions.
There was a problem hiding this comment.
Hi, this probably wasn't explained the best by me - the motivation behind this PR was to allow length-prefixed composites that with non-tagged subfields to be parsed only up to the specified length, so that non-populated subfields in the composite could be ignored without erroring/panicking.
The problem with the current implementation is when trying to parse a composite without tagged subfields, Composite loops through all the subfield IDs in the spec regardless and errors when it tried to unpack a zero-length data buffer for a subfield not included in the data.
A possible fix would have been to just add check to Composite to stop parsing when we'd read >= the length of the data, but this caused an issue where errors weren't being accurately reported for invalid length data that was too long (i.e the spec only allows 4 bytes but 7 bytes were provided, it would stop reading after >= 4 bytes, so it wouldn't accurately highlight the issue with the combined length of the subfields).
So now I attempted to get the desired result by focusing on not panicking when attempting to unpack zero-length data fields.
There was a problem hiding this comment.
In the original example, I was referring to the '03' prefix when mentioning the 3 byte limit (i.e. [03 | 102] -> [length prefix | data]), but I've updated it the most recent commit to hopefully make it clearer
Codecov Report
@@ Coverage Diff @@
## master #149 +/- ##
==========================================
- Coverage 70.84% 70.73% -0.12%
==========================================
Files 36 37 +1
Lines 1581 1616 +35
==========================================
+ Hits 1120 1143 +23
- Misses 298 304 +6
- Partials 163 169 +6
Continue to review full report at Codecov.
|
|
I'm looking for an approval from @alovak before we merge. I've included this in https://github.com/moov-io/iso8583/commits/v0.8-beta |
Currently, composite fields will error when they try and pack zero length fields, a situation which arises when not all subfields in a composite (w/out id tags) are present.
The problem occurs when call .Pack() on individual fields, non populated fields would try and encode a zero length buffer and return an error - instead, now the field types will simply return an empty buffer and not error.
This will allows the packing of composite fields with non-populated subfields that fall outside of the composite prefix length.