diff --git a/cmd/checkout.go b/cmd/checkout.go index c3884b6de..037fe0941 100644 --- a/cmd/checkout.go +++ b/cmd/checkout.go @@ -54,7 +54,7 @@ func (o *CheckoutOptions) Complete(f Factory, args []string) (err error) { return err } - o.Refs, err = GetCurrentRefSelect(f, args, 1, o.FSIMethods) + o.Refs, err = GetCurrentRefSelect(f, args, 1, EnsureFSIAgrees(o.FSIMethods)) if err != nil { return err } diff --git a/cmd/cmd_test.go b/cmd/cmd_test.go index b71d12c0d..d7abd7b11 100644 --- a/cmd/cmd_test.go +++ b/cmd/cmd_test.go @@ -12,6 +12,8 @@ import ( "github.com/google/go-cmp/cmp" golog "github.com/ipfs/go-log" + "github.com/qri-io/dataset" + "github.com/qri-io/qfs" "github.com/qri-io/qri/config" ) @@ -634,3 +636,29 @@ func TestListFormatJson(t *testing.T) { t.Errorf("unexpected (-want +got):\n%s", diff) } } + +// Test that a dataset name with bad upper-case characters, if it already exists, produces a +// warning but not an error when you try to Get it +func TestBadCaseIsJustWarning(t *testing.T) { + run := NewTestRunner(t, "test_peer", "qri_get_bad_case") + defer run.Delete() + + // Construct a dataset in order to have an existing version in the repo. + ds := dataset.Dataset{ + Structure: &dataset.Structure{ + Format: "json", + Schema: dataset.BaseSchemaArray, + }, + } + ds.SetBodyFile(qfs.NewMemfileBytes("body.json", []byte("[[\"one\",2],[\"three\",4]]"))) + + // Add the dataset to the repo directly, which avoids the name validation check. + ctx := context.Background() + run.AddDatasetToRefstore(ctx, t, "test_peer/a_New_Dataset", &ds) + + // Save the dataset, which will work now that a version already exists. + err := run.ExecCommand("qri get test_peer/a_New_Dataset") + if err != nil { + t.Errorf("expect no error (just a warning), got %q", err) + } +} diff --git a/cmd/fsi.go b/cmd/fsi.go index 21ffff949..e657016e9 100644 --- a/cmd/fsi.go +++ b/cmd/fsi.go @@ -69,12 +69,14 @@ func (o *FSIOptions) Complete(f Factory, args []string) (err error) { if err != nil { return err } - if o.Refs, err = GetCurrentRefSelect(f, args, 1, o.FSIMethods); err != nil { - return err - } if len(args) > 1 { o.Path = args[1] + args = args[:1] + } + + if o.Refs, err = GetCurrentRefSelect(f, args, 1, EnsureFSIAgrees(o.FSIMethods)); err != nil { + return err } return nil diff --git a/cmd/get.go b/cmd/get.go index 7552ae744..7fcc6ab9a 100644 --- a/cmd/get.go +++ b/cmd/get.go @@ -92,7 +92,7 @@ func (o *GetOptions) Complete(f Factory, args []string) (err error) { args = args[1:] } } - if o.Refs, err = GetCurrentRefSelect(f, args, -1, nil); err != nil { + if o.Refs, err = GetCurrentRefSelect(f, args, AnyNumberOfReferences, nil); err != nil { return } @@ -135,6 +135,11 @@ func (o *GetOptions) Run() (err error) { // TODO(dustmop): Consider setting o.Format if o.Outfile has an extension and o.Format // is not assigned anything + // TODO(dustmop): Allow any number of references. Right now we ignore everything after the + // first. The hard parts are: + // 1) Correctly handling the pager output, and having headers between each ref + // 2) Identifying cases that limit Get to only work on 1 dataset. For example, the -o flag + // convert Page and PageSize to Limit and Offset page := util.NewPage(o.Page, o.PageSize) p := lib.GetParams{ diff --git a/cmd/log.go b/cmd/log.go index ee935fa5e..447f1d4a9 100644 --- a/cmd/log.go +++ b/cmd/log.go @@ -89,7 +89,7 @@ func (o *LogOptions) Complete(f Factory, args []string) (err error) { return errors.New(err, "cannot use 'local' flag with either the 'remote' or 'pull' flags") } - if o.Refs, err = GetCurrentRefSelect(f, args, -1, nil); err != nil { + if o.Refs, err = GetCurrentRefSelect(f, args, AnyNumberOfReferences, nil); err != nil { if err == repo.ErrEmptyRef { return errors.New(err, "please provide a dataset reference") } diff --git a/cmd/ref_link_ensurer.go b/cmd/ref_link_ensurer.go new file mode 100644 index 000000000..c3adf1bbd --- /dev/null +++ b/cmd/ref_link_ensurer.go @@ -0,0 +1,42 @@ +package cmd + +import ( + "github.com/qri-io/qri/dsref" + "github.com/qri-io/qri/lib" +) + +// EnsureFSIAgrees should be passed to GetCurrentRefSelect in order to ensure that any references +// used by a command have agreement between what their .qri-ref linkfile thinks and what the +// qri repository thinks. If there's a disagreement, the linkfile wins and the repository will +// be updated to match. +// This is useful if a user has a working directory, and then manually deletes the .qri-ref (which +// will unlink the dataset), or renames / moves the directory and then runs a command in that +// directory (which will update the repository with the new working directory's path). +func EnsureFSIAgrees(f *lib.FSIMethods) *FSIRefLinkEnsurer { + if f == nil { + return nil + } + return &FSIRefLinkEnsurer{FSIMethods: f} +} + +// FSIRefLinkEnsurer is a simple wrapper for ensuring the linkfile agrees with the repository. We +// use it instead of a raw FSIMethods pointer so that users of this code see they need to call +// EnsureFSIAgrees(*fsiMethods) when calling GetRefSelect, hopefully providing a bit of insight +// about what this parameter is for. +type FSIRefLinkEnsurer struct { + FSIMethods *lib.FSIMethods +} + +// EnsureRef checks if the linkfile and repository agree on the dataset's working directory path. +// If not, it will modify the repository so that it matches the linkfile. The linkfile will +// never be modified. +func (e *FSIRefLinkEnsurer) EnsureRef(refs *RefSelect) error { + if e == nil { + return nil + } + p := lib.EnsureParams{Dir: refs.Dir(), Ref: refs.Ref()} + info := dsref.VersionInfo{} + // Lib call matches the gorpc method signature, but `out` is not used + err := e.FSIMethods.EnsureRef(&p, &info) + return err +} diff --git a/cmd/ref_select.go b/cmd/ref_select.go index 4c845bfd4..a1215c4b0 100644 --- a/cmd/ref_select.go +++ b/cmd/ref_select.go @@ -8,7 +8,6 @@ import ( "github.com/qri-io/qri/dsref" "github.com/qri-io/qri/fsi" - "github.com/qri-io/qri/lib" "github.com/qri-io/qri/repo" ) @@ -99,34 +98,57 @@ func (r *RefSelect) String() string { return fmt.Sprintf("%s dataset [%s]", r.kind, strings.Join(r.refs, ", ")) } +// AnyNumberOfReferences is for commands that can work on any number of dataset references +const AnyNumberOfReferences = -1 + +// BadUpperCaseOkayWhenSavingExistingDataset is for the save command, which can have bad +// upper-case characters in its reference but only if it already exists +const BadUpperCaseOkayWhenSavingExistingDataset = -2 + // GetCurrentRefSelect returns the current reference selection. This could be explicitly provided // as command-line arguments, or could be determined by being in a linked directory, or could be // selected by the `use` command. This order is also the precendence, from most important to least. -// This is the recommended method for command-line commands to get references, unless they have a -// special way of interacting with datasets (for example, `qri status`). -// If an fsi pointer is passed in, use it to ensure that the ref in the .qri-ref linkfile matches +// This is the recommended method for command-line commands to get references. +// If an Ensurer is passed in, it is used to ensure that the ref in the .qri-ref linkfile matches // what is in the repo. -func GetCurrentRefSelect(f Factory, args []string, allowed int, fsi *lib.FSIMethods) (*RefSelect, error) { - // TODO(dlong): Respect `allowed`, number of refs the command uses. -1 means any. - // TODO(dlong): For example, `get` allows -1, `diff` allows 2, `save` allows 1 +func GetCurrentRefSelect(f Factory, args []string, allowed int, ensurer *FSIRefLinkEnsurer) (*RefSelect, error) { // If reference is specified by the user provide command-line arguments, use that reference. if len(args) > 0 { - if allowed >= 2 { - // Diff allows multiple explicit references. + // If bad upper-case characters are allowed, skip checking for them + if allowed == BadUpperCaseOkayWhenSavingExistingDataset { + // Bad upper-case characters are ignored, references will be checked again inside lib. + allowed = 1 + } else { + // For each argument, make sure it's a valid and not using upper-case chracters. + for _, refstr := range args { + _, err := dsref.Parse(refstr) + if err == dsref.ErrBadCaseName { + // TODO(dustmop): For now, this is just a warning but not a fatal error. + // In the near future, change to: `return nil, dsref.ErrBadCaseShouldRename` + // The test `TestBadCaseIsJustWarning` in cmd/cmd_test.go verifies that this + // is not a fatal error. + log.Error(dsref.ErrBadCaseShouldRename) + } + } + } + if allowed == AnyNumberOfReferences { return NewListOfRefSelects(args), nil } - return NewExplicitRefSelect(args[0]), nil + if len(args) > allowed { + return nil, fmt.Errorf("%d references allowed but %d were given", allowed, len(args)) + } + if allowed == 1 { + return NewExplicitRefSelect(args[0]), nil + } + return NewListOfRefSelects(args), nil } // If in a working directory that is linked to a dataset, use that link's reference. refs, err := GetLinkedRefSelect() if err == nil { - if fsi != nil { - // Ensure that the link in the working directory matches what is in the repo. - out := &dsref.VersionInfo{} - err = fsi.EnsureRef(&lib.EnsureParams{Dir: refs.Dir(), Ref: refs.Ref()}, out) - if err != nil { - log.Debugf("%s", err) - } + // Ensure that the link in the working directory matches what is in the repo. + err = ensurer.EnsureRef(refs) + if err != nil { + log.Debugf("%s", err) } return refs, nil } diff --git a/cmd/remove.go b/cmd/remove.go index dceac290d..9bf5d2eef 100644 --- a/cmd/remove.go +++ b/cmd/remove.go @@ -76,7 +76,7 @@ func (o *RemoveOptions) Complete(f Factory, args []string) (err error) { if o.DatasetMethods, err = f.DatasetMethods(); err != nil { return err } - if o.Refs, err = GetCurrentRefSelect(f, args, -1, nil); err != nil { + if o.Refs, err = GetCurrentRefSelect(f, args, 1, nil); err != nil { // This error will be handled during validation if err != repo.ErrEmptyRef { return err diff --git a/cmd/rename_integration_test.go b/cmd/rename_integration_test.go index bf6923efa..59ee99b1b 100644 --- a/cmd/rename_integration_test.go +++ b/cmd/rename_integration_test.go @@ -1,11 +1,14 @@ package cmd import ( + "context" "os" "path/filepath" "testing" "github.com/google/go-cmp/cmp" + "github.com/qri-io/dataset" + "github.com/qri-io/qfs" ) // Test rename works if dataset has no history @@ -112,3 +115,52 @@ func TestRenameUpdatesLink(t *testing.T) { t.Errorf("read .qri-ref (-want +got):\n%s", diff) } } + +// Test that rename can be used on names with bad upper-case characters, but only to rename them +// to be valid instead +func TestRenameAwayFromBadCase(t *testing.T) { + run := NewTestRunner(t, "test_peer", "rename_human") + defer run.Delete() + + // Create a dataset with a valid name + run.MustExec(t, "qri save --body=testdata/movies/body_ten.csv me/first_name") + + // Cannot rename the dataset to a name with bad upper-case characters + err := run.ExecCommand("qri rename test_peer/first_name test_peer/useUpperCase") + if err == nil { + t.Fatal("expected error, did not get one") + } + expectErr := `dataset name may not contain any upper-case letters` + if diff := cmp.Diff(expectErr, err.Error()); diff != "" { + t.Errorf("unexpected (-want +got):\n%s", diff) + } + + // Construct a dataset + ds := dataset.Dataset{ + Structure: &dataset.Structure{ + Format: "json", + Schema: dataset.BaseSchemaArray, + }, + } + ds.SetBodyFile(qfs.NewMemfileBytes("body.json", []byte("[[\"one\",2],[\"three\",4]]"))) + + // Add the dataset to the repo directly, which avoids the name validation check. + ctx := context.Background() + run.AddDatasetToRefstore(ctx, t, "test_peer/a_New_Dataset", &ds) + + // Cannot rename the dataset to a name with bad upper-case characters still + err = run.ExecCommand("qri rename test_peer/a_New_Dataset test_peer/useUpperCase") + if err == nil { + t.Fatal("expected error, did not get one") + } + expectErr = `dataset name may not contain any upper-case letters` + if diff := cmp.Diff(expectErr, err.Error()); diff != "" { + t.Errorf("unexpected (-want +got):\n%s", diff) + } + + // Okay to rename a name with bad upper-case characters to a new valid name + err = run.ExecCommand("qri rename test_peer/a_New_Dataset test_peer/a_new_dataset") + if err != nil { + t.Errorf("got error: %s", err) + } +} diff --git a/cmd/render_test.go b/cmd/render_test.go index 8e344f480..354f94c7d 100644 --- a/cmd/render_test.go +++ b/cmd/render_test.go @@ -27,7 +27,6 @@ func TestRenderComplete(t *testing.T) { }{ {[]string{}, "", ""}, {[]string{"test"}, "test", ""}, - {[]string{"test", "test2"}, "test", ""}, } for i, c := range cases { diff --git a/cmd/restore.go b/cmd/restore.go index 4d5bdee78..2f393a463 100644 --- a/cmd/restore.go +++ b/cmd/restore.go @@ -114,7 +114,7 @@ func (o *RestoreOptions) Complete(f Factory, args []string) (err error) { if o.FSIMethods, err = f.FSIMethods(); err != nil { return err } - if o.Refs, err = GetCurrentRefSelect(f, dsRefList, 1, o.FSIMethods); err != nil { + if o.Refs, err = GetCurrentRefSelect(f, dsRefList, 1, EnsureFSIAgrees(o.FSIMethods)); err != nil { return err } return nil diff --git a/cmd/save.go b/cmd/save.go index b9d2a3d17..2ba18975a 100644 --- a/cmd/save.go +++ b/cmd/save.go @@ -114,7 +114,7 @@ func (o *SaveOptions) Complete(f Factory, args []string) (err error) { return } - if o.Refs, err = GetCurrentRefSelect(f, args, 1, nil); err != nil { + if o.Refs, err = GetCurrentRefSelect(f, args, BadUpperCaseOkayWhenSavingExistingDataset, nil); err != nil { // Not an error to use an empty reference, it will be inferred later on. if err != repo.ErrEmptyRef { return err diff --git a/cmd/save_test.go b/cmd/save_test.go index da6c4805f..a593a8aa9 100644 --- a/cmd/save_test.go +++ b/cmd/save_test.go @@ -36,7 +36,6 @@ func TestSaveComplete(t *testing.T) { }{ {[]string{}, "", ""}, {[]string{"test"}, "test", ""}, - {[]string{"test", "test2"}, "test", ""}, } for i, c := range cases { diff --git a/cmd/stats_test.go b/cmd/stats_test.go index 961478b8b..c9e9a8d6d 100644 --- a/cmd/stats_test.go +++ b/cmd/stats_test.go @@ -46,7 +46,6 @@ func TestStatsComplete(t *testing.T) { expectedRef string }{ {"given one ref", []string{"me/ref"}, "me/ref"}, - {"given multiple refs", []string{"me/ref", "me/ref_foo"}, "me/ref"}, } for i, c := range goodCases { diff --git a/cmd/status.go b/cmd/status.go index 69b7ad621..2fa286755 100644 --- a/cmd/status.go +++ b/cmd/status.go @@ -67,15 +67,17 @@ func (o *StatusOptions) Complete(f Factory, args []string) (err error) { return err } - o.Refs, err = GetCurrentRefSelect(f, args, 0, o.FSIMethods) - if err != nil { - return err - } // Cannot pass explicit reference, must be run in a working directory - if !o.Refs.IsLinked() { + if len(args) > 0 { + // TODO(dustmop): Fix this, see issue https://github.com/qri-io/qri/issues/1246 return fmt.Errorf("can only get status of the current working directory") } + o.Refs, err = GetCurrentRefSelect(f, args, 0, EnsureFSIAgrees(o.FSIMethods)) + if err != nil { + return err + } + return nil } diff --git a/cmd/validate_test.go b/cmd/validate_test.go index 8445e5812..3cd48848d 100644 --- a/cmd/validate_test.go +++ b/cmd/validate_test.go @@ -26,7 +26,6 @@ func TestValidateComplete(t *testing.T) { }{ {[]string{}, "filepath", "schemafilepath", "", ""}, {[]string{"test"}, "", "", "test", ""}, - {[]string{"foo", "bar"}, "", "", "foo", ""}, } for i, c := range cases { diff --git a/cmd/whatchanged.go b/cmd/whatchanged.go index c2bc66ac3..aaf42724d 100644 --- a/cmd/whatchanged.go +++ b/cmd/whatchanged.go @@ -44,7 +44,7 @@ func (o *WhatChangedOptions) Complete(f Factory, args []string) (err error) { if o.FSIMethods, err = f.FSIMethods(); err != nil { return err } - o.Refs, err = GetCurrentRefSelect(f, args, 1, o.FSIMethods) + o.Refs, err = GetCurrentRefSelect(f, args, 1, EnsureFSIAgrees(o.FSIMethods)) return nil } diff --git a/lib/datasets.go b/lib/datasets.go index 5cc4a14bb..d8f14b2f0 100644 --- a/lib/datasets.go +++ b/lib/datasets.go @@ -230,11 +230,6 @@ func (m *DatasetMethods) Get(p *GetParams, res *GetResult) error { } ctx := context.TODO() - // Check if the dataset ref uses bad-case characters, show a warning. - if _, err := dsref.Parse(p.Refstr); err == dsref.ErrBadCaseName { - log.Error(dsref.ErrBadCaseShouldRename) - } - var ds *dataset.Dataset ref, _, err := m.inst.ParseAndResolveRefWithWorkingDir(ctx, p.Refstr, p.Remote) if err != nil { diff --git a/lib/resolve.go b/lib/resolve.go index 27b0af39b..77db35196 100644 --- a/lib/resolve.go +++ b/lib/resolve.go @@ -32,7 +32,7 @@ func (inst *Instance) ParseAndResolveRef(ctx context.Context, refStr, source str // including setting default Path to a linked working directory if one exists func (inst *Instance) ParseAndResolveRefWithWorkingDir(ctx context.Context, refStr, source string) (dsref.Ref, string, error) { ref, err := dsref.Parse(refStr) - if err != nil { + if err != nil && err != dsref.ErrBadCaseName { return ref, "", fmt.Errorf("%q is not a valid dataset reference: %w", refStr, err) }