Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
27 changes: 25 additions & 2 deletions pinning/pinner/dspinner/pin.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,15 @@
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
}

Check warning on line 208 in pinning/pinner/dspinner/pin.go

View check run for this annotation

Codecov / codecov/patch

pinning/pinner/dspinner/pin.go#L207-L208

Added lines #L207 - L208 were not covered by tests
}

dirtyBefore := p.dirty
Expand All @@ -222,7 +229,7 @@

// 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)

Check warning on line 232 in pinning/pinner/dspinner/pin.go

View check run for this annotation

Codecov / codecov/patch

pinning/pinner/dspinner/pin.go#L232

Added line #L232 was not covered by tests
if err != nil {
return err
}
Expand Down Expand Up @@ -264,6 +271,22 @@
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
}

Check warning on line 282 in pinning/pinner/dspinner/pin.go

View check run for this annotation

Codecov / codecov/patch

pinning/pinner/dspinner/pin.go#L281-L282

Added lines #L281 - L282 were not covered by tests
if found {
_, err = p.removePinsForCid(ctx, c, ipfspinner.Direct)
if err != nil {
return err
}

Check warning on line 287 in pinning/pinner/dspinner/pin.go

View check run for this annotation

Codecov / codecov/patch

pinning/pinner/dspinner/pin.go#L286-L287

Added lines #L286 - L287 were not covered by tests
}

_, err = p.addPin(ctx, c, ipfspinner.Direct, name)
if err != nil {
return err
Expand Down
76 changes: 63 additions & 13 deletions pinning/pinner/dspinner/pin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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:
Expand Down
3 changes: 2 additions & 1 deletion pinning/pinner/pin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down