Merge pull request #2716 from ipfs/patch-add-link-paths

Fix #2715 and patch add-link paths

blocks/blockstore/blockstore.go

@@ -74,6 +74,10 @@ type blockstore struct {
 }
 
 func (bs *blockstore) Get(k key.Key) (blocks.Block, error) {
+	if k == "" {
+		return nil, ErrNotFound
+	}
+
 	maybeData, err := bs.datastore.Get(k.DsKey())
 	if err == ds.ErrNotFound {
 		return nil, ErrNotFound

blocks/blockstore/blockstore_test.go

@@ -27,6 +27,14 @@ func TestGetWhenKeyNotPresent(t *testing.T) {
 	t.Fail()
 }
 
+func TestGetWhenKeyIsEmptyString(t *testing.T) {
+	bs := NewBlockstore(ds_sync.MutexWrap(ds.NewMapDatastore()))
+	_, err := bs.Get(key.Key(""))
+	if err != ErrNotFound {
+		t.Fail()
+	}
+}
+
 func TestPutThenGetBlock(t *testing.T) {
 	bs := NewBlockstore(ds_sync.MutexWrap(ds.NewMapDatastore()))
 	block := blocks.NewBlock([]byte("some data"))

blockservice/blockservice.go

@@ -72,6 +72,11 @@ func (s *BlockService) AddBlocks(bs []blocks.Block) ([]key.Key, error) {
 // GetBlock retrieves a particular block from the service,
 // Getting it from the datastore using the key (hash).
 func (s *BlockService) GetBlock(ctx context.Context, k key.Key) (blocks.Block, error) {
+	if k == "" {
+		log.Debug("BlockService GetBlock: Nil Key")
+		return nil, ErrNotFound
+	}
+
 	log.Debugf("BlockService GetBlock: '%s'", k)
 	block, err := s.Blockstore.Get(k)
 	if err == nil {

blockservice/test/blocks_test.go

@@ -22,6 +22,11 @@ func TestBlocks(t *testing.T) {
 	bs := New(bstore, offline.Exchange(bstore))
 	defer bs.Close()
 
+	_, err := bs.GetBlock(context.Background(), key.Key(""))
+	if err != ErrNotFound {
+		t.Error("Empty String Key should error", err)
+	}
+
 	b := blocks.NewBlock([]byte("beep boop"))
 	h := u.Hash([]byte("beep boop"))
 	if !bytes.Equal(b.Multihash(), h) {

core/commands/object/patch.go

@@ -5,7 +5,6 @@ import (
 	"io/ioutil"
 	"strings"
 
-	key "github.com/ipfs/go-ipfs/blocks/key"
 	cmds "github.com/ipfs/go-ipfs/commands"
 	core "github.com/ipfs/go-ipfs/core"
 	dag "github.com/ipfs/go-ipfs/merkledag"
@@ -273,8 +272,12 @@ a file containing 'bar', and returns the hash of the new object.
 			return
 		}
 
-		path := req.Arguments()[1]
-		childk := key.B58KeyDecode(req.Arguments()[2])
+		npath := req.Arguments()[1]
+		childp, err := path.ParsePath(req.Arguments()[2])
+		if err != nil {
+			res.SetError(err, cmds.ErrNormal)
+			return
+		}
 
 		create, _, err := req.Option("create").Bool()
 		if err != nil {
@@ -291,13 +294,13 @@ a file containing 'bar', and returns the hash of the new object.
 
 		e := dagutils.NewDagEditor(root, nd.DAG)
 
-		childnd, err := nd.DAG.Get(req.Context(), childk)
+		childnd, err := core.Resolve(req.Context(), nd, childp)
 		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
 			return
 		}
 
-		err = e.InsertNodeAtPath(req.Context(), path, childnd, createfunc)
+		err = e.InsertNodeAtPath(req.Context(), npath, childnd, createfunc)
 		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
 			return

exchange/bitswap/bitswap.go

@@ -155,6 +155,9 @@ type blockRequest struct {
 // GetBlock attempts to retrieve a particular block from peers within the
 // deadline enforced by the context.
 func (bs *Bitswap) GetBlock(parent context.Context, k key.Key) (blocks.Block, error) {
+	if k == "" {
+		return nil, blockstore.ErrNotFound
+	}
 
 	// Any async work initiated by this function must end when this function
 	// returns. To ensure this, derive a new context. Note that it is okay to

exchange/bitswap/bitswap_test.go

@@ -11,6 +11,7 @@ import (
 	context "gx/ipfs/QmZy2y8t9zQH2a1b8q2ZSLKp17ATuJoCNxxyMFG5qFExpt/go-net/context"
 
 	blocks "github.com/ipfs/go-ipfs/blocks"
+	blockstore "github.com/ipfs/go-ipfs/blocks/blockstore"
 	blocksutil "github.com/ipfs/go-ipfs/blocks/blocksutil"
 	key "github.com/ipfs/go-ipfs/blocks/key"
 	tn "github.com/ipfs/go-ipfs/exchange/bitswap/testnet"
@@ -278,6 +279,18 @@ func TestSendToWantingPeer(t *testing.T) {
 
 }
 
+func TestEmptyKey(t *testing.T) {
+	net := tn.VirtualNetwork(mockrouting.NewServer(), delay.Fixed(kNetworkDelay))
+	sg := NewTestSessionGenerator(net)
+	defer sg.Close()
+	bs := sg.Instances(1)[0].Exchange
+
+	_, err := bs.GetBlock(context.Background(), key.Key(""))
+	if err != blockstore.ErrNotFound {
+		t.Error("empty str key should return ErrNotFound")
+	}
+}
+
 func TestBasicBitswap(t *testing.T) {
 	net := tn.VirtualNetwork(mockrouting.NewServer(), delay.Fixed(kNetworkDelay))
 	sg := NewTestSessionGenerator(net)

merkledag/merkledag.go

@@ -68,6 +68,9 @@ func (n *dagService) Batch() *Batch {
 
 // Get retrieves a node from the dagService, fetching the block in the BlockService
 func (n *dagService) Get(ctx context.Context, k key.Key) (*Node, error) {
+	if k == "" {
+		return nil, ErrNotFound
+	}
 	if n == nil {
 		return nil, fmt.Errorf("dagService is nil")
 	}

merkledag/merkledag_test.go

@@ -245,6 +245,14 @@ func assertCanGet(t *testing.T, ds DAGService, n *Node) {
 	}
 }
 
+func TestEmptyKey(t *testing.T) {
+	ds := dstest.Mock()
+	_, err := ds.Get(context.Background(), key.Key(""))
+	if err != ErrNotFound {
+		t.Error("dag service should error when key is nil", err)
+	}
+}
+
 func TestCantGet(t *testing.T) {
 	dsp := getDagservAndPinner(t)
 	a := &Node{Data: []byte("A")}

test/sharness/t0051-object.sh

@@ -180,6 +180,22 @@ test_object_cmd() {
 		ipfs object stat $OUTPUT
 	'
 
+	test_expect_success "'ipfs object patch add-link' should work with paths" '
+		EMPTY_DIR=$(ipfs object new unixfs-dir) &&
+		N1=$(ipfs object patch $EMPTY_DIR add-link baz $EMPTY_DIR) &&
+		N2=$(ipfs object patch $EMPTY_DIR add-link bar $N1) &&
+		N3=$(ipfs object patch $EMPTY_DIR add-link foo /ipfs/$N2/bar) &&
+		ipfs object stat /ipfs/$N3 > /dev/null &&
+		ipfs object stat $N3/foo > /dev/null &&
+		ipfs object stat /ipfs/$N3/foo/baz > /dev/null
+	'
+
+	test_expect_success "object patch creation looks right" '
+		echo "QmPc73aWK9dgFBXe86P4PvQizHo9e5Qt7n7DAMXWuigFuG" > hash_exp &&
+		echo $N3 > hash_actual &&
+		test_cmp hash_exp hash_actual
+	'
+
 	test_expect_success "multilayer ipfs patch works" '
 		echo "hello world" > hwfile &&
 		FILE=$(ipfs add -q hwfile) &&