test-results: Get handler cleanup

go/src/infra/appengine/test-results/frontend/get.go

 package frontend
 
 import (
         "bytes"
         "encoding/json"
         "fmt"
         "infra/appengine/test-results/model"
         "io"
         "net/http"
-        "regexp"
         "time"
 
         "github.com/luci/gae/service/datastore"
         "github.com/luci/luci-go/common/logging"
         "github.com/luci/luci-go/server/router"
         "github.com/luci/luci-go/server/templates"
         "golang.org/x/net/context"
 )
 
 const (
         // paramsTimeFormat is the time format string in incoming GET
         // /testfile requests.
         paramsTimeFormat = "2006-01-02T15:04:05Z" // RFC3339, but enforce Z for timezone.
 
         // httpTimeFormat is the time format used in HTTP headers.
         // See https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
         // (Section 14.18 Date).
         httpTimeFormat = time.RFC1123
 
         // httpNoTZTimeFormat is httpTimeFormat with the timezone removed.
         httpNoTZTimeFormat = "Mon, 02 Jan 2006 15:04:05"
 )
 
-// callbackNameRx matches start of strings that look like
-// JavaScript function names. Not a comprehensive solution.
-var callbackNameRx = regexp.MustCompile(`^[A-Za-z0-9_]+$`)

[estaab, 2016/08/16 15:33:23]

What's the consequence of removing this? Any security concerns?

-
-// GetHandler is the HTTP handler for GET /testfile requests.
-func GetHandler(ctx *router.Context) {
+// getHandler is the HTTP handler for GET /testfile requests.
+func getHandler(ctx *router.Context) {
         c, w, r := ctx.Context, ctx.Writer, ctx.Request
         if err := r.ParseForm(); err != nil {
                 http.Error(w, err.Error(), http.StatusInternalServerError)
                 logging.Errorf(c, "failed to parse form: %v", err)
                 return
         }
 
         params, err := NewURLParams(r.Form)
         if err != nil {
                 e := fmt.Sprintf("failed to parse URL parameters: %+v: %v", params, err)
@@ skipping 18 matching lines @@
 
         key, err := datastore.NewKeyEncoded(params.Key)
         if err != nil {
                 http.Error(w, err.Error(), http.StatusBadRequest)
                 logging.Errorf(c, "failed to encode key: %v: %v", key, err)
                 return
         }
 
         tf := model.TestFile{ID: key.IntID()}
 
-        if err := datastore.Get(c).Get(&tf); err == datastore.ErrNoSuchEntity {
-                http.Error(w, err.Error(), http.StatusNotFound)
-                logging.Errorf(c, "TestFile with ID %v not found: %v", key.IntID(), err)
-                return
-        } else if err != nil {
+        if err := datastore.Get(c).Get(&tf); err != nil {
+                if err == datastore.ErrNoSuchEntity {
+                        http.Error(w, err.Error(), http.StatusNotFound)
+                        logging.Errorf(c, "TestFile with ID %v not found: %v", key.IntID(), err)
+                        return
+                }
                 http.Error(w, err.Error(), http.StatusInternalServerError)
                 logging.Errorf(c, "failed to get TestFile with ID %v: %v", key.IntID(), err)
                 return
         }
 
         modTime, err := time.Parse(r.Header.Get("If-Modified-Since"), httpTimeFormat)
         if err == nil && !tf.LastMod.After(modTime) {
                 w.WriteHeader(http.StatusNotModified)
                 return
         }
@@ skipping 15 matching lines @@
                 logging.Errorf(c, "GetAll failed for query: %+v: %v", q, err)
                 return
         }
         if len(testFiles) == 0 {
                 e := fmt.Sprintf("no TestFile found for query: %+v", q)
                 http.Error(w, e, http.StatusNotFound)
                 logging.Errorf(c, e)
                 return
         }
 
-        args := templates.Args{
-                "Master":      params.Master,
-                "Builder":     params.Builder,
-                "TestType":    params.TestType,
-                "BuildNumber": params.BuildNumber,
-                "Name":        params.Name,
-                "Files":       testFiles,
-        }
-
         if params.Callback != "" {
                 b, err := keysJSON(c, testFiles)
                 if err != nil {
                         http.Error(w, err.Error(), http.StatusInternalServerError)
                         logging.Errorf(c, "failed to create callback JSON: %v: %v", testFiles, err)
                         return
                 }
                 respondJSON(c, w, bytes.NewReader(b), testFiles[0].LastMod, params.Callback)
                 return
         }
 
-        templates.MustRender(c, w, "pages/showfilelist.html", args)
+        templates.MustRender(c, w, "pages/showfilelist.html", templates.Args{
+                "Master":      params.Master,
+                "Builder":     params.Builder,
+                "TestType":    params.TestType,
+                "BuildNumber": params.BuildNumber,
+                "Name":        params.Name,
+                "Files":       testFiles,
+        })
 }
 
 func keysJSON(c context.Context, tfiles []*model.TestFile) ([]byte, error) {
         type K struct {
                 Key string `json:"key"`
         }
         keys := make([]K, len(tfiles))
         for i, tf := range tfiles {
                 keys[i] = K{datastore.Get(c).KeyForObj(tf).Encode()}
         }
@@ skipping 65 matching lines @@
                         http.Error(w, err.Error(), http.StatusInternalServerError)
                         logging.Errorf(c, "failed to clean test results JSON: %v", err)
                         return
                 }
                 aggr := model.AggregateResult{Builder: params.Builder}
                 if err := json.NewDecoder(data).Decode(&aggr); err != nil {
                         http.Error(w, err.Error(), http.StatusInternalServerError)
                         logging.Errorf(c, "failed to unmarshal test results JSON: %+v: %v", data, err)
                         return
                 }
-                aggr.Tests.ToTestList()
+
+                tl := aggr.TestList()
                 buf := &bytes.Buffer{}
-                if err := json.NewEncoder(buf).Encode(aggr.Tests); err != nil {
+                if err := json.NewEncoder(buf).Encode(&tl); err != nil {
                         http.Error(w, err.Error(), http.StatusInternalServerError)
                         logging.Errorf(c, "failed to marshal test list JSON: %+v, %v", aggr.Tests, err)
                         return
                 }
                 finalData = buf
         }
 
         respondJSON(c, w, finalData, tf.LastMod, params.Callback)
 }
 
@@ skipping 14 matching lines @@
                 return nil, err
         }
         if len(tfs) == 0 {
                 e := ErrNoMatches(fmt.Sprintf("no TestFile found for query: %+v", q))
                 logging.Errorf(c, e.Error())
                 return nil, e
         }
         return tfs[0], nil
 }
 
-// respondJSON writes the supplied JSON data to w. If the supplied callback string matches
-// callbackNameRx, data is wrapped in a JSONP-style function with the supplied callback
-// string as the function name.
+// respondJSON writes the supplied JSON data to w. If the supplied
+// callback string is not empty, data is wrapped in a JSONP-style
+// function with the supplied callback  string as the function name.
 func respondJSON(c context.Context, w http.ResponseWriter, data io.Reader, lastMod time.Time, callback string) {
-        if callbackNameRx.MatchString(callback) {
+        if callback != "" {
                 data = wrapCallback(data, callback)
         }
         w.Header().Set("Last-Modified", lastMod.Format(httpNoTZTimeFormat)+" GMT")
         w.Header().Set("Content-Type", "application/json")
         n, err := io.Copy(w, data)
         if err != nil {
                 logging.Errorf(c, "error writing JSON response: %#v, %v, wrote %d bytes", data, err, n)
         }
 }
 
 // wrapCallback returns an io.Reader that wraps the data in r in a
 // JavaScript-style function call with the supplied name as the function name.
 func wrapCallback(r io.Reader, name string) io.Reader {
         start := bytes.NewReader([]byte(name + "("))
         end := bytes.NewReader([]byte(");"))
         return io.MultiReader(start, r, end)
 }