Skip to content

Commit

Permalink
fix(lib): Align input params across all lib methods
Browse files Browse the repository at this point in the history 
Modify names of all input param structs that are used for lib methods. This includes things like:

* Refstr -> Ref
* Component -> Selector
* Peername -> Username (except for p2p)
* Source moving to the scope (Remote remains for remote `pull` and `remove`)
* a few other small changes

Some outstanding work renames: the All flag from Get should be removed, but doing so will break functionality (such as /body.csv on the API) until Cursors exist. Also, there's some dscache specific tests that are being skipped, as I think we should be using some other mechanism to enable dscache instead of a field on the input params, such as an environmental variable or an option in the config file. Finally, ListParams need some additional work to combine them into a common struct.
  • Loading branch information
@dustmop
dustmop committed Apr 13, 2021
1 parent add7809 commit 7ba26b6
Show file tree
Hide file tree
Showing 51 changed files with 370 additions and 347 deletions.
41 changes: 20 additions & 21 deletions api/fsi_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestFSIHandlers(t *testing.T) {
initHandler := func(w http.ResponseWriter, r *http.Request) {
lib.NewHTTPRequestHandler(inst, "fsi.init").ServeHTTP(w, r)
}
body := []byte(fmt.Sprintf(`{"peername":"me","name":"api_test_init_dataset","targetDir":%q,"format":"csv"}`, initDir))
body := []byte(fmt.Sprintf(`{"username":"me","name":"api_test_init_dataset","targetDir":%q,"format":"csv"}`, initDir))
initCases := []handlerTestCase{
{"POST", "/", nil, nil},
{"POST", fmt.Sprintf("/me/api_test_init_dataset?targetdir=%s&format=csv", initDir), body, nil},
Expand Down Expand Up @@ -105,7 +105,7 @@ func TestNoHistory(t *testing.T) {
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("/get/peer/test_ds", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds"})
gotStatusCode, gotBodyString := APICall("/get/peer/test_ds", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds"})
if gotStatusCode != 200 {
t.Errorf("expected status code 200, got %d", gotStatusCode)
}
Expand All @@ -115,7 +115,7 @@ func TestNoHistory(t *testing.T) {
}

// Dataset with a link to the filesystem, but no history and the api request says fsi=true
gotStatusCode, gotBodyString = APICall("/get/peer/test_ds?fsi=true", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds"})
gotStatusCode, gotBodyString = APICall("/get/peer/test_ds?fsi=true", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds"})
if gotStatusCode != 200 {
t.Errorf("expected status code 200, got %d", gotStatusCode)
}
Expand All @@ -128,7 +128,7 @@ func TestNoHistory(t *testing.T) {
expectBody = `{"data":{"path":"fsi_init_dir/body.csv","data":[["one","two",3],["four","five",6]]},"meta":{"code":200},"pagination":{"page":1,"pageSize":50,"nextUrl":"/get/peer/test_ds/body?page=2","prevUrl":""}}`

// Body with no history, but fsi working directory has body
gotStatusCode, gotBodyString = APICall("/get/peer/test_ds/body", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds", "selector": "body"})
gotStatusCode, gotBodyString = APICall("/get/peer/test_ds/body", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds", "selector": "body"})
if gotStatusCode != 200 {
t.Errorf("expected status code 200, got %d", gotStatusCode)
}
Expand All @@ -138,7 +138,7 @@ func TestNoHistory(t *testing.T) {
}

// Body with no history, but fsi working directory has body
gotStatusCode, gotBodyString = APICall("/get/peer/test_ds/body&fsi=true", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds", "selector": "body"})
gotStatusCode, gotBodyString = APICall("/get/peer/test_ds/body&fsi=true", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds", "selector": "body"})
if gotStatusCode != 200 {
t.Errorf("expected status code 200, got %d", gotStatusCode)
}
Expand All @@ -157,7 +157,7 @@ func TestNoHistory(t *testing.T) {
expectBody = fmt.Sprintf(templateBody, metaMtime, structureMtime, bodyMtime)

// Status at version with no history
body := map[string]string{"refstr": "peer/test_ds"}
body := map[string]string{"ref": "peer/test_ds"}
gotStatusCode, gotBodyString = JSONAPICallWithBody("POST", "/status", body, statusHandler, nil)
if gotStatusCode != 200 {
t.Errorf("expected status code 200, got %d", gotStatusCode)
Expand Down Expand Up @@ -249,8 +249,8 @@ func TestFSIWrite(t *testing.T) {
"POST",
"/checkout",
map[string]string{
"refstr": "peer/write_test",
"dir": workDir,
"ref": "peer/write_test",
"dir": workDir,
},
checkoutHandler,
nil,
Expand All @@ -267,8 +267,8 @@ func TestFSIWrite(t *testing.T) {
muxVarsToQueryParamMiddleware(lib.NewHTTPRequestHandler(inst, "fsi.write")).ServeHTTP(w, r)
}
p := lib.FSIWriteParams{
Refstr: "peer/write_test",
Ds: &dataset.Dataset{Meta: &dataset.Meta{Title: "oh hai there"}},
Ref: "peer/write_test",
Dataset: &dataset.Dataset{Meta: &dataset.Meta{Title: "oh hai there"}},
}
status, strRes := JSONAPICallWithBody("POST", "/fsi/write/me/write_test", p, writeHandler, nil)

Expand Down Expand Up @@ -339,9 +339,8 @@ func TestCheckoutAndRestore(t *testing.T) {
t.Fatal(err)
}

// Save the path from reference for later.
// TODO(dlong): Support full dataset refs, not just the path.
ref1Path := res.Path
// Save the version from reference for later.
ref1Version := res.Path

// Save version 2 with a different title
saveParams = lib.SaveParams{
Expand All @@ -366,8 +365,8 @@ func TestCheckoutAndRestore(t *testing.T) {
"POST",
"/checkout",
map[string]string{
"refstr": "me/fsi_checkout_restore",
"dir": workDir,
"ref": "me/fsi_checkout_restore",
"dir": workDir,
},
checkoutHandler,
nil,
Expand Down Expand Up @@ -408,7 +407,7 @@ func TestCheckoutAndRestore(t *testing.T) {
}

// Status should show that meta is modified
body := map[string]string{"refstr": "peer/fsi_checkout_restore", "fsi": "true"}
body := map[string]string{"ref": "peer/fsi_checkout_restore", "fsi": "true"}
actualStatusCode, actualBody = JSONAPICallWithBody("POST", "/status/peer/fsi_checkout_restore?fsi=true", body, statusHandler, nil)
if actualStatusCode != 200 {
t.Errorf("expected status code 200, got %d", actualStatusCode)
Expand All @@ -429,8 +428,8 @@ func TestCheckoutAndRestore(t *testing.T) {
"POST",
"/restore",
map[string]string{
"refstr": "me/fsi_checkout_restore",
"component": "meta",
"ref": "me/fsi_checkout_restore",
"selector": "meta",
},
restoreHandler,
nil,
Expand Down Expand Up @@ -458,11 +457,11 @@ func TestCheckoutAndRestore(t *testing.T) {
"POST",
"/restore",
map[string]string{
"refstr": "me/fsi_checkout_restore",
"ref": "me/fsi_checkout_restore",
// TODO(dlong): Have to pass "dir" to this method. In the test, the ref does
// not have an FSIPath. Might be because we're using /map/, not sure.
"dir": workDir,
"path": ref1Path,
"dir": workDir,
"version": ref1Version,
},
restoreHandler,
nil,
Expand Down
18 changes: 9 additions & 9 deletions api/handlers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func TestGetZip(t *testing.T) {
run.SaveDataset(ds, "testdata/cities/data.csv")

// Get a zip file binary over the API
gotStatusCode, gotBodyString := APICall("/get/peer/test_ds?format=zip", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds"})
gotStatusCode, gotBodyString := APICall("/get/peer/test_ds?format=zip", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds"})
if gotStatusCode != 200 {
t.Fatalf("expected status code 200, got %d", gotStatusCode)
}
Expand All @@ -48,41 +48,41 @@ func TestDatasetGet(t *testing.T) {
}
run.SaveDataset(&ds, "testdata/cities/data.csv")

actualStatusCode, actualBody := APICall("/get/peer/test_ds", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds"})
actualStatusCode, actualBody := APICall("/get/peer/test_ds", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds"})
assertStatusCode(t, "get dataset", actualStatusCode, 200)
got := datasetJSONResponse(t, actualBody)
dstest.CompareGoldenDatasetAndUpdateIfEnvVarSet(t, "testdata/expect/TestDatasetGet.test_ds.json", got)

// Get csv body using "body.csv" suffix
actualStatusCode, actualBody = APICall("/get/peer/test_ds/body.csv", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds", "selector": "body.csv"})
actualStatusCode, actualBody = APICall("/get/peer/test_ds/body.csv", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds", "selector": "body.csv"})
expectBody := "city,pop,avg_age,in_usa\ntoronto,40000000,55.5,false\nnew york,8500000,44.4,true\nchicago,300000,44.4,true\nchatham,35000,65.25,true\nraleigh,250000,50.65,true\n"
assertStatusCode(t, "get body.csv using suffix", actualStatusCode, 200)
if diff := cmp.Diff(expectBody, actualBody); diff != "" {
t.Errorf("output mismatch (-want +got):\n%s", diff)
}

// Can get zip file
actualStatusCode, _ = APICall("/get/peer/test_ds?format=zip", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds"})
actualStatusCode, _ = APICall("/get/peer/test_ds?format=zip", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds"})
assertStatusCode(t, "get zip file", actualStatusCode, 200)

// Can get a single component
actualStatusCode, _ = APICall("/get/peer/test_ds/meta", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds", "selector": "meta"})
actualStatusCode, _ = APICall("/get/peer/test_ds/meta", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds", "selector": "meta"})
assertStatusCode(t, "get meta component", actualStatusCode, 200)

// Can get at an ipfs version
actualStatusCode, _ = APICall("/get/peer/test_ds/at/mem/QmeTvt83npHg4HoxL8bp8yz5bmG88hUVvRc5k9taW8uxTr", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds", "fs": "mem", "hash": "QmeTvt83npHg4HoxL8bp8yz5bmG88hUVvRc5k9taW8uxTr"})
actualStatusCode, _ = APICall("/get/peer/test_ds/at/mem/QmeTvt83npHg4HoxL8bp8yz5bmG88hUVvRc5k9taW8uxTr", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds", "fs": "mem", "hash": "QmeTvt83npHg4HoxL8bp8yz5bmG88hUVvRc5k9taW8uxTr"})
assertStatusCode(t, "get at content-addressed version", actualStatusCode, 200)

// Error 404 if ipfs version doesn't exist
actualStatusCode, _ = APICall("/get/peer/test_ds/at/mem/QmissingEJUqFWNfdiPTPtxyba6wf86TmbQe1nifpZCRH6", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds", "fs": "mem", "hash": "QmissingEJUqFWNfdiPTPtxyba6wf86TmbQe1nifpZCRH6"})
actualStatusCode, _ = APICall("/get/peer/test_ds/at/mem/QmissingEJUqFWNfdiPTPtxyba6wf86TmbQe1nifpZCRH6", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds", "fs": "mem", "hash": "QmissingEJUqFWNfdiPTPtxyba6wf86TmbQe1nifpZCRH6"})
assertStatusCode(t, "get missing content-addressed version", actualStatusCode, 404)

// Error 400 due to unknown component
actualStatusCode, _ = APICall("/get/peer/test_ds/dunno", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test_ds", "selector": "dunno"})
actualStatusCode, _ = APICall("/get/peer/test_ds/dunno", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test_ds", "selector": "dunno"})
assertStatusCode(t, "unknown component", actualStatusCode, 400)

// Error 400 due to parse error of dsref
actualStatusCode, _ = APICall("/get/peer/test+ds", GetHandler(run.Inst, ""), map[string]string{"peername": "peer", "name": "test+ds"})
actualStatusCode, _ = APICall("/get/peer/test+ds", GetHandler(run.Inst, ""), map[string]string{"username": "peer", "name": "test+ds"})
assertStatusCode(t, "invalid dsref", actualStatusCode, 400)
}

Expand Down
8 changes: 4 additions & 4 deletions api/middleware.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func muxVarsToQueryParamMiddleware(next http.Handler) http.Handler {
q := r.URL.Query()
for varName, val := range mux.Vars(r) {
if q.Get(varName) != "" {
util.WriteErrResponse(w, http.StatusBadRequest, fmt.Errorf("unrecognized query param: %s", varName))
util.WriteErrResponse(w, http.StatusBadRequest, fmt.Errorf("conflict in query param: %s = %s", varName, val))
return
}
q.Add(varName, val)
Expand All @@ -94,14 +94,14 @@ func refStringMiddleware(next http.Handler) http.Handler {
func setRefStringFromMuxVars(r *http.Request) {
mvars := mux.Vars(r)
ref := dsref.Ref{
Username: mvars["peername"],
Username: mvars["username"],
Name: mvars["name"],
Path: muxVarsPath(mvars),
}

if refstr := ref.String(); refstr != "" {
q := r.URL.Query()
q.Add("refstr", refstr)
q.Add("ref", refstr)
r.URL.RawQuery = q.Encode()
}
}
Expand All @@ -117,6 +117,6 @@ func muxVarsPath(mvars map[string]string) string {

func stripServerSideQueryParams(r *http.Request) {
q := r.URL.Query()
q.Del("refstr")
q.Del("ref")
r.URL.RawQuery = q.Encode()
}
36 changes: 18 additions & 18 deletions api/params_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,65 +20,65 @@ func TestUnmarshalGetParams(t *testing.T) {
"basic get",
"/get/peer/my_ds",
&lib.GetParams{
Refstr: "peer/my_ds",
Ref: "peer/my_ds",
Format: "json",
All: true,
},
map[string]string{"peername": "peer", "name": "my_ds"},
map[string]string{"username": "peer", "name": "my_ds"},
},
{
"meta component",
"/get/peer/my_ds/meta",
&lib.GetParams{
Refstr: "peer/my_ds",
Ref: "peer/my_ds",
Format: "json",
Selector: "meta",
All: true,
},
map[string]string{"peername": "peer", "name": "my_ds", "selector": "meta"},
map[string]string{"username": "peer", "name": "my_ds", "selector": "meta"},
},
{
"body component",
"/get/peer/my_ds/body",
&lib.GetParams{
Refstr: "peer/my_ds",
Ref: "peer/my_ds",
Format: "json",
Selector: "body",
All: true,
},
map[string]string{"peername": "peer", "name": "my_ds", "selector": "body"},
map[string]string{"username": "peer", "name": "my_ds", "selector": "body"},
},
{
"body.csv path suffix",
"/get/peer/my_ds/body.csv",
&lib.GetParams{
Refstr: "peer/my_ds",
Ref: "peer/my_ds",
Format: "csv",
Selector: "body",
All: true,
},
map[string]string{"peername": "peer", "name": "my_ds", "selector": "body.csv"},
map[string]string{"username": "peer", "name": "my_ds", "selector": "body.csv"},
},
{
"download body as csv",
"/get/peer/my_ds/body?format=csv",
&lib.GetParams{
Refstr: "peer/my_ds",
Ref: "peer/my_ds",
Format: "csv",
Selector: "body",
All: true,
},
map[string]string{"peername": "peer", "name": "my_ds", "selector": "body"},
map[string]string{"username": "peer", "name": "my_ds", "selector": "body"},
},
{
"zip format",
"/get/peer/my_ds?format=zip",
&lib.GetParams{
Refstr: "peer/my_ds",
Ref: "peer/my_ds",
Format: "zip",
All: true,
},
map[string]string{"peername": "peer", "name": "my_ds"},
map[string]string{"username": "peer", "name": "my_ds"},
},
}
for _, c := range cases {
Expand Down Expand Up @@ -110,13 +110,13 @@ func TestUnmarshalGetParams(t *testing.T) {
"get me",
"/get/me/my_ds",
`username "me" not allowed`,
map[string]string{"peername": "me", "name": "my_ds"},
map[string]string{"username": "me", "name": "my_ds"},
},
{
"bad parse",
"/get/peer/my+ds",
`unexpected character at position 7: '+'`,
map[string]string{"peername": "peer", "name": "my+ds"},
map[string]string{"username": "peer", "name": "my+ds"},
},
}
for i, c := range badCases {
Expand All @@ -143,15 +143,15 @@ func TestParseGetParamsAcceptHeader(t *testing.T) {
// Construct a request with "Accept: text/csv"
r, _ := http.NewRequest("GET", "/get/peer/my_ds", nil)
r.Header.Add("Accept", "text/csv")
r = mux.SetURLVars(r, map[string]string{"peername": "peer", "name": "my_ds"})
r = mux.SetURLVars(r, map[string]string{"username": "peer", "name": "my_ds"})
setRefStringFromMuxVars(r)
args := &lib.GetParams{}
err := UnmarshalParams(r, args)
if err != nil {
t.Fatal(err)
}
expectArgs := &lib.GetParams{
Refstr: "peer/my_ds",
Ref: "peer/my_ds",
Selector: "body",
Format: "csv",
All: true,
Expand All @@ -164,7 +164,7 @@ func TestParseGetParamsAcceptHeader(t *testing.T) {
// Construct a request with format=csv and "Accept: text/csv", which is ok
r, _ = http.NewRequest("GET", "/get/peer/my_ds?format=csv", nil)
r.Header.Add("Accept", "text/csv")
r = mux.SetURLVars(r, map[string]string{"peername": "peer", "name": "my_ds"})
r = mux.SetURLVars(r, map[string]string{"username": "peer", "name": "my_ds"})
setRefStringFromMuxVars(r)
args = &lib.GetParams{}
err = UnmarshalParams(r, args)
Expand All @@ -178,7 +178,7 @@ func TestParseGetParamsAcceptHeader(t *testing.T) {
// Construct a request with format=json and "Accept: text/csv", which is an error
r, _ = http.NewRequest("GET", "/get/peer/my_ds?format=json", nil)
r.Header.Add("Accept", "text/csv")
r = mux.SetURLVars(r, map[string]string{"peername": "peer", "name": "my_ds"})
r = mux.SetURLVars(r, map[string]string{"username": "peer", "name": "my_ds"})
setRefStringFromMuxVars(r)
args = &lib.GetParams{}
err = UnmarshalParams(r, args)
Expand Down
5 changes: 1 addition & 4 deletions base/dataset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func CloseDataset(ds *dataset.Dataset) (err error) {
}

// ListDatasets lists datasets from a repo
func ListDatasets(ctx context.Context, r repo.Repo, term, profileID string, offset, limit int, RPC, publishedOnly, showVersions bool) ([]dsref.VersionInfo, error) {
func ListDatasets(ctx context.Context, r repo.Repo, term, profileID string, offset, limit int, publishedOnly, showVersions bool) ([]dsref.VersionInfo, error) {
fs := r.Filesystem()
num, err := r.RefCount()
if err != nil {
Expand Down Expand Up @@ -228,9 +228,6 @@ func ListDatasets(ctx context.Context, r repo.Repo, term, profileID string, offs
ds.Peername = ref.Peername
ds.Name = ref.Name
ref.Dataset = ds
if RPC {
ref.Dataset.Structure.Schema = nil
}

if showVersions {
dsVersions, err := DatasetLog(ctx, r, reporef.ConvertToDsref(ref), 1000000, 0, false)
Expand Down
Loading

0 comments on commit 7ba26b6

Please sign in to comment.