From a7c7d36917036f29f59327c6b09228a49753e14c Mon Sep 17 00:00:00 2001 From: Dustin Long Date: Thu, 6 Feb 2020 11:01:46 -0500 Subject: [PATCH] fix(dscache): A few dscache improvements Allow nilable dscache. Require context for constructor. Minor style cleanups. --- cmd/save_test.go | 16 ++++++++++++---- cmd/test_runner_test.go | 10 ---------- dscache/dscache.go | 20 +++++++++++++++----- dscache/dscache_test.go | 34 +++++++++++++++++++++++++++++++--- lib/lib.go | 6 +++--- repo/buildrepo/build.go | 6 +++--- repo/fs/fs_test.go | 4 +++- repo/mem_repo.go | 5 ++++- repo/repo.go | 6 +++--- 9 files changed, 74 insertions(+), 33 deletions(-) diff --git a/cmd/save_test.go b/cmd/save_test.go index 670161414..11c456933 100644 --- a/cmd/save_test.go +++ b/cmd/save_test.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "strings" "testing" "time" @@ -250,15 +251,21 @@ func TestSaveDscache(t *testing.T) { logbook.NewTimestamp = prevTimestampFunc }() - // Save a dataset with an inferred name. + // Save a dataset with one version. run.MustExec(t, "qri save --body testdata/movies/body_two.json me/movie_ds") // List with the --use-dscache flag, which builds the dscache from the logbook. run.MustExec(t, "qri list --use-dscache") + // Access the dscache + repo, err := run.RepoRoot.Repo() + if err != nil { + t.Fatal(err) + } + cache := repo.Dscache() + // Dscache should have one reference. It has topIndex 1 because there are two logbook // elements in the branch, one for "init", one for "commit". - cache := run.Dscache(t) actual := cache.VerboseString(false) expect := `Dscache: Dscache.Users: @@ -285,10 +292,11 @@ func TestSaveDscache(t *testing.T) { // the dscache is not reloaded. Manually reload it here by constructing a dscache from the // same filename. fs := localfs.NewFS() - cacheFilename := run.Dscache(t).Filename + cacheFilename := cache.Filename + ctx := context.Background() + cache = dscache.NewDscache(ctx, fs, cacheFilename) // Dscache should now have one reference. Now topIndex is 2 because there is another "commit". - cache = dscache.NewDscache(fs, cacheFilename) actual = cache.VerboseString(false) // TODO(dlong): bodySize, bodyRows, commitTime should all be filled in expect = `Dscache: diff --git a/cmd/test_runner_test.go b/cmd/test_runner_test.go index 5d680f03c..025119931 100644 --- a/cmd/test_runner_test.go +++ b/cmd/test_runner_test.go @@ -12,7 +12,6 @@ import ( "github.com/qri-io/ioes" "github.com/qri-io/qri/base/dsfs" - "github.com/qri-io/qri/dscache" "github.com/qri-io/qri/lib" "github.com/qri-io/qri/registry" "github.com/qri-io/qri/registry/regserver" @@ -169,15 +168,6 @@ func (run *TestRunner) DatasetMarshalJSON(t *testing.T, ref string) (data string return data } -// Dscache returns the dscache from the repo -func (run *TestRunner) Dscache(t *testing.T) *dscache.Dscache { - repo, err := run.RepoRoot.Repo() - if err != nil { - t.Fatal(err) - } - return repo.Dscache() -} - // MustExec runs a command, returning standard output, failing the test if there's an error func (run *TestRunner) MustExec(t *testing.T, cmdText string) string { if err := run.ExecCommand(cmdText); err != nil { diff --git a/dscache/dscache.go b/dscache/dscache.go index b454aec53..dbe6d771e 100644 --- a/dscache/dscache.go +++ b/dscache/dscache.go @@ -18,6 +18,8 @@ import ( var ( log = golog.Logger("dscache") + // ErrNoDscache is returned when methods are called on a non-existant Dscache + ErrNoDscache = fmt.Errorf("dscache: does not exist") ) // Dscache represents an in-memory serialized dscache flatbuffer @@ -28,10 +30,9 @@ type Dscache struct { ProfileIDToUsername map[string]string } -// NewDscache will construct a dscache from the given file, or will construct an empty dscache -// that will save to the given filename -func NewDscache(fsys qfs.Filesystem, filename string) *Dscache { - ctx := context.TODO() +// NewDscache will construct a dscache from the given filename, or will construct an empty dscache +// that will save to the given filename. Using an empty filename will disable loading and saving +func NewDscache(ctx context.Context, fsys qfs.Filesystem, filename string) *Dscache { f, err := fsys.Get(ctx, filename) if err != nil { // Ignore error, as dscache loading is optional @@ -49,11 +50,17 @@ func NewDscache(fsys qfs.Filesystem, filename string) *Dscache { // IsEmpty returns whether the dscache has any constructed data in it func (d *Dscache) IsEmpty() bool { + if d == nil { + return true + } return d.Root == nil } // Assign assigns the data from one dscache to this one func (d *Dscache) Assign(other *Dscache) error { + if d == nil { + return ErrNoDscache + } d.Root = other.Root d.Buffer = other.Buffer return d.save() @@ -116,6 +123,9 @@ func (d *Dscache) VerboseString(showEmpty bool) string { // ListRefs returns references to each dataset in the cache // TODO(dlong): Not alphabetized, which lib assumes it is func (d *Dscache) ListRefs() ([]reporef.DatasetRef, error) { + if d.IsEmpty() { + return nil, ErrNoDscache + } d.ensureProToUserMap() refs := make([]reporef.DatasetRef, 0, d.Root.RefsLength()) for i := 0; i < d.Root.RefsLength(); i++ { @@ -153,7 +163,7 @@ func (d *Dscache) ListRefs() ([]reporef.DatasetRef, error) { // Update modifies the dscache according to the provided action. func (d *Dscache) Update(act *logbook.Action) error { if d.IsEmpty() { - return fmt.Errorf("dscache: cannot Update an empty dscache") + return ErrNoDscache } if act.Type != logbook.ActionMoveCursor { diff --git a/dscache/dscache_test.go b/dscache/dscache_test.go index f48e4f3b2..8c6c80a0a 100644 --- a/dscache/dscache_test.go +++ b/dscache/dscache_test.go @@ -1,5 +1,33 @@ package dscache -// TODO(dlong): Test LoadDscacheFromFile -// TODO(dlong): Test SaveTo -// TODO(dlong): Test ListRefs +import ( + "strings" + "testing" + + "github.com/qri-io/qri/logbook" +) + +// TODO(dlong): Test NewDscache, IsEmpty, Assign, ListRefs, Update + +func TestNilCallable(t *testing.T) { + var ( + cache *Dscache + err error + ) + + if !cache.IsEmpty() { + t.Errorf("expected IsEmpty: got !IsEmpty") + } + if err = cache.Assign(&Dscache{}); err != ErrNoDscache { + t.Errorf("expected '%s': got '%s'", ErrNoDscache, err) + } + if str := cache.VerboseString(true); !strings.Contains(str, "empty dscache") { + t.Errorf("expected str to Contain 'empty dscache': got '%s'", str) + } + if _, err = cache.ListRefs(); err != ErrNoDscache { + t.Errorf("expected '%s': got '%s'", ErrNoDscache, err) + } + if err = cache.Update(&logbook.Action{}); err != ErrNoDscache { + t.Errorf("expected '%s': got '%s'", ErrNoDscache, err) + } +} diff --git a/lib/lib.go b/lib/lib.go index b1819bac1..94b88bb70 100644 --- a/lib/lib.go +++ b/lib/lib.go @@ -364,7 +364,7 @@ func NewInstance(ctx context.Context, repoPath string, opts ...Option) (qri *Ins } if inst.dscache == nil { - inst.dscache, err = newDscache(inst.qfs, cfg, inst.repoPath) + inst.dscache, err = newDscache(ctx, inst.qfs, cfg, inst.repoPath) if err != nil { return nil, fmt.Errorf("newDsache: %w", err) } @@ -474,9 +474,9 @@ func newLogbook(fs qfs.Filesystem, cfg *config.Config, repoPath string) (book *l return logbook.NewJournal(pro.PrivKey, pro.Peername, fs, logbookPath) } -func newDscache(fs qfs.Filesystem, cfg *config.Config, repoPath string) (*dscache.Dscache, error) { +func newDscache(ctx context.Context, fs qfs.Filesystem, cfg *config.Config, repoPath string) (*dscache.Dscache, error) { dscachePath := filepath.Join(repoPath, "dscache.qfb") - dscache := dscache.NewDscache(fs, dscachePath) + dscache := dscache.NewDscache(ctx, fs, dscachePath) return dscache, nil } diff --git a/repo/buildrepo/build.go b/repo/buildrepo/build.go index 5548cc94d..1d3ba8e2c 100644 --- a/repo/buildrepo/build.go +++ b/repo/buildrepo/build.go @@ -71,7 +71,7 @@ func New(ctx context.Context, path string, cfg *config.Config) (repo.Repo, error return nil, err } - cache, err := newDscache(fs, path) + cache, err := newDscache(ctx, fs, path) if err != nil { return nil, err } @@ -153,11 +153,11 @@ func newLogbook(fs qfs.Filesystem, pro *profile.Profile, repoPath string) (book return logbook.NewJournal(pro.PrivKey, pro.Peername, fs, logbookPath) } -func newDscache(fs qfs.Filesystem, repoPath string) (*dscache.Dscache, error) { +func newDscache(ctx context.Context, fs qfs.Filesystem, repoPath string) (*dscache.Dscache, error) { // This seems to be a bug, the repoPath does not end in "qri" in some tests. if !strings.HasSuffix(repoPath, "qri") { repoPath = repoPath + "/qri" } dscachePath := filepath.Join(repoPath, "dscache.qfb") - return dscache.NewDscache(fs, dscachePath), nil + return dscache.NewDscache(ctx, fs, dscachePath), nil } diff --git a/repo/fs/fs_test.go b/repo/fs/fs_test.go index e4276a43a..98196ec23 100644 --- a/repo/fs/fs_test.go +++ b/repo/fs/fs_test.go @@ -1,6 +1,7 @@ package fsrepo import ( + "context" "os" "path/filepath" "testing" @@ -34,7 +35,8 @@ func TestRepo(t *testing.T) { t.Fatal(err) } - cache := dscache.NewDscache(fs, "") + ctx := context.Background() + cache := dscache.NewDscache(ctx, fs, "") store := cafs.NewMapstore() r, err := NewRepo(store, fs, book, cache, pro, path) diff --git a/repo/mem_repo.go b/repo/mem_repo.go index 1db6749a3..70e3a93b8 100644 --- a/repo/mem_repo.go +++ b/repo/mem_repo.go @@ -1,6 +1,8 @@ package repo import ( + "context" + crypto "github.com/libp2p/go-libp2p-core/crypto" "github.com/qri-io/dataset/dsgraph" "github.com/qri-io/qfs" @@ -33,13 +35,14 @@ func NewMemRepo(p *profile.Profile, store cafs.Filestore, fsys qfs.Filesystem, p if err != nil { return nil, err } + ctx := context.Background() return &MemRepo{ store: store, filesystem: fsys, MemRefstore: &MemRefstore{}, refCache: &MemRefstore{}, logbook: book, - dscache: dscache.NewDscache(fsys, ""), + dscache: dscache.NewDscache(ctx, fsys, ""), profile: p, profiles: ps, }, nil diff --git a/repo/repo.go b/repo/repo.go index eeaf025d0..d342870b7 100644 --- a/repo/repo.go +++ b/repo/repo.go @@ -65,14 +65,14 @@ type Repo interface { Filesystem() qfs.Filesystem // All Repos must keep a Refstore, defining a store of known datasets + // NOTE(dlong): Refstore is going away soon, everything is going to move to Dscache Refstore + // Dscache is a cache of datasets that have been built according to logbook + Dscache() *dscache.Dscache // Repos have a logbook for recording & storing operation logs Logbook() *logbook.Book - // Dscache is a cache of datasets that have been built according to logbook - Dscache() *dscache.Dscache - // A repository must maintain profile information about the owner of this dataset. // The value returned by Profile() should represent the peer. Profile() (*profile.Profile, error)