Skip to content

Commit

Permalink
refactor(fsi): Remove fsi=true parameter, use FSIPath everywhere
Browse files Browse the repository at this point in the history 
Change semantics of lib operations so that, instead of looking for a UseFSI parameter, they see if a reference has an FSIPath. If so, they follow fsi semantics, otherwise refstore. Removes the need to have operations return ErrNoLink if UseFSI is requested but no FSIPath exists.

Most instances of checking for ErrNoHistory now disappear (except for `/history`) since only working directories that have been freshly `init`d will not have history, and now they'll just follow their FSIPath.

It is now illegal to use `status` to check the historical commits of a dataset. Callers should use `whatchanged` instead.

The "fsi=true" api parameter is now non-functional. It now not illegal to send requests with it, it just doesn't do anything.
  • Loading branch information
@dustmop
dustmop committed Mar 31, 2020
1 parent 2ff84ff commit 16785a2
Show file tree
Hide file tree
Showing 14 changed files with 123 additions and 172 deletions.
7 changes: 3 additions & 4 deletions api/dataset_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ func TestDatasetHandlers(t *testing.T) {
{"GET", "/me/family_relationships", nil},
{"GET", "/me/family_relationships/at/map/Qme7LVBp6hfi4Y5N29CXeXjpAqgT3fWtAmQWtZgjpQAZph", nil},
{"GET", "/at/map/Qme7LVBp6hfi4Y5N29CXeXjpAqgT3fWtAmQWtZgjpQAZph", nil},
// test that when fsi=true on a request that does not have a link to the filesystem
// we get the correct error code & message
// test that when fsi=true parameter doesn't affect the api response
{"GET", "/me/family_relationships?fsi=true", nil},
{"DELETE", "/", nil},
}
Expand Down Expand Up @@ -191,7 +190,7 @@ func TestSaveWithInferredNewName(t *testing.T) {
// Name is inferred from the body path
expectText := `"name":"datacsv"`
if !strings.Contains(bodyText, expectText) {
t.Errorf("expected, body response to contain %q, not found", expectText)
t.Errorf("expected, body response to contain %q, not found. got %q", expectText, bodyText)
}

// Save a second time
Expand All @@ -202,7 +201,7 @@ func TestSaveWithInferredNewName(t *testing.T) {
// Name is guaranteed to be unique
expectText = `"name":"datacsv_1"`
if !strings.Contains(bodyText, expectText) {
t.Errorf("expected, body response to contain %q, not found", expectText)
t.Errorf("expected, body response to contain %q, not found. got %q", expectText, bodyText)
}
}

Expand Down
13 changes: 2 additions & 11 deletions api/datasets.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"github.com/qri-io/dataset"
"github.com/qri-io/qri/base/archive"
"github.com/qri-io/qri/dsref"
"github.com/qri-io/qri/fsi"
"github.com/qri-io/qri/lib"
"github.com/qri-io/qri/p2p"
"github.com/qri-io/qri/repo"
Expand Down Expand Up @@ -284,12 +283,11 @@ func (h *DatasetHandlers) listHandler(w http.ResponseWriter, r *http.Request) {
func (h *DatasetHandlers) getHandler(w http.ResponseWriter, r *http.Request) {
p := lib.GetParams{
Path: HTTPPathToQriPath(r.URL.Path),
UseFSI: r.FormValue("fsi") == "true",
}
res := lib.GetResult{}
err := h.Get(&p, &res)
if err != nil {
if err == repo.ErrNoHistory || err == fsi.ErrNoLink {
if err == repo.ErrNoHistory {
util.WriteErrResponse(w, http.StatusUnprocessableEntity, err)
return
}
Expand Down Expand Up @@ -455,8 +453,6 @@ func (h *DatasetHandlers) saveHandler(w http.ResponseWriter, r *http.Request) {
ReturnBody: r.FormValue("return_body") == "true",
Force: r.FormValue("force") == "true",
ShouldRender: !(r.FormValue("no_render") == "true"),
ReadFSI: r.FormValue("fsi") == "true",
WriteFSI: r.FormValue("fsi") == "true",
NewName: r.FormValue("new") == "true",
BodyPath: r.FormValue("bodypath"),

Expand Down Expand Up @@ -589,7 +585,6 @@ func getParamsFromRequest(r *http.Request, readOnly bool, path string) (*lib.Get
Path: path,
Format: format,
Selector: "body",
UseFSI: r.FormValue("fsi") == "true",
Limit: listParams.Limit,
Offset: listParams.Offset,
All: r.FormValue("all") == "true" && !readOnly,
Expand Down Expand Up @@ -652,9 +647,6 @@ func (h DatasetHandlers) bodyHandler(w http.ResponseWriter, r *http.Request) {

page := util.PageFromRequest(r)
path := result.Dataset.BodyPath
if p.UseFSI {
path = result.Dataset.Path
}

dataResponse := DataResponse{
Path: path,
Expand All @@ -668,13 +660,12 @@ func (h DatasetHandlers) bodyHandler(w http.ResponseWriter, r *http.Request) {
func (h DatasetHandlers) statsHandler(w http.ResponseWriter, r *http.Request) {
p := lib.GetParams{
Path: HTTPPathToQriPath(r.URL.Path[len("/stats/"):]),
UseFSI: r.FormValue("fsi") == "true",
Selector: "stats",
}
res := lib.GetResult{}
err := h.Get(&p, &res)
if err != nil {
if err == repo.ErrNoHistory || err == fsi.ErrNoLink {
if err == repo.ErrNoHistory {
util.WriteErrResponse(w, http.StatusUnprocessableEntity, err)
return
}
Expand Down
28 changes: 7 additions & 21 deletions api/fsi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

util "github.com/qri-io/apiutil"
"github.com/qri-io/dataset"
"github.com/qri-io/qri/fsi"
"github.com/qri-io/qri/lib"
"github.com/qri-io/qri/repo"
"github.com/qri-io/qri/repo/profile"
Expand Down Expand Up @@ -53,38 +54,24 @@ func (h *FSIHandlers) StatusHandler(routePrefix string) http.HandlerFunc {

func (h *FSIHandlers) statusHandler(routePrefix string) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
useFSI := r.FormValue("fsi") == "true"
ref, err := DatasetRefFromPath(r.URL.Path[len(routePrefix):])
if err != nil {
util.WriteErrResponse(w, http.StatusBadRequest, fmt.Errorf("bad reference: %s", err.Error()))
return
}

res := []lib.StatusItem{}
if useFSI {
alias := ref.AliasString()
err := h.StatusForAlias(&alias, &res)
// Won't return ErrNoHistory.
if err != nil {
util.WriteErrResponse(w, http.StatusInternalServerError, fmt.Errorf("error getting status: %s", err.Error()))
return
}
util.WriteResponse(w, res)
alias := ref.AliasString()
err = h.StatusForAlias(&alias, &res)
if err == fsi.ErrNoLink {
util.WriteErrResponse(w, http.StatusBadRequest, fmt.Errorf("no working directory: %s", alias))
return
}

// TODO(dustmop): This is going away in the future, switch to /whatchanged instead.
refStr := ref.String()
err = h.WhatChanged(&refStr, &res)
if err != nil {
if err == repo.ErrNoHistory {
util.WriteErrResponse(w, http.StatusUnprocessableEntity, err)
return
}
} else if err != nil {
util.WriteErrResponse(w, http.StatusInternalServerError, fmt.Errorf("error getting status: %s", err.Error()))
return
}
util.WriteResponse(w, res)
return
}
}

Expand Down Expand Up @@ -179,7 +166,6 @@ func (h *FSIHandlers) initHandler(routePrefix string) http.HandlerFunc {
// taken from ./root.go
gp := lib.GetParams{
Path: name,
UseFSI: true,
}
res := lib.GetResult{}
err := h.dsm.Get(&gp, &res)
Expand Down
106 changes: 46 additions & 60 deletions api/fsi_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"github.com/qri-io/qri/fsi"
"github.com/qri-io/qri/lib"
"github.com/qri-io/qri/p2p"
"github.com/qri-io/qri/repo"
reporef "github.com/qri-io/qri/repo/ref"
)

Expand Down Expand Up @@ -149,13 +148,6 @@ func TestNoHistory(t *testing.T) {
t.Errorf("expected ref to be \"peer/test_ds\", got \"%s\"", ref)
}

expectNoHistoryBody := map[string]interface{}{
"meta": map[string]interface{}{
"code": float64(422),
"error": repo.ErrNoHistory.Error(),
},
}

// Get mtimes for the component files
st, _ := os.Stat(filepath.Join(workDir, "meta.json"))
metaMtime := st.ModTime().Format(time.RFC3339)
Expand All @@ -166,110 +158,104 @@ func TestNoHistory(t *testing.T) {

dsHandler := NewDatasetHandlers(run.Inst, false)

// Expected response for dataset head, regardless of fsi parameter
expectBody := `{"data":{"peername":"peer","name":"test_ds","fsiPath":"fsi_init_dir","dataset":{"bodyPath":"fsi_init_dir/body.csv","meta":{"qri":"md:0"},"name":"test_ds","peername":"peer","qri":"ds:0","structure":{"format":"csv","qri":"st:0"}},"published":false},"meta":{"code":200}}`

// Dataset with a link to the filesystem, but no history and the api request says fsi=false
gotStatusCode, gotBodyString := APICall("/peer/test_ds", dsHandler.GetHandler)
if gotStatusCode != 422 {
t.Errorf("expected status code 422, got %d", gotStatusCode)
}

gotBody := map[string]interface{}{}
if err := json.Unmarshal([]byte(gotBodyString), &gotBody); err != nil {
t.Errorf("could not unmarshal response into struct: %s", err)
if gotStatusCode != 200 {
t.Errorf("expected status code 200, got %d", gotStatusCode)
}

if diff := cmp.Diff(expectNoHistoryBody, gotBody); diff != "" {
t.Errorf("expected body %v, got %v\ndiff:%v", expectNoHistoryBody, gotBody, diff)
actualBody := strings.Replace(gotBodyString, workDir, subDir, -1)
if diff := cmp.Diff(expectBody, actualBody); diff != "" {
t.Errorf("expected body %v, got %v\ndiff:%v", expectBody, actualBody, diff)
}

// Dataset with a link to the filesystem, but no history and the api request says fsi=true
gotStatusCode, gotBodyString = APICall("/peer/test_ds?fsi=true", dsHandler.GetHandler)
if gotStatusCode != 200 {
t.Errorf("expected status code 200, got %d", gotStatusCode)
}
// Handle temporary directory by replacing the temp part with a shorter string.
resultBody := strings.Replace(gotBodyString, workDir, subDir, -1)
expectBody := `{"data":{"peername":"peer","name":"test_ds","fsiPath":"fsi_init_dir","dataset":{"bodyPath":"fsi_init_dir/body.csv","meta":{"qri":"md:0"},"name":"test_ds","peername":"peer","qri":"ds:0","structure":{"format":"csv","qri":"st:0"}},"published":false},"meta":{"code":200}}`
if diff := cmp.Diff(expectBody, resultBody); diff != "" {
actualBody = strings.Replace(gotBodyString, workDir, subDir, -1)
if diff := cmp.Diff(expectBody, actualBody); diff != "" {
t.Errorf("api response (-want +got):\n%s", diff)
}

// Body with no history
// Expected response for body of the dataset
expectBody = `{"data":{"path":"fsi_init_dir/body.csv","data":[["one","two",3],["four","five",6]]},"meta":{"code":200},"pagination":{"nextUrl":"/body/peer/test_ds?page=2"}}`

// Body with no history, but fsi working directory has body
gotStatusCode, gotBodyString = APICall("/body/peer/test_ds", dsHandler.BodyHandler)
if gotStatusCode != 422 {
t.Errorf("expected status code 422, got %d", gotStatusCode)
}
if err := json.Unmarshal([]byte(gotBodyString), &gotBody); err != nil {
t.Errorf("could not unmarshal response into struct: %s", err)
if gotStatusCode != 200 {
t.Errorf("expected status code 200, got %d", gotStatusCode)
}

if diff := cmp.Diff(expectNoHistoryBody, gotBody); diff != "" {
t.Errorf("expected body %v, got %v\ndiff:%v", expectNoHistoryBody, gotBody, diff)
actualBody = strings.Replace(gotBodyString, workDir, subDir, -1)
if diff := cmp.Diff(expectBody, actualBody); diff != "" {
t.Errorf("expected body %v, got %v\ndiff:%v", expectBody, actualBody, diff)
}

// Body with no history, but FSI working directory has body
// Body with no history, but fsi working directory has body
gotStatusCode, gotBodyString = APICall("/body/peer/test_ds?fsi=true", dsHandler.BodyHandler)
if gotStatusCode != 200 {
t.Errorf("expected status code 200, got %d", gotStatusCode)
}
expectBody = `{"data":{"path":"","data":[["one","two",3],["four","five",6]]},"meta":{"code":200},"pagination":{"nextUrl":"/body/peer/test_ds?fsi=true\u0026page=2"}}`
if expectBody != gotBodyString {
t.Errorf("expected body %s, got %s", expectBody, gotBodyString)
actualBody = strings.Replace(gotBodyString, workDir, subDir, -1)
actualBody = strings.Replace(actualBody, `fsi=true\u0026`, "", -1)
if diff := cmp.Diff(expectBody, actualBody); diff != "" {
t.Errorf("expected body %v, got %v\ndiff:%v", expectBody, actualBody, diff)
}

fsiHandler := NewFSIHandlers(run.Inst, false)

// Expected response for status of the dataset
templateBody := `{"data":[{"sourceFile":"fsi_init_dir/meta.json","component":"meta","type":"add","message":"","mtime":"%s"},{"sourceFile":"fsi_init_dir/structure.json","component":"structure","type":"add","message":"","mtime":"%s"},{"sourceFile":"fsi_init_dir/body.csv","component":"body","type":"add","message":"","mtime":"%s"}],"meta":{"code":200}}`
expectBody = fmt.Sprintf(templateBody, metaMtime, structureMtime, bodyMtime)

// Status at version with no history
gotStatusCode, gotBodyString = APICall("/status/peer/test_ds", fsiHandler.StatusHandler("/status"))
if gotStatusCode != 422 {
t.Errorf("expected status code 422, got %d", gotStatusCode)
}
if err := json.Unmarshal([]byte(gotBodyString), &gotBody); err != nil {
t.Errorf("could not unmarshal response into struct: %s", err)
if gotStatusCode != 200 {
t.Errorf("expected status code 200, got %d", gotStatusCode)
}

if diff := cmp.Diff(expectNoHistoryBody, gotBody); diff != "" {
t.Errorf("expected body %v, got %v\ndiff:%v", expectNoHistoryBody, gotBody, diff)
actualBody = strings.Replace(gotBodyString, workDir, subDir, -1)
if diff := cmp.Diff(expectBody, actualBody); diff != "" {
t.Errorf("expected body %v, got %v\ndiff:%v", expectBody, actualBody, diff)
}

// Status with no history, but FSI working directory has contents
gotStatusCode, gotBodyString = APICall("/status/peer/test_ds?fsi=true", fsiHandler.StatusHandler("/status"))
if gotStatusCode != 200 {
t.Errorf("expected status code 200, got %d", gotStatusCode)
}
// Handle temporary directory by replacing the temp part with a shorter string.
resultBody = strings.Replace(gotBodyString, workDir, subDir, -1)
templateBody := `{"data":[{"sourceFile":"fsi_init_dir/meta.json","component":"meta","type":"add","message":"","mtime":"%s"},{"sourceFile":"fsi_init_dir/structure.json","component":"structure","type":"add","message":"","mtime":"%s"},{"sourceFile":"fsi_init_dir/body.csv","component":"body","type":"add","message":"","mtime":"%s"}],"meta":{"code":200}}`
expectBody = fmt.Sprintf(templateBody, metaMtime, structureMtime, bodyMtime)
if diff := cmp.Diff(expectBody, resultBody); diff != "" {
actualBody = strings.Replace(gotBodyString, workDir, subDir, -1)
if diff := cmp.Diff(expectBody, actualBody); diff != "" {
t.Errorf("api response (-want +got):\n%s", diff)
}

logHandler := NewLogHandlers(run.Node)

expectNoHistoryBody := `{
"meta": {
"code": 422,
"error": "repo: no history"
}
}`

// History with no history
gotStatusCode, gotBodyString = APICall("/history/peer/test_ds", logHandler.LogHandler)
if gotStatusCode != 422 {
t.Errorf("expected status code 422, got %d", gotStatusCode)
}
if err := json.Unmarshal([]byte(gotBodyString), &gotBody); err != nil {
t.Errorf("could not unmarshal response into struct: %s", err)
}

if diff := cmp.Diff(expectNoHistoryBody, gotBody); diff != "" {
t.Errorf("expected body %v, got %v\ndiff:%v", expectNoHistoryBody, gotBody, diff)
if diff := cmp.Diff(expectNoHistoryBody, gotBodyString); diff != "" {
t.Errorf("expected body %v, got %v\ndiff:%v", expectNoHistoryBody, gotBodyString, diff)
}

// History with no history, still returns ErrNoHistory since this route ignores fsi param
gotStatusCode, gotBodyString = APICall("/history/peer/test_ds?fsi=true", logHandler.LogHandler)
if gotStatusCode != 422 {
t.Errorf("expected status code 422, got %d", gotStatusCode)
}
if err := json.Unmarshal([]byte(gotBodyString), &gotBody); err != nil {
t.Errorf("could not unmarshal response into struct: %s", err)
}

if diff := cmp.Diff(expectNoHistoryBody, gotBody); diff != "" {
t.Errorf("expected body %v, got %v\ndiff:%v", expectNoHistoryBody, gotBody, diff)
if diff := cmp.Diff(expectNoHistoryBody, gotBodyString); diff != "" {
t.Errorf("expected body %v, got %v\ndiff:%v", expectNoHistoryBody, gotBodyString, diff)
}
}

Expand Down
1 change: 0 additions & 1 deletion api/root.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ func (mh *RootHandler) Handler(w http.ResponseWriter, r *http.Request) {

p := lib.GetParams{
Path: ref.String(),
UseFSI: r.FormValue("fsi") == "true",
}
res := lib.GetResult{}
err := mh.dsh.Get(&p, &res)
Expand Down
Binary file modified api/testdata/api.snapshot
View file
Binary file not shown.
12 changes: 5 additions & 7 deletions base/ref.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ func SetPublishStatus(r repo.Repo, ref *reporef.DatasetRef, published bool) erro
return r.PutRef(*ref)
}

// ToDatasetRef parses the dataset ref and returns it, allowing datasets with no history only
// if FSI is enabled.
func ToDatasetRef(path string, r repo.Repo, allowFSI bool) (*reporef.DatasetRef, error) {
// ToDatasetRef parses the dataset ref and looks it up in the refstore, allows refs with no history
// TODO(dustmop): In a future change, remove the third parameter from this function
func ToDatasetRef(path string, r repo.Repo, _ bool) (*reporef.DatasetRef, error) {
if path == "" {
return nil, repo.ErrEmptyRef
}
Expand All @@ -43,10 +43,8 @@ func ToDatasetRef(path string, r repo.Repo, allowFSI bool) (*reporef.DatasetRef,
return nil, fmt.Errorf("'%s' is not a valid dataset reference", path)
}
err = repo.CanonicalizeDatasetRef(r, &ref)
if err != nil {
if err != repo.ErrNoHistory || !allowFSI {
return nil, err
}
if err != nil && err != repo.ErrNoHistory {
return nil, err
}
return &ref, nil
}
Expand Down
1 change: 0 additions & 1 deletion cmd/get.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ func (o *GetOptions) Run() (err error) {
p := lib.GetParams{
Path: o.Refs.Ref(),
Selector: o.Selector,
UseFSI: o.Refs.IsLinked(),
Format: o.Format,
FormatConfig: fc,
Offset: page.Offset(),
Expand Down
10 changes: 3 additions & 7 deletions cmd/rename_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,10 @@ func TestRenameNoHistory(t *testing.T) {
t.Errorf("error mismatch, expect: %s, got: %s", expect, err.Error())
}

// New dataset name can be used, but still has no history
// New dataset name can be used
err = run.ExecCommand("qri get me/remove_second_name")
if err == nil {
t.Error("expected error, did not get one")
}
expect = "repo: no history"
if err.Error() != expect {
t.Errorf("error mismatch, expect: %s, got: %s", expect, err.Error())
if err != nil {
t.Errorf("unexpected error: %s", err)
}

// Read .qri-ref file, it contains the new reference name
Expand Down
Loading

0 comments on commit 16785a2

Please sign in to comment.