Repository: google/exposure-notifications-server
Pull request #296 (2020-05-15 17:08:34)

Emit log if conflict on ExportFile insertion (skipping overwrite).
@sethvargo I guess I was too slow :)


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/296#" title="Author self-approved">squee1945</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **crwilcox**
You can assign the PR to them by writing `/assign @crwilcox` 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":["crwilcox"]} -->
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/296#pullrequestreview-412829397" title="LGTM">gurayAlsac</a>*, *<a href="https://github.com/google/exposure-notifications-server/pull/296#" title="Author self-approved">squee1945</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **crwilcox**
You can assign the PR to them by writing `/assign @crwilcox` 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":["crwilcox"]} -->

Patch:
diff --git a/docs/server_functional_requirements.md b/docs/server_functional_requirements.md
index 264f1eb..87fb163 100644
--- a/docs/server_functional_requirements.md
+++ b/docs/server_functional_requirements.md
@@ -86,7 +86,7 @@ Minimum required fields, followed by a JSON example:
   * Type: Integer
   * **The values and meanings of this enum are not finalized at this time.** //TODO(llatif): check status
   * Constraints:
-    * Valid values range from 1-8
+    * Valid values range from 0-8
   * Description: //TODO(llatif): Add description
 * `deviceVerificationPayload` (**REQUIRED**)
   * Type: String
diff --git a/internal/database/database.go b/internal/database/database.go
index 3ba3f73..4bdde53 100644
--- a/internal/database/database.go
+++ b/internal/database/database.go
@@ -26,6 +26,9 @@ import (
 var (
 	// ErrNotFound indicates that the requested record was not found in the database.
 	ErrNotFound = errors.New("record not found")
+
+	// ErrKeyConflict indicates that there was a key conflict inserting a row.
+	ErrKeyConflict = errors.New("key conflict")
 )
 
 func toNullString(s string) sql.NullString {
diff --git a/internal/database/export.go b/internal/database/export.go
index a6d5e26..cd5cc03 100644
--- a/internal/database/export.go
+++ b/internal/database/export.go
@@ -39,6 +39,9 @@ func (db *DB) AddExportConfig(ctx context.Context, ec *model.ExportConfig) error
 	if ec.Period > oneDay {
 		return errors.New("maximum period is 24h")
 	}
+	if ec.Period == 0 {
+		return errors.New("period must be non-zero")
+	}
 	if int64(oneDay.Seconds())%int64(ec.Period.Seconds()) != 0 {
 		return errors.New("period must divide equally into 24 hours (e.g., 2h, 4h, 12h, 15m, 30m)")
 	}
@@ -333,7 +336,11 @@ func (db *DB) FinalizeBatch(ctx context.Context, eb *model.ExportBatch, files []
 				Status:     model.ExportBatchComplete,
 			}
 			if err := addExportFile(ctx, tx, &ef); err != nil {
-				return fmt.Errorf("adding export file entry: %w", err)
+				if err == ErrKeyConflict {
+					logging.FromContext(ctx).Infof("ExportFile %q already exists in database, skipping without overwriting. This can occur when reprocessing a failed batch.", file)
+				} else {
+					return fmt.Errorf("adding export file entry: %w", err)
+				}
 			}
 		}
 
@@ -512,8 +519,10 @@ func (db *DB) DeleteFilesBefore(ctx context.Context, before time.Time, blobstore
 	return count, nil
 }
 
+// addExportFile adds a row to ExportFile. If the row already exists (based on the primary key),
+// ErrKeyConflict is returned.
 func addExportFile(ctx context.Context, tx pgx.Tx, ef *model.ExportFile) error {
-	_, err := tx.Exec(ctx, `
+	tag, err := tx.Exec(ctx, `
 		INSERT INTO
 			ExportFile
 			(bucket_name, filename, batch_id, region, batch_num, batch_size, status)
@@ -524,6 +533,9 @@ func addExportFile(ctx context.Context, tx pgx.Tx, ef *model.ExportFile) error {
 	if err != nil {
 		return fmt.Errorf("inserting to ExportFile: %w", err)
 	}
+	if tag.RowsAffected() == 0 {
+		return ErrKeyConflict
+	}
 	return nil
 }
 
diff --git a/internal/model/exposure.go b/internal/model/exposure.go
index 5cd205c..20be90d 100644
--- a/internal/model/exposure.go
+++ b/internal/model/exposure.go
@@ -55,7 +55,7 @@ const (
 // AppPackageName: The identifier for the mobile application.
 //  - Android: The App Package AppPackageName
 //  - iOS: The BundleID
-// TransmissionRisk: An integer from 1-8 (inclusive) that represnets
+// TransmissionRisk: An integer from 0-8 (inclusive) that represnets
 //  the transmission risk for this publish.
 // Verification: The attestation payload for this request. (iOS or Android specific)
 //   Base64 encoded.


Test patch:
diff --git a/internal/database/export_test.go b/internal/database/export_test.go
index 69b5f13..51cd2f9 100644
--- a/internal/database/export_test.go
+++ b/internal/database/export_test.go
@@ -16,6 +16,7 @@ package database
 
 import (
 	"context"
+	"errors"
 	"sort"
 	"testing"
 	"time"
@@ -428,3 +429,78 @@ func TestKeysInBatch(t *testing.T) {
 		t.Fatalf("Incorrect exposure key in batch, got %q, want %q", got[0].ExposureKey, want)
 	}
 }
+
+// TestAddExportFileSkipsDuplicates ensures that ExportFile records are not overwritten.
+func TestAddExportFileSkipsDuplicates(t *testing.T) {
+	if testDB == nil {
+		t.Skip("no test DB")
+	}
+	defer resetTestDB(t)
+	ctx := context.Background()
+
+	// Add foreign key records.
+	ec := &model.ExportConfig{Period: time.Hour}
+	if err := testDB.AddExportConfig(ctx, ec); err != nil {
+		t.Fatal(err)
+	}
+	eb := &model.ExportBatch{ConfigID: ec.ConfigID, Status: model.ExportBatchOpen}
+	if err := testDB.AddExportBatches(ctx, []*model.ExportBatch{eb}); err != nil {
+		t.Fatal(err)
+	}
+	// Lease the batch to get the ID.
+	eb, err := testDB.LeaseBatch(ctx, time.Hour, time.Now())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	wantBucketName := "bucket-1"
+	ef := &model.ExportFile{
+		Filename:   "file",
+		BucketName: wantBucketName,
+		BatchID:    eb.BatchID,
+	}
+
+	// Add a record.
+	err = testDB.inTx(ctx, pgx.Serializable, func(tx pgx.Tx) error {
+		if err := addExportFile(ctx, tx, ef); err != nil {
+			return err
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Check that the row is present.
+	got, err := testDB.LookupExportFile(ctx, ef.Filename)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got.BucketName != wantBucketName {
+		t.Fatalf("bucket name mismatch got %q, want %q", got.BucketName, wantBucketName)
+	}
+
+	// Add a second record with same filename, must return ErrKeyConflict, and not overwrite.
+	ef.BucketName = "bucket-2"
+	err = testDB.inTx(ctx, pgx.Serializable, func(tx pgx.Tx) error {
+		if err := addExportFile(ctx, tx, ef); err != nil {
+			if err == ErrKeyConflict {
+				return nil // Expected result.
+			}
+			return err
+		}
+		return errors.New("missing expected ErrKeyConflict")
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Row must not be updated.
+	got, err = testDB.LookupExportFile(ctx, ef.Filename)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got.BucketName != wantBucketName {
+		t.Fatalf("bucket name mismatch got %q, want %q", got.BucketName, wantBucketName)
+	}
+}