api, jsonmanifest, manifest: getter for jsonmanifest entries (#503)

make jsonmanifest thread-safe, less exposed and have it implement manifest interfaces in its signatures

pkg/api/bzz.go

@@ -120,9 +120,9 @@ func (s *server) bzzDownloadHandler(w http.ResponseWriter, r *http.Request) {
 
 	var additionalHeaders http.Header
 
-	// copy headers from manifest
-	if me.Headers() != nil {
-		additionalHeaders = me.Headers().Clone()
+	// copy header from manifest
+	if me.Header() != nil {
+		additionalHeaders = me.Header().Clone()
 	} else {
 		additionalHeaders = http.Header{}
 	}

pkg/api/dirs.go

@@ -123,9 +123,9 @@ func storeDir(ctx context.Context, reader io.ReadCloser, s storage.Storer, logge
 		dirManifest.Add(filePath, fileEntry)
 	}
 
-	// check if files were uploaded through the manifest
-	if len(dirManifest.Entries) == 0 {
-		return swarm.ZeroAddress, fmt.Errorf("no files in tar")
+	// check if files were uploaded by querying manifest length
+	if dirManifest.Length() == 0 {
+		return swarm.ZeroAddress, fmt.Errorf("no files added from tar")
 	}
 
 	// upload manifest

pkg/api/dirs_test.go

@@ -76,14 +76,14 @@ func TestDirs(t *testing.T) {
 	}{
 		{
 			name:         "non-nested files without extension",
-			expectedHash: "2fa041bd35ebff676727eb3023272f43b1e0fa71c8735cc1a7487e9131f963c4",
+			expectedHash: "3609d0521d34469ecbffc1d2401ce7a34c7c54bb63e8d23933ef0073015aa9e7",
 			files: []f{
 				{
 					data:      []byte("first file data"),
 					name:      "file1",
 					dir:       "",
 					reference: swarm.MustParseHexAddress("3c07cd2cf5c46208d69d554b038f4dce203f53ac02cb8a313a0fe1e3fe6cc3cf"),
-					headers: http.Header{
+					header: http.Header{
 						"Content-Type": {""},
 					},
 				},
@@ -92,7 +92,7 @@ func TestDirs(t *testing.T) {
 					name:      "file2",
 					dir:       "",
 					reference: swarm.MustParseHexAddress("47e1a2a8f16e02da187fac791d57e6794f3e9b5d2400edd00235da749ad36683"),
-					headers: http.Header{
+					header: http.Header{
 						"Content-Type": {""},
 					},
 				},
@@ -100,14 +100,14 @@ func TestDirs(t *testing.T) {
 		},
 		{
 			name:         "nested files with extension",
-			expectedHash: "c3cb9fbe2efa7bbc979245d9bac1400bd4894371776b7560309d49e687514dd6",
+			expectedHash: "983869d469f0eab1f1bb6c2daeac1fdf476968246410b3001e59e9f2e0236da0",
 			files: []f{
 				{
 					data:      []byte("robots text"),
 					name:      "robots.txt",
 					dir:       "",
 					reference: swarm.MustParseHexAddress("17b96d0a800edca59aaf7e40c6053f7c4c0fb80dd2eb3f8663d51876bf350b12"),
-					headers: http.Header{
+					header: http.Header{
 						"Content-Type": {"text/plain; charset=utf-8"},
 					},
 				},
@@ -116,7 +116,7 @@ func TestDirs(t *testing.T) {
 					name:      "1.png",
 					dir:       "img",
 					reference: swarm.MustParseHexAddress("3c1b3fc640e67f0595d9c1db23f10c7a2b0bdc9843b0e27c53e2ac2a2d6c4674"),
-					headers: http.Header{
+					header: http.Header{
 						"Content-Type": {"image/png"},
 					},
 				},
@@ -125,7 +125,7 @@ func TestDirs(t *testing.T) {
 					name:      "2.png",
 					dir:       "img",
 					reference: swarm.MustParseHexAddress("b234ea7954cab7b2ccc5e07fe8487e932df11b2275db6b55afcbb7bad0be73fb"),
-					headers: http.Header{
+					header: http.Header{
 						"Content-Type": {"image/png"},
 					},
 				},
@@ -146,7 +146,7 @@ func TestDirs(t *testing.T) {
 			// create expected manifest
 			expectedManifest := jsonmanifest.NewManifest()
 			for _, file := range tc.files {
-				e := jsonmanifest.NewEntry(file.reference, file.name, file.headers)
+				e := jsonmanifest.NewEntry(file.reference, file.name, file.header)
 				expectedManifest.Add(path.Join(file.dir, file.name), e)
 			}
 
@@ -198,5 +198,5 @@ type f struct {
 	name      string
 	dir       string
 	reference swarm.Address
-	headers   http.Header
+	header    http.Header
 }

pkg/manifest/jsonmanifest/entry.go

@@ -5,71 +5,42 @@
 package jsonmanifest
 
 import (
-	"encoding/json"
 	"net/http"
 
 	"github.com/ethersphere/bee/pkg/manifest"
 	"github.com/ethersphere/bee/pkg/swarm"
 )
 
-// verify JSONEntry implements manifest.Entry.
-var _ manifest.Entry = (*JSONEntry)(nil)
+// verify jsonEntry implements manifest.Entry.
+var _ manifest.Entry = (*jsonEntry)(nil)
 
-// JSONEntry is a JSON representation of a single manifest entry for a JSONManifest.
-type JSONEntry struct {
-	reference swarm.Address
-	name      string
-	headers   http.Header
+// jsonEntry is a JSON representation of a single manifest entry for a jsonManifest.
+type jsonEntry struct {
+	R swarm.Address `json:"reference"`
+	N string        `json:"name"`
+	H http.Header   `json:"header"`
 }
 
-// NewEntry creates a new JSONEntry struct and returns it.
-func NewEntry(reference swarm.Address, name string, headers http.Header) *JSONEntry {
-	return &JSONEntry{
-		reference: reference,
-		name:      name,
-		headers:   headers,
+// NewEntry creates a new jsonEntry struct and returns it.
+func NewEntry(reference swarm.Address, name string, headers http.Header) manifest.Entry {
+	return &jsonEntry{
+		R: reference,
+		N: name,
+		H: headers,
 	}
 }
 
 // Reference returns the address of the file in the entry.
-func (me *JSONEntry) Reference() swarm.Address {
-	return me.reference
+func (me *jsonEntry) Reference() swarm.Address {
+	return me.R
 }
 
 // Name returns the name of the file in the entry.
-func (me *JSONEntry) Name() string {
-	return me.name
+func (me *jsonEntry) Name() string {
+	return me.N
 }
 
-// Headers returns the headers for the file in the manifest entry.
-func (me *JSONEntry) Headers() http.Header {
-	return me.headers
-}
-
-// exportEntry is a struct used for marshaling and unmarshaling JSONEntry structs.
-type exportEntry struct {
-	Reference swarm.Address `json:"reference"`
-	Name      string        `json:"name"`
-	Headers   http.Header   `json:"headers"`
-}
-
-// MarshalJSON implements the json.Marshaler interface.
-func (me *JSONEntry) MarshalJSON() ([]byte, error) {
-	return json.Marshal(exportEntry{
-		Reference: me.reference,
-		Name:      me.name,
-		Headers:   me.headers,
-	})
-}
-
-// UnmarshalJSON implements the json.Unmarshaler interface.
-func (me *JSONEntry) UnmarshalJSON(b []byte) error {
-	e := exportEntry{}
-	if err := json.Unmarshal(b, &e); err != nil {
-		return err
-	}
-	me.reference = e.Reference
-	me.name = e.Name
-	me.headers = e.Headers
-	return nil
+// Header returns the HTTP header for the file in the manifest entry.
+func (me *jsonEntry) Header() http.Header {
+	return me.H
 }

pkg/manifest/jsonmanifest/jsonmanifest_test.go

+// Copyright 2020 The Swarm Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package jsonmanifest_test
+
+import (
+	"net/http"
+	"path/filepath"
+	"reflect"
+	"testing"
+
+	"github.com/ethersphere/bee/pkg/manifest"
+	"github.com/ethersphere/bee/pkg/manifest/jsonmanifest"
+	"github.com/ethersphere/bee/pkg/swarm"
+	"github.com/ethersphere/bee/pkg/swarm/test"
+)
+
+var testCases = []testCase{
+	{
+		name:    "empty-manifest",
+		entries: nil,
+	},
+	{
+		name: "one-entry",
+		entries: []e{
+			{
+				reference: test.RandomAddress(),
+				path:      "entry-1",
+				header:    http.Header{},
+			},
+		},
+	},
+	{
+		name: "two-entries",
+		entries: []e{
+			{
+				reference: test.RandomAddress(),
+				path:      "entry-1.txt",
+				header:    http.Header{"Content-Type": {"text/plain; charset=utf-8"}},
+			},
+			{
+				reference: test.RandomAddress(),
+				path:      "entry-2.png",
+				header:    http.Header{"Content-Type": {"image/png"}},
+			},
+		},
+	},
+	{
+		name: "nested-entries",
+		entries: []e{
+			{
+				reference: test.RandomAddress(),
+				path:      "text/robots.txt",
+				header:    http.Header{"Content-Type": {"text/plain; charset=utf-8"}},
+			},
+			{
+				reference: test.RandomAddress(),
+				path:      "img/1.png",
+				header:    http.Header{"Content-Type": {"image/png"}},
+			},
+			{
+				reference: test.RandomAddress(),
+				path:      "img/2.jpg",
+				header:    http.Header{"Content-Type": {"image/jpg"}},
+			},
+			{
+				reference: test.RandomAddress(),
+				path:      "readme.md",
+				header:    http.Header{"Content-Type": {"text/markdown; charset=UTF-8"}},
+			},
+		},
+	},
+}
+
+// TestEntries tests the Add, Length and Entry functions.
+// This test will add multiple entries to a manifest, checking that they are correctly retrieved each time,
+// and that the length of the manifest is as expected.
+// It will verify that the manifest length remains unchanged when replacing entries or removing inexistent ones.
+// Finally, it will remove all entries in the manifest, checking that they are correctly not found each time,
+// and that the length of the manifest is as expected.
+func TestEntries(t *testing.T) {
+	tc := testCases[len(testCases)-1] // get non-trivial test case
+
+	m := jsonmanifest.NewManifest()
+	checkLength(t, m, 0)
+
+	// add entries
+	for i, e := range tc.entries {
+		_, name := filepath.Split(e.path)
+		entry := jsonmanifest.NewEntry(e.reference, name, e.header)
+		m.Add(e.path, entry)
+
+		checkLength(t, m, i+1)
+		checkEntry(t, m, entry, e.path)
+	}
+
+	manifestLen := m.Length()
+
+	// replace entry
+	lastEntry := tc.entries[len(tc.entries)-1]
+	_, name := filepath.Split(lastEntry.path)
+
+	newEntry := jsonmanifest.NewEntry(test.RandomAddress(), name, lastEntry.header)
+	m.Add(lastEntry.path, newEntry)
+
+	checkLength(t, m, manifestLen) // length should not have changed
+	checkEntry(t, m, newEntry, lastEntry.path)
+
+	// remove entries
+	m.Remove("invalid/path.ext")   // try removing inexistent entry
+	checkLength(t, m, manifestLen) // length should not have changed
+
+	for i, e := range tc.entries {
+		m.Remove(e.path)
+
+		entry, err := m.Entry(e.path)
+		if entry != nil || err != manifest.ErrNotFound {
+			t.Fatalf("expected path %v not to be present in the manifest, but it was found", e.path)
+		}
+
+		checkLength(t, m, manifestLen-i-1)
+	}
+}
+
+// checkLength verifies that the given manifest length and integer match.
+func checkLength(t *testing.T, m manifest.Interface, length int) {
+	if m.Length() != length {
+		t.Fatalf("expected length to be %d, but is %d instead", length, m.Length())
+	}
+}
+
+// checkEntry verifies that an entry is equal to the one retrieved from the given manifest and path.
+func checkEntry(t *testing.T, m manifest.Interface, entry manifest.Entry, path string) {
+	re, err := m.Entry(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !reflect.DeepEqual(entry, re) {
+		t.Fatalf("original and retrieved entry are not equal: %v, %v", entry, re)
+	}
+}
+
+// TestEntryModification verifies that manifest entries are not modifiable from outside of the manifest.
+// This test will add a single entry to a manifest, retrieve it, and modify it.
+// After, it will re-retrieve that same entry from the manifest, and check that it has not changed.
+func TestEntryModification(t *testing.T) {
+	m := jsonmanifest.NewManifest()
+
+	e := jsonmanifest.NewEntry(test.RandomAddress(), "single_entry.png", http.Header{"Content-Type": {"image/png"}})
+	m.Add("", e)
+
+	re, err := m.Entry("")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	re.Header().Add("Content-Type", "text/plain; charset=utf-8") // modify retrieved entry
+
+	rre, err := m.Entry("") // re-retrieve entry
+	if err != nil {
+		t.Fatal(err)
+	}
+	if reflect.DeepEqual(rre, re) {
+		t.Fatalf("manifest entry %v was unexpectedly modified externally", rre)
+	}
+}
+
+// TestMarshal verifies that created manifests are successfully marshalled and unmarshalled.
+// This function wil add all test case entries to a manifest and marshal it.
+// After, it will unmarshal the result, and verify that it is equal to the original manifest.
+func TestMarshal(t *testing.T) {
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			m := jsonmanifest.NewManifest()
+
+			for _, e := range tc.entries {
+				_, name := filepath.Split(e.path)
+				entry := jsonmanifest.NewEntry(e.reference, name, e.header)
+				m.Add(e.path, entry)
+			}
+
+			b, err := m.MarshalBinary()
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			um := jsonmanifest.NewManifest()
+			if err := um.UnmarshalBinary(b); err != nil {
+				t.Fatal(err)
+			}
+
+			if !reflect.DeepEqual(m, um) {
+				t.Fatalf("marshalled and unmarshalled manifests are not equal: %v, %v", m, um)
+			}
+		})
+	}
+}
+
+// struct for manifest test cases
+type testCase struct {
+	name    string
+	entries []e // entries to add to manifest
+}
+
+// struct for manifest entries for test cases
+type e struct {
+	reference swarm.Address
+	path      string
+	header    http.Header
+}

pkg/manifest/jsonmanifest/manifest.go

@@ -6,51 +6,83 @@ package jsonmanifest
 
 import (
 	"encoding/json"
+	"sync"
 
 	"github.com/ethersphere/bee/pkg/manifest"
 )
 
-// verify JSONManifest implements manifest.Interface.
-var _ manifest.Interface = (*JSONManifest)(nil)
+// verify jsonManifest implements manifest.Interface.
+var _ manifest.Interface = (*jsonManifest)(nil)
 
-// JSONManifest is a JSON representation of a manifest.
+// jsonManifest is a JSON representation of a manifest.
 // It stores manifest entries in a map based on string keys.
-type JSONManifest struct {
-	Entries map[string]*JSONEntry `json:"entries,omitempty"`
+type jsonManifest struct {
+	entriesMu sync.RWMutex          // mutex for accessing the entries map
+	Entries   map[string]*jsonEntry `json:"entries,omitempty"`
 }
 
-// NewManifest creates a new JSONManifest struct and returns a pointer to it.
-func NewManifest() *JSONManifest {
-	return &JSONManifest{
-		Entries: make(map[string]*JSONEntry),
+// NewManifest creates a new jsonManifest struct and returns a pointer to it.
+func NewManifest() manifest.Interface {
+	return &jsonManifest{
+		Entries: make(map[string]*jsonEntry),
 	}
 }
 
 // Add adds a manifest entry to the specified path.
-func (m *JSONManifest) Add(path string, entry manifest.Entry) {
-	m.Entries[path] = NewEntry(entry.Reference(), entry.Name(), entry.Headers())
+func (m *jsonManifest) Add(path string, entry manifest.Entry) {
+	m.entriesMu.Lock()
+	defer m.entriesMu.Unlock()
+
+	m.Entries[path] = &jsonEntry{
+		R: entry.Reference(),
+		N: entry.Name(),
+		H: entry.Header(),
+	}
 }
 
 // Remove removes a manifest entry on the specified path.
-func (m *JSONManifest) Remove(path string) {
+func (m *jsonManifest) Remove(path string) {
+	m.entriesMu.Lock()
+	defer m.entriesMu.Unlock()
+
 	delete(m.Entries, path)
 }
 
 // Entry returns a manifest entry if one is found in the specified path.
-func (m *JSONManifest) Entry(path string) (manifest.Entry, error) {
-	if entry, ok := m.Entries[path]; ok {
-		return entry, nil
+func (m *jsonManifest) Entry(path string) (manifest.Entry, error) {
+	m.entriesMu.RLock()
+	defer m.entriesMu.RUnlock()
+
+	entry, ok := m.Entries[path]
+	if !ok {
+		return nil, manifest.ErrNotFound
 	}
 
-	return nil, manifest.ErrNotFound
+	// return a copy to prevent external modification
+	return NewEntry(entry.Reference(), entry.Name(), entry.Header().Clone()), nil
+}
+
+// Length returns an implementation-specific count of elements in the manifest.
+// For jsonManifest, this means the number of all the existing entries.
+func (m *jsonManifest) Length() int {
+	m.entriesMu.RLock()
+	defer m.entriesMu.RUnlock()
+
+	return len(m.Entries)
 }
 
 // MarshalBinary implements encoding.BinaryMarshaler.
-func (m *JSONManifest) MarshalBinary() (data []byte, err error) {
+func (m *jsonManifest) MarshalBinary() ([]byte, error) {
+	m.entriesMu.RLock()
+	defer m.entriesMu.RUnlock()
+
 	return json.Marshal(m)
 }
 
 // UnmarshalBinary implements encoding.BinaryUnmarshaler.
-func (m *JSONManifest) UnmarshalBinary(data []byte) error {
-	return json.Unmarshal(data, m)
+func (m *jsonManifest) UnmarshalBinary(b []byte) error {
+	m.entriesMu.Lock()
+	defer m.entriesMu.Unlock()
+
+	return json.Unmarshal(b, m)
 }

pkg/manifest/manifest.go

@@ -23,6 +23,8 @@ type Interface interface {
 	Remove(string)
 	// Entry returns a manifest entry if one is found in the specified path.
 	Entry(string) (Entry, error)
+	// Length returns an implementation-specific count of elements in the manifest.
+	Length() int
 	encoding.BinaryMarshaler
 	encoding.BinaryUnmarshaler
 }
@@ -33,6 +35,6 @@ type Entry interface {
 	Reference() swarm.Address
 	// Name returns the name of the file in the entry.
 	Name() string
-	// Headers returns the headers for the file in the manifest entry.
-	Headers() http.Header
+	// Header returns the HTTP header for the file in the manifest entry.
+	Header() http.Header
 }