Save uploaded logs during upload (#17)

I got fed up with shoving everything into the structure the JSON api uses for
upload, so now parseRequest returns its own structure, and saves the log files
to disk in the same way as the other attachments.

This also means that we can test the saving of log files more representatively.

src/github.com/matrix-org/rageshake/submit.go

@@ -51,22 +51,32 @@ type submitServer struct {
 	githubProjectMappings map[string]string
 }
 
-type payload struct {
+// the type of payload which can be uploaded as JSON to the submit endpoint
+type jsonPayload struct {
 	Text      string            `json:"text"`
 	AppName   string            `json:"app"`
 	Version   string            `json:"version"`
 	UserAgent string            `json:"user_agent"`
-	Logs      []logEntry        `json:"logs"`
+	Logs      []jsonLogEntry    `json:"logs"`
 	Data      map[string]string `json:"data"`
 	Labels    []string          `json:"labels"`
-	Files     []string
 }
 
-type logEntry struct {
+type jsonLogEntry struct {
 	ID    string `json:"id"`
 	Lines string `json:"lines"`
 }
 
+// the payload after parsing
+type parsedPayload struct {
+	UserText string
+	AppName  string
+	Data     map[string]string
+	Labels   []string
+	Logs     []string
+	Files    []string
+}
+
 type submitResponse struct {
 	ReportURL string `json:"report_url,omitempty"`
 }
@@ -125,7 +135,7 @@ func (s *submitServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 
 // parseRequest attempts to parse a received request as a bug report. If
 // the request cannot be parsed, it responds with an error and returns nil.
-func parseRequest(w http.ResponseWriter, req *http.Request, reportDir string) *payload {
+func parseRequest(w http.ResponseWriter, req *http.Request, reportDir string) *parsedPayload {
 	length, err := strconv.Atoi(req.Header.Get("Content-Length"))
 	if err != nil {
 		log.Println("Couldn't parse content-length", err)
@@ -144,7 +154,7 @@ func parseRequest(w http.ResponseWriter, req *http.Request, reportDir string) *p
 		if d == "multipart/form-data" {
 			p, err1 := parseMultipartRequest(w, req, reportDir)
 			if err1 != nil {
-				log.Println("Error parsing multipart data", err1)
+				log.Println("Error parsing multipart data:", err1)
 				http.Error(w, "Bad multipart data", 400)
 				return nil
 			}
@@ -152,7 +162,7 @@ func parseRequest(w http.ResponseWriter, req *http.Request, reportDir string) *p
 		}
 	}
 
-	p, err := parseJSONRequest(w, req)
+	p, err := parseJSONRequest(w, req, reportDir)
 	if err != nil {
 		log.Println("Error parsing JSON body", err)
 		http.Error(w, fmt.Sprintf("Could not decode payload: %s", err.Error()), 400)
@@ -161,22 +171,35 @@ func parseRequest(w http.ResponseWriter, req *http.Request, reportDir string) *p
 	return p
 }
 
-func parseJSONRequest(w http.ResponseWriter, req *http.Request) (*payload, error) {
+func parseJSONRequest(w http.ResponseWriter, req *http.Request, reportDir string) (*parsedPayload, error) {
-	var p payload
+	var p jsonPayload
 	if err := json.NewDecoder(req.Body).Decode(&p); err != nil {
 		return nil, err
 	}
 
-	p.Text = strings.TrimSpace(p.Text)
+	parsed := parsedPayload{
+		UserText: strings.TrimSpace(p.Text),
+		Data:     make(map[string]string),
+		Labels:   p.Labels,
+	}
 
-	if p.Data == nil {
+	if p.Data != nil {
-		p.Data = make(map[string]string)
+		parsed.Data = p.Data
+	}
+
+	for i, log := range p.Logs {
+		buf := bytes.NewBufferString(log.Lines)
+		leafName, err := saveLogPart(i, buf, reportDir)
+		if err != nil {
+			return nil, err
+		}
+		parsed.Logs = append(parsed.Logs, leafName)
 	}
 
 	// backwards-compatibility hack: current versions of riot-android
 	// don't set 'app', so we don't correctly file github issues.
 	if p.AppName == "" && p.UserAgent == "Android" {
-		p.AppName = "riot-android"
+		parsed.AppName = "riot-android"
 
 		// they also shove lots of stuff into 'Version' which we don't really
 		// want in the github report
@@ -191,22 +214,28 @@ func parseJSONRequest(w http.ResponseWriter, req *http.Request) (*payload, error
 			if len(parts) > 1 {
 				val = strings.TrimSpace(parts[1])
 			}
-			p.Data[key] = val
+			parsed.Data[key] = val
+		}
+	} else {
+		if p.UserAgent != "" {
+			parsed.Data["User-Agent"] = p.UserAgent
+		}
+		if p.Version != "" {
+			parsed.Data["Version"] = p.Version
 		}
-		p.Version = ""
 	}
 
-	return &p, nil
+	return &parsed, nil
 }
 
-func parseMultipartRequest(w http.ResponseWriter, req *http.Request, reportDir string) (*payload, error) {
+func parseMultipartRequest(w http.ResponseWriter, req *http.Request, reportDir string) (*parsedPayload, error) {
 	rdr, err := req.MultipartReader()
 	if err != nil {
 		return nil, err
 	}
 
-	p := payload{
+	p := parsedPayload{
-		Logs: make([]logEntry, 0),
+		Logs: make([]string, 0),
 		Data: make(map[string]string),
 	}
 
@@ -225,13 +254,17 @@ func parseMultipartRequest(w http.ResponseWriter, req *http.Request, reportDir s
 	return &p, nil
 }
 
-func parseFormPart(part *multipart.Part, p *payload, reportDir string) error {
+func parseFormPart(part *multipart.Part, p *parsedPayload, reportDir string) error {
 	defer part.Close()
 	field := part.FormName()
 
 	var partReader io.Reader
 	if field == "compressed-log" {
-		// decompress logs as we read them
+		// decompress logs as we read them.
+		//
+		// we could save the log directly rather than unzipping and re-zipping,
+		// but doing so conveys the benefit of checking the validity of the
+		// gzip at upload time.
 		zrdr, err := gzip.NewReader(part)
 		if err != nil {
 			return err
@@ -252,36 +285,35 @@ func parseFormPart(part *multipart.Part, p *payload, reportDir string) error {
 		return nil
 	}
 
+	if field == "log" || field == "compressed-log" {
+		leafName, err := saveLogPart(len(p.Logs), partReader, reportDir)
+		if err != nil {
+			return err
+		}
+		p.Logs = append(p.Logs, leafName)
+		return nil
+	}
+
 	b, err := ioutil.ReadAll(partReader)
 	if err != nil {
 		return err
 	}
 	data := string(b)
-
-	if field == "log" || field == "compressed-log" {
-		// todo: we could save the log directly rather than pointlessly
-		// unzipping and re-zipping.
-		p.Logs = append(p.Logs, logEntry{
-			ID:    part.FileName(),
-			Lines: data,
-		})
-	} else {
 	formPartToPayload(field, data, p)
-	}
 	return nil
 }
 
 // formPartToPayload updates the relevant part of *p from a name/value pair
 // read from the form data.
-func formPartToPayload(field, data string, p *payload) {
+func formPartToPayload(field, data string, p *parsedPayload) {
 	if field == "text" {
-		p.Text = data
+		p.UserText = data
 	} else if field == "app" {
 		p.AppName = data
 	} else if field == "version" {
-		p.Version = data
+		p.Data["Version"] = data
 	} else if field == "user_agent" {
-		p.UserAgent = data
+		p.Data["User-Agent"] = data
 	} else if field == "label" {
 		p.Labels = append(p.Labels, data)
 	} else {
@@ -305,7 +337,6 @@ var filenameRegexp = regexp.MustCompile(`^[a-zA-Z0-9_-]+\.(jpg|png|txt)$`)
 //
 // Returns the leafname of the saved file.
 func saveFormPart(leafName string, reader io.Reader, reportDir string) (string, error) {
-
 	if !filenameRegexp.MatchString(leafName) {
 		return "", fmt.Errorf("Invalid upload filename")
 	}
@@ -328,14 +359,38 @@ func saveFormPart(leafName string, reader io.Reader, reportDir string) (string,
 	return leafName, nil
 }
 
-func (s *submitServer) saveReport(ctx context.Context, p payload, reportDir, listingURL string) (*submitResponse, error) {
+// saveLogPart saves a log upload to the report directory.
+//
+// Returns the leafname of the saved file.
+func saveLogPart(logNum int, reader io.Reader, reportDir string) (string, error) {
+	leafName := fmt.Sprintf("logs-%04d.log.gz", logNum)
+	fullname := filepath.Join(reportDir, leafName)
+
+	f, err := os.Create(fullname)
+	if err != nil {
+		return "", err
+	}
+	defer f.Close()
+
+	gz := gzip.NewWriter(f)
+	defer gz.Close()
+
+	_, err = io.Copy(gz, reader)
+	if err != nil {
+		return "", err
+	}
+
+	return leafName, nil
+}
+
+func (s *submitServer) saveReport(ctx context.Context, p parsedPayload, reportDir, listingURL string) (*submitResponse, error) {
 	resp := submitResponse{}
 
 	var summaryBuf bytes.Buffer
 	fmt.Fprintf(
 		&summaryBuf,
-		"%s\n\nNumber of logs: %d\nApplication: %s\nVersion: %s\nUser-Agent: %s\n",
+		"%s\n\nNumber of logs: %d\nApplication: %s\n",
-		p.Text, len(p.Logs), p.AppName, p.Version, p.UserAgent,
+		p.UserText, len(p.Logs), p.AppName,
 	)
 	fmt.Fprintf(&summaryBuf, "Labels: %s\n", strings.Join(p.Labels, ", "))
 	for k, v := range p.Data {
@@ -345,12 +400,6 @@ func (s *submitServer) saveReport(ctx context.Context, p payload, reportDir, lis
 		return nil, err
 	}
 
-	for i, log := range p.Logs {
-		if err := gzipAndSave([]byte(log.Lines), reportDir, fmt.Sprintf("logs-%04d.log.gz", i)); err != nil {
-			return nil, err // TODO: Rollback?
-		}
-	}
-
 	if s.ghClient == nil {
 		// we're done here
 		log.Println("GH issue submission disabled")
@@ -383,27 +432,24 @@ func (s *submitServer) saveReport(ctx context.Context, p payload, reportDir, lis
 	return &resp, nil
 }
 
-func buildGithubIssueRequest(p payload, listingURL string) github.IssueRequest {
+func buildGithubIssueRequest(p parsedPayload, listingURL string) github.IssueRequest {
 	var title string
-	if p.Text == "" {
+	if p.UserText == "" {
 		title = "Untitled report"
 	} else {
 		// set the title to the first line of the user's report
-		if i := strings.IndexAny(p.Text, "\r\n"); i < 0 {
+		if i := strings.IndexAny(p.UserText, "\r\n"); i < 0 {
-			title = p.Text
+			title = p.UserText
 		} else {
-			title = p.Text[0:i]
+			title = p.UserText[0:i]
 		}
 	}
 
 	var bodyBuf bytes.Buffer
-	fmt.Fprintf(&bodyBuf, "User message:\n```\n%s\n```\n", p.Text)
+	fmt.Fprintf(&bodyBuf, "User message:\n```\n%s\n```\n", p.UserText)
 	for k, v := range p.Data {
 		fmt.Fprintf(&bodyBuf, "%s: `%s`\n", k, v)
 	}
-	if p.Version != "" {
-		fmt.Fprintf(&bodyBuf, "Version: `%s`\n", p.Version)
-	}
 	fmt.Fprintf(&bodyBuf, "[Logs](%s)", listingURL)
 
 	for _, file := range p.Files {

src/github.com/matrix-org/rageshake/submit_test.go

@@ -17,6 +17,8 @@ limitations under the License.
 package main
 
 import (
+	"compress/gzip"
+	"io"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
@@ -32,7 +34,7 @@ import (
 //
 // if tempDir is empty, a new temp dir is created, and deleted when the test
 // completes.
-func testParsePayload(t *testing.T, body, contentType string, tempDir string) (*payload, *http.Response) {
+func testParsePayload(t *testing.T, body, contentType string, tempDir string) (*parsedPayload, *http.Response) {
 	req, err := http.NewRequest("POST", "/api/submit", strings.NewReader(body))
 	if err != nil {
 		t.Fatal(err)
@@ -73,14 +75,14 @@ func TestUnpickAndroidMangling(t *testing.T) {
 	if p == nil {
 		t.Fatal("parseRequest returned nil")
 	}
-	if p.Text != "test ylc 001" {
+	if p.UserText != "test ylc 001" {
-		t.Errorf("user text: got %s, want %s", p.Text, "test ylc 001")
+		t.Errorf("user text: got %s, want %s", p.UserText, "test ylc 001")
 	}
 	if p.AppName != "riot-android" {
 		t.Errorf("appname: got %s, want %s", p.AppName, "riot-android")
 	}
-	if p.Version != "" {
+	if p.Data["Version"] != "" {
-		t.Errorf("version: got %s, want ''", p.Version)
+		t.Errorf("version: got %s, want ''", p.Data["Version"])
 	}
 	if p.Data["User"] != "@ylc8001:matrix.org" {
 		t.Errorf("data.user: got %s, want %s", p.Data["User"], "@ylc8001:matrix.org")
@@ -108,17 +110,13 @@ func TestMultipartUpload(t *testing.T) {
 
 	checkParsedMultipartUpload(t, p)
 
+	// check logs uploaded correctly
+	checkUploadedFile(t, reportDir, "logs-0000.log.gz", true, "log\nlog\nlog")
+	checkUploadedFile(t, reportDir, "logs-0001.log.gz", true, "log")
+	checkUploadedFile(t, reportDir, "logs-0002.log.gz", true, "test\n")
+
 	// check file uploaded correctly
-	dat, err := ioutil.ReadFile(filepath.Join(reportDir, "passwd.txt"))
+	checkUploadedFile(t, reportDir, "passwd.txt", false, "bibblybobbly")
-	if err != nil {
-		t.Error("unable to read uploaded file", err)
-	} else {
-		datstr := string(dat)
-		wanted := "bibblybobbly"
-		if datstr != wanted {
-			t.Errorf("File contents: got %s, want %s", datstr, wanted)
-		}
-	}
 }
 
 func multipartBody() (body string) {
@@ -186,16 +184,16 @@ bibblybobbly
 	return
 }
 
-func checkParsedMultipartUpload(t *testing.T, p *payload) {
+func checkParsedMultipartUpload(t *testing.T, p *parsedPayload) {
 	wanted := "test words."
-	if p.Text != wanted {
+	if p.UserText != wanted {
-		t.Errorf("User text: got %s, want %s", p.Text, wanted)
+		t.Errorf("User text: got %s, want %s", p.UserText, wanted)
 	}
 	if len(p.Logs) != 3 {
 		t.Errorf("Log length: got %d, want 3", len(p.Logs))
 	}
-	if len(p.Data) != 1 {
+	if len(p.Data) != 3 {
-		t.Errorf("Data length: got %d, want 1", len(p.Data))
+		t.Errorf("Data length: got %d, want 3", len(p.Data))
 	}
 	wantedLabels := []string{"label1", "label2"}
 	if !stringSlicesEqual(p.Labels, wantedLabels) {
@@ -205,25 +203,17 @@ func checkParsedMultipartUpload(t *testing.T, p *payload) {
 	if p.Data["test-field"] != wanted {
 		t.Errorf("test-field: got %s, want %s", p.Data["test-field"], wanted)
 	}
-	wanted = "log\nlog\nlog"
+	wanted = "logs-0000.log.gz"
-	if p.Logs[0].Lines != wanted {
+	if p.Logs[0] != wanted {
-		t.Errorf("Log 0: got %s, want %s", p.Logs[0].Lines, wanted)
+		t.Errorf("Log 0: got %s, want %s", p.Logs[0], wanted)
 	}
-	wanted = "instance-0.215954445471346461492087122412"
+	wanted = "logs-0001.log.gz"
-	if p.Logs[0].ID != wanted {
+	if p.Logs[1] != wanted {
-		t.Errorf("Log 0 ID: got %s, want %s", p.Logs[0].ID, wanted)
+		t.Errorf("Log 1: got %s, want %s", p.Logs[1], wanted)
 	}
-	wanted = "log"
+	wanted = "logs-0002.log.gz"
-	if p.Logs[1].Lines != wanted {
+	if p.Logs[2] != wanted {
-		t.Errorf("Log 1: got %s, want %s", p.Logs[1].Lines, wanted)
+		t.Errorf("Log 2: got %s, want %s", p.Logs[2], wanted)
-	}
-	wanted = "instance-0.067644760733513781492004890379"
-	if p.Logs[1].ID != wanted {
-		t.Errorf("Log 1 ID: got %s, want %s", p.Logs[1].ID, wanted)
-	}
-	wanted = "test\n"
-	if p.Logs[2].Lines != wanted {
-		t.Errorf("Log 2: got %s, want %s", p.Logs[2].Lines, wanted)
 	}
 }
 
@@ -274,6 +264,37 @@ file
 	}
 }
 
+func checkUploadedFile(t *testing.T, reportDir, leafName string, gzipped bool, wanted string) {
+	fi, err := os.Open(filepath.Join(reportDir, leafName))
+	if err != nil {
+		t.Errorf("unable to open uploaded file %s: %v", leafName, err)
+		return
+	}
+	defer fi.Close()
+	var rdr io.Reader
+	if !gzipped {
+		rdr = fi
+	} else {
+		gz, err2 := gzip.NewReader(fi)
+		if err2 != nil {
+			t.Errorf("unable to ungzip uploaded file %s: %v", leafName, err2)
+			return
+		}
+		defer gz.Close()
+		rdr = gz
+	}
+	dat, err := ioutil.ReadAll(rdr)
+	if err != nil {
+		t.Errorf("unable to read uploaded file %s: %v", leafName, err)
+		return
+	}
+
+	datstr := string(dat)
+	if datstr != wanted {
+		t.Errorf("File %s: got %s, want %s", leafName, datstr, wanted)
+	}
+}
+
 func mkTempDir(t *testing.T) string {
 	td, err := ioutil.TempDir("", "rageshake_test")
 	if err != nil {