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

@@ -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,27 +85,33 @@ type ErrResolve error
 func (self *Api) Resolve(uri *URI) (storage.Key, error) {
 	log.Trace(fmt.Sprintf("Resolving : %v", uri.Addr))
 
-	var err error
+	// if the URI is immutable, check if the address is a hash
-	if !uri.Immutable() {
+	isHash := hashMatcher.MatchString(uri.Addr)
-		if self.dns != nil {
+	if uri.Immutable() {
-			resolved, err := self.dns.Resolve(uri.Addr)
+		if !isHash {
-			if err == nil {
+			return nil, fmt.Errorf("immutable address not a content hash: %q", uri.Addr)
-				return resolved[:], nil
-			}
-		} else {
-			err = fmt.Errorf("no DNS to resolve name")
 		}
+		return common.Hex2Bytes(uri.Addr), nil
 	}
-	if hashMatcher.MatchString(uri.Addr) {
+
-		return storage.Key(common.Hex2Bytes(uri.Addr)), nil
+	// 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
 	}
-	if err != nil {
+
-		return nil, fmt.Errorf("'%s' does not resolve: %v but is not a content hash", uri.Addr, err)
+	// 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 nil, fmt.Errorf("'%s' is not a content hash", uri.Addr)
+	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) {
 	r := strings.NewReader(content)

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)
+				}
+			}
+		})
+	}
+}

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 name: no DNS to resolve name: \"name\"\n",
-		"error resolving nonhash: 'nonhash' is not a content hash\n",
+		"error resolving nonhash: immutable address not a content hash: \"nonhash\"\n",
-		"error resolving nonhash: 'nonhash' does not resolve: no DNS to resolve name but is not a content hash\n",
+		"error resolving nonhash: no DNS to resolve name: \"nonhash\"\n",
 	}
 
 	for i, url := range nonhashtests {