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
446 stars 149 forks source link

Incorrect Batch number reassigned when batches are in descending order in nacha file #1431

Open tamil-muthukumar opened 1 month ago

tamil-muthukumar commented 1 month ago

ACH Version

v1.33.3

What were you trying to do?

We are trying to process a file that looks something like this:

batch 3
   - entry 1
 batch 2
   - entry 1
   - entry 2
 batch 1
   - entry 1
   - entry 2

When the file is getting parsed, we noticed that the batch numbers were incorrectly reassigned because of the <= logic here. In the loop, when we get to 3rd batch (batchSeq = 3) which is batch 1, the <= condition allows the if block to execute where we are resetting the batch number to 3. So we end up with 2 different batches set to the same batch number.

What did you expect to see?

Batch number should not be reset to 1 when the batches are in descending order

What did you see?

batch number 1 is reassigned to 3

How can we reproduce the problem?

Create a small file with descending batches and atleast one entry in each batch and run it through the parser.

tamil-muthukumar commented 1 month ago

Side note: If we deem this to be a valid issue, i'd love to work on this as my first good task :)

adamdecaf commented 1 month ago

Interesting.. My recollection of the Nacha spec says batch numbers must be in ascending order, so this likely isn't handled. Go ahead and open a PR for this! How were you planning on fixing this?

tamil-muthukumar commented 1 month ago

Yeah, batches can be unordered as well it looks like.

I'm thinking that just removing = here should resolve it.

adamdecaf commented 1 month ago

I'm not sure if that will work. We rely on tabulation of BatchNumbers in a few code paths, but I'm curious to see what you come up with.

tamil-muthukumar commented 1 month ago

Yeah Im digging more into this today. But the general direction im looking at is that, do not reassign a batch number unless its set to anything less than 1 instead of the current behavior which is resetting the value for anything <= 1.