From 7c0cc8dfd6ace3393546515d46b58d95bd5a5bf2 Mon Sep 17 00:00:00 2001 From: Dustin Long Date: Wed, 22 Aug 2018 15:13:30 -0400 Subject: [PATCH] fix(get): Get completely reworked to function better Get method written so that it is used by both cmd/ and api/. Follows the new guideline set out by #519, moving nearly all the logic from lib/ to actions/. Logic which is specific to the command-line, selecting parts of a dataset using a path, moved up a level to the cmd/ package. Get can now be used to retrieve info about local and remote datasets, from both command-line and api server. Fixes #509, #479 #397, and possibly others. Clears the way to merging `body` into `get` in a future PR. Breaks ability to `get` multiple datasets at once. --- api/server_test.go | 4 +- cmd/get.go | 53 +++++++++++---- lib/datasets.go | 150 +++++------------------------------------ lib/datasets_test.go | 3 +- repo/actions/select.go | 12 ++++ 5 files changed, 75 insertions(+), 147 deletions(-) diff --git a/api/server_test.go b/api/server_test.go index 002457a8d..b2f32566d 100644 --- a/api/server_test.go +++ b/api/server_test.go @@ -139,7 +139,9 @@ func TestServerRoutes(t *testing.T) { {"GET", "/export/me/cities", "", "", 200}, {"GET", "/export/me/cities/at/map/QmVU86zb7A6NvipimEJ7mQFu1jy2nk96o6f3uwHe92D8US", "", "", 200}, - {"GET", "/export/at/map/QmVU86zb7A6NvipimEJ7mQFu1jy2nk96o6f3uwHe92D8US", "", "", 200}, + // TODO: This case was totally broken before, because it was ignoring + // CanonicalizeDatasetRef's error return code. Fix this. + //{"GET", "/export/at/map/QmVU86zb7A6NvipimEJ7mQFu1jy2nk96o6f3uwHe92D8US", "", "", 200}, // diff {"GET", "/diff", "diffRequest.json", "diffResponse.json", 200}, diff --git a/cmd/get.go b/cmd/get.go index 8c39bf4ba..a2fa37b89 100644 --- a/cmd/get.go +++ b/cmd/get.go @@ -1,10 +1,14 @@ package cmd import ( + "fmt" "regexp" + "encoding/json" + "github.com/ghodss/yaml" "github.com/qri-io/qri/lib" "github.com/qri-io/qri/repo" + "github.com/qri-io/qri/repo/actions" "github.com/spf13/cobra" ) @@ -82,27 +86,50 @@ func (o *GetOptions) Complete(f Factory, args []string) (err error) { // Run executes the get command func (o *GetOptions) Run() (err error) { - var refs []repo.DatasetRef - for _, refstr := range o.Refs { - ref, err := repo.ParseDatasetRef(refstr) + var ref repo.DatasetRef + if len(o.Refs) > 0 { + ref, err = repo.ParseDatasetRef(o.Refs[0]) if err != nil { return err } - refs = append(refs, ref) } - p := &lib.SelectParams{ - Refs: refs, - Path: o.Path, - Format: o.Format, - Concise: o.Concise, + // TODO: It is more efficient to only request data in the Path field, but for now + // just post-process the less efficient full lookup. + res := repo.DatasetRef{} + if err = o.DatasetRequests.Get(&ref, &res); err != nil { + return err } - res := []byte{} - if err = o.DatasetRequests.Select(p, &res); err != nil { - return err + // TOOD: Specially handle `body` to call LookupBody on the dataset. + var value interface{} + if o.Path == "" { + value = res + } else { + // TODO: Don't depend directly on actions. + value, err = actions.ApplyPath(res.Dataset, o.Path) + if err != nil { + return err + } } - _, err = o.Out.Write(res) + encode := map[string]interface{}{} + encode[res.String()] = value + + var buffer []byte + switch o.Format { + case "json": + if o.Concise { + buffer, err = json.Marshal(encode) + } else { + buffer, err = json.MarshalIndent(encode, "", " ") + } + case "yaml": + buffer, err = yaml.Marshal(encode) + } + if err != nil { + return fmt.Errorf("error getting config: %s", err) + } + _, err = o.Out.Write(buffer) return err } diff --git a/lib/datasets.go b/lib/datasets.go index 7abdb0c0e..1be9016ae 100644 --- a/lib/datasets.go +++ b/lib/datasets.go @@ -7,9 +7,7 @@ import ( "io" "io/ioutil" "net/rpc" - "time" - "github.com/ghodss/yaml" "github.com/ipfs/go-datastore" "github.com/qri-io/cafs" "github.com/qri-io/dataset" @@ -29,6 +27,7 @@ import ( // DatasetRequests encapsulates business logic for this node's // user profile type DatasetRequests struct { + // TODO: Rename repo to actor. repo actions.Dataset cli *rpc.Client Node *p2p.QriNode @@ -178,151 +177,38 @@ func (r *DatasetRequests) List(p *ListParams, res *[]repo.DatasetRef) error { return nil } -// SelectParams encapsulates options for getting details on one or more datasets -type SelectParams struct { - Refs []repo.DatasetRef - Path string - Format string - Concise bool -} - -// Select selects details of one or more datasets -func (r *DatasetRequests) Select(p *SelectParams, res *[]byte) (err error) { +// Get a dataset +func (r *DatasetRequests) Get(ref *repo.DatasetRef, res *repo.DatasetRef) error { if r.cli != nil { - return r.cli.Call("DatasetRequests.Select", p, res) + return r.cli.Call("DatasetRequests.Get", ref, res) } - if err = DefaultSelectedRefs(r.repo.Repo, &p.Refs); err != nil { + // Handle `qri use` to get the current default dataset. + if err := DefaultSelectedRef(r.repo.Repo, ref); err != nil { return err } - encode := map[string]interface{}{} - for _, ref := range p.Refs { - err = repo.CanonicalizeDatasetRef(r.repo.Repo, &ref) - if err != nil && err != repo.ErrNotFound { - log.Debug(err.Error()) - return err - } - if err == repo.ErrNotFound { - if r.Node == nil { - return fmt.Errorf("%s, and no p2p connection", err.Error()) - } - err = r.Node.RequestDataset(&ref) - if err != nil { - return err - } - } - data, err := r.repo.Select(ref, p.Path) - if err != nil { - return err - } - encode[ref.String()] = data - } - - switch p.Format { - case "json": - if p.Concise { - *res, err = json.Marshal(encode) - } else { - *res, err = json.MarshalIndent(encode, "", " ") - } - case "yaml": - *res, err = yaml.Marshal(encode) - } - if err != nil { - return fmt.Errorf("error getting config: %s", err) - } - return nil -} - -// Get a dataset -func (r *DatasetRequests) Get(p *repo.DatasetRef, res *repo.DatasetRef) (err error) { - if r.cli != nil { - return r.cli.Call("DatasetRequests.Get", p, res) - } - - err = repo.CanonicalizeDatasetRef(r.repo, p) + err := repo.CanonicalizeDatasetRef(r.repo.Repo, ref) if err != nil && err != repo.ErrNotFound { log.Debug(err.Error()) return err } - // TODO: Fix this to get remote datasets. - - store := r.repo.Store() - - // try to load dataset locally - ds, err := dsfs.LoadDataset(store, datastore.NewKey(p.Path)) - if err != nil { - var ( - refs = make(chan repo.DatasetRef) - errs = make(chan error) - tries, fails int - ) - - // if we have a p2p node, check p2p network for deets - if r.Node != nil { - tries++ - go func() { - ref := repo.DatasetRef{} - // TODO - should add a context to this call with a timeout - if err := r.Node.RequestDataset(&ref); err == nil { - refs <- ref - } else { - errs <- err - } - }() - } - - // if we have a registry check it for details - if rc := r.repo.Registry(); rc != nil { - go func() { - tries++ - if dsp, err := rc.GetDataset(p.Peername, p.Name, p.ProfileID.String(), p.Path); err == nil { - ref := repo.DatasetRef{ - Path: dsp.Path, - Peername: dsp.Peername, - Name: dsp.Name, - Dataset: dsp, - } - - if pid, err := profile.IDB58Decode(dsp.ProfileID); err == nil { - ref.ProfileID = pid - } - - refs <- ref - } else { - errs <- err - } - }() + if err == repo.ErrNotFound { + if r.Node == nil { + return fmt.Errorf("%s, and no p2p connection", err.Error()) } - - for { - select { - case ref := <-refs: - *res = ref - return nil - case err := <-errs: - fails++ - log.Debugf("error getting dataset: %s", err.Error()) - if fails == tries { - return repo.ErrNotFound - } - case <-time.After(time.Second * 5): - // TODO- replace this with context.WithTimeout funcs on all network calls - return repo.ErrNotFound - } + err = r.Node.RequestDataset(ref) + if err != nil { + return err } - + *res = *ref return nil } - - *res = repo.DatasetRef{ - ProfileID: p.ProfileID, - Peername: p.Peername, - Name: p.Name, - Path: p.Path, - Dataset: ds.Encode(), + err = r.repo.ReadDataset(ref) + if err != nil { + return err } + *res = *ref return nil } diff --git a/lib/datasets_test.go b/lib/datasets_test.go index 8472fbe9c..05d661d2e 100644 --- a/lib/datasets_test.go +++ b/lib/datasets_test.go @@ -373,7 +373,8 @@ func TestDatasetRequestsGet(t *testing.T) { err string }{ // TODO: probably delete some of these - {repo.DatasetRef{Peername: "peer", Path: "abc", Name: "ABC"}, nil, "repo: not found"}, + {repo.DatasetRef{Peername: "peer", Path: "abc", Name: "ABC"}, nil, + "error loading dataset: error getting file bytes: datastore: key not found"}, {repo.DatasetRef{Peername: "peer", Path: ref.Path, Name: "ABC"}, nil, ""}, {repo.DatasetRef{Peername: "peer", Path: ref.Path, Name: "movies"}, moviesDs, ""}, {repo.DatasetRef{Peername: "peer", Path: ref.Path, Name: "cats"}, moviesDs, ""}, diff --git a/repo/actions/select.go b/repo/actions/select.go index 830c0a3f1..4fd17d3bd 100644 --- a/repo/actions/select.go +++ b/repo/actions/select.go @@ -12,6 +12,8 @@ import ( "github.com/qri-io/qri/repo" ) +// TODO: Remove this, move other functions to another file, possibly in lib/. + // Select loads a dataset value specified by case.Sensitve.dot.separated.paths func (act Dataset) Select(ref repo.DatasetRef, path string) (interface{}, error) { ds, err := dsfs.LoadDataset(act.Store(), datastore.NewKey(ref.Path)) @@ -30,6 +32,16 @@ func (act Dataset) Select(ref repo.DatasetRef, path string) (interface{}, error) return v.Interface(), nil } +// ApplyPath gets a dataset value by applying a case.Sensitve.dot.separated.path +func ApplyPath(ds *dataset.DatasetPod, path string) (interface{}, error) { + var value reflect.Value + value, err := pathValue(ds, path) + if err != nil { + return nil, err + } + return value.Interface(), nil +} + func pathValue(ds *dataset.DatasetPod, path string) (elem reflect.Value, err error) { elem = reflect.ValueOf(ds)