diff --git a/internal/adapters/http/client_body_test.go b/internal/adapters/http/client_body_test.go new file mode 100644 index 0000000..2450873 --- /dev/null +++ b/internal/adapters/http/client_body_test.go @@ -0,0 +1,55 @@ +package http + +import ( + "bytes" + "context" + "encoding/json" + "io" + "net/http" + "testing" + + "qualflare-cli/internal/core/domain" +) + +// TestSendReport_BodyIsUncompressedJSONEncoding pins the wire format that +// services.collectBodySize measures against /collect's 10 MB limit: the launch +// encoded by encoding/json with HTML escaping (json.Marshal bytes plus the +// encoder's trailing newline), with no Content-Encoding. If this changes — +// compression, a different encoder — the body-limit measurement must change +// with it, or it will drop attachments it need not, or let a 413 through. +func TestSendReport_BodyIsUncompressedJSONEncoding(t *testing.T) { + launch := &domain.Launch{Suites: []domain.Suite{{ + Name: "s <&>", + Cases: []domain.Case{{ + Name: "c", + Status: domain.StatusPassed, + Attachments: []domain.Attachment{ + {Name: "log.json", MimeType: "application/json", Content: "eyJhIjoiPGI+In0="}, + }, + }}, + }}} + + var got []byte + var encoding string + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + encoding = r.Header.Get("Content-Encoding") + got, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusOK) + }) + if err := c.SendReport(context.Background(), launch); err != nil { + t.Fatalf("SendReport() = %v", err) + } + + var want bytes.Buffer + enc := json.NewEncoder(&want) + enc.SetEscapeHTML(true) + if err := enc.Encode(launch); err != nil { + t.Fatal(err) + } + if encoding != "" { + t.Errorf("Content-Encoding = %q, want none", encoding) + } + if !bytes.Equal(got, want.Bytes()) { + t.Errorf("wire body differs from the encoding the body-limit check measures\n got: %s\nwant: %s", got, want.Bytes()) + } +} diff --git a/internal/core/services/body_limit.go b/internal/core/services/body_limit.go new file mode 100644 index 0000000..31afef4 --- /dev/null +++ b/internal/core/services/body_limit.go @@ -0,0 +1,218 @@ +package services + +import ( + "encoding/json" + "fmt" + "sort" + + "qualflare-cli/internal/core/domain" +) + +// maxCollectBodyBytes is the largest /collect body the CLI will send. +// +// The server rejects anything over 10 MB with a bare 413 before parsing, which +// loses the entire launch. 9.5 MB (decimal) leaves 500 KB of headroom: it sits +// under the limit whether the server counts 10 MB as 10,000,000 or 10 MiB, and +// absorbs the bytes this measurement cannot see — HTTP framing, and any proxy +// in front of the server that counts the limit slightly differently. +const maxCollectBodyBytes = 9_500_000 + +// countingWriter counts bytes without keeping them, so measuring a ~10 MB body +// does not allocate a second copy of it. +type countingWriter struct{ n int } + +func (w *countingWriter) Write(p []byte) (int, error) { + w.n += len(p) + return len(p), nil +} + +// collectBodySize is the exact byte length of the /collect request body for +// launch. +// +// It must match what the HTTP adapter sends. SendReport passes the *Launch to +// resty's SetBody with no Content-Type override, no custom encoder and no +// request compression, so resty serialises it with its default JSON encoder: +// json.NewEncoder(w) with SetEscapeHTML(true), then Encode — the json.Marshal +// bytes plus one trailing newline, sent uncompressed. The same encoder here +// produces the same bytes. client_test.go's +// TestSendReport_BodyIsUncompressedJSONEncoding pins the adapter side, so a +// change there (gzip, a different encoder) fails a test rather than silently +// making this measurement wrong. +func collectBodySize(launch *domain.Launch) (int, error) { + var w countingWriter + enc := json.NewEncoder(&w) + enc.SetEscapeHTML(true) + if err := enc.Encode(launch); err != nil { + return 0, err + } + return w.n, nil +} + +// inlineCandidate is one attachment still carrying base64 content in the body. +type inlineCandidate struct { + suite, kase, att int + name, caseName string + contentLen int + isImage bool + // saving is a lower bound on how much removing this attachment shrinks the + // body: its own encoded JSON object. The real saving is at least this — plus + // a separating comma, or the whole `"attachments":[...]` key when it was the + // case's last one — so subtracting it never under-estimates the new size. + saving int +} + +// enforceBodyLimit keeps the /collect body under maxCollectBodyBytes. +// +// It runs after offloadInlineAttachments, so whatever is still inline either +// has a type the upload endpoint does not accept (JSON, logs, markdown) or is an +// image whose upload failed. Losing some of those beats losing the whole launch +// to a 413, so while the body is over the limit it drops inline attachments: +// every non-image before any image (an image the user can look at is the more +// valuable thing to keep), and within each group the largest first, so the +// fewest attachments are lost. Every drop is reported. +// +// A dropped attachment is removed outright rather than left as an empty +// placeholder: the server persists a row from Name alone, and an undownloadable +// row says less than the warning does. +// +// If the body is still over the limit with nothing inline left to drop, the +// results themselves are too large; that is an error before sending, not a 413 +// after. +func (s *ReportService) enforceBodyLimit(launch *domain.Launch) error { + size, err := collectBodySize(launch) + if err != nil { + return fmt.Errorf("failed to measure the request body: %w", err) + } + + var dropped []inlineCandidate + for size > maxCollectBodyBytes { + candidates, err := inlineCandidates(launch) + if err != nil { + return fmt.Errorf("failed to measure the request body: %w", err) + } + if len(candidates) == 0 { + break + } + + // Drop until a conservative estimate fits. Because each saving is a lower + // bound, an estimate at or under the limit means the real size is too; + // the re-measure below confirms it exactly. + estimate := size + var batch []inlineCandidate + for _, c := range candidates { + if estimate <= maxCollectBodyBytes { + break + } + batch = append(batch, c) + estimate -= c.saving + } + removeAttachments(launch, batch) + dropped = append(dropped, batch...) + + if size, err = collectBodySize(launch); err != nil { + return fmt.Errorf("failed to measure the request body: %w", err) + } + } + + s.warnDroppedAttachments(dropped) + + if size > maxCollectBodyBytes { + return fmt.Errorf("the results are %s, over /collect's 10 MB limit even without attachments; "+ + "split the upload, e.g. collect fewer files per run", humanBytes(size)) + } + return nil +} + +// inlineCandidates lists every attachment still carrying content and no +// storage key, in drop order: non-images before images, then largest content +// first. The sort is stable, so equal sizes keep report order. +func inlineCandidates(launch *domain.Launch) ([]inlineCandidate, error) { + var out []inlineCandidate + for i := range launch.Suites { + for j := range launch.Suites[i].Cases { + c := &launch.Suites[i].Cases[j] + for k := range c.Attachments { + a := &c.Attachments[k] + if a.Content == "" || a.StorageKey != "" { + continue + } + encoded, err := json.Marshal(a) + if err != nil { + return nil, err + } + _, isImage := offloadableExtensions[a.MimeType] + out = append(out, inlineCandidate{ + suite: i, kase: j, att: k, + name: a.Name, caseName: c.Name, + contentLen: len(a.Content), + isImage: isImage, + saving: len(encoded), + }) + } + } + } + sort.SliceStable(out, func(a, b int) bool { + if out[a].isImage != out[b].isImage { + return !out[a].isImage + } + return out[a].contentLen > out[b].contentLen + }) + return out, nil +} + +// removeAttachments deletes the given attachments. Each touched case gets a +// fresh slice rather than an in-place filter, because a parser may hand back +// cases whose attachment slices share a backing array with another file's. +func removeAttachments(launch *domain.Launch, drop []inlineCandidate) { + type caseKey struct{ suite, kase int } + byCase := make(map[caseKey]map[int]bool) + for _, d := range drop { + key := caseKey{d.suite, d.kase} + if byCase[key] == nil { + byCase[key] = make(map[int]bool) + } + byCase[key][d.att] = true + } + for key, idx := range byCase { + c := &launch.Suites[key.suite].Cases[key.kase] + kept := make([]domain.Attachment, 0, len(c.Attachments)-len(idx)) + for k, a := range c.Attachments { + if !idx[k] { + kept = append(kept, a) + } + } + if len(kept) == 0 { + kept = nil + } + c.Attachments = kept + } +} + +func (s *ReportService) warnDroppedAttachments(dropped []inlineCandidate) { + if len(dropped) == 0 { + return + } + total := 0 + for _, d := range dropped { + total += d.contentLen + fmt.Fprintf(s.warnWriter(), + "warning: dropped attachment %q (%s) from %q to keep the upload under the server's 10 MB limit\n", + d.name, humanBytes(d.contentLen), d.caseName) + } + fmt.Fprintf(s.warnWriter(), + "warning: dropped %d inline attachment(s) totalling %s to keep the upload under the server's 10 MB limit\n", + len(dropped), humanBytes(total)) +} + +// humanBytes formats n in decimal units, matching how the server's "10 MB" +// limit and maxCollectBodyBytes are stated. +func humanBytes(n int) string { + switch { + case n >= 1_000_000: + return fmt.Sprintf("%.1f MB", float64(n)/1_000_000) + case n >= 1_000: + return fmt.Sprintf("%.1f KB", float64(n)/1_000) + default: + return fmt.Sprintf("%d B", n) + } +} diff --git a/internal/core/services/report_service.go b/internal/core/services/report_service.go index f53a5b8..602f412 100644 --- a/internal/core/services/report_service.go +++ b/internal/core/services/report_service.go @@ -86,7 +86,11 @@ func (s *ReportService) ProcessTestResults(ctx context.Context, files []string, // internal timeout be the only deadline governing upload duration. s.resolveArtifactAttachments(context.Background(), report) s.offloadInlineAttachments(context.Background(), report) - s.warnIfBodyLooksTooLarge(report) + // After the offload, so only what genuinely could not leave the body (non-image + // types, and images whose upload failed) is at risk of being dropped. + if err := s.enforceBodyLimit(report); err != nil { + return err + } return s.sender.SendReport(ctx, report) } @@ -181,12 +185,6 @@ func (s *ReportService) resolveArtifactAttachments(ctx context.Context, launch * s.warnSkippedArtifacts(skipped) } -// maxInlineBodyWarnBytes is the point past which the assembled request is close -// enough to /collect's 10MB server-side limit to be worth saying so. The server -// answers an oversized body with a bare 413 before parsing anything, which -// reaches the user as an unexplained failure with the whole launch lost. -const maxInlineBodyWarnBytes = 8 << 20 - // attachmentUploadConcurrency bounds how many attachment PUTs are in flight. // Serial uploads would make a suite with hundreds of screenshots unusable at // collect time; unbounded would open a socket per attachment. @@ -367,25 +365,6 @@ func (s *ReportService) offloadInlineAttachments(ctx context.Context, launch *do } } -// warnIfBodyLooksTooLarge says plainly what the server would otherwise answer -// with a bare 413. Nothing here prevents the failure — it only stops it being -// mysterious when an attachment could not be offloaded. -func (s *ReportService) warnIfBodyLooksTooLarge(launch *domain.Launch) { - inline := 0 - for i := range launch.Suites { - for j := range launch.Suites[i].Cases { - for _, a := range launch.Suites[i].Cases[j].Attachments { - inline += len(a.Content) - } - } - } - if inline > maxInlineBodyWarnBytes { - fmt.Fprintf(s.warnWriter(), - "warning: %d MB of attachments are still inline; /collect rejects a body over 10MB outright, "+ - "which fails the whole launch\n", inline>>20) - } -} - // warnSkippedArtifacts tells the user exactly what was left out and how to // include it. Silence here would be the worst outcome of the gate: videos used // to upload automatically, so someone upgrading loses them, and must be told diff --git a/internal/core/services/report_service_body_limit_test.go b/internal/core/services/report_service_body_limit_test.go new file mode 100644 index 0000000..f2531c9 --- /dev/null +++ b/internal/core/services/report_service_body_limit_test.go @@ -0,0 +1,203 @@ +package services + +import ( + "bytes" + "context" + "encoding/base64" + "encoding/json" + "errors" + "strings" + "testing" + + "qualflare-cli/internal/config" + "qualflare-cli/internal/core/domain" + "qualflare-cli/internal/core/ports" +) + +// wireBytes encodes a launch the way the HTTP adapter's resty client puts it on +// the wire for /collect: encoding/json with HTML escaping on, plus the encoder's +// trailing newline, uncompressed. client_test.go's +// TestSendReport_BodyIsUncompressedJSONEncoding pins that side of the contract. +func wireBytes(t *testing.T, l *domain.Launch) []byte { + t.Helper() + var buf bytes.Buffer + if err := json.NewEncoder(&buf).Encode(l); err != nil { + t.Fatal(err) + } + return buf.Bytes() +} + +func blobAttachment(name, mime string, decoded int, fill byte) domain.Attachment { + return domain.Attachment{ + Name: name, + MimeType: mime, + Content: base64.StdEncoding.EncodeToString(bytes.Repeat([]byte{fill}, decoded)), + } +} + +// bodyLimitService runs ProcessTestResults over one parsed suite, with every +// image upload failing — the only way an image is still inline by the time the +// body limit is enforced. +func bodyLimitService(suite *domain.Suite) (*ReportService, *videoStubSender, *strings.Builder) { + parser := &stubParser{framework: domain.FrameworkJUnit, suite: suite} + fac := &stubFactory{parsers: map[domain.Framework]ports.Parser{domain.FrameworkJUnit: parser}} + sender := &videoStubSender{attachmentErr: errors.New("presign: 500")} + warn := &strings.Builder{} + svc := &ReportService{parserFactory: fac, sender: sender, config: config.DefaultConfig(), warn: warn} + return svc, sender, warn +} + +func TestBodyLimit_DropsLargestNonImagesFirst(t *testing.T) { + const mb = 1_000_000 + // Encoded (base64) sizes: JSON 1.0 / 0.6 / 0.2 MB, PNG 6.0 / 3.6 MB — about + // 11.4 MB inline. Dropping every JSON still leaves 9.6 MB, so exactly one + // image (the larger) has to go too. + suite := &domain.Suite{ + Name: "suite", + Cases: []domain.Case{ + {Name: "case-json-mid", Status: domain.StatusPassed, Attachments: []domain.Attachment{ + blobAttachment("mid.json", "application/json", 450_000, 'b'), + }}, + {Name: "case-png-big", Status: domain.StatusFailed, Attachments: []domain.Attachment{ + blobAttachment("big.png", "image/png", 4_500_000, 'p'), + {Name: "already.png", MimeType: "image/png", StorageKey: "k/already.png", FileSize: 10}, + }}, + {Name: "case-json-big", Status: domain.StatusPassed, Attachments: []domain.Attachment{ + blobAttachment("big.json", "application/json", 750_000, 'a'), + }}, + {Name: "case-png-small", Status: domain.StatusFailed, Attachments: []domain.Attachment{ + blobAttachment("small.png", "image/png", 2_700_000, 'q'), + }}, + {Name: "case-json-small", Status: domain.StatusPassed, Attachments: []domain.Attachment{ + blobAttachment("small.json", "application/json", 150_000, 'c'), + }}, + }, + } + if n := len(wireBytes(t, &domain.Launch{Suites: []domain.Suite{*suite}})); n < 11*mb { + t.Fatalf("fixture should be ~11.4 MB to be meaningful, got %d", n) + } + + svc, sender, warn := bodyLimitService(suite) + f := writeFile(t, t.TempDir(), "r.xml", "") + if err := svc.ProcessTestResults(context.Background(), []string{f}, domain.FrameworkJUnit); err != nil { + t.Fatalf("ProcessTestResults() = %v", err) + } + if sender.sent != 1 || sender.last == nil { + t.Fatalf("expected one send, got %d", sender.sent) + } + + if n := len(wireBytes(t, sender.last)); n > maxCollectBodyBytes { + t.Errorf("sent body is %d bytes, want <= %d", n, maxCollectBodyBytes) + } + + kept := map[string]bool{} + for _, c := range sender.last.Suites[0].Cases { + for _, a := range c.Attachments { + kept[a.Name] = true + } + } + for _, name := range []string{"big.json", "mid.json", "small.json", "big.png"} { + if kept[name] { + t.Errorf("%s should have been dropped", name) + } + } + for _, name := range []string{"small.png", "already.png"} { + if !kept[name] { + t.Errorf("%s should have been kept", name) + } + } + + out := warn.String() + wantOrder := []string{ + `warning: dropped attachment "big.json" (1.0 MB) from "case-json-big" to keep the upload under the server's 10 MB limit`, + `warning: dropped attachment "mid.json" (600.0 KB) from "case-json-mid"`, + `warning: dropped attachment "small.json" (200.0 KB) from "case-json-small"`, + `warning: dropped attachment "big.png" (6.0 MB) from "case-png-big"`, + "dropped 4 inline attachment(s) totalling 7.8 MB", + } + last := -1 + for _, w := range wantOrder { + i := strings.Index(out, w) + if i < 0 { + t.Errorf("warnings missing %q\ngot:\n%s", w, out) + continue + } + if i < last { + t.Errorf("%q is out of order (JSON largest-first, then images)\ngot:\n%s", w, out) + } + last = i + } +} + +func TestBodyLimit_SmallLaunchUntouched(t *testing.T) { + l := &domain.Launch{Suites: []domain.Suite{{Name: "s", Cases: []domain.Case{{ + Name: "c <&>", // HTML-escaped by the encoder; must survive unchanged + Status: domain.StatusPassed, + Attachments: []domain.Attachment{ + blobAttachment("log.json", "application/json", 10_000, 'x'), + blobAttachment("shot.png", "image/png", 10_000, 'y'), + }, + }}}}} + before := wireBytes(t, l) + warn := &strings.Builder{} + + if err := (&ReportService{warn: warn}).enforceBodyLimit(l); err != nil { + t.Fatalf("enforceBodyLimit() = %v", err) + } + + if after := wireBytes(t, l); !bytes.Equal(before, after) { + t.Error("a launch under the limit must be sent byte-identical") + } + if warn.String() != "" { + t.Errorf("nothing to say about a small launch, got %q", warn.String()) + } +} + +func TestBodyLimit_ResultsAloneTooLarge(t *testing.T) { + // ~12 MB of results with no attachments: nothing can be dropped. + cases := make([]domain.Case, 0, 12_000) + msg := strings.Repeat("m", 1_000) + for i := 0; i < 12_000; i++ { + cases = append(cases, domain.Case{Name: "case", Status: domain.StatusFailed, Error: msg}) + } + suite := &domain.Suite{Name: "huge", Cases: cases} + + svc, sender, _ := bodyLimitService(suite) + f := writeFile(t, t.TempDir(), "r.xml", "") + err := svc.ProcessTestResults(context.Background(), []string{f}, domain.FrameworkJUnit) + if err == nil { + t.Fatal("ProcessTestResults() = nil, want an error before sending") + } + for _, want := range []string{"MB, over /collect's 10 MB limit even without attachments", "split the upload"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q should contain %q", err.Error(), want) + } + } + if sender.sent != 0 { + t.Errorf("nothing should be sent, got %d sends", sender.sent) + } +} + +// --dry-run never offloads (that needs the network), so its launch still holds +// every image inline and is not the body that would be sent. Applying the drop +// there would report images as dropped that a real run uploads, so dry-run +// leaves the launch alone. +func TestBodyLimit_NotAppliedUnderDryRun(t *testing.T) { + suite := &domain.Suite{Name: "s", Cases: []domain.Case{{Name: "c", Status: domain.StatusFailed, + Attachments: []domain.Attachment{blobAttachment("big.json", "application/json", 8_000_000, 'a')}}}} + svc, sender, warn := bodyLimitService(suite) + cfg := config.DefaultConfig() + cfg.DryRun = true + svc.config = cfg + f := writeFile(t, t.TempDir(), "r.xml", "") + + if err := svc.ProcessTestResults(context.Background(), []string{f}, domain.FrameworkJUnit); err != nil { + t.Fatalf("ProcessTestResults() = %v", err) + } + if sender.sent != 0 { + t.Errorf("dry run sent %d times", sender.sent) + } + if strings.Contains(warn.String(), "dropped") { + t.Errorf("dry run should not drop anything, got %q", warn.String()) + } +} diff --git a/internal/core/services/report_service_offload_test.go b/internal/core/services/report_service_offload_test.go index 8cd5e8a..76f7c84 100644 --- a/internal/core/services/report_service_offload_test.go +++ b/internal/core/services/report_service_offload_test.go @@ -12,9 +12,9 @@ import ( "qualflare-cli/internal/core/domain" ) -func inline(name, content string) domain.Attachment { +func inlinePNG(content string) domain.Attachment { return domain.Attachment{ - Name: name, + Name: "shot.png", MimeType: "image/png", Content: base64.StdEncoding.EncodeToString([]byte(content)), } @@ -31,7 +31,7 @@ func launchWithAttachments(atts ...domain.Attachment) *domain.Launch { func TestOffload_ReplacesInlineContentWithAStorageKey(t *testing.T) { sender := &videoStubSender{} svc := offloadSvc(sender) - l := launchWithAttachments(inline("shot.png", "PNGBYTES")) + l := launchWithAttachments(inlinePNG("PNGBYTES")) svc.offloadInlineAttachments(context.Background(), l) @@ -63,7 +63,7 @@ func TestOffload_TwelveShardsWorthOfAttachmentsLeaveTheBody(t *testing.T) { // distinct content per shard, or dedupe would mask the problem blob := strings.Repeat(string(rune('a'+i)), perShard) suites = append(suites, domain.Suite{Cases: []domain.Case{{ - Attachments: []domain.Attachment{inline("shot.png", blob)}, + Attachments: []domain.Attachment{inlinePNG(blob)}, }}}) } l := &domain.Launch{Suites: suites} @@ -87,7 +87,7 @@ func TestOffload_TwelveShardsWorthOfAttachmentsLeaveTheBody(t *testing.T) { func TestOffload_IdenticalContentUploadsOnce(t *testing.T) { sender := &videoStubSender{} svc := offloadSvc(sender) - same := inline("shot.png", "IDENTICAL") + same := inlinePNG("IDENTICAL") l := &domain.Launch{Suites: []domain.Suite{{Cases: []domain.Case{ {Attachments: []domain.Attachment{same}}, {Attachments: []domain.Attachment{same}}, @@ -112,7 +112,7 @@ func TestOffload_FailedUploadLeavesTheAttachmentInline(t *testing.T) { sender := &videoStubSender{attachmentErr: errors.New("network down")} warn := &strings.Builder{} svc := &ReportService{sender: sender, config: config.DefaultConfig(), warn: warn} - l := launchWithAttachments(inline("shot.png", "PNGBYTES")) + l := launchWithAttachments(inlinePNG("PNGBYTES")) svc.offloadInlineAttachments(context.Background(), l) @@ -154,21 +154,6 @@ func TestOffload_LeavesResolvedAndEmptyAttachmentsAlone(t *testing.T) { } } -func TestWarnIfBodyLooksTooLarge_OnlyWhenItActuallyIs(t *testing.T) { - quiet := &strings.Builder{} - (&ReportService{warn: quiet}).warnIfBodyLooksTooLarge(launchWithAttachments(inline("s.png", "small"))) - if quiet.String() != "" { - t.Errorf("a small body should warn about nothing, got %q", quiet.String()) - } - - loud := &strings.Builder{} - (&ReportService{warn: loud}).warnIfBodyLooksTooLarge( - launchWithAttachments(inline("s.png", strings.Repeat("x", 9<<20)))) - if !strings.Contains(loud.String(), "10MB") { - t.Errorf("a body near the limit should say so; got %q", loud.String()) - } -} - func inlineBytes(l *domain.Launch) int { n := 0 for i := range l.Suites {