Repository: google/exposure-notifications-server
Pull request #338 (2020-05-18 14:50:07)

internal/database: defer resetTestDB
The existing code failed to reset the DB if t.Fatal was called.

google-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/google/exposure-notifications-server/pull/338#" title="Author self-approved">jba</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mikehelmick**
You can assign the PR to them by writing `/assign @mikehelmick` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=google%2Fexposure-notifications-server).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/google/exposure-notifications-server/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mikehelmick"]} -->

Patch:
diff --git a/internal/export/exportfile.go b/internal/export/exportfile.go
index 740fdd7..70a8c55 100644
--- a/internal/export/exportfile.go
+++ b/internal/export/exportfile.go
@@ -146,7 +146,7 @@ func createSignatureInfo(si *model.SignatureInfo) *export.SignatureInfo {
 	return sigInfo
 }
 
-func marshalSignature(eb *model.ExportBatch, exportContents []byte, batchNum int32, batchSize int32, signers []exportSigners) ([]byte, error) {
+func marshalSignature(eb *model.ExportBatch, exportContents []byte, batchNum, batchSize int32, signers []exportSigners) ([]byte, error) {
 
 	var signatures []*export.TEKSignature
 	for _, s := range signers {
diff --git a/internal/export/worker.go b/internal/export/worker.go
index 89f3308..66ed000 100644
--- a/internal/export/worker.go
+++ b/internal/export/worker.go
@@ -42,18 +42,11 @@ func (s *Server) WorkerHandler(w http.ResponseWriter, r *http.Request) {
 
 	emitIndexForEmptyBatch := true
 	for {
-		select {
-		case <-ctx.Done():
-			if err := ctx.Err(); err != context.DeadlineExceeded && err != context.Canceled {
-				logger.Errorf("Context error while processing batches: %v", err)
-				return
-			}
+		if ctx.Err() != nil {
 			msg := "Timed out processing batches. Will continue on next invocation."
 			logger.Info(msg)
 			fmt.Fprintln(w, msg)
 			return
-		default:
-			// Fallthrough
 		}
 
 		// Only consider batches that closed a few minutes ago to allow the publish windows to close properly.
@@ -137,15 +130,9 @@ func (s *Server) exportBatch(ctx context.Context, eb *model.ExportBatch, emitInd
 	batchSize := len(groups)
 	var objectNames []string
 	for i, exposures := range groups {
-		select {
-		case <-ctx.Done():
-			if err := ctx.Err(); err != context.DeadlineExceeded && err != context.Canceled {
-				return err
-			}
+		if ctx.Err() != nil {
 			logger.Infof("Timed out writing export files for batch %s, the entire batch will be retried once the batch lease expires on %v", eb.BatchID, eb.LeaseExpires)
 			return nil
-		default:
-			// Fallthrough
 		}
 
 		// TODO(squee1945): Uploading in parallel (to a point) probably makes better use of network.
@@ -224,15 +211,9 @@ func (s *Server) retryingCreateIndex(ctx context.Context, eb *model.ExportBatch,
 	lockID := fmt.Sprintf("export-batch-%d", eb.BatchID)
 	sleep := 10 * time.Second
 	for {
-		select {
-		case <-ctx.Done():
-			if err := ctx.Err(); err != context.DeadlineExceeded && err != context.Canceled {
-				return err
-			}
+		if ctx.Err() != nil {
 			logger.Infof("Timed out acquiring index file lock for batch %s, the entire batch will be retried once the batch lease expires on %v", eb.BatchID, eb.LeaseExpires)
 			return nil
-		default:
-			// Fallthrough
 		}
 
 		unlock, err := s.db.Lock(ctx, lockID, time.Minute)
diff --git a/internal/federationin/federationin.go b/internal/federationin/federationin.go
index 28fc4d5..7ec73b9 100644
--- a/internal/federationin/federationin.go
+++ b/internal/federationin/federationin.go
@@ -48,9 +48,11 @@ var (
 	fetchBatchSize = database.InsertExposuresBatchSize
 )
 
-type fetchFn func(context.Context, *pb.FederationFetchRequest, ...grpc.CallOption) (*pb.FederationFetchResponse, error)
-type insertExposuresFn func(context.Context, []*model.Exposure) error
-type startFederationSyncFn func(context.Context, *model.FederationInQuery, time.Time) (int64, database.FinalizeSyncFn, error)
+type (
+	fetchFn               func(context.Context, *pb.FederationFetchRequest, ...grpc.CallOption) (*pb.FederationFetchResponse, error)
+	insertExposuresFn     func(context.Context, []*model.Exposure) error
+	startFederationSyncFn func(context.Context, *model.FederationInQuery, time.Time) (int64, database.FinalizeSyncFn, error)
+)
 
 type pullDependencies struct {
 	fetch               fetchFn
diff --git a/internal/federationout/federationout.go b/internal/federationout/federationout.go
index abcf848..eaca84c 100644
--- a/internal/federationout/federationout.go
+++ b/internal/federationout/federationout.go
@@ -251,7 +251,7 @@ func (s Server) AuthInterceptor(ctx context.Context, req interface{}, info *grpc
 
 	auth, err := s.db.GetFederationOutAuthorization(ctx, token.Issuer, token.Subject)
 	if err != nil {
-		if err == database.ErrNotFound {
+		if errors.Is(err, database.ErrNotFound) {
 			metrics.WriteInt("federation-fetch-unauthorized", true, 1)
 			logger.Infof("Authorization not found (issuer %q, subject %s)", token.Issuer, token.Subject)
 			return nil, status.Errorf(codes.Unauthenticated, "Invalid issuer/subject")
@@ -298,20 +298,16 @@ func rawToken(ctx context.Context) (string, error) {
 
 func intersect(aa, bb []string) []string {
 	if len(aa) == 0 || len(bb) == 0 {
-		return []string{}
+		return nil
 	}
-	result := []string{}
+	var result []string
 	for _, a := range aa {
-		found := false
 		for _, b := range bb {
 			if a == b {
-				found = true
+				result = append(result, a)
 				break
 			}
 		}
-		if found {
-			result = append(result, a)
-		}
 	}
 	return result
 }


Test patch:
diff --git a/internal/database/authorized_app_test.go b/internal/database/authorized_app_test.go
index d5b7f0a..c157673 100644
--- a/internal/database/authorized_app_test.go
+++ b/internal/database/authorized_app_test.go
@@ -167,6 +167,7 @@ func TestGetAuthorizedApp(t *testing.T) {
 				t.Fatal(err)
 			}
 			defer conn.Release()
+			defer resetTestDB(t)
 
 			// Insert the data
 			if _, err := conn.Exec(ctx, c.sql, c.args...); err != nil {
@@ -183,8 +184,6 @@ func TestGetAuthorizedApp(t *testing.T) {
 			if diff := cmp.Diff(config, c.exp, opts); diff != "" {
 				t.Errorf("mismatch (-want, +got):\n%s", diff)
 			}
-
-			resetTestDB(t)
 		})
 	}
 }
diff --git a/internal/export/batcher_test.go b/internal/export/batcher_test.go
index 45c961d..e86288e 100644
--- a/internal/export/batcher_test.go
+++ b/internal/export/batcher_test.go
@@ -111,8 +111,7 @@ func TestMakeBatchRanges(t *testing.T) {
 			got := makeBatchRanges(tc.period, latestEndT, nowT, time.Hour)
 
 			if len(got) != len(tc.want) {
-				t.Errorf("incorrect number of batches got %v, want %v", toSimpleBatchRange(t, got), tc.want)
-				return
+				t.Fatalf("incorrect number of batches got %v, want %v", toSimpleBatchRange(t, got), tc.want)
 			}
 
 			for i := range got {
diff --git a/internal/export/server_test.go b/internal/export/server_test.go
index 4e2a22e..58e7a9d 100644
--- a/internal/export/server_test.go
+++ b/internal/export/server_test.go
@@ -63,10 +63,8 @@ func TestNewServer(t *testing.T) {
 				}
 			} else if err != nil {
 				t.Fatalf("got unexpected error: %v", err)
-			} else {
-				if got.env != tc.env {
-					t.Fatalf("got %+v: want %v", got.env, tc.env)
-				}
+			} else if got.env != tc.env {
+				t.Fatalf("got %+v: want %v", got.env, tc.env)
 			}
 		})
 	}
diff --git a/internal/export/worker_test.go b/internal/export/worker_test.go
index c522e65..dc4c298 100644
--- a/internal/export/worker_test.go
+++ b/internal/export/worker_test.go
@@ -103,8 +103,8 @@ func TestEnsureMinExposures(t *testing.T) {
 	if len(expectedTR) != 0 {
 		t.Errorf("Didn't cover all expected transmission risks in batch of 1000: %v", expectedTR)
 	}
-	if len(eIntervals) != 4 {
-		t.Errorf("Unexpected number of intervalNum/count combinations, want: 4, got: %v", len(eIntervals))
+	if got, want := len(eIntervals), 4; got != want {
+		t.Errorf("Unexpected number of intervalNum/count combinations, got %d, want %d", got, want)
 	}
 	for k, v := range eIntervals {
 		if !v {
diff --git a/internal/federationout/federationout_test.go b/internal/federationout/federationout_test.go
index fb30fb6..8b61eb5 100644
--- a/internal/federationout/federationout_test.go
+++ b/internal/federationout/federationout_test.go
@@ -335,25 +335,25 @@ func TestIntersect(t *testing.T) {
 			name: "empty",
 			aa:   []string{},
 			bb:   []string{},
-			want: []string{},
+			want: nil,
 		},
 		{
 			name: "aa only values",
 			aa:   []string{"1", "2"},
 			bb:   []string{},
-			want: []string{},
+			want: nil,
 		},
 		{
 			name: "bb only values",
 			aa:   []string{},
 			bb:   []string{"1", "2"},
-			want: []string{},
+			want: nil,
 		},
 		{
 			name: "mutually exclusive",
 			aa:   []string{"1", "2"},
 			bb:   []string{"7", "8", "9"},
-			want: []string{},
+			want: nil,
 		},
 		{
 			name: "full overlap",