imagecashletter icon indicating copy to clipboard operation
imagecashletter copied to clipboard

Double check handling of Go zero values

Open atonks2 opened this issue 4 years ago • 10 comments

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 avatar Jan 28 '21 21:01 atonks2

ANSI X9.100-187:

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

Bundle Control (Type 70)

  • MICRValidTotalAmount: Can update logic to replace "0" with " "
  • CreditTotalIndicator: Valid values are 0, 1. Need to be able to replace with empty space.

Cash Letter Control Record (Type 90)

  • SettlementDate: Should check IsZero() for time.Time values and render as empty space if true
  • CreditTotalIndicator: Valid values are 0, 1. Need to be able to replace with empty space.

Check Detail Record (Type 25)

  • MICRValidIndicator: Valid values are 1-4. Can update logic to replace 0 with " " because 0 isn't a valid value.
  • CorrectionIndicator: Valid values are 0-4.

Check Detail Addendum A Record (Type 26)

  • BOFDCorrectionIndicator: Valid values are 0-4.

Check Detail Addendum C Record (Type 28)

  • EndorsingBankCorrectionIndicator: Valid values are 0-4.

File Control Record (Type 99)

  • CreditTotalIndicator: Valid values are 0, 1. Need to be able to replace with empty space.

Image View Analysis Record (Type 54)

  • PartialImage: Valid values are 0-2. Need to be able to replace with empty space.
  • ExcessiveImageSkew: Valid values are 0-2. Need to be able to replace with empty space.
  • PiggybackImage: Valid values are 0-2. Need to be able to replace with empty space.
  • TooLightOrTooDark: Valid values are 0-2. Need to be able to replace with empty space.
  • StreaksAndOrBands: Valid values are 0-2. Need to be able to replace with empty space.
  • BelowMinimumImageSize: Valid values are 0-2. Need to be able to replace with empty space.
  • ExceedsMaximumImageSize: Valid values are 0-2. Need to be able to replace with empty space.
  • ImageEnabledPOD: Valid values are 0-2. Need to be able to replace with empty space.
  • SourceDocumentBad: Valid values are 0-2. Need to be able to replace with empty space.
  • DateUsability: Valid values are 0-2. Need to be able to replace with empty space.
  • PayeeUsability: Valid values are 0-2. Need to be able to replace with empty space.
  • ConvenienceAmountUsability: Valid values are 0-2. Need to be able to replace with empty space.
  • AmountInWordsUsability: Valid values are 0-2. Need to be able to replace with empty space.
  • SignatureUsability: Valid values are 0-2. Need to be able to replace with empty space.
  • PayorNameAddressUsability: Valid values are 0-2. Need to be able to replace with empty space.
  • MICRLineUsability: Valid values are 0-2. Need to be able to replace with empty space.
  • MemoLineUsability: Valid values are 0-2. Need to be able to replace with empty space.
  • PayorBankNameAddressUsability: Valid values are 0-2. Need to be able to replace with empty space.
  • PayeeEndorsementUsability: Valid values are 0-2. Need to be able to replace with empty space.
  • BOFDEndorsementUsability: Valid values are 0-2. Need to be able to replace with empty space.
  • TransitEndorsementUsability: Valid values are 0-2. Need to be able to replace with empty space.

Image View Data Record (Type 52)

  • ClippingOrigin: Valid values are 0-4. Need to be able to replace with empty space.

Image View Detail Record (Type 50)

  • DigitalSignatureIndicator: 0 or 1. Need to be able to replace with empty space.
  • SecurityKeySize: 00001-99999. Can render as blank space if value = 0.
  • ProtectedDataStart: 0000000–9999999. Need to be able to replace with empty space.
  • ProtectedDataLength: 0000000–9999999. Need to be able to replace with empty space.
  • ImageRecreateIndicator: 0 or 1. Need to be able to replace with empty space.

Return Record (Type 31)

  • TimesReturned: 0-3. Need to be able to replace with empty space.

Return Addendum A Record (Type 32)

  • BOFDCorrectionIndicator: 0-4. Need to be able to replace with empty space.

Return Addendum B Record (Type 33)

  • PayorBankBusinessDate: Check if value IsZero() and replace with empty space if so.

Return Addendum D Record (Type 35)

  • EndorsingBankCorrectionIndicator: 0-4. Need to be able to replace with empty space.
  • EndorsingBankIdentifier: 0-3. Need to be able to replace with empty space.

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

  • EndorsementDate: Check if value IsZero() and replace with empty space if so.
  • Time: Check if value IsZero() and replace with empty space if so.
  • EndorsementIndicator: 0-9. Need to be able to replace with empty space.

atonks2 avatar Apr 26 '21 22:04 atonks2

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.

intricate avatar Feb 09 '22 20:02 intricate

@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.

atonks2 avatar Feb 09 '22 20:02 atonks2

@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

smithbk avatar Jul 14 '22 20:07 smithbk

@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.

atonks2 avatar Jul 14 '22 20:07 atonks2

@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 avatar Jul 15 '22 13:07 smithbk

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

smithbk avatar Jul 20 '22 17:07 smithbk

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.

atonks2 avatar Jul 20 '22 21:07 atonks2

@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.

smithbk avatar Jul 27 '22 13:07 smithbk

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.

atonks2 avatar Jul 27 '22 15:07 atonks2