From 6e5cde73d08653adb975b2979b002c04f58fad51 Mon Sep 17 00:00:00 2001 From: Dustin Long Date: Wed, 24 Jul 2019 16:51:54 -0400 Subject: [PATCH] fix(status): Status displays parse errors, instead of bailing out --- cmd/fsi_test.go | 68 +++++++++++++++++++++++++++++++++++++++++++++ cmd/save.go | 6 +++- cmd/status.go | 6 ++++ fsi/mapping.go | 18 +++++++----- fsi/mapping_test.go | 4 +-- fsi/status.go | 18 +++++++++++- 6 files changed, 109 insertions(+), 11 deletions(-) diff --git a/cmd/fsi_test.go b/cmd/fsi_test.go index cea1ba66b..85bb50927 100644 --- a/cmd/fsi_test.go +++ b/cmd/fsi_test.go @@ -449,6 +449,74 @@ run ` + "`qri save`" + ` to commit this dataset } } +// Test that status displays parse errors correctly +func TestStatusParseError(t *testing.T) { + if err := confirmQriNotRunning(); err != nil { + t.Skip(err.Error()) + } + + r := NewTestRepoRoot(t, "qri_test_status_parse_error") + defer r.Delete() + + ctx, done := context.WithCancel(context.Background()) + defer done() + + pwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + + rootPath, err := ioutil.TempDir("", "") + if err != nil { + t.Fatal(err) + } + + // Save a dataset containing a body.json and meta component + cmdR := r.CreateCommandRunner(ctx) + err = executeCommand(cmdR, "qri save --body=testdata/movies/body_two.json --file=testdata/movies/meta_override.yaml me/bad_movies") + if err != nil { + t.Fatalf(err.Error()) + } + + // Change to a temporary directory. + os.Chdir(rootPath) + defer os.Chdir(pwd) + + // Checkout the newly created dataset. + cmdR = r.CreateCommandRunner(ctx) + err = executeCommand(cmdR, "qri checkout me/bad_movies") + if err != nil { + t.Fatalf(err.Error()) + } + + workPath := filepath.Join(rootPath, "bad_movies") + os.Chdir(workPath) + defer os.Chdir(pwd) + + // Modify the meta.json so that it fails to parse. + if err = ioutil.WriteFile("meta.json", []byte(`{"title": "hello}`), os.ModePerm); err != nil { + t.Fatalf(err.Error()) + } + + // Status, check that status shows the parse error. + cmdR = r.CreateCommandRunner(ctx) + err = executeCommand(cmdR, "qri status") + if err != nil { + t.Fatalf(err.Error()) + } + + output := r.GetOutput() + expect := `for dataset [test_peer/bad_movies] + + parse_error: meta (source: meta.json) + +fix these problems before saving this dataset +` + if diff := cmpTextLines(expect, output); diff != "" { + t.Errorf("qri status (-want +got):\n%s", diff) + } +} + func cmpTextLines(left, right string) string { lside := strings.Split(left, "\n") rside := strings.Split(right, "\n") diff --git a/cmd/save.go b/cmd/save.go index a2c0688bb..d595692e6 100644 --- a/cmd/save.go +++ b/cmd/save.go @@ -113,11 +113,15 @@ func (o *SaveOptions) Complete(f Factory, args []string) (err error) { return } if o.Refs.IsLinked() { + var problems map[string]string o.FSIMethods = lib.NewFSIMethods(f.Instance()) - o.fsids, _, err = fsi.ReadDir(o.Refs.Dir()) + o.fsids, _, problems, err = fsi.ReadDir(o.Refs.Dir()) if err != nil { return } + if problems != nil { + return fmt.Errorf("cannot save, dataset has errors") + } } // Make all paths absolute. Especially important if we are running diff --git a/cmd/status.go b/cmd/status.go index 7550793f3..611e8cbd5 100644 --- a/cmd/status.go +++ b/cmd/status.go @@ -78,6 +78,10 @@ func (o *StatusOptions) Run() (err error) { case fsi.STAdd, fsi.STChange: printErr(o.Out, fmt.Errorf(" %s: %s (source: %s)", si.Type, si.Component, filepath.Base(si.SourceFile))) clean = false + case fsi.STParseError: + printErr(o.Out, fmt.Errorf(" %s: %s (source: %s)", si.Type, si.Component, filepath.Base(si.SourceFile))) + clean = false + valid = false } // TODO(dlong): Validate each file / component, set `valid` to false if any problems exist } @@ -86,6 +90,8 @@ func (o *StatusOptions) Run() (err error) { printSuccess(o.Out, "working directory clean") } else if valid { printSuccess(o.Out, "\nrun `qri save` to commit this dataset") + } else { + printErr(o.Out, fmt.Errorf("\nfix these problems before saving this dataset")) } return nil } diff --git a/fsi/mapping.go b/fsi/mapping.go index 5f962e243..65d81adea 100644 --- a/fsi/mapping.go +++ b/fsi/mapping.go @@ -32,10 +32,11 @@ var ( // a map of component names to the files they came from. Files can be specified // in either JSON or YAML format. It is an error to specify any component more // than once -func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, err error) { +func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, problems map[string]string, err error) { mapping = map[string]string{} ds = &dataset.Dataset{} schema := map[string]interface{}{} + problems = nil components := map[string]interface{}{ componentNameDataset: ds, @@ -75,7 +76,7 @@ func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, err er } if _, err = os.Stat(filepath.Join(dir, "body.json")); !os.IsNotExist(err) { if bodyFormat == "csv" { - return ds, mapping, fmt.Errorf("body.csv and body.json both exist") + return ds, mapping, problems, fmt.Errorf("body.csv and body.json both exist") } bodyFormat = "json" } @@ -84,7 +85,7 @@ func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, err er if bodyFormat != "" { bodyFilename = fmt.Sprintf("body.%s", bodyFormat) if err = addMapping(componentNameBody, bodyFilename); err != nil { - return ds, mapping, err + return ds, mapping, problems, err } if ds.BodyPath == "" { ds.BodyPath = filepath.Join(dir, bodyFilename) @@ -101,11 +102,14 @@ func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, err er if f, e := os.Open(path); e == nil { if cmpName != componentNameBody { if err = mkDec(f).Decode(cmp); err != nil { - err = fmt.Errorf("reading %s: %s", filename, err) - return ds, mapping, err + if problems == nil { + problems = make(map[string]string) + } + problems[cmpName] = filename + continue } if err = addMapping(cmpName, path); err != nil { - return ds, mapping, err + return ds, mapping, problems, err } } @@ -178,7 +182,7 @@ func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, err er err = ErrNoDatasetFiles } - return ds, mapping, err + return ds, mapping, problems, err } type decoderFactory func(io.Reader) decoder diff --git a/fsi/mapping_test.go b/fsi/mapping_test.go index 25c3400cd..c685c8f27 100644 --- a/fsi/mapping_test.go +++ b/fsi/mapping_test.go @@ -16,7 +16,7 @@ func TestReadDir(t *testing.T) { for _, c := range good { t.Run(fmt.Sprintf("good: %s", filepath.Base(c.path)), func(t *testing.T) { - _, _, err := ReadDir(c.path) + _, _, _, err := ReadDir(c.path) if err != nil { t.Errorf("expected no error. got: %s", err) } @@ -34,7 +34,7 @@ func TestReadDir(t *testing.T) { for _, c := range bad { t.Run(fmt.Sprintf("bad: %s", filepath.Base(c.path)), func(t *testing.T) { - _, _, err := ReadDir(c.path) + _, _, _, err := ReadDir(c.path) t.Log(err) if err == nil { t.Errorf("expected error. got: %s", err) diff --git a/fsi/status.go b/fsi/status.go index 9d2d984ee..cc49c0d2f 100644 --- a/fsi/status.go +++ b/fsi/status.go @@ -24,6 +24,8 @@ var ( STChange = "modified" // STRemoved is a removed component STRemoved = "removed" + // STParseError is a component that didn't parse + STParseError = "parse_error" ) // StatusItem is a component that has status representation on the filesystem @@ -127,7 +129,7 @@ func (fsi *FSI) Status(dir string) (changes []StatusItem, err error) { stored.Transform = nil stored.Peername = "" - working, mapping, err := ReadDir(dir) + working, mapping, problems, err := ReadDir(dir) if err != nil { return nil, err } @@ -143,6 +145,20 @@ func (fsi *FSI) Status(dir string) (changes []StatusItem, err error) { // when reporting deletes, ignore "bound" components that must/must-not // exist based on external conditions if cmpName != componentNameDataset && cmpName != componentNameStructure && cmpName != componentNameCommit && cmpName != componentNameViz { + + if problems != nil { + // Problems is nil unless some components have errors. + if cmpFilename, ok := problems[cmpName]; ok { + change := StatusItem{ + SourceFile: cmpFilename, + Component: cmpName, + Type: STParseError, + } + changes = append(changes, change) + continue + } + } + cmp := dsComponent(stored, cmpName) // If the component was not in the previous version, it can't have been removed. if cmp == nil {