From 0ddafe603a3ae788f22d933687018fe13f1d903f Mon Sep 17 00:00:00 2001 From: jbenet Date: Mon, 16 May 2016 22:39:39 -0700 Subject: [PATCH 1/4] add error checking for nil keys Checks in: - blockstore - blockservice - dagservice - bitswap Do not anger the pokemans #2715 License: MIT Signed-off-by: Juan Benet --- blocks/blockstore/blockstore.go | 4 ++++ blocks/blockstore/blockstore_test.go | 8 ++++++++ blockservice/blockservice.go | 5 +++++ blockservice/test/blocks_test.go | 5 +++++ exchange/bitswap/bitswap.go | 3 +++ exchange/bitswap/bitswap_test.go | 13 +++++++++++++ merkledag/merkledag.go | 3 +++ merkledag/merkledag_test.go | 15 +++++++++++++++ 8 files changed, 56 insertions(+) diff --git a/blocks/blockstore/blockstore.go b/blocks/blockstore/blockstore.go index 671ae2c25..d3a9b1aa1 100644 --- a/blocks/blockstore/blockstore.go +++ b/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 diff --git a/blocks/blockstore/blockstore_test.go b/blocks/blockstore/blockstore_test.go index 446d4b776..8b0609f1f 100644 --- a/blocks/blockstore/blockstore_test.go +++ b/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")) diff --git a/blockservice/blockservice.go b/blockservice/blockservice.go index 78838757a..945f60ae6 100644 --- a/blockservice/blockservice.go +++ b/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 { diff --git a/blockservice/test/blocks_test.go b/blockservice/test/blocks_test.go index 584505b21..ed61dad59 100644 --- a/blockservice/test/blocks_test.go +++ b/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) { diff --git a/exchange/bitswap/bitswap.go b/exchange/bitswap/bitswap.go index 9f5c92d04..0afed265e 100644 --- a/exchange/bitswap/bitswap.go +++ b/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 diff --git a/exchange/bitswap/bitswap_test.go b/exchange/bitswap/bitswap_test.go index 024de3389..136fa85d2 100644 --- a/exchange/bitswap/bitswap_test.go +++ b/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) diff --git a/merkledag/merkledag.go b/merkledag/merkledag.go index 938b71308..b98fdafe6 100644 --- a/merkledag/merkledag.go +++ b/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") } diff --git a/merkledag/merkledag_test.go b/merkledag/merkledag_test.go index e475fa680..2739e2195 100644 --- a/merkledag/merkledag_test.go +++ b/merkledag/merkledag_test.go @@ -32,6 +32,13 @@ type dagservAndPinner struct { mp pin.Pinner } +func getDagserv(t *testing.T) DAGService { + db := dssync.MutexWrap(ds.NewMapDatastore()) + bs := bstore.NewBlockstore(db) + blockserv := bserv.New(bs, offline.Exchange(bs)) + return NewDAGService(blockserv) +} + func getDagservAndPinner(t *testing.T) dagservAndPinner { db := dssync.MutexWrap(ds.NewMapDatastore()) bs := bstore.NewBlockstore(db) @@ -245,6 +252,14 @@ func assertCanGet(t *testing.T, ds DAGService, n *Node) { } } +func TestEmptyKey(t *testing.T) { + ds := getDagserv(t) + _, 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")} From 9f79dde058955585924698a183620615bafbf371 Mon Sep 17 00:00:00 2001 From: jbenet Date: Mon, 16 May 2016 22:56:46 -0700 Subject: [PATCH 2/4] ipfs object patch add-link supports paths previously, paths were not supported as link values. this would not work, and now can: ipfs object patch $root add-link foo /ipfs/$hash/foo/bar License: MIT Signed-off-by: Juan Benet --- core/commands/object/patch.go | 13 ++++++++----- test/sharness/t0051-object.sh | 10 ++++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/core/commands/object/patch.go b/core/commands/object/patch.go index a0a635d20..f100f1862 100644 --- a/core/commands/object/patch.go +++ b/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 diff --git a/test/sharness/t0051-object.sh b/test/sharness/t0051-object.sh index 0506235b2..9be1a063e 100755 --- a/test/sharness/t0051-object.sh +++ b/test/sharness/t0051-object.sh @@ -180,6 +180,16 @@ 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 && + ipfs object stat $N3/foo && + ipfs object stat /ipfs/$N3/foo/baz + ' + test_expect_success "multilayer ipfs patch works" ' echo "hello world" > hwfile && FILE=$(ipfs add -q hwfile) && From 562c801976e788e091eacc41ac980f34be2d9bb8 Mon Sep 17 00:00:00 2001 From: Juan Benet Date: Tue, 17 May 2016 00:22:19 -0700 Subject: [PATCH 3/4] address CR - use dstest.Mock License: MIT Signed-off-by: Juan Benet --- merkledag/merkledag_test.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/merkledag/merkledag_test.go b/merkledag/merkledag_test.go index 2739e2195..430e31f7a 100644 --- a/merkledag/merkledag_test.go +++ b/merkledag/merkledag_test.go @@ -32,13 +32,6 @@ type dagservAndPinner struct { mp pin.Pinner } -func getDagserv(t *testing.T) DAGService { - db := dssync.MutexWrap(ds.NewMapDatastore()) - bs := bstore.NewBlockstore(db) - blockserv := bserv.New(bs, offline.Exchange(bs)) - return NewDAGService(blockserv) -} - func getDagservAndPinner(t *testing.T) dagservAndPinner { db := dssync.MutexWrap(ds.NewMapDatastore()) bs := bstore.NewBlockstore(db) @@ -253,7 +246,7 @@ func assertCanGet(t *testing.T, ds DAGService, n *Node) { } func TestEmptyKey(t *testing.T) { - ds := getDagserv(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) From 60cae2079a8e9155db788e440eff706aee87b064 Mon Sep 17 00:00:00 2001 From: Jeromy Date: Tue, 17 May 2016 10:49:13 -0700 Subject: [PATCH 4/4] check output hash for correctness License: MIT Signed-off-by: Jeromy --- test/sharness/t0051-object.sh | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/test/sharness/t0051-object.sh b/test/sharness/t0051-object.sh index 9be1a063e..c47bf90f2 100755 --- a/test/sharness/t0051-object.sh +++ b/test/sharness/t0051-object.sh @@ -185,9 +185,15 @@ test_object_cmd() { 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 && - ipfs object stat $N3/foo && - ipfs object stat /ipfs/$N3/foo/baz + 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" '