Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double check handling of Go zero values #160

Open
atonks2 opened this issue Jan 28, 2021 · 10 comments
Open

Double check handling of Go zero values #160

atonks2 opened this issue Jan 28, 2021 · 10 comments
Labels
bug Something isn't working hacktoberfest-accepted

Comments

@atonks2
Copy link
Collaborator

atonks2 commented Jan 28, 2021

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.

@adamdecaf adamdecaf added the bug Something isn't working label Apr 23, 2021
@atonks2
Copy link
Collaborator Author

atonks2 commented Apr 26, 2021

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.

@intricate
Copy link

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
Copy link
Collaborator Author

atonks2 commented Feb 9, 2022

@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
Copy link
Contributor

smithbk commented Jul 14, 2022

@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
Copy link
Collaborator Author

atonks2 commented Jul 14, 2022

@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
Copy link
Contributor

smithbk commented Jul 15, 2022

@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
Copy link
Contributor

smithbk commented Jul 20, 2022

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

@atonks2
Copy link
Collaborator Author

atonks2 commented Jul 20, 2022

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
Copy link
Contributor

smithbk commented Jul 27, 2022

@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
Copy link
Collaborator Author

atonks2 commented Jul 27, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working hacktoberfest-accepted
Projects
None yet
Development

No branches or pull requests

4 participants