fix(save): Cannot save new datasets if name contains upper-case chara…

…cters

cmd/cmd.go

@@ -12,14 +12,11 @@ import (
  "io"
  "os"
  "path/filepath"
- "strings"
 
  golog "github.com/ipfs/go-log"
  "github.com/qri-io/ioes"
  "github.com/qri-io/qri/lib"
- "github.com/qri-io/qri/repo"
  "github.com/qri-io/qri/repo/gen"
- reporef "github.com/qri-io/qri/repo/ref"
 )
 
 var log = golog.Logger("cmd")
@@ -112,11 +109,3 @@ func parseSecrets(secrets ...string) (map[string]string, error) {
  }
  return s, nil
 }
-
-// parseCmdLineDatasetRef parses DatasetRefs, assuming peer "me" if none given.
-func parseCmdLineDatasetRef(ref string) (reporef.DatasetRef, error) {
- if !strings.ContainsAny(ref, "@/") {
- ref = "me/" + ref
- }
- return repo.ParseDatasetRef(ref)
-}

cmd/fsi_integration_test.go

@@ -126,7 +126,7 @@ func TestInitBadName(t *testing.T) {
  if err == nil {
  t.Fatal("expected error trying to init, did not get an error")
  }
- expect := `dataset name must start with a letter, and only contain letters, numbers, and underscore`
+ expect := `dataset name must start with a letter, and only contain letters, numbers, and underscore. Maximum length is 144 characters`
  if err.Error() != expect {
  t.Errorf("error mismatch, expect: %s, got: %s", expect, err.Error())
  }

cmd/save.go

@@ -148,14 +148,8 @@ func (o *SaveOptions) Run() (err error) {
  o.StartSpinner()
  defer o.StopSpinner()
 
- // TODO (b5): cmd should never need to parse a dataset reference
- ref, err := parseCmdLineDatasetRef(o.Refs.Ref())
- if err != nil && len(o.FilePaths) == 0 {
- return lib.NewError(lib.ErrBadArgs, "error parsing dataset reference '"+o.Refs.Ref()+"'")
- }
-
  p := &lib.SaveParams{
- Ref: ref.AliasString(),
+ Ref: o.Refs.Ref(),
  BodyPath: o.BodyPath,
  Title: o.Title,
  Message: o.Message,

cmd/save_test.go

@@ -10,7 +10,9 @@ import (
  "time"
 
  "github.com/google/go-cmp/cmp"
+ "github.com/qri-io/dataset"
  "github.com/qri-io/ioes"
+ "github.com/qri-io/qfs"
  "github.com/qri-io/qfs/localfs"
  "github.com/qri-io/qri/base/dsfs"
  "github.com/qri-io/qri/dscache"
@@ -286,6 +288,43 @@ func TestSaveBasicCommands(t *testing.T) {
  t.Errorf("%s: result mismatch (-want +got):%s\n", c.description, diff)
  }
  }
+
+ badCases := []struct {
+ description string
+ command string
+ expectErr string
+ }{
+ {
+ "dataset file other username",
+ "qri save --file dataset.yaml other/my_dataset",
+ "cannot save using a different username than \"test_peer\"",
+ },
+ {
+ "dataset file explicit version",
+ "qri save --file dataset.yaml me/my_dataset@/ipfs/QmVersion",
+ "ref can only have username/name",
+ },
+ {
+ "body file other username",
+ "qri save --body body_ten.csv other/my_dataset",
+ "cannot save using a different username than \"test_peer\"",
+ },
+ }
+ for _, c := range badCases {
+ run := NewTestRunner(t, "test_peer", "qri_test_save_basic")
+ defer run.Delete()
+
+ err := run.ExecCommand(c.command)
+ if err == nil {
+ output := run.GetCommandOutput()
+ t.Errorf("%s: expected an error, did not get one, output: %s\n", c.description, output)
+ continue
+ }
+ if err.Error() != c.expectErr {
+ t.Errorf("%s: mismatch, expect: %s, got: %s\n", c.description, c.expectErr, err.Error())
+ continue
+ }
+ }
 }
 
 func TestSaveInferName(t *testing.T) {
@@ -492,6 +531,45 @@ func TestSaveDscacheExistingDataset(t *testing.T) {
  }
 }
 
+func TestSaveBadCaseCantBeUsedForNewDatasets(t *testing.T) {
+ run := NewTestRunner(t, "test_peer", "qri_save_bad_case")
+ defer run.Delete()
+
+ prevTimestampFunc := logbook.NewTimestamp
+ logbook.NewTimestamp = func() int64 {
+ return 1000
+ }
+ defer func() {
+ logbook.NewTimestamp = prevTimestampFunc
+ }()
+
+ // Try to save a new dataset, but its name has upper-case characters.
+ err := run.ExecCommand("qri save --body testdata/movies/body_two.json test_peer/a_New_Dataset")
+ if err == nil {
+ t.Fatal("expected error trying to save, did not get an error")
+ }
+ expect := `dataset name may not contain any upper-case letters`
+ if err.Error() != expect {
+ t.Errorf("error mismatch, expect: %s, got: %s", expect, err.Error())
+ }
+
+ // 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.
+ run.MustExec(t, "qri save --body testdata/movies/body_two.json test_peer/a_New_Dataset")
+}
+
 func parseDatasetRefFromOutput(text string) string {
  pos := strings.Index(text, "dataset saved:")
  if pos == -1 {

cmd/test_runner_test.go

@@ -10,8 +10,11 @@ import (
  "testing"
  "time"
 
+ "github.com/qri-io/dataset"
  "github.com/qri-io/ioes"
+ "github.com/qri-io/qri/base"
  "github.com/qri-io/qri/base/dsfs"
+ "github.com/qri-io/qri/dsref"
  "github.com/qri-io/qri/lib"
  "github.com/qri-io/qri/registry"
  "github.com/qri-io/qri/registry/regserver"
@@ -246,3 +249,49 @@ func executeCommandC(root *cobra.Command, args ...string) (err error) {
  _, err = root.ExecuteC()
  return err
 }
+
+// AddDatasetToRefstore adds a dataset to the test runner's refstore. It ignores the upper-levels
+// of our stack, namely cmd/ and lib/, which means it can be used to add a dataset with a name
+// that is using upper-case characters.
+func (run *TestRunner) AddDatasetToRefstore(ctx context.Context, t *testing.T, refStr string, ds *dataset.Dataset) {
+ ref, err := dsref.ParseHumanFriendly(refStr)
+ if err != nil && err != dsref.ErrBadCaseName {
+ t.Fatal(err)
+ }
+
+ ds.Peername = ref.Username
+ ds.Name = ref.Name
+
+ inst, err := lib.NewInstance(ctx, run.RepoRoot.QriPath)
+ // NOTE(dustmop): There's a bug with TestRepo that I don't understand completely. The commands
+ // are run using a different refstore than the refstore returned by accessing the fields of the
+ // TestRepo directly. The command runner constructs a repo and then refstore which has a path
+ // similar to "/var/folders/tmpDir/T/qri_save_bad_case1234" with "qri" and "ipfs" directories
+ // within. However, trying to directly access the Repo object from TestRepo will return a
+ // refstore with the path "/var/folders/tmpDir/T/qri_save_bad_case1234" as the *qri repository*.
+ //
+ // So doing:
+ // run.RepoRoot.Repo()
+ // gives a refstore that saves to:
+ // "/var/folders/tmpDir/T/qri_save_bad_case1234/refs.fbs"
+ // While the commandRunner is using:
+ // "/var/folders/tmpDir/T/qri_save_bad_case1234/qri/refs.fbs"
+ //
+ // We work around this by constructing a lib.Instance, which uses the PathFactory to get the
+ // qri subfolder and correctly use the refstore at:
+ // "/var/folders/tmpDir/T/qri_save_bad_case1234/qri/refs.fbs"
+ //
+ // This is probably the same bug that is handled in repo/buildrepo/build.go with a hack that
+ // appends "/qri" to the repoPath.
+ r := inst.Repo()
+
+ str := ioes.NewStdIOStreams()
+ secrets := make(map[string]string)
+ scriptOut := &bytes.Buffer{}
+ sw := base.SaveDatasetSwitches{}
+
+ _, err = base.SaveDataset(ctx, r, str, ds, secrets, scriptOut, sw)
+ if err != nil {
+ t.Fatal(err)
+ }
+}

cmd/whatchanged.go

@@ -12,9 +12,9 @@ import (
 func NewWhatChangedCommand(f Factory, ioStreams ioes.IOStreams) *cobra.Command {
  o := &WhatChangedOptions{IOStreams: ioStreams}
  cmd := &cobra.Command{
- Use: "whatchanged DATASET",
+ Use:  "whatchanged DATASET",
  Hidden: true,
- Short: "shows what changed at a particular commit",
+ Short:  "shows what changed at a particular commit",
  Long: `Shows what changed for components at a particular commit, that is, which
 were added, modified or removed. This is analagous to the status command,
 except only available for dataset versions in history.`,

dsref/parse.go

@@ -3,6 +3,7 @@ package dsref
 import (
  "fmt"
  "regexp"
+ "unicode"
 )
 
 // These functions parse a string to create a dsref. We refer to a "human-friendly reference"
@@ -41,20 +42,26 @@ var (
  concreteRef = regexp.MustCompile(`^@(` + b58Id + `)?\/(` + alphaNumeric + `)\/(` + b58Id + `)`)
  b58StrictCheck = regexp.MustCompile(`^Qm[1-9A-HJ-NP-Za-km-z]*$`)
 
+ // ErrEmptyRef is an error for when a reference is empty
+ ErrEmptyRef = fmt.Errorf("empty reference")
  // ErrParseError is an error returned when parsing fails
  ErrParseError = fmt.Errorf("could not parse ref")
  // ErrNotHumanFriendly is an error returned when a reference is not human-friendly
  ErrNotHumanFriendly = fmt.Errorf("ref can only have username/name")
+ // ErrBadCaseName is the error when a bad case is used in the dataset name
+ ErrBadCaseName = fmt.Errorf("dataset name may not contain any upper-case letters")
+ // ErrBadCaseShouldRename is the error when a dataset should be renamed to not use upper case letters
+ ErrBadCaseShouldRename = fmt.Errorf("dataset name should not contain any upper-case letters, rename it to only use lower-case letters, numbers, and underscores")
  // ErrDescribeValidName is an error describing a valid dataset name
- ErrDescribeValidName = fmt.Errorf("dataset name must start with a letter, and only contain letters, numbers, and underscore")
+ ErrDescribeValidName = fmt.Errorf("dataset name must start with a letter, and only contain letters, numbers, and underscore. Maximum length is 144 characters")
 )
 
 // Parse a reference from a string
 func Parse(text string) (Ref, error) {
  var r Ref
  origLength := len(text)
  if origLength == 0 {
- return r, fmt.Errorf("empty reference")
+ return r, ErrEmptyRef
  }
 
  remain, partial, err := parseHumanFriendly(text)
@@ -80,6 +87,14 @@ func Parse(text string) (Ref, error) {
  return r, fmt.Errorf("parsing ref, unexpected character at position %d: '%c'", pos, text[0])
  }
 
+ // Dataset names are not supposed to contain upper-case characters. For now, return an error
+ // but also the reference; callers should display a warning, but continue to work for now.
+ for _, rune := range r.Name {
+ if unicode.IsUpper(rune) {
+ return r, ErrBadCaseName
+ }
+ }
+
  return r, nil
 }
 
@@ -88,7 +103,7 @@ func ParseHumanFriendly(text string) (Ref, error) {
  var r Ref
  origLength := len(text)
  if origLength == 0 {
- return r, fmt.Errorf("empty reference")
+ return r, ErrEmptyRef
  }
 
  remain, partial, err := parseHumanFriendly(text)
@@ -108,13 +123,21 @@ func ParseHumanFriendly(text string) (Ref, error) {
  return r, fmt.Errorf("parsing ref, unexpected character at position %d: '%c'", pos, text[0])
  }
 
+ // Dataset names are not supposed to contain upper-case characters. For now, return an error
+ // but also the reference; callers should display a warning, but continue to work for now.
+ for _, rune := range r.Name {
+ if unicode.IsUpper(rune) {
+ return r, ErrBadCaseName
+ }
+ }
+
  return r, nil
 }
 
 // IsRefString returns whether the string parses as a valid reference
 func IsRefString(text string) bool {
  _, err := Parse(text)
- return err == nil
+ return err == nil || err == ErrBadCaseName
 }
 
 // IsValidName returns whether the dataset name is valid

dsref/parse_test.go

@@ -51,6 +51,17 @@ func TestParse(t *testing.T) {
  }
 }
 
+func TestParseBadCase(t *testing.T) {
+ ref, err := Parse("test_peer/a_New_Dataset")
+ if err != ErrBadCaseName {
+ t.Errorf("expected to get error %s, but got %s", ErrBadCaseName, err)
+ }
+ expect := Ref{Username: "test_peer", Name: "a_New_Dataset"}
+ if !ref.Equals(expect) {
+ t.Errorf("mismatch: expect %s, got %s", expect, ref)
+ }
+}
+
 func TestParseHumanFriendly(t *testing.T) {
  goodCases := []struct {
  description string