feat(dispatch): Abs paths on inputs to dispatch methods

Background: when `qri connect` is running, it holds an exclusive lock, and
other invocations of qri will send their commands to that connected process
using RPC. The two processes may be running in different directories, so it
is necessary to change file paths to be absolute before sending the params
across RPC. Before, we would do this explicitly, which was error-prone and
easy to forget about, since the broken behavior would not happen in the
common case.

The proposed solution implemented here adds tags to structs that are passed
to methods. Most use `qri:"fspath"` to specify fields that are file system
paths. Diff is special and uses `qri:"dsrefOrFspath"` because its inputs
can be either dsrefs or file paths. Adding this behavior for new commands
is simple; it just requires adding this tag.

An alternative would be to add a type for qfs.FilePath that does this
behavior itself. However, that requires users of qri to know about this type,
and FilePath types are typically quite tricky to fully implement. This
solution is simpler and adds less maintenance vs creating our own path type,
though it is admittedly a bit magical. Personally, I don't think it's that
bad, as it matches the standard use case of tags for serialization, if we
view fully absolute paths being sent over RPC as a type of data marshalling.

Plus, the Diff inputs act quite special. Simply creating a qfs.FilePath would
not be sufficient to handle Diff's parameters, and fixing Diff in some
way would probably add more complication than its worth.

Author: dustmop

cmd/checkout.go

@@ -6,7 +6,6 @@ import (
 	"strings"
 
 	"github.com/qri-io/ioes"
-	"github.com/qri-io/qfs"
 	"github.com/qri-io/qri/dsref"
 	"github.com/qri-io/qri/lib"
 	"github.com/spf13/cobra"
@@ -88,10 +87,6 @@ func (o *CheckoutOptions) Run() (err error) {
 		o.Dir = dsref.GenerateName(ref[pos+1:], "")
 	}
 
-	if err = qfs.AbsPath(&o.Dir); err != nil {
-		return err
-	}
-
 	if err = inst.Filesys().Checkout(ctx, &lib.LinkParams{Dir: o.Dir, Refstr: ref}); err != nil {
 		return err
 	}

cmd/save.go

@@ -6,7 +6,6 @@ import (
 	"strings"
 
 	"github.com/qri-io/ioes"
-	"github.com/qri-io/qfs"
 	"github.com/qri-io/qri/dsref"
 	"github.com/qri-io/qri/lib"
 	"github.com/qri-io/qri/repo"
@@ -131,18 +130,6 @@ func (o *SaveOptions) Complete(f Factory, args []string) (err error) {
 		}
 	}
 
-	// Make all paths absolute. Especially important if we are running
-	// `qri connect` in a different terminal, and that instance is in a different directory;
-	// that instance won't correctly find the body file we want to load if it's not absolute.
-	for i := range o.FilePaths {
-		if err = qfs.AbsPath(&o.FilePaths[i]); err != nil {
-			return
-		}
-	}
-
-	if err := qfs.AbsPath(&o.BodyPath); err != nil {
-		return fmt.Errorf("body file: %s", err)
-	}
 	return nil
 }
 

fsi/init.go

@@ -18,10 +18,10 @@ import (
 
 // InitParams encapsulates parameters for fsi.InitDataset
 type InitParams struct {
-	TargetDir  string `json:"targetDir"`
+	TargetDir  string `json:"targetDir" qri:"fspath"`
 	Name       string `json:"name"`
 	Format     string `json:"format"`
-	BodyPath   string `json:"bodyPath"`
+	BodyPath   string `json:"bodyPath" qri:"fspath"`
 	UseDscache bool   `json:"useDscache"`
 }
 

lib/datasets.go

@@ -108,7 +108,7 @@ type GetParams struct {
 	All    bool `json:"all"`
 
 	// outfile is a filename to save the dataset to
-	Outfile string `json:"outfile"`
+	Outfile string `json:"outfile" qri:"fspath"`
 	// whether to generate a filename from the dataset name instead
 	GenFilename bool   `json:"genfilename"`
 	Remote      string `json:"remote"`
@@ -249,11 +249,6 @@ type DataResponse struct {
 // then res.Bytes is loaded with the body. If the selector is "stats", then res.Bytes is loaded
 // with the generated stats.
 func (m DatasetMethods) Get(ctx context.Context, p *GetParams) (*GetResult, error) {
-	// TODO(dustmop): Have Dispatch perform this AbsPath call automatically
-	if err := qfs.AbsPath(&p.Outfile); err != nil {
-		return nil, err
-	}
-
 	got, _, err := m.d.Dispatch(ctx, dispatchMethodName(m, "get"), p)
 	if res, ok := got.(*GetResult); ok {
 		return res, err
@@ -305,9 +300,9 @@ type SaveParams struct {
 	// commit message, defaults to blank
 	Message string
 	// path to body data
-	BodyPath string
+	BodyPath string `qri:"fspath"`
 	// absolute path or URL to the list of dataset files or components to load
-	FilePaths []string
+	FilePaths []string `qri:"fspath"`
 	// secrets for transform execution
 	Secrets map[string]string
 	// optional writer to have transform script record standard output to
@@ -513,7 +508,7 @@ func (m DatasetMethods) Remove(ctx context.Context, p *RemoveParams) (*RemoveRes
 // PullParams encapsulates parameters to the add command
 type PullParams struct {
 	Ref      string
-	LinkDir  string
+	LinkDir  string `qri:"fspath"`
 	Remote   string // remote to attempt to pull from
 	LogsOnly bool   // only fetch logbook data
 }
@@ -530,9 +525,6 @@ func (p *PullParams) UnmarshalFromRequest(r *http.Request) error {
 // Pull downloads and stores an existing dataset to a peer's repository via
 // a network connection
 func (m DatasetMethods) Pull(ctx context.Context, p *PullParams) (*dataset.Dataset, error) {
-	if err := qfs.AbsPath(&p.LinkDir); err != nil {
-		return nil, err
-	}
 	got, _, err := m.d.Dispatch(ctx, dispatchMethodName(m, "pull"), p)
 	if res, ok := got.(*dataset.Dataset); ok {
 		return res, err

lib/diff.go

@@ -7,7 +7,6 @@ import (
 
 	"github.com/qri-io/dataset/tabular"
 	"github.com/qri-io/deepdiff"
-	"github.com/qri-io/qfs"
 	"github.com/qri-io/qri/base/component"
 	"github.com/qri-io/qri/base/dsfs"
 	"github.com/qri-io/qri/dsref"
@@ -27,10 +26,10 @@ type DiffStat = deepdiff.Stats
 // LeftSide set with the UseLeftPrevVersion flag.
 type DiffParams struct {
 	// File paths or reference to datasets
-	LeftSide  string `schema:"leftPath" json:"leftPath"`
-	RightSide string `schema:"rightPath" json:"rightPath"`
+	LeftSide  string `schema:"leftPath" json:"leftPath" qri:"dsrefOrFspath"`
+	RightSide string `schema:"rightPath" json:"rightPath" qri:"dsrefOrFspath"`
 	// If not null, the working directory that the diff is using
-	WorkingDir string
+	WorkingDir string `qri:"fspath"`
 	// Whether to get the previous version of the left parameter
 	UseLeftPrevVersion bool
 
@@ -107,17 +106,6 @@ const (
 
 // Diff computes the diff of two sources
 func (m DatasetMethods) Diff(ctx context.Context, p *DiffParams) (*DiffResponse, error) {
-	// absolutize any local paths before a possible trip over RPC to another local process
-	if !dsref.IsRefString(p.LeftSide) {
-		if err := qfs.AbsPath(&p.LeftSide); err != nil {
-			return nil, err
-		}
-	}
-	if !dsref.IsRefString(p.RightSide) {
-		if err := qfs.AbsPath(&p.RightSide); err != nil {
-			return nil, err
-		}
-	}
 	got, _, err := m.d.Dispatch(ctx, dispatchMethodName(m, "diff"), p)
 	if res, ok := got.(*DiffResponse); ok {
 		return res, err

lib/dispatch.go

@@ -8,7 +8,9 @@ import (
 	"strings"
 	"time"
 
+	"github.com/qri-io/qfs"
 	"github.com/qri-io/qri/auth/token"
+	"github.com/qri-io/qri/dsref"
 	"github.com/qri-io/qri/profile"
 )
 
@@ -135,6 +137,9 @@ func (inst *Instance) Dispatch(ctx context.Context, method string, param interfa
 			return nil, nil, err
 		}
 
+		// Handle filepaths in the params by calling qfs.Abs on each of them
+		param = normalizeInputParams(param)
+
 		// Construct the parameter list for the function call, then call it
 		args := make([]reflect.Value, 3)
 		args[0] = reflect.ValueOf(c.Impl)
@@ -417,3 +422,50 @@ func dispatchReturnError(got interface{}, err error) error {
 	}
 	return err
 }
+
+// normalizeInputParams will look at each field of the params, and modify filepaths so that
+// they are absolute paths, making them safe to send across RPC to another process
+func normalizeInputParams(param interface{}) interface{} {
+	typ := reflect.TypeOf(param)
+	val := reflect.ValueOf(param)
+	if typ.Kind() == reflect.Ptr {
+		typ = typ.Elem()
+		val = val.Elem()
+	}
+	if typ.Kind() == reflect.Struct {
+		num := typ.NumField()
+		for i := 0; i < num; i++ {
+			tfield := typ.Field(i)
+			vfield := val.Field(i)
+			qriTag := tfield.Tag.Get("qri")
+			if qriTag == "fspath" || qriTag == "dsrefOrFspath" {
+				normalizeFileField(vfield, qriTag)
+			} else if qriTag != "" {
+				log.Errorf("unknown qri struct tag %q", qriTag)
+			}
+		}
+	}
+	return param
+}
+
+func normalizeFileField(vfield reflect.Value, qriTag string) {
+	interf := vfield.Interface()
+	if str, ok := interf.(string); ok {
+		if qriTag == "dsrefOrFspath" && dsref.IsRefString(str) {
+			return
+		}
+		if err := qfs.AbsPath(&str); err == nil {
+			vfield.SetString(str)
+		}
+	}
+	if strList, ok := interf.([]string); ok {
+		build := make([]string, 0, len(strList))
+		for _, str := range strList {
+			if qriTag != "dsrefOrFspath" || !dsref.IsRefString(str) {
+				_ = qfs.AbsPath(&str)
+			}
+			build = append(build, str)
+		}
+		vfield.Set(reflect.ValueOf(build))
+	}
+}

lib/fsi.go

@@ -9,7 +9,6 @@ import (
 	"path/filepath"
 
 	"github.com/qri-io/dataset"
-	"github.com/qri-io/qfs"
 	"github.com/qri-io/qri/base"
 	"github.com/qri-io/qri/base/component"
 	"github.com/qri-io/qri/base/dsfs"
@@ -46,7 +45,7 @@ func (m FSIMethods) Attributes() map[string]AttributeSet {
 
 // LinkParams encapsulate parameters for linked datasets
 type LinkParams struct {
-	Dir    string
+	Dir    string `qri:"fspath"`
 	Refstr string
 }
 
@@ -65,7 +64,7 @@ type FSIWriteParams struct {
 
 // RestoreParams provides parameters to the restore method.
 type RestoreParams struct {
-	Dir       string
+	Dir       string `qri:"fspath"`
 	Refstr    string
 	Path      string
 	Component string
@@ -100,10 +99,6 @@ func (m FSIMethods) Unlink(ctx context.Context, p *LinkParams) (string, error) {
 // Status checks for any modifications or errors in a linked directory against its previous
 // version in the repo. Must only be called if FSI is enabled for this dataset.
 func (m FSIMethods) Status(ctx context.Context, p *LinkParams) ([]StatusItem, error) {
-	// TODO(dustmop): Have Dispatch perform this AbsPath call automatically
-	if err := qfs.AbsPath(&p.Dir); err != nil {
-		return nil, err
-	}
 	got, _, err := m.d.Dispatch(ctx, dispatchMethodName(m, "status"), p)
 	if res, ok := got.([]StatusItem); ok {
 		return res, err
@@ -144,13 +139,6 @@ func (m FSIMethods) Restore(ctx context.Context, p *RestoreParams) error {
 
 // Init initializes a new working directory for a linked dataset
 func (m FSIMethods) Init(ctx context.Context, p *InitDatasetParams) (string, error) {
-	// TODO(dustmop): Have Dispatch perform these AbsPath calls automatically
-	if err := qfs.AbsPath(&p.TargetDir); err != nil {
-		return "", err
-	}
-	if err := qfs.AbsPath(&p.BodyPath); err != nil {
-		return "", err
-	}
 	got, _, err := m.d.Dispatch(ctx, dispatchMethodName(m, "init"), p)
 	if res, ok := got.(string); ok {
 		return res, err