From 035370d9be3610d6ca2fa2eff0a637cae300a1f8 Mon Sep 17 00:00:00 2001
From: dbemiller <27972385+dbemiller@users.noreply.github.com>
Date: Tue, 20 Feb 2018 15:12:34 -0500
Subject: [PATCH] Discard data more liberally on database errors. (#339)

* Discard data more liberally on database errors.

* Removed logs from tests.
---
 .../backends/db_fetcher/fetcher.go            | 16 +++++++---
 .../backends/db_fetcher/fetcher_test.go       | 30 ++++++++++++++++++-
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/stored_requests/backends/db_fetcher/fetcher.go b/stored_requests/backends/db_fetcher/fetcher.go
index 6e9b41862b5..e228c07692c 100644
--- a/stored_requests/backends/db_fetcher/fetcher.go
+++ b/stored_requests/backends/db_fetcher/fetcher.go
@@ -49,23 +49,31 @@ func (fetcher *dbFetcher) FetchRequests(ctx context.Context, ids []string) (map[
 		}
 		return nil, []error{err}
 	}
-	defer rows.Close()
+	defer func() {
+		if err := rows.Close(); err != nil {
+			glog.Errorf("error closing DB connection: %v", err)
+		}
+	}()
 
 	reqData := make(map[string]json.RawMessage, len(ids))
-	var errs []error = nil
 	for rows.Next() {
 		var id string
 		var thisReqData []byte
+
+		// Fixes #338?
 		if err := rows.Scan(&id, &thisReqData); err != nil {
-			errs = append(errs, err)
+			return nil, []error{err}
 		}
 
 		reqData[id] = thisReqData
 	}
+
+	// Fixes #338?
 	if rows.Err() != nil {
-		errs = append(errs, rows.Err())
+		return nil, []error{rows.Err()}
 	}
 
+	var errs []error
 	for _, id := range ids {
 		if _, ok := reqData[id]; !ok {
 			errs = append(errs, fmt.Errorf(`Stored Request with ID="%s" not found.`, id))
diff --git a/stored_requests/backends/db_fetcher/fetcher_test.go b/stored_requests/backends/db_fetcher/fetcher_test.go
index a2f906bb4d9..df94b77b029 100644
--- a/stored_requests/backends/db_fetcher/fetcher_test.go
+++ b/stored_requests/backends/db_fetcher/fetcher_test.go
@@ -6,10 +6,11 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"github.com/DATA-DOG/go-sqlmock"
 	"regexp"
 	"testing"
 	"time"
+
+	"github.com/DATA-DOG/go-sqlmock"
 )
 
 func TestEmptyQuery(t *testing.T) {
@@ -165,6 +166,33 @@ func TestContextCancelled(t *testing.T) {
 	}
 }
 
+// Prevents #338?
+func TestRowErrors(t *testing.T) {
+	db, mock, err := sqlmock.New()
+	if err != nil {
+		t.Fatalf("Failed to create mock: %v", err)
+	}
+	rows := sqlmock.NewRows([]string{"id", "config"})
+	rows.AddRow("foo", []byte(`{"data":1}`))
+	rows.AddRow("bar", []byte(`{"data":2}`))
+	rows.RowError(1, errors.New("Error reading from row 1"))
+	mock.ExpectQuery(".*").WillReturnRows(rows)
+	fetcher := &dbFetcher{
+		db:         db,
+		queryMaker: successfulQueryMaker("SELECT id, config FROM my_table WHERE id IN (?)"),
+	}
+	data, errs := fetcher.FetchRequests(context.Background(), []string{"foo", "bar"})
+	if len(errs) != 1 {
+		t.Fatalf("Expected 1 error. Got %v", errs)
+	}
+	if errs[0].Error() != "Error reading from row 1" {
+		t.Errorf("Unexpected error message: %v", errs[0].Error())
+	}
+	if len(data) != 0 {
+		t.Errorf("We should not return data from the row where the error occurred.")
+	}
+}
+
 func newFetcher(rows *sqlmock.Rows, query string, args ...driver.Value) (sqlmock.Sqlmock, *dbFetcher, error) {
 	db, mock, err := sqlmock.New()
 	if err != nil {