Encoding.ascii tlv tag - Githubissues

jerome-laforge commented 1 year ago

Hello, I want to be able to ignore unknown subfield with ASCII Encoder. This PR tries to add this feature for ASCII Encoder (as BerTLVTag).

Can you review this PR?

Thank in advance,

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.15 :tada:

Comparison is base (fb3b4df) 73.72% compared to head (dfa3107) 73.88%.

:exclamation: Current head dfa3107 differs from pull request most recent head 9155ee5. Consider uploading reports for the commit 9155ee5 to get more accurate results

:exclamation: 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

```diff @@ Coverage Diff @@ ## master #236 +/- ## ========================================== + Coverage 73.72% 73.88% +0.15% ========================================== Files 42 42 Lines 2158 2236 +78 ========================================== + Hits 1591 1652 +61 - Misses 339 353 +14 - Partials 228 231 +3 ``` | [Impacted Files](https://app.codecov.io/gh/moov-io/iso8583/pull/236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io) | Coverage Δ | | |---|---|---| | [encoding/bertlv.go](https://app.codecov.io/gh/moov-io/iso8583/pull/236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZW5jb2RpbmcvYmVydGx2Lmdv) | `91.30% <ø> (ø)` | | | [field/bitmap.go](https://app.codecov.io/gh/moov-io/iso8583/pull/236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZmllbGQvYml0bWFwLmdv) | `80.64% <ø> (ø)` | | | [field/spec.go](https://app.codecov.io/gh/moov-io/iso8583/pull/236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZmllbGQvc3BlYy5nbw==) | `66.66% <ø> (ø)` | | | [field/composite.go](https://app.codecov.io/gh/moov-io/iso8583/pull/236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZmllbGQvY29tcG9zaXRlLmdv) | `83.22% <100.00%> (+0.40%)` | :arrow_up: | | [field\_filter.go](https://app.codecov.io/gh/moov-io/iso8583/pull/236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZmllbGRfZmlsdGVyLmdv) | `68.33% <100.00%> (ø)` | | | [prefix/bertlv.go](https://app.codecov.io/gh/moov-io/iso8583/pull/236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-cHJlZml4L2JlcnRsdi5nbw==) | `92.00% <100.00%> (ø)` | | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/moov-io/iso8583/pull/236/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io)

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

alovak commented 1 year ago

With the suggested change with LLL encoders, you still may get an "unknown" field with length that doesn't match specified in the Tag. Let's say you assume that all fields have L, but what if some will have LL?

With BerTLV it's clear how we can skip unknown fields - their length is encoded into data, not defined in the spec. But with regular fields, I don't see how it's possible.

jerome-laforge commented 1 year ago

With the suggested change with LLL encoders, you still may get an "unknown" field with length that doesn't match specified in the Tag. Let's say you assume that all fields have L, but what if some will have LL?

This PR assumes that all sub-fields are encoding in the same way (i.e. L, LL, LLL or LLLL) without mix. If there is a mix, then it is likely that error will occurred while decoding.

In our case, the partners can ensure that all sub-fields (in our case sub-fields of field 48) are encoding in the same way (L, LL, LLL or LLLL). But, the partners can add new fields and those fields must be ignored as they are unknown. That is the reason why of this PR.

alovak commented 1 year ago

But how do you know that they don't add LLL field when you expect it to be L?

jerome-laforge commented 1 year ago

But how do you know that they don't add LLL field when you expect it to be L?

Unfortunately, I can't. If they add LLL instead of L, then it is likely that the decoding will fail. I can understand if you think that it is better to reject this PR because this change doesn't expect the quality or so on. I will keep our driver on my fork :)

alovak commented 1 year ago

I have one option that may work here without too much changes.

Here it is:

Add Pref into TagSpec - with comment that by default it's prefix.BerTLV otherwise, it will be used for all unknown tags and may fail to parse if unknown tag prefixer will be different

update unpackSubfieldsByTag in the following way:

        if f.skipUnknownTLVTags() {
            var pref prefix.Prefixer
            // to obtain the length of the unknown tag and add it to the offset we need to decode its length
            // if prefixer is set, use it and hope that length of all unknown tags is encoded using this prefixer
            if f.Pref != nil {
                pref = f.spec.Pref
                maxLen = maxLenOfUnknownTag
            } else {
                // Because BER-TLV lengths are decoded dynamically, the maxLen method argument is ignored.
                pref = prefix.BerTLV
                maxLen = ignoredMaxLen
            }

            fieldLength, readed, err := pref.DecodeLength(maxLen, data[offset:])
            if err != nil {
                return 0, err
            }
            offset += fieldLength + readed
            continue
        }

Update skipUnknownTLVTags like this:

func (f *Composite) skipUnknownTLVTags() bool {
return f.spec.Tag != nil && f.spec.Tag.SkipUnknownTLVTags && (f.spec.Tag.Enc == encoding.BerTLVTag || f.spec.Tag.Pref != nil)
}

In this way we:

don't add new encodings
don't break API as we add new field and don't change current behaviour
support your case

What do you think about it?

jerome-laforge commented 1 year ago

What do you think about it?

It seems great.

But, could you also show me the code of CustomCompositeField struct?

And I forgot to tell you: thank for your support :)

alovak commented 1 year ago

@jerome-laforge CustomCompositeField was just my internal experiment I decided to run to see if you can create a custom field as an alternative to this PR. Then as the result of the experiment (code that I wrote) I realized that we can make the change I suggested in previous comment and it is acceptable (not introducing a lot of new things or changing a lot) :)

jerome-laforge commented 1 year ago

By f.Pref, did you mean f.spec.Pref? If yes, then I am surely miss something, because f.spec.Pref is for field (ex: field 38), isn't it?

For example, I have field 38 on LLL with 999 maxLen, but if all partners decide that all its sub-fields are encode on L. Did I miss something?

jerome-laforge commented 1 year ago

or maybe, you want enforce that field & sub-fields have the same encoding? (and it is acceptable for me)

alovak commented 1 year ago

Add Pref prefixer.Prefixer to field.TagSpec:

type TagSpec struct {
    Pref prefixer.Prefixer
}

Sorry, my example is from the experiment code and it says f.Pref. I'll update it.

alovak commented 1 year ago

We are not adding new encodings, we can add length prefixer for the TagSpec and use it if we want to skip unknown tags and it's not BerTLV.

jerome-laforge commented 1 year ago

Please find the update. Let me know if something (for example naming or doc is not ok for you).

Thank again for your support

alovak commented 1 year ago

@jerome-laforge it looks good! One final question. May I ask you to keep tests only for one type of prefix (you can choose any L or LL or LLL)? I'm not sure there is a benefit in testing composite with unknown tags all prefixers as they have their own tests.

jerome-laforge commented 1 year ago

@jerome-laforge it looks good! One final question. May I ask you to keep tests only for one type of prefix (you can choose any L or LL or LLL)? I'm not sure there is a benefit in testing composite with unknown tags all prefixers as they have their own tests.

Ok, I will do that

jerome-laforge commented 1 year ago

Great job @jerome-laforge ! Thanks a lot for the contributions!

Thank for your support !

moov-io / iso8583

Encoding.ascii tlv tag #236

Codecov Report