fix: well-formed Write after Parse, trimmed ids, overflow-safe subject numbers, regexps compiled once

nzbparser.go

@@ -4,9 +4,9 @@ import (
 	"bytes"
 	"encoding/xml"
 	"fmt"
-	"html"
 	"io"
 	"sort"
+	"strings"
 
 	"golang.org/x/net/html/charset"
 )
@@ -68,7 +68,7 @@ func (s NzbSegments) Swap(i, j int)      { s[i], s[j] = s[j], s[i] }
 type NzbSegment struct {
 	Bytes  int    `xml:"bytes,attr"`
 	Number int    `xml:"number,attr"`
-	ID     string `xml:",innerxml"`
+	ID     string `xml:",chardata"` // the message-id, XML-decoded and trimmed
 }
 
 // parse nzb file provided as string
@@ -95,20 +95,29 @@ func ParseWithOptions(buf io.Reader, opts ParseOptions) (*Nzb, error) {
 	decoder.Strict = false // ignore unknown or malformed character entities
 
 	if err := decoder.Decode(xnzb); err != nil {
-		return nil, fmt.Errorf("unable to parse NZB file: %s", err.Error())
+		return nil, fmt.Errorf("unable to parse NZB file: %w", err)
 	}
 
 	// convert to nicer format
 	nzb := new(Nzb)
 
-	// copy elements
-	nzb.Comment = xnzb.Comment
+	// copy elements. Comment, ids, groups and meta values are trimmed: pretty
+	// printed NZBs put newlines around them, and Write pads the comment.
+	nzb.Comment = strings.TrimSpace(xnzb.Comment)
 	nzb.Files = xnzb.Files
+	for i := range nzb.Files {
+		for j := range nzb.Files[i].Groups {
+			nzb.Files[i].Groups[j] = strings.TrimSpace(nzb.Files[i].Groups[j])
+		}
+		for j := range nzb.Files[i].Segments {
+			nzb.Files[i].Segments[j].ID = strings.TrimSpace(nzb.Files[i].Segments[j].ID)
+		}
+	}
 
 	// convert metadata
 	nzb.Meta = make(map[string]string)
 	for _, md := range xnzb.Metadata {
-		nzb.Meta[md.Type] = md.Value
+		nzb.Meta[md.Type] = strings.TrimSpace(md.Value)
 	}
 
 	// conditionally remove duplicate entries
@@ -119,11 +128,12 @@ func ParseWithOptions(buf io.Reader, opts ParseOptions) (*Nzb, error) {
 	// scan the nzb for the additional information
 	ScanNzbFile(nzb)
 
-	// sort the files and segments
-	sort.Sort(nzb.Files)
+	// sort the files and segments; stable, so files without a number in the
+	// subject (all 1) and repeated segment numbers keep the NZB's order
+	sort.Stable(nzb.Files)
 
 	for id := range nzb.Files {
-		sort.Sort(nzb.Files[id].Segments)
+		sort.Stable(nzb.Files[id].Segments)
 	}
 
 	return nzb, nil
@@ -150,9 +160,14 @@ func Write(nzb *Nzb) ([]byte, error) {
 	// add namespace
 	xnzb.Xmlns = Xmlns
 
-	// add metadata
-	for t, v := range nzb.Meta {
-		xnzb.Metadata = append(xnzb.Metadata, xNzbMeta{Type: t, Value: v})
+	// add metadata, in a fixed order (map order made the output differ per run)
+	types := make([]string, 0, len(nzb.Meta))
+	for t := range nzb.Meta {
+		types = append(types, t)
+	}
+	sort.Strings(types)
+	for _, t := range types {
+		xnzb.Metadata = append(xnzb.Metadata, xNzbMeta{Type: t, Value: nzb.Meta[t]})
 	}
 
 	// Marshal and prepend header
@@ -187,6 +202,7 @@ func ScanNzbFile(nzb *Nzb) {
 			} else {
 				nzb.Files[id].Filename = subject.Header
 			}
+			nzb.Files[id].Basefilename = subject.Basefilename // was never set
 
 			totalFileSegments = subject.TotalSegments
 
@@ -195,14 +211,13 @@ func ScanNzbFile(nzb *Nzb) {
 			}
 		}
 
-		for i, segment := range file.Segments {
+		for _, segment := range file.Segments {
 			if segment.Number > totalFileSegments {
 				totalFileSegments = segment.Number
 			}
 
 			totalBytes = totalBytes + int64(segment.Bytes)
 			totalFileBytes = totalFileBytes + int64(segment.Bytes)
-			nzb.Files[id].Segments[i].ID = html.UnescapeString(segment.ID)
 		}
 
 		segments = segments + file.Segments.Len()
@@ -278,5 +293,5 @@ type xNzb struct {
 // temp raw meta data for (un)marshalling
 type xNzbMeta struct {
 	Type  string `xml:"type,attr"`
-	Value string `xml:",innerxml"`
+	Value string `xml:",chardata"`
 }

review_test.go

@@ -0,0 +1,131 @@
+package nzbparser
+
+import (
+	"encoding/xml"
+	"fmt"
+	"strings"
+	"testing"
+)
+
+func reviewDoc(head, files string) string {
+	return Header + `<nzb xmlns="http://www.newzbin.com/DTD/2003/nzb">` + head + files + `</nzb>`
+}
+
+func reviewFile(subject, id string) string {
+	return `<file poster="p" date="1" subject="` + subject + `"><groups><group>alt.test</group></groups>` +
+		`<segments><segment bytes="10" number="1">` + id + `</segment></segments></file>`
+}
+
+// A parsed NZB written back is well-formed XML with the same ids
+// (ids were unescaped after parsing, then written raw).
+func TestWriteAfterParseIsWellFormed(t *testing.T) {
+	nzb, err := ParseString(reviewDoc("", reviewFile("&quot;a.bin&quot; yEnc (1/1)", "part1&amp;x&lt;y&gt;@example.test")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	out, err := WriteString(nzb)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := xml.Unmarshal([]byte(out[len(Header):]), new(xNzb)); err != nil {
+		t.Fatalf("Write output is not well-formed: %v\n%s", err, out)
+	}
+	again, err := ParseString(out)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := again.Files[0].Segments[0].ID; got != "part1&x<y>@example.test" {
+		t.Fatalf("id after a round trip: %q", got)
+	}
+}
+
+// Pretty-printed NZBs put whitespace around ids, groups and meta values.
+func TestWhitespaceIsTrimmed(t *testing.T) {
+	doc := reviewDoc("<head><meta type=\"title\">\n  A Title\n</meta></head>",
+		"<file poster=\"p\" date=\"1\" subject=\"&quot;a.bin&quot; (1/1)\"><groups><group>\n alt.test \n</group></groups>"+
+			"<segments><segment bytes=\"10\" number=\"1\">\n      p@example.test\n    </segment></segments></file>")
+	nzb, err := ParseString(doc)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if id := nzb.Files[0].Segments[0].ID; id != "p@example.test" {
+		t.Errorf("id %q", id)
+	}
+	if g := nzb.Files[0].Groups[0]; g != "alt.test" {
+		t.Errorf("group %q", g)
+	}
+	if m := nzb.Meta["title"]; m != "A Title" {
+		t.Errorf("meta %q", m)
+	}
+}
+
+// A segment count too big for an int counts as missing; the real segments decide.
+func TestOverflowingSubjectNumbers(t *testing.T) {
+	nzb, err := ParseString(reviewDoc("", reviewFile("&quot;b.bin&quot; yEnc (1/99999999999999999999)", "b@example.test")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if nzb.TotalSegments != 1 || nzb.Files[0].TotalSegments != 1 {
+		t.Fatalf("TotalSegments = %d / %d, want 1", nzb.TotalSegments, nzb.Files[0].TotalSegments)
+	}
+}
+
+func TestBasefilenameIsSet(t *testing.T) {
+	nzb, err := ParseString(reviewDoc("", reviewFile("[1/2] &quot;release.part01.rar&quot; yEnc (1/1)", "c@example.test")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if f := nzb.Files[0]; f.Filename != "release.part01.rar" || f.Basefilename != "release" {
+		t.Fatalf("Filename %q Basefilename %q", f.Filename, f.Basefilename)
+	}
+}
+
+// Parse → Write → Parse keeps the comment and the output is the same every time.
+func TestWriteIsStable(t *testing.T) {
+	doc := reviewDoc("<!-- hi --><head><meta type=\"b\">2</meta><meta type=\"a\">1</meta><meta type=\"c\">3</meta></head>",
+		reviewFile("&quot;d.bin&quot; (1/1)", "d@example.test"))
+	nzb, err := ParseString(doc)
+	if err != nil {
+		t.Fatal(err)
+	}
+	first, _ := WriteString(nzb)
+	for i := 0; i < 20; i++ {
+		again, err := ParseString(first)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if again.Comment != "hi" {
+			t.Fatalf("comment after %d round trips: %q", i+1, again.Comment)
+		}
+		out, _ := WriteString(again)
+		if out != first {
+			t.Fatalf("Write output changed on round trip %d:\n%s\n---\n%s", i+1, first, out)
+		}
+	}
+}
+
+// Files without a number in the subject keep the NZB's order (sort.Sort is not stable).
+func TestFilesWithoutNumbersKeepOrder(t *testing.T) {
+	var files []string
+	for i := 0; i < 200; i++ {
+		files = append(files, reviewFile(fmt.Sprintf("post &quot;f%03d.bin&quot; yEnc", i), fmt.Sprintf("m%03d@example.test", i)))
+	}
+	nzb, err := ParseString(reviewDoc("", strings.Join(files, "")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	for i, f := range nzb.Files {
+		if want := fmt.Sprintf("f%03d.bin", i); f.Filename != want {
+			t.Fatalf("file %d is %q, want %q", i, f.Filename, want)
+		}
+	}
+}
+
+func BenchmarkParseSubject(b *testing.B) {
+	b.ReportAllocs()
+	for i := 0; i < b.N; i++ {
+		if _, err := ParseSubject(`[1/7] "ubuntu-22.04-live-server-amd64.iso" yEnc (1/2047)`); err != nil {
+			b.Fatal(err)
+		}
+	}
+}