fix(naming): Handle names with upper-case characters

Datasets that have names with upper-case characters should still work. But they should produce a warning when used, recommending a rename. The rename command doesn't warn about this.

Handle everything to do with this check inside GetRefSelect. Clean up how GetRefSelect is called so that it seems less magical, and works closer to how it was intended. Add constants to make it more readable.

Ignore bad case errors in lib.Resolve, since only cmd knows if such names are valid or not.

Follow-up to #1308. Makes major progress on #1132.

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
 	}

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)
+	}
+}

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

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{

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")
 		}

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
+}

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
 	}

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

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)
+	}
+}

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 {

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

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

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 {

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 {

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
 }
 

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 {

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
 }
 

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 {