moov-io / ach

ACH implements a reader, writer, and validator for Automated Clearing House (ACH) files. The HTTP server is available in a Docker image and the Go package is available.
https://moov-io.github.io/ach/
Apache License 2.0
455 stars 154 forks source link

return as many validation errors as possible #265

Closed adamdecaf closed 5 years ago

adamdecaf commented 6 years ago

Currently, calling .Create() or .Validate() on a File struct returns only one error. This is problematic because clients of the library have to slowly iterate error-by-error fixing their file. Instead we should offer as many errors as possible about the file at once.

adamdecaf commented 6 years ago

cc @wadearnold

wadearnold commented 6 years ago

@adamdecaf it's worth reading "APPENDIX TWO Specifications for Data Acceptance by ACH Operators" of nacha operating rules and guidelines in it's entirity.

PART 2.4 outlines that: A Sending Point must choose either a batch level reject option or a File level reject option for any batch(es) within the File that meets one or more of the error conditions defined below. If the Sending Point chooses the File level reject option, the ACH Operator will reject the entire File containing the erroneous batch(es). If the Sending Point chooses the batch level reject option, those specific batches with an erroneous condition will be rejected, but the remaining batches within the File will be accepted and processed by the ACH Operator.

The current library implementation errors on the entire file. This makes a lot of sense if the file is being used to create a file to be sent to the bank. If it isn't a valid file then why send it! Now that banks are using the library to validate files that are originated from different software and from multiple clients the use case has changed to error on a per batch which is commonly per client rather than not processing the entire file because of one customers error.

adamdecaf commented 6 years ago

@wadearnold That makes much more sense. I've worked out (most of) a branch that introduces a new Error struct, which satisfies the error interface.

I'm not totally sold on this, but it allows us to collect up multiple errors inside of one (much like the common error packages on github).

type Error struct {
    underlying []error
}

func New(msg string) error {
    return WithError(errors.New(msg))
}

func WithError(err error) error {
    if err == nil {
        return nil
    }

    var es []error
    return &Error{
        underlying: append(es, err),
    }
}

func Contains(err error, slug string) bool {
    return err != nil && strings.Contains(err.Error(), slug)
}

func (e *Error) Error() string {
    if e == nil {
        return "<nil>"
    }

    var buf strings.Builder
    if e.underlying == nil {
        e.underlying = make([]error, 0)
    }
    for i := range e.underlying {
        if i > 0 {
            buf.WriteString("\n")
        }
        buf.WriteString(e.underlying[i].Error())
    }
    return buf.String()
}

// func (e *Error) Append(err error) *Error {
//  if err == nil {
//      return nil
//  }
//  e.underlying = append(e.underlying, err)
//  return e
// }

// func (e *Error) Wrap(msg string) *Error {
//  return e.Append(errors.New(msg))
// }

Then, there are a few replacements for the structs.

func File(field, value, msg string) error {
    if field == "" && value == "" {
        return New(fmt.Sprintf("file error message=%q", msg))
    }
    return New(fmt.Sprintf("file error field=%q value=%q message=%q", field, value, msg))
}

func Parse(line int, record string, err error) error {
    if record == "" {
        return New(fmt.Sprintf("line:%d %T %s", line, err, err))
    }
    return New(fmt.Sprintf("line:%d record:%s %T %s", line, record, err, err))
}
adamdecaf commented 6 years ago

I'm also open to returning []error for building/validating batches. This way would give us a lot more backwards compatibility.