diff --git a/src/github.com/matrix-org/rageshake/submit.go b/src/github.com/matrix-org/rageshake/submit.go index 820e2ab..461c150 100644 --- a/src/github.com/matrix-org/rageshake/submit.go +++ b/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) { - var p payload +func parseJSONRequest(w http.ResponseWriter, req *http.Request, reportDir string) (*parsedPayload, error) { + 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 { - p.Data = make(map[string]string) + if p.Data != nil { + 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{ - Logs: make([]logEntry, 0), + p := parsedPayload{ + 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) - } + 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", - p.Text, len(p.Logs), p.AppName, p.Version, p.UserAgent, + "%s\n\nNumber of logs: %d\nApplication: %s\n", + 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 { - title = p.Text + if i := strings.IndexAny(p.UserText, "\r\n"); i < 0 { + 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 { diff --git a/src/github.com/matrix-org/rageshake/submit_test.go b/src/github.com/matrix-org/rageshake/submit_test.go index 669311a..31ce798 100644 --- a/src/github.com/matrix-org/rageshake/submit_test.go +++ b/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" { - t.Errorf("user text: got %s, want %s", p.Text, "test ylc 001") + if p.UserText != "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 != "" { - t.Errorf("version: got %s, want ''", p.Version) + if p.Data["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")) - 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) - } - } + checkUploadedFile(t, reportDir, "passwd.txt", false, "bibblybobbly") } 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 { - t.Errorf("User text: got %s, want %s", p.Text, wanted) + if p.UserText != 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 { - t.Errorf("Data length: got %d, want 1", len(p.Data)) + if len(p.Data) != 3 { + 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" - if p.Logs[0].Lines != wanted { - t.Errorf("Log 0: got %s, want %s", p.Logs[0].Lines, wanted) + wanted = "logs-0000.log.gz" + if p.Logs[0] != wanted { + t.Errorf("Log 0: got %s, want %s", p.Logs[0], wanted) } - wanted = "instance-0.215954445471346461492087122412" - if p.Logs[0].ID != wanted { - t.Errorf("Log 0 ID: got %s, want %s", p.Logs[0].ID, wanted) + wanted = "logs-0001.log.gz" + if p.Logs[1] != wanted { + t.Errorf("Log 1: got %s, want %s", p.Logs[1], wanted) } - wanted = "log" - if p.Logs[1].Lines != wanted { - t.Errorf("Log 1: got %s, want %s", p.Logs[1].Lines, 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) + wanted = "logs-0002.log.gz" + if p.Logs[2] != wanted { + t.Errorf("Log 2: got %s, want %s", p.Logs[2], 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 {