search

moov-io / imagecashletter

X9’s Specifications for ICL (Image Cash Letter) to provide Check 21 services. The HTTP server is available in a Docker image and the Go package is available.
https://moov-io.github.io/imagecashletter/
Apache License 2.0
63 stars 39 forks source link

Double check handling of Go zero values #160

Open atonks2 opened 3 years ago

atonks2 commented 3 years ago

Due to the nature of Go's zero values, x9 files created from JSON can end up with 0s and empty time.Time{} values in them. We need to ensure that we're not accidentally writing zero values in x9 files where a field should actually be empty.

atonks2 commented 3 years ago

ANSI X9.100-187:

All fields that are conditional and are not used shall be filled with Blanks

Bundle Control (Type 70)

Cash Letter Control Record (Type 90)

Check Detail Record (Type 25)

Check Detail Addendum A Record (Type 26)

Check Detail Addendum C Record (Type 28)

File Control Record (Type 99)

Image View Analysis Record (Type 54)

Image View Data Record (Type 52)

Image View Detail Record (Type 50)

Return Record (Type 31)

Return Addendum A Record (Type 32)

Return Addendum B Record (Type 33)

Return Addendum D Record (Type 35)

User Record (Type 68)–Format Type 001 Payee Endorsement Record

intricate commented 2 years ago

For the MICRValidIndicator field in the Check Detail Record, this is particularly important as X9 ICL files received from the FRB will specify this field as blank.

atonks2 commented 2 years ago

@intricate We can probably handle that field pretty simply. One of the reasons I haven't started working on this issue is that fixing all of these fields will require breaking changes. I've been debating whether we should use pointers to primitive types (i.e. *int instead of int) so that we can treat nil as empty, or possibly create a wrapper similar to SQL's NullInt64 (https://pkg.go.dev/database/sql#NullInt64)

For MICRValidIndicator, however, we should be able to handle it without a breaking change.

smithbk commented 2 years ago

@atonks2 Daniel, is there any progress on this? We are trying out this library for the first time and looks really good, but we are currently having to comment out some checks in order to verify our testing. Thanks

atonks2 commented 2 years ago

@smithbk Unfortunately I haven't been able to make progress on this yet. Would you mind sharing which checks you're having to disable? Perhaps with such a list we'd be able to prioritize specific fields and tackle this in multiple smaller PRs.

smithbk commented 2 years ago

@atonks2 Daniel, here are the checks I've had to comment out for now.

In imageViewDetail.go:

    if ivDetail.DigitalSignatureMethod != "" {
        if err := ivDetail.isDigitalSignatureMethod(ivDetail.DigitalSignatureMethod); err != nil {
            return &FieldError{FieldName: "DigitalSignatureMethod",
                Value: ivDetail.DigitalSignatureMethod, Msg: err.Error()}
        }
    }
    if ivDetail.ImageCreatorRoutingNumberField() == "000000000" {
        return &FieldError{FieldName: "ImageCreatorRoutingNumber",
            Value: ivDetail.ImageCreatorRoutingNumber,
            Msg:   msgFieldInclusion + ", did you use ImageViewDetail()?"}
    }
    if ivDetail.ImageCreatorDate.IsZero() {
        return &FieldError{FieldName: "ImageCreatorDate",
            Value: ivDetail.ImageCreatorDate.String(),
            Msg:   msgFieldInclusion + ", did you use ImageViewDetail()?"}
    }

And in ReturnDetailAddendumA.go:

    if rdAddendumA.BOFDEndorsementDate.IsZero() {
        return &FieldError{FieldName: "BOFDEndorsementDate",
            Value: rdAddendumA.BOFDEndorsementDate.String(),
            Msg:   msgFieldInclusion + ", 5. did you use ReturnDetailAddendumA()?"}
    }

Thanks, Keith

smithbk commented 2 years ago

@atonks2 Daniel, could you provide an estimate of when this might be fixed? Thanks

atonks2 commented 2 years ago

Hey @smithbk . I'm afraid I don't have an estimate on this one. Updating ICL to better handle default vs empty values is almost certainly going to be a breaking change and I haven't had time to give it the consideration it needs to minimize that impact.

I started digging into the fields you mentioned and I don't believe they relate to this issue. The scope of this issue is optional fields, which are erroneously flagged as invalid because of Go's default (aka "zero") values. For example you may have a file where MICRValidIndicator is omitted/empty (which is valid), but that file in this library would end up with Go's default int value for this field (0), which is not a valid MICRValidIndicator value.

For the fields/checks you mentioned:

Digital Signature Method

The specification says this is conditional, and our library only performs validation on this field if a value is present. If DigitalSignatureMethod is empty, the check is skipped.

Image Creator Routing Number

This is defined as a mandatory field in ANSI-X9.100-187, so our library returns an error if the field is missing/invalid.

Image Creator Date

This is also a mandatory field, so omitting it will result in an error.

BOFD/Endorsement Date

Also a mandatory field.

If any of the above is a misunderstanding on my part, or you have a version of the specification that differs in these field definitions from ANSI-X9.100-187 (2016), lets open new issues to address those concerns.

smithbk commented 2 years ago

@atonks2 Daniel, in order to not introduce any breaking changes, I'm curious if you've considered (and if it would work) to add "HasXXX" bool fields for each of these problematic fields. For example, for MICRValidTotalAmount, add a field named HasMICRValidTotalAmount which is a bool for someone to use to distinguish between the MICRValidTotalAmount field being non-existent and the field existing with a zero value? Just a thought.

atonks2 commented 2 years ago

I appreciate the suggestion @smithbk. That would work for people importing this codebase as a library to construct files, but would not work for those using the API to upload raw x9 files.

The two options I have in mind each come with their own pros and cons:

  1. Use pointers to primitive types so fields that are intentionally absent would be nil (e.g. use *int instead of int)
  2. Use or implement a wrapper around the primitive types, like this sql package.