Fix concurrent R/W access to link properties (#87)

Author: chocolatkey

pkg/fetcher/fetcher_archive.go

@@ -36,16 +36,6 @@ func (f *ArchiveFetcher) Links() (manifest.LinkList, error) {
 				link.Type = mt.String()
 			}
 		}
-		cl := af.CompressedLength()
-		if cl == 0 {
-			cl = af.Length()
-		}
-		link.Properties.Add(manifest.Properties{
-			"https://readium.org/webpub-manifest/properties#archive": manifest.Properties{
-				"entryLength":       cl,
-				"isEntryCompressed": af.CompressedLength() > 0,
-			},
-		})
 		links = append(links, link)
 	}
 	return links, nil
@@ -57,10 +47,25 @@ func (f *ArchiveFetcher) Get(link manifest.Link) Resource {
 	if err != nil {
 		return NewFailureResource(link, NotFound(err))
 	}
-	return &entryResource{
+
+	// Compute archive properties
+	cl := entry.CompressedLength()
+	if cl == 0 {
+		cl = entry.Length()
+	}
+
+	er := &entryResource{
 		link:  link,
 		entry: entry,
+		properties: manifest.Properties{
+			"https://readium.org/webpub-manifest/properties#archive": map[string]interface{}{
+				"entryLength":       cl,
+				"isEntryCompressed": entry.CompressedLength() > 0,
+			},
+		},
 	}
+
+	return er
 }
 
 // Close implements Fetcher
@@ -90,8 +95,9 @@ func NewArchiveFetcherFromPathWithFactory(path string, factory archive.ArchiveFa
 
 // Resource from archive entry
 type entryResource struct {
-	link  manifest.Link
-	entry archive.Entry
+	link       manifest.Link
+	entry      archive.Entry
+	properties manifest.Properties
 }
 
 // File implements Resource
@@ -104,21 +110,16 @@ func (r *entryResource) Close() {
 	// Nothing needs to be done at the moment
 }
 
+// Link implements Resource
 func (r *entryResource) Link() manifest.Link {
-	cl := r.entry.CompressedLength()
-	if cl == 0 {
-		cl = r.entry.Length()
-	}
-	r.link.Properties.Add(manifest.Properties{
-		"https://readium.org/webpub-manifest/properties#archive": manifest.Properties{
-			"entryLength":       cl,
-			"isEntryCompressed": r.entry.CompressedLength() > 0,
-		},
-	})
-
 	return r.link
 }
 
+// Properties implements Resource
+func (r *entryResource) Properties() manifest.Properties {
+	return r.properties
+}
+
 // Read implements Resource
 func (r *entryResource) Read(start int64, end int64) ([]byte, *ResourceError) {
 	data, err := r.entry.Read(start, end)

pkg/fetcher/fetcher_archive_test.go

@@ -15,20 +15,30 @@ func withArchiveFetcher(t *testing.T, callback func(a *ArchiveFetcher)) {
 }
 
 func TestArchiveFetcherLinks(t *testing.T) {
-	makeTestLink := func(href string, typ string, entryLength uint64, isCompressed bool) manifest.Link {
-		return manifest.Link{
+	makeTestLink := func(href string, typ string, entryLength uint64, isCompressed bool) struct {
+		manifest.Link
+		manifest.Properties
+	} {
+		l := manifest.Link{
 			Href: href,
 			Type: typ,
-			Properties: manifest.Properties{
-				"https://readium.org/webpub-manifest/properties#archive": manifest.Properties{
-					"entryLength":       entryLength,
-					"isEntryCompressed": isCompressed,
-				},
+		}
+		p := manifest.Properties{
+			"https://readium.org/webpub-manifest/properties#archive": map[string]interface{}{
+				"entryLength":       entryLength,
+				"isEntryCompressed": isCompressed,
 			},
 		}
+		return struct {
+			manifest.Link
+			manifest.Properties
+		}{l, p}
 	}
 
-	mustContain := manifest.LinkList{
+	mustContain := []struct {
+		manifest.Link
+		manifest.Properties
+	}{
 		makeTestLink("/mimetype", "", 20, false),
 		makeTestLink("/EPUB/cover.xhtml", "application/xhtml+xml", 259, true),
 		makeTestLink("/EPUB/css/epub.css", "text/css", 595, true),
@@ -45,7 +55,12 @@ func TestArchiveFetcherLinks(t *testing.T) {
 		links, err := a.Links()
 		assert.Nil(t, err)
 
-		assert.ElementsMatch(t, mustContain, links)
+		mustLinks := make([]manifest.Link, len(mustContain))
+		for i, l := range mustContain {
+			assert.Equal(t, l.Properties, a.Get(l.Link).Properties())
+			mustLinks[i] = l.Link
+		}
+		assert.ElementsMatch(t, mustLinks, links)
 	})
 }
 
@@ -128,25 +143,10 @@ func TestArchiveFetcherAddsProperties(t *testing.T) {
 	withArchiveFetcher(t, func(a *ArchiveFetcher) {
 		resource := a.Get(manifest.Link{Href: "/EPUB/css/epub.css"})
 		assert.Equal(t, manifest.Properties{
-			"https://readium.org/webpub-manifest/properties#archive": manifest.Properties{
-				"entryLength":       uint64(595),
-				"isEntryCompressed": true,
-			},
-		}, resource.Link().Properties)
-	})
-}
-
-func TestArchiveFetcherOriginalPropertiesKept(t *testing.T) {
-	withArchiveFetcher(t, func(a *ArchiveFetcher) {
-		resource := a.Get(manifest.Link{Href: "/EPUB/css/epub.css", Properties: manifest.Properties{
-			"other": "property",
-		}})
-		assert.Equal(t, manifest.Properties{
-			"other": "property",
-			"https://readium.org/webpub-manifest/properties#archive": manifest.Properties{
+			"https://readium.org/webpub-manifest/properties#archive": map[string]interface{}{
 				"entryLength":       uint64(595),
 				"isEntryCompressed": true,
 			},
-		}, resource.Link().Properties)
+		}, resource.Properties())
 	})
 }

pkg/fetcher/fetcher_file.go

@@ -128,6 +128,10 @@ func (r *FileResource) Link() manifest.Link {
 	return r.link
 }
 
+func (r *FileResource) Properties() manifest.Properties {
+	return manifest.Properties{}
+}
+
 // Close implements Resource
 func (r *FileResource) Close() {
 	if r.file != nil {

pkg/fetcher/resource.go

@@ -38,6 +38,10 @@ type Resource interface {
 	// It might be modified by the [Resource] to include additional metadata, e.g. the `Content-Type` HTTP header in [Link.Type].
 	Link() manifest.Link
 
+	// Returns the properties associated with the resource.
+	// This is opened for extensions.
+	Properties() manifest.Properties
+
 	// Returns data length from metadata if available, or calculated from reading the bytes otherwise.
 	// This value must be treated as a hint, as it might not reflect the actual bytes length. To get the real length, you need to read the whole resource.
 	Length() (int64, *ResourceError)
@@ -265,6 +269,10 @@ func (r FailureResource) Link() manifest.Link {
 	return r.link
 }
 
+func (r FailureResource) Properties() manifest.Properties {
+	return manifest.Properties{}
+}
+
 // Length implements Resource
 func (r FailureResource) Length() (int64, *ResourceError) {
 	return 0, r.ex
@@ -323,6 +331,10 @@ func (r ProxyResource) Link() manifest.Link {
 	return r.Res.Link()
 }
 
+func (r ProxyResource) Properties() manifest.Properties {
+	return manifest.Properties{}
+}
+
 // Length implements Resource
 func (r ProxyResource) Length() (int64, *ResourceError) {
 	return r.Res.Length()

pkg/fetcher/resource_bytes.go

@@ -29,6 +29,11 @@ func (r *BytesResource) Link() manifest.Link {
 	return r.link
 }
 
+// Properties implements Resource
+func (r *BytesResource) Properties() manifest.Properties {
+	return manifest.Properties{}
+}
+
 // Length implements Resource
 func (r *BytesResource) Length() (int64, *ResourceError) {
 	bin, err := r.Read(0, 0)

pkg/manifest/link.go

@@ -181,6 +181,48 @@ func (l *Link) UnmarshalJSON(b []byte) error {
 	return nil
 }
 
+func (l Link) MarshalJSON() ([]byte, error) {
+	res := make(map[string]interface{})
+	res["href"] = l.Href
+	if l.Type != "" {
+		res["type"] = l.Type
+	}
+	if l.Templated {
+		res["templated"] = l.Templated
+	}
+	if l.Title != "" {
+		res["title"] = l.Title
+	}
+	if len(l.Rels) > 0 {
+		res["rel"] = l.Rels
+	}
+	if len(l.Properties) > 0 {
+		res["properties"] = l.Properties
+	}
+	if l.Height > 0 {
+		res["height"] = l.Height
+	}
+	if l.Width > 0 {
+		res["width"] = l.Width
+	}
+	if l.Bitrate > 0 {
+		res["bitrate"] = l.Bitrate
+	}
+	if l.Duration > 0 {
+		res["duration"] = l.Duration
+	}
+	if len(l.Languages) > 0 {
+		res["language"] = l.Languages
+	}
+	if len(l.Alternates) > 0 {
+		res["alternate"] = l.Alternates
+	}
+	if len(l.Children) > 0 {
+		res["children"] = l.Children
+	}
+	return json.Marshal(res)
+}
+
 // Slice of links
 type LinkList []Link
 

pkg/manifest/link_test.go

@@ -66,17 +66,19 @@ func TestLinkUnmarshalFullJSON(t *testing.T) {
 		]
 	}`), &l))
 	assert.Equal(t, Link{
-		Href:       "http://href",
-		Type:       "application/pdf",
-		Templated:  true,
-		Title:      "Link Title",
-		Rels:       []string{"publication", "cover"},
-		Properties: map[string]interface{}{"orientation": "landscape"},
-		Height:     1024,
-		Width:      768,
-		Bitrate:    74.2,
-		Duration:   45.6,
-		Languages:  []string{"fr"},
+		Href:      "http://href",
+		Type:      "application/pdf",
+		Templated: true,
+		Title:     "Link Title",
+		Rels:      []string{"publication", "cover"},
+		Properties: Properties{
+			"orientation": "landscape",
+		},
+		Height:    1024,
+		Width:     768,
+		Bitrate:   74.2,
+		Duration:  45.6,
+		Languages: []string{"fr"},
 		Alternates: []Link{
 			{Href: "/alternate1"},
 			{Href: "/alternate2"},
@@ -181,17 +183,19 @@ func TestLinkMinimalJSON(t *testing.T) {
 
 func TestLinkFullJSON(t *testing.T) {
 	b, err := json.Marshal(Link{
-		Href:       "http://href",
-		Type:       "application/pdf",
-		Templated:  true,
-		Title:      "Link Title",
-		Rels:       []string{"publication", "cover"},
-		Properties: map[string]interface{}{"orientation": "landscape"},
-		Height:     1024,
-		Width:      768,
-		Bitrate:    74.2,
-		Duration:   45.6,
-		Languages:  []string{"fr"},
+		Href:      "http://href",
+		Type:      "application/pdf",
+		Templated: true,
+		Title:     "Link Title",
+		Rels:      []string{"publication", "cover"},
+		Properties: Properties{
+			"orientation": "landscape",
+		},
+		Height:    1024,
+		Width:     768,
+		Bitrate:   74.2,
+		Duration:  45.6,
+		Languages: []string{"fr"},
 		Alternates: []Link{
 			{Href: "/alternate1"},
 			{Href: "/alternate2"},

pkg/manifest/properties.go

@@ -8,7 +8,10 @@ import (
 
 type Properties map[string]interface{}
 
-func (p *Properties) Add(newProperties Properties) Properties {
+// Properties should be immutable, therefore these functions have been removed.
+// The code is left here in case it's useful in a future implementation.
+
+/*func (p *Properties) Add(newProperties Properties) Properties {
 	if *p == nil {
 		*p = make(Properties)
 	}
@@ -18,6 +21,14 @@ func (p *Properties) Add(newProperties Properties) Properties {
 	return *p
 }
 
+func (p *Properties) Delete(key string) Properties {
+	if p == nil {
+		p = &Properties{}
+	}
+	delete(*p, key)
+	return *p
+}*/
+
 func (p *Properties) Get(key string) interface{} {
 	if p != nil {
 		return (*p)[key]

pkg/manifest/properties_test.go

@@ -32,7 +32,7 @@ func TestPropertiesUnmarshalFullJSON(t *testing.T) {
 	}, p)
 }
 
-func TestPropertiesAddGiven(t *testing.T) {
+/*func TestPropertiesAddGiven(t *testing.T) {
 	p2 := Properties{
 		"other-property1": "value",
 		"other-property2": []interface{}{float64(42)},
@@ -44,7 +44,7 @@ func TestPropertiesAddGiven(t *testing.T) {
 	}, p2.Add(Properties{
 		"additional": "property",
 	}))
-}
+}*/
 
 // Presentation-specific properties
 

pkg/parser/epub/deobfuscator_test.go

@@ -28,11 +28,11 @@ func withDeobfuscator(t *testing.T, href string, algorithm string, start, end in
 		Href: href,
 	}
 	if algorithm != "" {
-		link.Properties.Add(manifest.Properties{
+		link.Properties = manifest.Properties{
 			"encrypted": map[string]interface{}{
 				"algorithm": algorithm,
 			},
-		})
+		}
 	}
 	obfu, err := NewDeobfuscator(identifier).Transform(ft.Get(link)).Read(start, end)
 	if !assert.Nil(t, err) {