Composites with private bitmaps

[FacundoMora]

Hi Everyone!,

A few month ago me and my team encountered with a peculiar type of field, that implied a structure like this:
[LL BCD Length] + [3 bytes bitmap] + [Subfields]

In this structure we were missing two features:

• Arbitrary sized bitmaps, when defining it on spec we could indicate its block size (3 bytes) and the amount of blocks.
• Some way to have a field with subfields, where its subfields existence could be determined by a private bitmap (We are talking about you, composite 🤨)

As you already know, in a previous release, a change about arbitrary size bitmaps made by @alovak took place, in this changes our first feature needed to solve this situation was finished.

So now, we bring the second feature, where we can use this bitmap as a private bitmap of a composite 👏

To achieve this we decided to reutilize most of the logic in iso8583.Message, specifically the way that uses its bitmap to find the message fields for packing and unpacking. Obviously we had to modify a bit of code to make sense on a composite context.

PD: Yes, iso8583.Message could be replaced as a composite given that their behaviour are pretty the same, but with @alovak agree on leave it for a future potential change, so that these changes do not become of great impact.

Finally, a minor change we made was that we moved the spec validation location inside the SetSpec along with the ContructSubfields call. We made this because we thought this makes more sense given that this two things have to run if the spec is changed.

I look forward for your comments on this, thanks!

[codecov-commenter]

Codecov Report

Patch coverage: 85.18% and project coverage change: +0.83 🎉

Comparison is base (49dc79f) 73.10% compared to head (2060db4) 73.94%.

❗ Current head 2060db4 differs from pull request most recent head d126ec4. Consider uploading reports for the commit d126ec4 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   73.10%   73.94%   +0.83%     
==========================================
  Files          41       41              
  Lines        1997     2069      +72     
==========================================
+ Hits         1460     1530      +70     
+ Misses        327      325       -2     
- Partials      210      214       +4     

Impacted Files	Coverage Δ
field/spec.go	66.66% <ø> (ø)
field/composite.go	82.81% <85.18%> (+1.08%)	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[alovak]

@FacundoMora hey! Thanks for the PR! Great job! I'm reviewing it.

[adamdecaf]

Can we return an error here instead of the panics? It's a minor break to the public API, but returning valid errors is more important to me.

[FacundoMora]

We initially thought on that, but the problem with this is that we have to change the field.Field interface, this means that we will have to change all the implementations for this interface, what do you think?

[alovak]

I have checked, and it is not an easy change to make because SetSpec is being used somewhere along the call chain by func NewMessage(spec *MessageSpec) *Message - and I don't know how big would be the breaking change if we add error to NewMessage.

[alovak]

Here are some of my comments:

1. It seems if we add a separate field Bitmap into the Spec as we did for Tag that should simplify the code a little bit.
2. I think we can add a bitmap to the composite field without changing the structure of the constructor and SetSpec. Let’s keep them as they are. I'm not sure that such change is needed.
3. In the validation we can have distinct blocks for validation when Bitmap is set and when … to make it simpler. In the PR I can’t easily understand all conditions 🙈
4. Let’s use []string and not []int, we still need to convert int to string or string to int, but keeping it []string allows us to use the same sort package that we already have and use
5. I would suggest adding validation of all indexes (that they can be converted into int without errors) into validateCompositeSpec when Bitmap is set.

[alovak]

we should not use bitmap from the spec as being a pointer it will be the same Bitmap for all messages. We should create a new Bitmap field.

[FacundoMora]

Made a deep copy on Bitmap()

[alovak]

@FacundoMora It looks good, just some tiny changes are needed and we can merge it.

[alovak]

Great job!