Merge pull request #15 from qmuntal/fix-rel-ids

Generate rel IDs using rIdN pattern

Author: qmuntal

relationship.go

@@ -4,8 +4,8 @@ import (
 	"encoding/xml"
 	"fmt"
 	"io"
-	"math/rand"
 	"net/url"
+	"sort"
 	"strings"
 )
 
@@ -25,8 +25,8 @@ const externalMode = "External"
 const charBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456789"
 
 // Relationship is used to express a relationship between a source and a target part.
-// If the ID is not specified a random string with 8 characters will be generated.
-// If The TargetMode is not specified the default value is Internal.
+// If the ID is not specified a unique ID will be generated following the pattern rIdN.
+// If the TargetMode is not specified the default value is Internal.
 // Defined in ISO/IEC 29500-2 §9.3.
 type Relationship struct {
 	ID         string     // The relationship identifier which shall conform the xsd:ID naming restrictions and unique within the part.
@@ -48,16 +48,22 @@ type relationshipXML struct {
 	Mode      string `xml:"TargetMode,attr,omitempty"`
 }
 
-func (r *Relationship) ensureID(rnd *rand.Rand) {
-	if r.ID != "" {
-		return
-	}
-
-	b := make([]byte, 8)
-	for i := range b {
-		b[i] = charBytes[rnd.Intn(len(charBytes))]
-	}
-	r.ID = string(b)
+func newRelationshipID(rels []*Relationship) string {
+	ids := make([]string, len(rels))
+	for i, rel := range rels {
+		ids[i] = rel.ID
+	}
+	sort.Strings(ids)
+	idFunc := func(i int) string { return fmt.Sprintf("rId%d", i) }
+	var (
+		i int
+		id = idFunc(0)
+	)
+	for sort.SearchStrings(ids, id) < 0 {
+		i++
+		id = idFunc(i)
+	}
+	return id
 }
 
 func (r *Relationship) validate(sourceURI string) error {

writer.go

@@ -5,7 +5,6 @@ import (
 	"compress/flate"
 	"fmt"
 	"io"
-	"math/rand"
 	"path/filepath"
 	"strings"
 	"time"
@@ -34,7 +33,6 @@ type Writer struct {
 	p             *pkg
 	w             *zip.Writer
 	last          *Part
-	rnd           *rand.Rand
 }
 
 // NewWriter returns a new Writer writing an OPC file to w.
@@ -48,7 +46,7 @@ func NewWriter(w io.Writer) *Writer {
 			},
 			overrides: map[string]string{},
 		},
-	}, w: zip.NewWriter(w), rnd: rand.New(rand.NewSource(42))}
+	}, w: zip.NewWriter(w)}
 }
 
 // Flush flushes any buffered data to the underlying writer.
@@ -148,7 +146,9 @@ func (w *Writer) createOwnRelationships() error {
 		return nil
 	}
 	for _, r := range w.Relationships {
-		r.ensureID(w.rnd)
+		if r.ID == "" {
+			r.ID = newRelationshipID(w.Relationships)
+		}
 	}
 	if err := validateRelationships("/", w.Relationships); err != nil {
 		return err
@@ -165,7 +165,9 @@ func (w *Writer) createLastPartRelationships() error {
 		return nil
 	}
 	for _, r := range w.last.Relationships {
-		r.ensureID(w.rnd)
+		if r.ID == "" {
+			r.ID = newRelationshipID(w.Relationships)
+		}
 	}
 	if err := validateRelationships(w.last.Name, w.last.Relationships); err != nil {
 		return err

writer_test.go

@@ -3,7 +3,6 @@ package opc
 import (
 	"archive/zip"
 	"bytes"
-	"math/rand"
 	"testing"
 )
 
@@ -24,10 +23,6 @@ func TestWriter_Flush(t *testing.T) {
 	}
 }
 
-func fakeRand() *rand.Rand {
-	return rand.New(rand.NewSource(42))
-}
-
 func TestWriter_Close(t *testing.T) {
 	p := newPackage()
 	p.contentTypes.add("/a.xml", "a/b")
@@ -44,17 +39,17 @@ func TestWriter_Close(t *testing.T) {
 		wantErr bool
 	}{
 		{"base", NewWriter(&bytes.Buffer{}), false},
-		{"invalidContentType", &Writer{p: pC, w: zip.NewWriter(&bytes.Buffer{}), rnd: fakeRand()}, true},
-		{"withCt", &Writer{p: p, w: zip.NewWriter(&bytes.Buffer{}), rnd: fakeRand()}, false},
-		{"invalidPartRel", &Writer{p: newPackage(), w: zip.NewWriter(&bytes.Buffer{}), last: &Part{Name: "/b.xml", Relationships: []*Relationship{{}}}, rnd: fakeRand()}, true},
-		{"invalidOwnRel", &Writer{p: newPackage(), w: zip.NewWriter(&bytes.Buffer{}), Relationships: []*Relationship{{}}, rnd: fakeRand()}, true},
-		{"withDuplicatedCoreProps", &Writer{p: pCore, w: zip.NewWriter(&bytes.Buffer{}), Properties: CoreProperties{Title: "Song"}, rnd: fakeRand()}, true},
-		{"withDuplicatedRels", &Writer{p: pRel, w: zip.NewWriter(&bytes.Buffer{}), Properties: CoreProperties{Title: "Song"}, rnd: fakeRand()}, true},
-		{"withCoreProps", &Writer{p: newPackage(), w: zip.NewWriter(&bytes.Buffer{}), Properties: CoreProperties{Title: "Song"}, rnd: fakeRand()}, false},
+		{"invalidContentType", &Writer{p: pC, w: zip.NewWriter(&bytes.Buffer{})}, true},
+		{"withCt", &Writer{p: p, w: zip.NewWriter(&bytes.Buffer{})}, false},
+		{"invalidPartRel", &Writer{p: newPackage(), w: zip.NewWriter(&bytes.Buffer{}), last: &Part{Name: "/b.xml", Relationships: []*Relationship{{}}}}, true},
+		{"invalidOwnRel", &Writer{p: newPackage(), w: zip.NewWriter(&bytes.Buffer{}), Relationships: []*Relationship{{}}}, true},
+		{"withDuplicatedCoreProps", &Writer{p: pCore, w: zip.NewWriter(&bytes.Buffer{}), Properties: CoreProperties{Title: "Song"}}, true},
+		{"withDuplicatedRels", &Writer{p: pRel, w: zip.NewWriter(&bytes.Buffer{}), Properties: CoreProperties{Title: "Song"}}, true},
+		{"withCoreProps", &Writer{p: newPackage(), w: zip.NewWriter(&bytes.Buffer{}), Properties: CoreProperties{Title: "Song"}}, false},
 		{"withCorePropsWithName", &Writer{p: newPackage(), w: zip.NewWriter(&bytes.Buffer{}), Relationships: []*Relationship{
 			{TargetURI: "props.xml", Type: corePropsRel},
-		}, Properties: CoreProperties{Title: "Song", PartName: "props.xml"}, rnd: fakeRand()}, false},
-		{"withCorePropsWithNameAndId", &Writer{p: newPackage(), w: zip.NewWriter(&bytes.Buffer{}), Properties: CoreProperties{Title: "Song", PartName: "/docProps/props.xml", RelationshipID: "rId1"}, rnd: fakeRand()}, false},
+		}, Properties: CoreProperties{Title: "Song", PartName: "props.xml"}}, false},
+		{"withCorePropsWithNameAndId", &Writer{p: newPackage(), w: zip.NewWriter(&bytes.Buffer{}), Properties: CoreProperties{Title: "Song", PartName: "/docProps/props.xml", RelationshipID: "rId1"}}, false},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -154,8 +149,8 @@ func TestWriter_CreatePart(t *testing.T) {
 	}{
 		{"fhErr", NewWriter(&bytes.Buffer{}), args{&Part{"/a.xml", "a/b", nil}, -3}, true},
 		{"nameErr", NewWriter(&bytes.Buffer{}), args{&Part{"a.xml", "a/b", nil}, CompressionNone}, true},
-		{"failRel", &Writer{w: zip.NewWriter(nil), last: &Part{Name: "/b.xml", Relationships: []*Relationship{{}}}, rnd: fakeRand()}, args{&Part{"/a.xml", "a/b", nil}, CompressionNone}, true},
-		{"failRel2", &Writer{p: pRel, w: zip.NewWriter(nil), last: &Part{Name: "/a.xml", Relationships: []*Relationship{rel}}, rnd: fakeRand()}, args{&Part{"/b.xml", "a/b", nil}, CompressionNone}, true},
+		{"failRel", &Writer{w: zip.NewWriter(nil), last: &Part{Name: "/b.xml", Relationships: []*Relationship{{}}}}, args{&Part{"/a.xml", "a/b", nil}, CompressionNone}, true},
+		{"failRel2", &Writer{p: pRel, w: zip.NewWriter(nil), last: &Part{Name: "/a.xml", Relationships: []*Relationship{rel}}}, args{&Part{"/b.xml", "a/b", nil}, CompressionNone}, true},
 		{"base", w, args{&Part{"/a.xml", "a/b", nil}, CompressionNone}, false},
 		{"multipleDiffName", w, args{&Part{"/b.xml", "a/b", nil}, CompressionNone}, false},
 		{"multipleDiffContentType", w, args{&Part{"/c.xml", "c/d", nil}, CompressionNone}, false},
@@ -184,11 +179,11 @@ func TestWriter_createLastPartRelationships(t *testing.T) {
 		w       *Writer
 		wantErr bool
 	}{
-		{"base", &Writer{p: newPackage(), w: zip.NewWriter(nil), last: &Part{Name: "/a.xml", Relationships: []*Relationship{rel}}, rnd: fakeRand()}, false},
-		{"base2", &Writer{p: newPackage(), w: zip.NewWriter(nil), last: &Part{Name: "/b/a.xml", Relationships: []*Relationship{rel}}, rnd: fakeRand()}, false},
+		{"base", &Writer{p: newPackage(), w: zip.NewWriter(nil), last: &Part{Name: "/a.xml", Relationships: []*Relationship{rel}}}, false},
+		{"base2", &Writer{p: newPackage(), w: zip.NewWriter(nil), last: &Part{Name: "/b/a.xml", Relationships: []*Relationship{rel}}}, false},
 		{"hasSome", w, false},
-		{"duplicated", &Writer{w: zip.NewWriter(nil), last: &Part{Name: "/a.xml", Relationships: []*Relationship{rel, rel}}, rnd: fakeRand()}, true},
-		{"invalidRelation", &Writer{w: zip.NewWriter(nil), last: &Part{Name: "/a.xml", Relationships: []*Relationship{{}}}, rnd: fakeRand()}, true},
+		{"duplicated", &Writer{w: zip.NewWriter(nil), last: &Part{Name: "/a.xml", Relationships: []*Relationship{rel, rel}}}, true},
+		{"invalidRelation", &Writer{w: zip.NewWriter(nil), last: &Part{Name: "/a.xml", Relationships: []*Relationship{{}}}}, true},
 		{"empty", NewWriter(&bytes.Buffer{}), false},
 	}
 	for _, tt := range tests {