From e2b4e09ae172b1b92669461ffe277649c89e13ad Mon Sep 17 00:00:00 2001
From: Dmitriy Vyukov <dvyukov@google.com>
Date: Fri, 17 Oct 2014 14:51:03 +0400
Subject: [PATCH] dashboard: ensure that we ever store valid commits

LGTM=bradfitz
R=bradfitz
CC=golang-codereviews, rsc
https://golang.org/cl/160960043
---
 dashboard/app/build/build.go   | 23 +++++++++++++++--------
 dashboard/app/build/handler.go |  4 ++--
 dashboard/app/build/notify.go  |  3 +--
 dashboard/app/build/update.go  |  4 ++--
 4 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/dashboard/app/build/build.go b/dashboard/app/build/build.go
index 8a135cf9..d2488a33 100644
--- a/dashboard/app/build/build.go
+++ b/dashboard/app/build/build.go
@@ -127,6 +127,19 @@ func (c *Commit) Valid() error {
 	return nil
 }
 
+func putCommit(c appengine.Context, com *Commit) error {
+	if err := com.Valid(); err != nil {
+		return fmt.Errorf("putting Commit: %v", err)
+	}
+	if com.Num == 0 {
+		return fmt.Errorf("putting Commit: com.Num == 0")
+	}
+	if _, err := datastore.Put(c, com.Key(c), com); err != nil {
+		return fmt.Errorf("putting Commit: %v", err)
+	}
+	return nil
+}
+
 // each result line is approx 105 bytes. This constant is a tradeoff between
 // build history and the AppEngine datastore limit of 1mb.
 const maxResults = 1000
@@ -150,10 +163,7 @@ func (com *Commit) AddResult(c appengine.Context, r *Result) error {
 		// otherwise, add the new result data for this builder.
 		com.ResultData = trim(append(com.ResultData, r.Data()), maxResults)
 	}
-	if _, err := datastore.Put(c, com.Key(c), com); err != nil {
-		return fmt.Errorf("putting Commit: %v", err)
-	}
-	return nil
+	return putCommit(c, com)
 }
 
 // AddPerfResult remembers that the builder has run the benchmark on the commit.
@@ -172,10 +182,7 @@ func (com *Commit) AddPerfResult(c appengine.Context, builder, benchmark string)
 		}
 	}
 	com.PerfResults = append(com.PerfResults, s)
-	if _, err := datastore.Put(c, com.Key(c), com); err != nil {
-		return fmt.Errorf("putting Commit: %v", err)
-	}
-	return nil
+	return putCommit(c, com)
 }
 
 func trim(s []string, n int) []string {
diff --git a/dashboard/app/build/handler.go b/dashboard/app/build/handler.go
index 7834afa4..2e8b84e3 100644
--- a/dashboard/app/build/handler.go
+++ b/dashboard/app/build/handler.go
@@ -192,8 +192,8 @@ func addCommit(c appengine.Context, com *Commit) error {
 		}
 	}
 	// put the Commit
-	if _, err = datastore.Put(c, com.Key(c), com); err != nil {
-		return fmt.Errorf("putting Commit: %v", err)
+	if err = putCommit(c, com); err != nil {
+		return err
 	}
 	if com.NeedsBenchmarking {
 		// add to CommitRun
diff --git a/dashboard/app/build/notify.go b/dashboard/app/build/notify.go
index bbd710e7..293eb67b 100644
--- a/dashboard/app/build/notify.go
+++ b/dashboard/app/build/notify.go
@@ -230,8 +230,7 @@ func commonNotify(c appengine.Context, com *Commit, builder, logHash string) err
 	c.Infof("%s is broken commit; notifying", com.Hash)
 	notifyLater.Call(c, com, builder, logHash) // add task to queue
 	com.FailNotificationSent = true
-	_, err := datastore.Put(c, com.Key(c), com)
-	return err
+	return putCommit(c, com)
 }
 
 // sendFailMail sends a mail notification that the build failed on the
diff --git a/dashboard/app/build/update.go b/dashboard/app/build/update.go
index 75390434..5810c6f6 100644
--- a/dashboard/app/build/update.go
+++ b/dashboard/app/build/update.go
@@ -58,8 +58,8 @@ func updateBenchmark(w http.ResponseWriter, r *http.Request) {
 				continue
 			}
 			com.NeedsBenchmarking = true
-			if _, err := datastore.Put(c, com.Key(c), com); err != nil {
-				return fmt.Errorf("putting Commit: %v", err)
+			if err := putCommit(c, com); err != nil {
+				return err
 			}
 			ncommit++