fix(remove): When removing foreign dataset, remove the log

base/remove.go

@@ -9,7 +9,6 @@ import (
 	"github.com/qri-io/qri/base/dsfs"
 	"github.com/qri-io/qri/dsref"
 	"github.com/qri-io/qri/logbook"
-	"github.com/qri-io/qri/logbook/oplog"
 	"github.com/qri-io/qri/repo"
 	"github.com/qri-io/qri/repo/profile"
 	reporef "github.com/qri-io/qri/repo/ref"
@@ -42,15 +41,22 @@ func RemoveEntireDataset(ctx context.Context, r repo.Repo, ref dsref.Ref, histor
 		log.Debugf("Remove, logbook.RefToInitID failed, error: %s", err)
 		removeErr = err
 	}
-	if err := book.WriteDatasetDelete(ctx, initID); err == nil {
-		didRemove = appendString(didRemove, "logbook")
-	} else {
-		// If the logbook is missing, it's not an error worth stopping for, since we're
-		// deleting the dataset anyway. This can happen from adding a foreign dataset.
-		if err != oplog.ErrNotFound {
+	if ref.Username == book.AuthorName() {
+		// TOOD(dustmop): Logbook should validate the fact that author's should only be able to
+		// write to their own logs. Trying to write to another user's log should throw an error.
+		if err := book.WriteDatasetDelete(ctx, initID); err == nil {
+			didRemove = appendString(didRemove, "logbook")
+		} else {
 			log.Debugf("Remove, logbook.WriteDatasetDelete failed, error: %s", err)
 			removeErr = err
 		}
+	} else {
+		if err := book.RemoveAnyoneLog(ctx, ref); err == nil {
+			didRemove = appendString(didRemove, "logbook")
+		} else {
+			log.Debugf("Remove, logbook.RemoveLog failed, error: %s", err)
+			removeErr = err
+		}
 	}
 	// remove the ref from the ref store
 	datasetRef := reporef.DatasetRef{

cmd/remove_integration_test.go

@@ -785,18 +785,37 @@ func TestRemoveEvenIfLogbookGone(t *testing.T) {
 	}
 }
 
-// Test that an added dataset can be removed
+// Test that an added dataset can be removed, which removes it from the logbook
 func TestRemoveEvenIfForeignDataset(t *testing.T) {
 	run := NewTestRunnerWithMockRemoteClient(t, "test_peer", "remove_foreign")
 	defer run.Delete()
 
+	actual := run.MustExec(t, "qri logbook --raw")
+	expectEmpty := `[{"ops":[{"type":"init","model":"user","name":"test_peer","authorID":"QmeL2mdVka1eahKENjehK6tBxkkpk5dNQ1qMcgWi7Hrb4B","timestamp":"1969-12-31T19:00:00.978310861-05:00"}]}]`
+	if diff := cmp.Diff(expectEmpty, actual); diff != "" {
+		t.Errorf("unexpected (-want +got):\n%s", diff)
+	}
+
 	// Save a foreign dataset
 	run.MustExec(t, "qri add other_peer/their_dataset")
 
+	actual = run.MustExec(t, "qri logbook --raw")
+	expectHasForiegn := `[{"ops":[{"type":"init","model":"user","name":"test_peer","authorID":"QmeL2mdVka1eahKENjehK6tBxkkpk5dNQ1qMcgWi7Hrb4B","timestamp":"1969-12-31T19:00:00.978310861-05:00"}]},{"ops":[{"type":"init","model":"user","name":"other_peer","authorID":"QmWYgD49r9HnuXEppQEq1a7SUUryja4QNs9E6XCH2PayCD","timestamp":"1969-12-31T19:00:00.978310921-05:00"}],"logs":[{"ops":[{"type":"init","model":"dataset","name":"their_dataset","authorID":"xstfcrqf26suws6dnjih4ugvmfk6w5o7e6b7rmflt7aso6htyufa","timestamp":"1969-12-31T19:00:00.978310981-05:00"}],"logs":[{"ops":[{"type":"init","model":"branch","name":"main","authorID":"xstfcrqf26suws6dnjih4ugvmfk6w5o7e6b7rmflt7aso6htyufa","timestamp":"1969-12-31T19:00:00.978311041-05:00"},{"type":"init","model":"commit","ref":"QmExample","timestamp":"1969-12-31T19:00:00.978311101-05:00","note":"their commit"}]}]}]}]`
+	if diff := cmp.Diff(expectHasForiegn, actual); diff != "" {
+		t.Errorf("unexpected (-want +got):\n%s", diff)
+	}
+
 	// Remove all should still work, even though the dataset is foreign
 	if err := run.ExecCommand("qri remove --revisions=all other_peer/their_dataset"); err != nil {
 		t.Error(err)
 	}
+
+	actual = run.MustExec(t, "qri logbook --raw")
+	// Log is removed for the database, but author init still remains
+	expectEmptyAuthor := `[{"ops":[{"type":"init","model":"user","name":"test_peer","authorID":"QmeL2mdVka1eahKENjehK6tBxkkpk5dNQ1qMcgWi7Hrb4B","timestamp":"1969-12-31T19:00:00.978310861-05:00"}]},{"ops":[{"type":"init","model":"user","name":"other_peer","authorID":"QmWYgD49r9HnuXEppQEq1a7SUUryja4QNs9E6XCH2PayCD","timestamp":"1969-12-31T19:00:00.978310921-05:00"}]}]`
+	if diff := cmp.Diff(expectEmptyAuthor, actual); diff != "" {
+		t.Errorf("unexpected (-want +got):\n%s", diff)
+	}
 }
 
 // Test that an added dataset can be removed even if the logbook is missing

lib/datasets.go

@@ -1063,6 +1063,9 @@ func (m *DatasetMethods) Add(p *AddParams, res *reporef.DatasetRef) error {
 	if p.LogsOnly {
 		return mergeLogsError
 	}
+	if mergeLogsError != nil {
+		return mergeLogsError
+	}
 
 	rref := reporef.RefFromDsref(ref)
 	if err = m.inst.remoteClient.AddDataset(ctx, &rref, p.RemoteAddr); err != nil {

lib/lib.go

@@ -443,7 +443,7 @@ func NewInstance(ctx context.Context, repoPath string, opts ...Option) (qri *Ins
 	key := InstanceContextKey("RemoteClient")
 	if v := ctx.Value(key); v != nil && v == "mock" && inst.node != nil {
 		inst.node.LocalStreams = o.Streams
-		if inst.remoteClient, err = remote.NewMockClient(inst.node); err != nil {
+		if inst.remoteClient, err = remote.NewMockClient(inst.node, inst.logbook); err != nil {
 			return
 		}
 	} else if inst.node != nil {

logbook/logbook.go

@@ -861,6 +861,15 @@ func (book *Book) RemoveLog(ctx context.Context, sender identity.Author, ref dsr
 	return book.save(ctx)
 }
 
+// RemoveAnyoneLog removes a log owned by anyone.
+func (book *Book) RemoveAnyoneLog(ctx context.Context, ref dsref.Ref) error {
+	if book == nil {
+		return ErrNoLogbook
+	}
+	book.store.RemoveLog(ctx, dsRefToLogPath(ref)...)
+	return book.save(ctx)
+}
+
 func dsRefToLogPath(ref dsref.Ref) (path []string) {
 	for _, str := range []string{
 		ref.Username,

remote/mock_client.go

@@ -3,11 +3,17 @@ package remote
 import (
 	"context"
 	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
 
 	"github.com/qri-io/dataset"
+	"github.com/qri-io/qfs"
 	"github.com/qri-io/qri/base/dsfs"
 	cfgtest "github.com/qri-io/qri/config/test"
 	"github.com/qri-io/qri/dsref"
+	"github.com/qri-io/qri/identity"
+	"github.com/qri-io/qri/logbook"
 	"github.com/qri-io/qri/logbook/oplog"
 	"github.com/qri-io/qri/p2p"
 	"github.com/qri-io/qri/repo/profile"
@@ -20,11 +26,12 @@ var ErrNotImplemented = fmt.Errorf("not implemented")
 // MockClient is a remote client suitable for tests
 type MockClient struct {
 	node *p2p.QriNode
+	book *logbook.Book
 }
 
 // NewMockClient returns a mock remote client
-func NewMockClient(node *p2p.QriNode) (c Client, err error) {
-	return &MockClient{node: node}, nil
+func NewMockClient(node *p2p.QriNode, book *logbook.Book) (c Client, err error) {
+	return &MockClient{node: node, book: book}, nil
 }
 
 // ListDatasets is not implemented
@@ -44,7 +51,39 @@ func (c *MockClient) FetchLogs(ctx context.Context, ref dsref.Ref, remoteAddr st
 
 // CloneLogs is not implemented
 func (c *MockClient) CloneLogs(ctx context.Context, ref dsref.Ref, remoteAddr string) error {
-	return ErrNotImplemented
+	tmpdir, err := ioutil.TempDir("", "")
+	if err != nil {
+		log.Fatal(err)
+	}
+	defer os.RemoveAll(tmpdir)
+
+	theirBookPath := filepath.Join(tmpdir, "their_logbook.qfs")
+
+	otherUsername := ref.Username
+	otherPeerInfo := cfgtest.GetTestPeerInfo(1)
+	sender := identity.NewAuthor("abc", otherPeerInfo.PubKey)
+
+	fs := qfs.NewMemFS()
+
+	foreignBuilder := logbook.NewLogbookTempBuilder(nil, otherPeerInfo.PrivKey, otherUsername, fs, theirBookPath)
+
+	initID := foreignBuilder.DatasetInit(ctx, nil, ref.Name)
+	// NOTE: Need to assign ProfileID because nothing is resolving the username
+	ref.ProfileID = otherPeerInfo.EncodedPeerID
+	ref.Path = "QmExample"
+	foreignBuilder.Commit(ctx, nil, initID, "their commit", ref.Path)
+	foreignBook := foreignBuilder.Logbook()
+	foreignLog, err := foreignBook.UserDatasetRef(ctx, ref)
+	if err != nil {
+		panic(err)
+	}
+
+	err = foreignBook.SignLog(foreignLog)
+	if err != nil {
+		panic(err)
+	}
+
+	return c.book.MergeLog(ctx, sender, foreignLog)
 }
 
 // RemoveDataset is not implemented