feat(pinning): pinning existing CID with different name updates pin

Author: hacdias

CHANGELOG.md

@@ -16,7 +16,7 @@ The following emojis are used to highlight certain changes:
 
 ### Added
 
-* 🛠 `pinning/pinner`: you can now give a custom name when pinning a CID. To reflect this, the `Pinner` has been adjusted.
+* 🛠 `pinning/pinner`: you can now give a custom name when pinning a CID. To reflect this, the `Pinner` has been adjusted. Note that calling `Pin` for the same CID with a different name will replace its current name by the newly given name.
 
 ### Changed
 

pinning/pinner/dspinner/pin.go

@@ -197,8 +197,15 @@ func (p *pinner) doPinRecursive(ctx context.Context, c cid.Cid, fetch bool, name
 	if err != nil {
 		return err
 	}
+	// Do not return immediately! Just remove the recursive pins for the current CID.
+	// This allows the process to continue and the pin to be re-added with a new name.
+	//
+	// TODO: remove this to support multiple pins per CID
 	if found {
-		return nil
+		_, err = p.removePinsForCid(ctx, c, ipfspinner.Recursive)
+		if err != nil {
+			return err
+		}
 	}
 
 	dirtyBefore := p.dirty
@@ -222,7 +229,7 @@ func (p *pinner) doPinRecursive(ctx context.Context, c cid.Cid, fetch bool, name
 
 	// Only look again if something has changed.
 	if p.dirty != dirtyBefore {
-		found, err = p.cidRIndex.HasAny(ctx, cidKey)
+		found, err := p.cidRIndex.HasAny(ctx, cidKey)
 		if err != nil {
 			return err
 		}
@@ -264,6 +271,22 @@ func (p *pinner) doPinDirect(ctx context.Context, c cid.Cid, name string) error
 		return fmt.Errorf("%s already pinned recursively", c.String())
 	}
 
+	// Remove existing direct pins for this CID. This ensures that the pin will be
+	// re-saved with the new name and that there aren't clashing pins for the same
+	// CID.
+	//
+	// TODO: remove this to support multiple pins per CID.
+	found, err = p.cidDIndex.HasAny(ctx, cidKey)
+	if err != nil {
+		return err
+	}
+	if found {
+		_, err = p.removePinsForCid(ctx, c, ipfspinner.Direct)
+		if err != nil {
+			return err
+		}
+	}
+
 	_, err = p.addPin(ctx, c, ipfspinner.Direct, name)
 	if err != nil {
 		return err

pinning/pinner/dspinner/pin_test.go

@@ -99,6 +99,16 @@ func assertUnpinned(t *testing.T, p ipfspin.Pinner, c cid.Cid, failmsg string) {
 	}
 }
 
+func allPins(t *testing.T, ch <-chan ipfspin.StreamedPin) (pins []ipfspin.Pinned) {
+	for val := range ch {
+		if val.Err != nil {
+			t.Fatal(val.Err)
+		}
+		pins = append(pins, val.Pin)
+	}
+	return pins
+}
+
 func TestPinnerBasic(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
@@ -201,17 +211,7 @@ func TestPinnerBasic(t *testing.T) {
 	dk := d.Cid()
 	assertPinned(t, p, dk, "pinned node not found.")
 
-	allPins := func(ch <-chan ipfspin.StreamedPin) (pins []ipfspin.Pinned) {
-		for val := range ch {
-			if val.Err != nil {
-				t.Fatal(val.Err)
-			}
-			pins = append(pins, val.Pin)
-		}
-		return pins
-	}
-
-	pins := allPins(p.RecursiveKeys(ctx, true))
+	pins := allPins(t, p.RecursiveKeys(ctx, true))
 	if len(pins) != 2 {
 		t.Error("expected 2 recursive pins")
 	}
@@ -256,15 +256,15 @@ func TestPinnerBasic(t *testing.T) {
 		}
 	}
 
-	pins = allPins(p.DirectKeys(ctx, false))
+	pins = allPins(t, p.DirectKeys(ctx, false))
 	if len(pins) != 1 {
 		t.Error("expected 1 direct pin")
 	}
 	if pins[0].Key != ak {
 		t.Error("wrong direct pin")
 	}
 
-	pins = allPins(p.InternalPins(ctx, false))
+	pins = allPins(t, p.InternalPins(ctx, false))
 	if len(pins) != 0 {
 		t.Error("should not have internal keys")
 	}
@@ -385,6 +385,56 @@ func TestAddLoadPin(t *testing.T) {
 	}
 }
 
+func TestPinAddOverwriteName(t *testing.T) {
+	makeTest := func(recursive bool) func(t *testing.T) {
+		return func(t *testing.T) {
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+			dstore := dssync.MutexWrap(ds.NewMapDatastore())
+			bstore := blockstore.NewBlockstore(dstore)
+			bserv := bs.New(bstore, offline.Exchange(bstore))
+
+			dserv := mdag.NewDAGService(bserv)
+
+			p, err := New(ctx, dstore, dserv)
+			require.NoError(t, err)
+
+			a, aCid := randNode()
+			err = dserv.Add(ctx, a)
+			require.NoError(t, err)
+
+			var (
+				getPins func(ctx context.Context, detailed bool) <-chan ipfspin.StreamedPin
+				mode    ipfspin.Mode
+			)
+
+			if recursive {
+				getPins = p.RecursiveKeys
+				mode = ipfspin.Recursive
+			} else {
+				getPins = p.DirectKeys
+				mode = ipfspin.Direct
+			}
+
+			for _, name := range []string{"", "pin label", "yet another pin label"} {
+				err = p.Pin(ctx, a, recursive, name)
+				require.NoError(t, err)
+
+				err = p.Flush(ctx)
+				require.NoError(t, err)
+				pins := allPins(t, getPins(ctx, true))
+				require.Len(t, pins, 1)
+				require.Equal(t, aCid, pins[0].Key)
+				require.Equal(t, mode, pins[0].Mode)
+				require.Equal(t, name, pins[0].Name)
+			}
+		}
+	}
+
+	t.Run("Direct", makeTest(false))
+	t.Run("Recursive", makeTest(true))
+}
+
 func TestIsPinnedLookup(t *testing.T) {
 	// Test that lookups work in pins which share
 	// the same branches.  For that construct this tree:

pinning/pinner/pin.go

@@ -94,7 +94,8 @@ type Pinner interface {
 
 	// Pin the given node, optionally recursively.
 	// Pin will make sure that the given node and its children if recursive is set
-	// are stored locally.
+	// are stored locally. Pinning with a different name for an already existing
+	// pin must replace the existing name.
 	Pin(ctx context.Context, node ipld.Node, recursive bool, name string) error
 
 	// Unpin the given cid. If recursive is true, removes either a recursive or