Skip to content

swarm/api: bug fix exact match for manifest #15329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
karalabe merged 1 commit into from
Nov 24, 2017
Merged

swarm/api: bug fix exact match for manifest #15329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions swarm/api/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,16 @@ func (self *manifestTrie) findPrefixOf(path string, quitC chan bool) (entry *man
if len(path) <= epl {
if entry.Path[:len(path)] == path {
if entry.ContentType == ManifestType {
err := self.loadSubTrie(entry, quitC)
if err == nil && entry.subtrie != nil {
subentries := entry.subtrie.entries
Copy link
Copy Markdown
Contributor

@nolash nolash Nov 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadSubTrie() returns no error, but probably it should? The underlying errors that would bubble up would be from readManifest() Have it been evaluated whether they are relevant for this case?

Copy link
Copy Markdown
Contributor Author

@holisticode holisticode Nov 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?
func (self *manifestTrie) loadSubTrie(entry *manifestTrieEntry, quitC chan bool) (err error) {
looks like it does return an error to me?

Copy link
Copy Markdown
Contributor

@nolash nolash Nov 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant the error is always nil the way it is now

Copy link
Copy Markdown
Contributor Author

@holisticode holisticode Nov 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure I understand:

361 func (self *manifestTrie) loadSubTrie(entry *manifestTrieEntry, quitC chan bool) (err error) {
362   if entry.subtrie == nil {
363     hash := common.Hex2Bytes(entry.Hash)
364     entry.subtrie, err = loadManifest(self.dpa, hash, quitC)
365     entry.Hash = "" // might not match, should be recalculated
366   }
367   return
368 }

if loadManifest returns an error, it would be assigned to err on line 364?

Copy link
Copy Markdown
Contributor

@nolash nolash Nov 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeez, forget I said anything. I'm a moron. You are right, of course.

for i := 0; i < len(subentries); i++ {
sub := subentries[i]
if sub != nil && sub.Path == "" {
return sub, len(path)
}
}
}
entry.Status = http.StatusMultipleChoices
}
pos = len(path)
Expand Down
78 changes: 51 additions & 27 deletions swarm/api/manifest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"encoding/json"
"fmt"
"io"
"net/http"
"strings"
"testing"

Expand All @@ -39,17 +40,17 @@ func manifest(paths ...string) (manifestReader storage.LazySectionReader) {
}
}

func testGetEntry(t *testing.T, path, match string, paths ...string) *manifestTrie {
func testGetEntry(t *testing.T, path, match string, multiple bool, paths ...string) *manifestTrie {
quitC := make(chan bool)
trie, err := readManifest(manifest(paths...), nil, nil, quitC)
if err != nil {
t.Errorf("unexpected error making manifest: %v", err)
}
checkEntry(t, path, match, trie)
checkEntry(t, path, match, multiple, trie)
return trie
}

func checkEntry(t *testing.T, path, match string, trie *manifestTrie) {
func checkEntry(t *testing.T, path, match string, multiple bool, trie *manifestTrie) {
entry, fullpath := trie.getEntry(path)
if match == "-" && entry != nil {
t.Errorf("expected no match for '%s', got '%s'", path, fullpath)
Expand All @@ -60,32 +61,55 @@ func checkEntry(t *testing.T, path, match string, trie *manifestTrie) {
} else if fullpath != match {
t.Errorf("incorrect entry retrieved for '%s'. expected path '%v', got '%s'", path, match, fullpath)
}

if multiple && entry.Status != http.StatusMultipleChoices {
Copy link
Copy Markdown
Member

@nonsense nonsense Nov 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to test the else-condition:

else if !multiple && entry.Status == http.StatusMultipleChoices

in order to cover incorrect response status if we didn't expect multiple choices status, but got one.

Copy link
Copy Markdown
Contributor Author

@holisticode holisticode Nov 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added! Thanks for comment

t.Errorf("Expected %d Multiple Choices Status for path %s, match %s, got %d", http.StatusMultipleChoices, path, match, entry.Status)
} else if !multiple && entry != nil && entry.Status == http.StatusMultipleChoices {
t.Errorf("Were not expecting %d Multiple Choices Status for path %s, match %s, but got it", http.StatusMultipleChoices, path, match)
}
}

func TestGetEntry(t *testing.T) {
// file system manifest always contains regularized paths
testGetEntry(t, "a", "a", "a")
testGetEntry(t, "b", "-", "a")
testGetEntry(t, "/a//", "a", "a")
testGetEntry(t, "a", "a", false, "a")
testGetEntry(t, "b", "-", false, "a")
testGetEntry(t, "/a//", "a", false, "a")
// fallback
testGetEntry(t, "/a", "", "")
testGetEntry(t, "/a/b", "a/b", "a/b")
testGetEntry(t, "/a", "", false, "")
testGetEntry(t, "/a/b", "a/b", false, "a/b")
// longest/deepest math
testGetEntry(t, "read", "read", "readme.md", "readit.md")
testGetEntry(t, "rf", "-", "readme.md", "readit.md")
testGetEntry(t, "readme", "readme", "readme.md")
testGetEntry(t, "readme", "-", "readit.md")
testGetEntry(t, "readme.md", "readme.md", "readme.md")
testGetEntry(t, "readme.md", "-", "readit.md")
testGetEntry(t, "readmeAmd", "-", "readit.md")
testGetEntry(t, "readme.mdffff", "-", "readme.md")
testGetEntry(t, "ab", "ab", "ab/cefg", "ab/cedh", "ab/kkkkkk")
testGetEntry(t, "ab/ce", "ab/ce", "ab/cefg", "ab/cedh", "ab/ceuuuuuuuuuu")
testGetEntry(t, "abc", "abc", "abcd", "abczzzzef", "abc/def", "abc/e/g")
testGetEntry(t, "a/b", "a/b", "a", "a/bc", "a/ba", "a/b/c")
testGetEntry(t, "a/b", "a/b", "a", "a/b", "a/bb", "a/b/c")
testGetEntry(t, "//a//b//", "a/b", "a", "a/b", "a/bb", "a/b/c")
testGetEntry(t, "read", "read", true, "readme.md", "readit.md")
testGetEntry(t, "rf", "-", false, "readme.md", "readit.md")
testGetEntry(t, "readme", "readme", false, "readme.md")
testGetEntry(t, "readme", "-", false, "readit.md")
testGetEntry(t, "readme.md", "readme.md", false, "readme.md")
testGetEntry(t, "readme.md", "-", false, "readit.md")
testGetEntry(t, "readmeAmd", "-", false, "readit.md")
testGetEntry(t, "readme.mdffff", "-", false, "readme.md")
testGetEntry(t, "ab", "ab", true, "ab/cefg", "ab/cedh", "ab/kkkkkk")
testGetEntry(t, "ab/ce", "ab/ce", true, "ab/cefg", "ab/cedh", "ab/ceuuuuuuuuuu")
testGetEntry(t, "abc", "abc", true, "abcd", "abczzzzef", "abc/def", "abc/e/g")
testGetEntry(t, "a/b", "a/b", true, "a", "a/bc", "a/ba", "a/b/c")
testGetEntry(t, "a/b", "a/b", false, "a", "a/b", "a/bb", "a/b/c")
testGetEntry(t, "//a//b//", "a/b", false, "a", "a/b", "a/bb", "a/b/c")
}

func TestExactMatch(t *testing.T) {
quitC := make(chan bool)
mf := manifest("shouldBeExactMatch.css", "shouldBeExactMatch.css.map")
trie, err := readManifest(mf, nil, nil, quitC)
Copy link
Copy Markdown
Contributor

@nolash nolash Nov 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the .map one?

Copy link
Copy Markdown
Contributor Author

@holisticode holisticode Nov 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is exactly what caused the error we are trying to fix: if there was a file "shouldBeExactMatch.css" and a file "shouldBeExactMatch.css.map" (.map files are often generated by SASS generators for CSS and are actually used in development only, but that's how we found out: our own webpage would not load...), then if the client requested "shouldBeExactMatch.css", the result would be 300 Multiple Choices, instead of serving the file itself

if err != nil {
t.Errorf("unexpected error making manifest: %v", err)
}
entry, _ := trie.getEntry("shouldBeExactMatch.css")
if entry.Path != "" {
t.Errorf("Expected entry to match %s, got: %s", "shouldBeExactMatch.css", entry.Path)
}
if entry.Status == http.StatusMultipleChoices {
t.Errorf("Got status %d, which is unexepcted", http.StatusMultipleChoices)
}
}

func TestDeleteEntry(t *testing.T) {

}
Expand All @@ -108,15 +132,15 @@ func TestAddFileWithManifestPath(t *testing.T) {
if err != nil {
t.Fatal(err)
}
checkEntry(t, "ab", "ab", trie)
checkEntry(t, "ac", "ac", trie)
checkEntry(t, "ab", "ab", false, trie)
checkEntry(t, "ac", "ac", false, trie)

// now add path "a" and check we can still get "ab" and "ac"
entry := &manifestTrieEntry{}
entry.Path = "a"
entry.Hash = "a"
trie.addEntry(entry, nil)
checkEntry(t, "ab", "ab", trie)
checkEntry(t, "ac", "ac", trie)
checkEntry(t, "a", "a", trie)
checkEntry(t, "ab", "ab", false, trie)
checkEntry(t, "ac", "ac", false, trie)
checkEntry(t, "a", "a", false, trie)
}