Skip to content

Commit

Permalink
Replaced old logic to check for valid path
Browse files Browse the repository at this point in the history
Added the original logic to check for a invalid path and a simple test.
  • Loading branch information
travisperson committed May 21, 2015
1 parent 55c74f1 commit c8cde25
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 2 deletions.
6 changes: 4 additions & 2 deletions core/pathresolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package core
import (
"errors"
"strings"
"fmt"

context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"

Expand All @@ -28,8 +29,9 @@ func Resolve(ctx context.Context, n *IpfsNode, p path.Path) (*merkledag.Node, er
}

seg := p.Segments()
if len(seg) < 2 {
return nil, errors.New("No path given")

if len(seg) < 2 || seg[1] == "" { // just "/<protocol/>" without further segments
return nil, fmt.Errorf("invalid path: %s", string(p))
}

extensions := seg[2:]
Expand Down
41 changes: 41 additions & 0 deletions core/pathresolver_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package core

import (
"testing"

context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"
config "github.com/ipfs/go-ipfs/repo/config"
"github.com/ipfs/go-ipfs/util/testutil"
"github.com/ipfs/go-ipfs/repo"
path "github.com/ipfs/go-ipfs/path"
)

func TestResolveInvalidPath(t *testing.T) {
ctx := context.TODO()

This comment has been minimized.

Copy link
@wking

wking May 21, 2015

In ipfs#1208, @jbenet pointed out that context.Background() is a better choice for tests (so you can grep for TODO to see things that you indend to fix later).

This comment has been minimized.

Copy link
@jbenet

jbenet May 21, 2015

👍

id := testIdentity

r := &repo.Mock{
C: config.Config{
Identity: id,
Datastore: config.Datastore{
Type: "memory",
},
Addresses: config.Addresses{
Swarm: []string{"/ip4/0.0.0.0/tcp/4001"},
API: "/ip4/127.0.0.1/tcp/8000",
},
},
D: testutil.ThreadSafeCloserMapDatastore(),
}

n, err := NewIPFSNode(ctx, Standard(r, false))

This comment has been minimized.

Copy link
@wking

wking May 21, 2015

If we don't have one already, a testutil for creating mock nodes seems useful. But maybe what you need from a mocked node is too inconsistent for that to make sense? In this instance anyway, none of these mocked defaults seem particularly unique to the path-resolver tests.

if n == nil || err != nil {
t.Error("Should have constructed.", err)
}

_, err = Resolve(ctx, n, path.Path("/ipfs/"))
if err == nil {
t.Error("Should get invalid path")

This comment has been minimized.

Copy link
@wking

wking May 21, 2015

Hmm. I'm not sure how error comparisons work, but it might be good to make this test:

if err != expectedError {
    …
}

That might mean making a variable for the error (like ErrResolveRecursion, used here via here).

And I'm not sure we have an institutional preference for Fail vs. FailNow, but the few go-ipfs tests I've looked over so far have prefered the now-failing Fatal.

}

}

0 comments on commit c8cde25

Please sign in to comment.