From b0d0fafd68f526ceed98e59a423b6470f2327a21 Mon Sep 17 00:00:00 2001 From: Lewis Marshall Date: Fri, 12 May 2017 17:02:25 -0700 Subject: [PATCH] swarm/api: fix error reporting in api.Resolve (#14464) Previously, resolve errors were being swallowed and the returned error was a generic "not a content hash" which isn't helpful. This updates the Resolve function to fail fast rather than only returning an error at the end, and also adds test coverage. --- swarm/api/api.go | 47 +++++++------ swarm/api/api_test.go | 120 ++++++++++++++++++++++++++++++++++ swarm/api/http/server_test.go | 6 +- 3 files changed, 150 insertions(+), 23 deletions(-) diff --git a/swarm/api/api.go b/swarm/api/api.go index 26a9445d5..803265a3e 100644 --- a/swarm/api/api.go +++ b/swarm/api/api.go @@ -25,12 +25,13 @@ import ( "sync" "bytes" - "github.com/ethereum/go-ethereum/common" - "github.com/ethereum/go-ethereum/log" - "github.com/ethereum/go-ethereum/swarm/storage" "mime" "path/filepath" "time" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/log" + "github.com/ethereum/go-ethereum/swarm/storage" ) var ( @@ -84,26 +85,32 @@ type ErrResolve error func (self *Api) Resolve(uri *URI) (storage.Key, error) { log.Trace(fmt.Sprintf("Resolving : %v", uri.Addr)) - var err error - if !uri.Immutable() { - if self.dns != nil { - resolved, err := self.dns.Resolve(uri.Addr) - if err == nil { - return resolved[:], nil - } - } else { - err = fmt.Errorf("no DNS to resolve name") + // if the URI is immutable, check if the address is a hash + isHash := hashMatcher.MatchString(uri.Addr) + if uri.Immutable() { + if !isHash { + return nil, fmt.Errorf("immutable address not a content hash: %q", uri.Addr) } + return common.Hex2Bytes(uri.Addr), nil } - if hashMatcher.MatchString(uri.Addr) { - return storage.Key(common.Hex2Bytes(uri.Addr)), nil - } - if err != nil { - return nil, fmt.Errorf("'%s' does not resolve: %v but is not a content hash", uri.Addr, err) - } - return nil, fmt.Errorf("'%s' is not a content hash", uri.Addr) -} + // if DNS is not configured, check if the address is a hash + if self.dns == nil { + if !isHash { + return nil, fmt.Errorf("no DNS to resolve name: %q", uri.Addr) + } + return common.Hex2Bytes(uri.Addr), nil + } + + // try and resolve the address + resolved, err := self.dns.Resolve(uri.Addr) + if err == nil { + return resolved[:], nil + } else if !isHash { + return nil, err + } + return common.Hex2Bytes(uri.Addr), nil +} // Put provides singleton manifest creation on top of dpa store func (self *Api) Put(content, contentType string) (storage.Key, error) { diff --git a/swarm/api/api_test.go b/swarm/api/api_test.go index c2d78c2dc..f9caed27f 100644 --- a/swarm/api/api_test.go +++ b/swarm/api/api_test.go @@ -17,6 +17,7 @@ package api import ( + "errors" "fmt" "io" "io/ioutil" @@ -117,3 +118,122 @@ func TestApiPut(t *testing.T) { checkResponse(t, resp, exp) }) } + +// testResolver implements the Resolver interface and either returns the given +// hash if it is set, or returns a "name not found" error +type testResolver struct { + hash *common.Hash +} + +func newTestResolver(addr string) *testResolver { + r := &testResolver{} + if addr != "" { + hash := common.HexToHash(addr) + r.hash = &hash + } + return r +} + +func (t *testResolver) Resolve(addr string) (common.Hash, error) { + if t.hash == nil { + return common.Hash{}, fmt.Errorf("DNS name not found: %q", addr) + } + return *t.hash, nil +} + +// TestAPIResolve tests resolving URIs which can either contain content hashes +// or ENS names +func TestAPIResolve(t *testing.T) { + ensAddr := "swarm.eth" + hashAddr := "1111111111111111111111111111111111111111111111111111111111111111" + resolvedAddr := "2222222222222222222222222222222222222222222222222222222222222222" + doesResolve := newTestResolver(resolvedAddr) + doesntResolve := newTestResolver("") + + type test struct { + desc string + dns Resolver + addr string + immutable bool + result string + expectErr error + } + + tests := []*test{ + { + desc: "DNS not configured, hash address, returns hash address", + dns: nil, + addr: hashAddr, + result: hashAddr, + }, + { + desc: "DNS not configured, ENS address, returns error", + dns: nil, + addr: ensAddr, + expectErr: errors.New(`no DNS to resolve name: "swarm.eth"`), + }, + { + desc: "DNS configured, hash address, hash resolves, returns resolved address", + dns: doesResolve, + addr: hashAddr, + result: resolvedAddr, + }, + { + desc: "DNS configured, immutable hash address, hash resolves, returns hash address", + dns: doesResolve, + addr: hashAddr, + immutable: true, + result: hashAddr, + }, + { + desc: "DNS configured, hash address, hash doesn't resolve, returns hash address", + dns: doesntResolve, + addr: hashAddr, + result: hashAddr, + }, + { + desc: "DNS configured, ENS address, name resolves, returns resolved address", + dns: doesResolve, + addr: ensAddr, + result: resolvedAddr, + }, + { + desc: "DNS configured, immutable ENS address, name resolves, returns error", + dns: doesResolve, + addr: ensAddr, + immutable: true, + expectErr: errors.New(`immutable address not a content hash: "swarm.eth"`), + }, + { + desc: "DNS configured, ENS address, name doesn't resolve, returns error", + dns: doesntResolve, + addr: ensAddr, + expectErr: errors.New(`DNS name not found: "swarm.eth"`), + }, + } + for _, x := range tests { + t.Run(x.desc, func(t *testing.T) { + api := &Api{dns: x.dns} + uri := &URI{Addr: x.addr, Scheme: "bzz"} + if x.immutable { + uri.Scheme = "bzzi" + } + res, err := api.Resolve(uri) + if err == nil { + if x.expectErr != nil { + t.Fatalf("expected error %q, got result %q", x.expectErr, res) + } + if res.String() != x.result { + t.Fatalf("expected result %q, got %q", x.result, res) + } + } else { + if x.expectErr == nil { + t.Fatalf("expected no error, got %q", err) + } + if err.Error() != x.expectErr.Error() { + t.Fatalf("expected error %q, got %q", x.expectErr, err) + } + } + }) + } +} diff --git a/swarm/api/http/server_test.go b/swarm/api/http/server_test.go index ceb8db75b..0b124a19a 100644 --- a/swarm/api/http/server_test.go +++ b/swarm/api/http/server_test.go @@ -106,9 +106,9 @@ func TestBzzrGetPath(t *testing.T) { } nonhashresponses := []string{ - "error resolving name: 'name' does not resolve: no DNS to resolve name but is not a content hash\n", - "error resolving nonhash: 'nonhash' is not a content hash\n", - "error resolving nonhash: 'nonhash' does not resolve: no DNS to resolve name but is not a content hash\n", + "error resolving name: no DNS to resolve name: \"name\"\n", + "error resolving nonhash: immutable address not a content hash: \"nonhash\"\n", + "error resolving nonhash: no DNS to resolve name: \"nonhash\"\n", } for i, url := range nonhashtests {