Repository: moov-io/ach
Pull request #1393 (2024-03-13 18:36:41)

fix: MergeFiles should keep ValidateOpts
In fact we should merge them together rather than take the first `ValidateOpts` 


Patch:
diff --git a/file.go b/file.go
index a26b1b03..6bcf5ee2 100644
--- a/file.go
+++ b/file.go
@@ -706,6 +706,45 @@ type ValidateOpts struct {
 	AllowInvalidAmounts bool `json:"allowInvalidAmounts"`
 }
 
+// merge will combine two ValidateOpts structs and keep any non-zero field values.
+func (v *ValidateOpts) merge(other *ValidateOpts) *ValidateOpts {
+	// If either ValidateOpts is nil return the other
+	if v == nil {
+		return other
+	}
+	if other == nil {
+		return v
+	}
+
+	out := &ValidateOpts{
+		SkipAll:                          v.SkipAll || other.SkipAll,
+		RequireABAOrigin:                 v.RequireABAOrigin || other.RequireABAOrigin,
+		BypassOriginValidation:           v.BypassOriginValidation || other.BypassOriginValidation,
+		BypassDestinationValidation:      v.BypassDestinationValidation || other.BypassDestinationValidation,
+		CustomTraceNumbers:               v.CustomTraceNumbers || other.CustomTraceNumbers,
+		AllowZeroBatches:                 v.AllowZeroBatches || other.AllowZeroBatches,
+		AllowMissingFileHeader:           v.AllowMissingFileHeader || other.AllowMissingFileHeader,
+		AllowMissingFileControl:          v.AllowMissingFileControl || other.AllowMissingFileControl,
+		BypassCompanyIdentificationMatch: v.BypassCompanyIdentificationMatch || other.BypassCompanyIdentificationMatch,
+		CustomReturnCodes:                v.CustomReturnCodes || other.CustomReturnCodes,
+		UnequalServiceClassCode:          v.UnequalServiceClassCode || other.UnequalServiceClassCode,
+		AllowUnorderedBatchNumbers:       v.AllowUnorderedBatchNumbers || other.AllowUnorderedBatchNumbers,
+		AllowInvalidCheckDigit:           v.AllowInvalidCheckDigit || other.AllowInvalidCheckDigit,
+		UnequalAddendaCounts:             v.UnequalAddendaCounts || other.UnequalAddendaCounts,
+		PreserveSpaces:                   v.PreserveSpaces || other.PreserveSpaces,
+		AllowInvalidAmounts:              v.AllowInvalidAmounts || other.AllowInvalidAmounts,
+	}
+
+	if v.CheckTransactionCode != nil {
+		out.CheckTransactionCode = v.CheckTransactionCode
+	}
+	if other.CheckTransactionCode != nil {
+		out.CheckTransactionCode = other.CheckTransactionCode
+	}
+
+	return out
+}
+
 // ValidateWith performs checks on each record according to Nacha guidelines.
 // ValidateWith will never modify the File.
 //
diff --git a/merge.go b/merge.go
index 2943f6d8..958e7bbe 100644
--- a/merge.go
+++ b/merge.go
@@ -98,7 +98,8 @@ func MergeFilesWith(incoming []*File, conditions Conditions) ([]*File, error) {
 	}
 
 	sorted := &outFile{
-		header: incoming[0].Header,
+		header:       incoming[0].Header,
+		validateOpts: incoming[0].GetValidation(),
 	}
 
 	for i := range incoming {
@@ -106,6 +107,7 @@ func MergeFilesWith(incoming []*File, conditions Conditions) ([]*File, error) {
 		if outFile == nil {
 			return nil, fmt.Errorf("finding outfile from incoming[%d]: %w", i, ErrPleaseReportBug)
 		}
+		outFile.validateOpts = outFile.validateOpts.merge(incoming[i].GetValidation())
 
 		for j := range incoming[i].Batches {
 			bh := incoming[i].Batches[j].GetHeader()
@@ -144,6 +146,10 @@ func MergeFilesWith(incoming []*File, conditions Conditions) ([]*File, error) {
 		file := NewFile()
 		file.Header = sorted.header
 
+		if sorted.validateOpts != nil {
+			file.SetValidation(sorted.validateOpts)
+		}
+
 		currentFileLineCount := 2 // FileHeader, FileControl
 		var currentFileDollarAmount int
 
@@ -251,6 +257,8 @@ type outFile struct {
 	header  FileHeader
 	batches []*batch
 
+	validateOpts *ValidateOpts
+
 	next *outFile
 }
 


Test patch:
diff --git a/file_test.go b/file_test.go
index 8f1cd366..de5d35a0 100644
--- a/file_test.go
+++ b/file_test.go
@@ -2120,3 +2120,50 @@ func TestFile_SkipValidation(t *testing.T) {
 	})
 	require.NoError(t, err)
 }
+
+func TestFile_ValidateOpts_Merge(t *testing.T) {
+	first := &ValidateOpts{
+		RequireABAOrigin: true,
+		CheckTransactionCode: func(code int) error {
+			return nil
+		},
+		CustomReturnCodes: true,
+	}
+	second := &ValidateOpts{
+		RequireABAOrigin: false,
+		PreserveSpaces:   true,
+	}
+
+	merged := first.merge(second)
+
+	require.False(t, merged.SkipAll)                 // keep false when both are false
+	require.True(t, merged.RequireABAOrigin)         // was true in first
+	require.NotNil(t, merged.CheckTransactionCode)   // non-nil function
+	require.Nil(t, merged.CheckTransactionCode(123)) // func always returns nil
+	require.True(t, merged.CustomReturnCodes)
+	require.True(t, merged.PreserveSpaces)
+
+	// Now make sure that second's CheckTransactionCode can overwrite first's
+	second.CheckTransactionCode = func(code int) error {
+		return fmt.Errorf("code %d is invalid", code)
+	}
+
+	merged = first.merge(second)
+	require.False(t, merged.SkipAll)               // keep false when both are false
+	require.True(t, merged.RequireABAOrigin)       // was true in first
+	require.NotNil(t, merged.CheckTransactionCode) // non-nil function
+	require.ErrorContains(t, merged.CheckTransactionCode(123), "code 123 is invalid")
+	require.True(t, merged.CustomReturnCodes)
+	require.True(t, merged.PreserveSpaces)
+
+	t.Run("empty", func(t *testing.T) {
+		var empty *ValidateOpts
+		full := &ValidateOpts{
+			CustomReturnCodes: true,
+		}
+
+		require.Nil(t, empty.merge(empty))
+		require.NotNil(t, empty.merge(full))
+		require.NotNil(t, full.merge(empty))
+	})
+}
diff --git a/merge_bench_test.go b/merge_bench_test.go
index 769fa71b..e9ee9c72 100644
--- a/merge_bench_test.go
+++ b/merge_bench_test.go
@@ -35,13 +35,23 @@ func BenchmarkMergeFiles(b *testing.B) {
 		}
 	)
 
-	randomFile := func(B *testing.B) *File {
+	type options struct {
+		withValidateOpts bool
+	}
+
+	randomFile := func(B *testing.B, opts options) *File {
 		B.Helper()
 
 		file := NewFile()
 		fh := staticFileHeader()
 		file.SetHeader(fh)
 
+		if opts.withValidateOpts {
+			file.SetValidation(&ValidateOpts{
+				PreserveSpaces: true,
+			})
+		}
+
 		for b := 0; b < batchesPerFile; b++ {
 			base, _ := rand.Int(rand.Reader, big.NewInt(1e7))
 			traceNumber := int(base.Int64())
@@ -64,10 +74,10 @@ func BenchmarkMergeFiles(b *testing.B) {
 		return file
 	}
 
-	randomFiles := func(b *testing.B) (out []*File) {
+	randomFiles := func(b *testing.B, opts options) (out []*File) {
 		b.Helper()
 		for i := 0; i < b.N; i++ {
-			out = append(out, randomFile(b))
+			out = append(out, randomFile(b, opts))
 		}
 		return
 	}
@@ -99,10 +109,10 @@ func BenchmarkMergeFiles(b *testing.B) {
 		b.Fatalf("unexpected indices: %#v", indices)
 	}
 
-	mergeInGroups := func(b *testing.B, groups int) []*File {
+	mergeInGroups := func(b *testing.B, groups int, opts options) []*File {
 		b.Helper()
 
-		files := randomFiles(b)
+		files := randomFiles(b, opts)
 		indices := makeIndices(len(files), groups)
 
 		b.ReportAllocs()
@@ -145,19 +155,25 @@ func BenchmarkMergeFiles(b *testing.B) {
 	}
 
 	b.Run("MergeFiles", func(b *testing.B) {
-		mergeInGroups(b, 1)
+		mergeInGroups(b, 1, options{})
+	})
+
+	b.Run("MergeFiles_ValidateOpts", func(b *testing.B) {
+		mergeInGroups(b, 1, options{
+			withValidateOpts: true,
+		})
 	})
 
 	b.Run("MergeFiles_3Groups", func(b *testing.B) {
-		mergeInGroups(b, 3)
+		mergeInGroups(b, 3, options{})
 	})
 	b.Run("MergeFiles_5Groups", func(b *testing.B) {
-		mergeInGroups(b, 5)
+		mergeInGroups(b, 5, options{})
 	})
 	b.Run("MergeFiles_10Groups", func(b *testing.B) {
-		mergeInGroups(b, 10)
+		mergeInGroups(b, 10, options{})
 	})
 	b.Run("MergeFiles_100Groups", func(b *testing.B) {
-		mergeInGroups(b, 100)
+		mergeInGroups(b, 100, options{})
 	})
 }
diff --git a/merge_test.go b/merge_test.go
index fcf6a7e6..955498fa 100644
--- a/merge_test.go
+++ b/merge_test.go
@@ -513,6 +513,32 @@ func populateFileWithMockBatches(t testing.TB, numBatches int, file *File) {
 	}
 }
 
+func TestMergeFiles__ValidateOpts(t *testing.T) {
+	f1, err := readACHFilepath(filepath.Join("test", "testdata", "ppd-debit.ach"))
+	require.NoError(t, err)
+
+	f1.SetValidation(&ValidateOpts{
+		CustomReturnCodes: true,
+	})
+
+	f2, err := readACHFilepath(filepath.Join("test", "testdata", "web-debit.ach"))
+	require.NoError(t, err)
+
+	f2.Header = f1.Header
+	f2.SetValidation(&ValidateOpts{
+		AllowInvalidAmounts: true,
+	})
+
+	merged, err := MergeFiles([]*File{f1, f2})
+	require.NoError(t, err)
+	require.Len(t, merged, 1)
+
+	opts := merged[0].GetValidation()
+	require.False(t, opts.SkipAll)
+	require.True(t, opts.CustomReturnCodes)
+	require.True(t, opts.AllowInvalidAmounts)
+}
+
 func TestMergeFilesHelpers(t *testing.T) {
 	t.Run("pickOutFile", func(t *testing.T) {
 		fh := mockFileHeader()