diff --git a/api/datastore/internal/datastoretest/test.go b/api/datastore/internal/datastoretest/test.go index 12780d284..f4d1c7b67 100644 --- a/api/datastore/internal/datastoretest/test.go +++ b/api/datastore/internal/datastoretest/test.go @@ -4,15 +4,13 @@ import ( "bytes" "context" "log" + "net/http" + "reflect" "testing" + "time" "github.com/fnproject/fn/api/id" "github.com/fnproject/fn/api/models" - - "net/http" - "reflect" - "time" - "github.com/gin-gonic/gin" "github.com/go-openapi/strfmt" "github.com/sirupsen/logrus" @@ -28,7 +26,7 @@ func setLogBuffer() *bytes.Buffer { return &buf } -func Test(t *testing.T, ds models.Datastore) { +func Test(t *testing.T, dsf func() models.Datastore) { buf := setLogBuffer() ctx := context.Background() @@ -42,6 +40,7 @@ func Test(t *testing.T, ds models.Datastore) { call.Path = testRoute.Path t.Run("call-insert", func(t *testing.T) { + ds := dsf() call.ID = id.New().String() err := ds.InsertCall(ctx, call) if err != nil { @@ -51,6 +50,7 @@ func Test(t *testing.T, ds models.Datastore) { }) t.Run("call-get", func(t *testing.T) { + ds := dsf() call.ID = id.New().String() ds.InsertCall(ctx, call) newCall, err := ds.GetCall(ctx, call.AppName, call.ID) @@ -64,20 +64,120 @@ func Test(t *testing.T, ds models.Datastore) { }) t.Run("calls-get", func(t *testing.T) { - filter := &models.CallFilter{AppName: call.AppName, Path: call.Path} + ds := dsf() + filter := &models.CallFilter{AppName: call.AppName, Path: call.Path, PerPage: 100} call.ID = id.New().String() - ds.InsertCall(ctx, call) + call.CreatedAt = strfmt.DateTime(time.Now()) + err := ds.InsertCall(ctx, call) + if err != nil { + t.Fatal(err) + } calls, err := ds.GetCalls(ctx, filter) if err != nil { t.Fatalf("Test GetCalls(ctx, filter): unexpected error `%v`", err) } - if len(calls) == 0 { + if len(calls) != 1 { t.Log(buf.String()) + t.Fatalf("Test GetCalls(ctx, filter): unexpected length `%v`", len(calls)) + } + + c2 := *call + c3 := *call + c2.ID = id.New().String() + c2.CreatedAt = strfmt.DateTime(time.Now().Add(100 * time.Millisecond)) // add ms cuz db uses it for sort + c3.ID = id.New().String() + c3.CreatedAt = strfmt.DateTime(time.Now().Add(200 * time.Millisecond)) + + err = ds.InsertCall(ctx, &c2) + if err != nil { + t.Fatal(err) + } + err = ds.InsertCall(ctx, &c3) + if err != nil { + t.Fatal(err) + } + + // test that no filter works too + calls, err = ds.GetCalls(ctx, &models.CallFilter{PerPage: 100}) + if err != nil { t.Fatalf("Test GetCalls(ctx, filter): unexpected error `%v`", err) } + if len(calls) != 3 { + t.Log(buf.String()) + t.Fatalf("Test GetCalls(ctx, filter): unexpected length `%v`", len(calls)) + } + + // test that pagination stuff works. id, descending + filter.PerPage = 1 + calls, err = ds.GetCalls(ctx, filter) + if err != nil { + t.Fatalf("Test GetCalls(ctx, filter): unexpected error `%v`", err) + } + if len(calls) != 1 { + t.Log(buf.String()) + t.Fatalf("Test GetCalls(ctx, filter): unexpected length `%v`", len(calls)) + } else if calls[0].ID != c3.ID { + t.Log(buf.String()) + t.Fatalf("Test GetCalls: call ids not in expected order: %v %v", calls[0].ID, c3.ID) + } + + filter.PerPage = 100 + filter.Cursor = calls[0].ID + calls, err = ds.GetCalls(ctx, filter) + if err != nil { + t.Fatalf("Test GetCalls(ctx, filter): unexpected error `%v`", err) + } + if len(calls) != 2 { + t.Log(buf.String()) + t.Fatalf("Test GetCalls(ctx, filter): unexpected length `%v`", len(calls)) + } else if calls[0].ID != c2.ID { + t.Log(buf.String()) + t.Fatalf("Test GetCalls: call ids not in expected order: %v %v", calls[0].ID, c2.ID) + } else if calls[1].ID != call.ID { + t.Log(buf.String()) + t.Fatalf("Test GetCalls: call ids not in expected order: %v %v", calls[1].ID, call.ID) + } + + // test that filters actually applied + calls, err = ds.GetCalls(ctx, &models.CallFilter{AppName: "wrongappname", PerPage: 100}) + if err != nil { + t.Fatalf("Test GetCalls(ctx, filter): unexpected error `%v`", err) + } + if len(calls) != 0 { + t.Log(buf.String()) + t.Fatalf("Test GetCalls(ctx, filter): unexpected length `%v`", len(calls)) + } + + calls, err = ds.GetCalls(ctx, &models.CallFilter{Path: "wrongpath", PerPage: 100}) + if err != nil { + t.Fatalf("Test GetCalls(ctx, filter): unexpected error `%v`", err) + } + if len(calls) != 0 { + t.Log(buf.String()) + t.Fatalf("Test GetCalls(ctx, filter): unexpected length `%v`", len(calls)) + } + + // make sure from_time and to_time work + filter = &models.CallFilter{ + PerPage: 100, + FromTime: call.CreatedAt, + ToTime: c3.CreatedAt, + } + calls, err = ds.GetCalls(ctx, filter) + if err != nil { + t.Fatalf("Test GetCalls(ctx, filter): unexpected error `%v`", err) + } + if len(calls) != 1 { + t.Log(buf.String()) + t.Fatalf("Test GetCalls(ctx, filter): unexpected length `%v`", len(calls)) + } else if calls[0].ID != c2.ID { + t.Log(buf.String()) + t.Fatalf("Test GetCalls: call id not expected", calls[0].ID, c2.ID) + } }) t.Run("apps", func(t *testing.T) { + ds := dsf() // Testing insert app _, err := ds.InsertApp(ctx, nil) if err != models.ErrDatastoreEmptyApp { @@ -166,7 +266,7 @@ func Test(t *testing.T, ds models.Datastore) { } // Testing list apps - apps, err := ds.GetApps(ctx, &models.AppFilter{}) + apps, err := ds.GetApps(ctx, &models.AppFilter{PerPage: 100}) if err != nil { t.Log(buf.String()) t.Fatalf("Test GetApps: unexpected error %v", err) @@ -179,15 +279,74 @@ func Test(t *testing.T, ds models.Datastore) { t.Fatalf("Test GetApps: expected `app.Name` to be `%s` but it was `%s`", app.Name, testApp.Name) } - apps, err = ds.GetApps(ctx, &models.AppFilter{Name: "Tes%"}) + // test pagination stuff (ordering / limits / cursoring) + a2 := *testApp + a3 := *testApp + a2.Name = "Testa" + a3.Name = "Testb" + if _, err = ds.InsertApp(ctx, &a2); err != nil { + t.Fatal(err) + } + if _, err = ds.InsertApp(ctx, &a3); err != nil { + t.Fatal(err) + } + + apps, err = ds.GetApps(ctx, &models.AppFilter{PerPage: 1}) if err != nil { t.Log(buf.String()) - t.Fatalf("Test GetApps(filter): unexpected error %v", err) + t.Fatalf("Test GetApps: error: %s", err) } - if len(apps) == 0 { - t.Fatal("Test GetApps(filter): expected result count to be greater than 0") + if len(apps) != 1 { + t.Fatalf("Test GetApps: expected result count to be 1 but got %d", len(apps)) + } else if apps[0].Name != testApp.Name { + t.Log(buf.String()) + t.Fatalf("Test GetApps: expected `app.Name` to be `%s` but it was `%s`", testApp.Name, apps[0].Name) } + apps, err = ds.GetApps(ctx, &models.AppFilter{PerPage: 100, Cursor: apps[0].Name}) + if err != nil { + t.Log(buf.String()) + t.Fatalf("Test GetApps: error: %s", err) + } + if len(apps) != 2 { + t.Fatalf("Test GetApps: expected result count to be 2 but got %d", len(apps)) + } else if apps[0].Name != a2.Name { + t.Log(buf.String()) + t.Fatalf("Test GetApps: expected `app.Name` to be `%s` but it was `%s`", a2.Name, apps[0].Name) + } else if apps[1].Name != a3.Name { + t.Log(buf.String()) + t.Fatalf("Test GetApps: expected `app.Name` to be `%s` but it was `%s`", a3.Name, apps[1].Name) + } + + a4 := *testApp + a4.Name = "Abcdefg" // < /test lexicographically, but not in length + + if _, err = ds.InsertApp(ctx, &a4); err != nil { + t.Fatal(err) + } + + apps, err = ds.GetApps(ctx, &models.AppFilter{PerPage: 100}) + if err != nil { + t.Log(buf.String()) + t.Fatalf("Test GetApps: error: %s", err) + } + if len(apps) != 4 { + t.Fatalf("Test GetApps: expected result count to be 4 but got %d", len(apps)) + } else if apps[0].Name != a4.Name { + t.Log(buf.String()) + t.Fatalf("Test GetApps: expected `app.Name` to be `%s` but it was `%s`", a4.Name, apps[0].Name) + } + + // TODO fix up prefix stuff + //apps, err = ds.GetApps(ctx, &models.AppFilter{Name: "Tes"}) + //if err != nil { + //t.Log(buf.String()) + //t.Fatalf("Test GetApps(filter): unexpected error %v", err) + //} + //if len(apps) != 3 { + //t.Fatal("Test GetApps(filter): expected result count to be 3, got", len(apps)) + //} + // Testing app delete err = ds.RemoveApp(ctx, "") if err != models.ErrDatastoreEmptyAppName { @@ -224,6 +383,7 @@ func Test(t *testing.T, ds models.Datastore) { }) t.Run("routes", func(t *testing.T) { + ds := dsf() // Insert app again to test routes _, err := ds.InsertApp(ctx, testApp) if err != nil && err != models.ErrAppsAlreadyExists { @@ -374,7 +534,7 @@ func Test(t *testing.T, ds models.Datastore) { } // Testing list routes - routes, err := ds.GetRoutesByApp(ctx, testApp.Name, &models.RouteFilter{}) + routes, err := ds.GetRoutesByApp(ctx, testApp.Name, &models.RouteFilter{PerPage: 1}) if err != nil { t.Log(buf.String()) t.Fatalf("Test GetRoutesByApp: unexpected error %v", err) @@ -390,7 +550,7 @@ func Test(t *testing.T, ds models.Datastore) { t.Fatalf("Test GetRoutes: expected `app.Name` to be `%s` but it was `%s`", testRoute.Path, routes[0].Path) } - routes, err = ds.GetRoutesByApp(ctx, testApp.Name, &models.RouteFilter{Image: testRoute.Image}) + routes, err = ds.GetRoutesByApp(ctx, testApp.Name, &models.RouteFilter{Image: testRoute.Image, PerPage: 1}) if err != nil { t.Log(buf.String()) t.Fatalf("Test GetRoutesByApp: unexpected error %v", err) @@ -400,13 +560,13 @@ func Test(t *testing.T, ds models.Datastore) { } if routes[0] == nil { t.Log(buf.String()) - t.Fatalf("Test GetRoutes: expected non-nil route") + t.Fatalf("Test GetRoutesByApp: expected non-nil route") } else if routes[0].Path != testRoute.Path { t.Log(buf.String()) - t.Fatalf("Test GetRoutes: expected `app.Name` to be `%s` but it was `%s`", testRoute.Path, routes[0].Path) + t.Fatalf("Test GetRoutesByApp: expected `route.Path` to be `%s` but it was `%s`", testRoute.Path, routes[0].Path) } - routes, err = ds.GetRoutesByApp(ctx, "notreal", nil) + routes, err = ds.GetRoutesByApp(ctx, "notreal", &models.RouteFilter{PerPage: 1}) if err != nil { t.Log(buf.String()) t.Fatalf("Test GetRoutesByApp: error: %s", err) @@ -415,20 +575,68 @@ func Test(t *testing.T, ds models.Datastore) { t.Fatalf("Test GetRoutesByApp: expected result count to be 0 but got %d", len(routes)) } - // Testing list routes - routes, err = ds.GetRoutes(ctx, &models.RouteFilter{Image: testRoute.Image}) + // test pagination stuff + r2 := *testRoute + r3 := *testRoute + r2.Path = "/testa" + r3.Path = "/testb" + + if _, err = ds.InsertRoute(ctx, &r2); err != nil { + t.Fatal(err) + } + if _, err = ds.InsertRoute(ctx, &r3); err != nil { + t.Fatal(err) + } + + routes, err = ds.GetRoutesByApp(ctx, testApp.Name, &models.RouteFilter{PerPage: 1}) if err != nil { t.Log(buf.String()) - t.Fatalf("Test GetRoutes: error: %s", err) + t.Fatalf("Test GetRoutesByApp: error: %s", err) } - if len(routes) == 0 { - t.Fatal("Test GetRoutes: expected result count to be greater than 0") - } - if routes[0].Path != testRoute.Path { + if len(routes) != 1 { + t.Fatalf("Test GetRoutesByApp: expected result count to be 1 but got %d", len(routes)) + } else if routes[0].Path != testRoute.Path { t.Log(buf.String()) - t.Fatalf("Test GetRoutes: expected `app.Name` to be `%s` but it was `%s`", testRoute.Path, routes[0].Path) + t.Fatalf("Test GetRoutesByApp: expected `route.Path` to be `%s` but it was `%s`", testRoute.Path, routes[0].Path) } + routes, err = ds.GetRoutesByApp(ctx, testApp.Name, &models.RouteFilter{PerPage: 2, Cursor: routes[0].Path}) + if err != nil { + t.Log(buf.String()) + t.Fatalf("Test GetRoutesByApp: error: %s", err) + } + if len(routes) != 2 { + t.Fatalf("Test GetRoutesByApp: expected result count to be 2 but got %d", len(routes)) + } else if routes[0].Path != r2.Path { + t.Log(buf.String()) + t.Fatalf("Test GetRoutesByApp: expected `route.Path` to be `%s` but it was `%s`", r2.Path, routes[0].Path) + } else if routes[1].Path != r3.Path { + t.Log(buf.String()) + t.Fatalf("Test GetRoutesByApp: expected `route.Path` to be `%s` but it was `%s`", r3.Path, routes[1].Path) + } + + r4 := *testRoute + r4.Path = "/abcdefg" // < /test lexicographically, but not in length + + if _, err = ds.InsertRoute(ctx, &r4); err != nil { + t.Fatal(err) + } + + routes, err = ds.GetRoutesByApp(ctx, testApp.Name, &models.RouteFilter{PerPage: 100}) + if err != nil { + t.Log(buf.String()) + t.Fatalf("Test GetRoutesByApp: error: %s", err) + } + if len(routes) != 4 { + t.Fatalf("Test GetRoutesByApp: expected result count to be 4 but got %d", len(routes)) + } else if routes[0].Path != r4.Path { + t.Log(buf.String()) + t.Fatalf("Test GetRoutesByApp: expected `route.Path` to be `%s` but it was `%s`", r4.Path, routes[0].Path) + } + + // TODO test weird ordering possibilities ? + // TODO test prefix filtering + // Testing route delete err = ds.RemoveRoute(ctx, "", "") if err != models.ErrDatastoreEmptyAppName { diff --git a/api/datastore/internal/datastoreutil/metrics.go b/api/datastore/internal/datastoreutil/metrics.go index 34c79f042..d3b733d8e 100644 --- a/api/datastore/internal/datastoreutil/metrics.go +++ b/api/datastore/internal/datastoreutil/metrics.go @@ -53,12 +53,6 @@ func (m *metricds) GetRoute(ctx context.Context, appName, routePath string) (*mo return m.ds.GetRoute(ctx, appName, routePath) } -func (m *metricds) GetRoutes(ctx context.Context, filter *models.RouteFilter) (routes []*models.Route, err error) { - span, ctx := opentracing.StartSpanFromContext(ctx, "ds_get_routes") - defer span.Finish() - return m.ds.GetRoutes(ctx, filter) -} - func (m *metricds) GetRoutesByApp(ctx context.Context, appName string, filter *models.RouteFilter) (routes []*models.Route, err error) { span, ctx := opentracing.StartSpanFromContext(ctx, "ds_get_routes_by_app") defer span.Finish() diff --git a/api/datastore/internal/datastoreutil/validator.go b/api/datastore/internal/datastoreutil/validator.go index c08728820..a95c79612 100644 --- a/api/datastore/internal/datastoreutil/validator.go +++ b/api/datastore/internal/datastoreutil/validator.go @@ -73,14 +73,6 @@ func (v *validator) GetRoute(ctx context.Context, appName, routePath string) (*m return v.Datastore.GetRoute(ctx, appName, routePath) } -func (v *validator) GetRoutes(ctx context.Context, routeFilter *models.RouteFilter) (routes []*models.Route, err error) { - if routeFilter != nil && routeFilter.AppName != "" { - return v.Datastore.GetRoutesByApp(ctx, routeFilter.AppName, routeFilter) - } - - return v.Datastore.GetRoutes(ctx, routeFilter) -} - // appName will never be empty func (v *validator) GetRoutesByApp(ctx context.Context, appName string, routeFilter *models.RouteFilter) (routes []*models.Route, err error) { if appName == "" { diff --git a/api/datastore/mock.go b/api/datastore/mock.go index 2aa6f9b33..420338a57 100644 --- a/api/datastore/mock.go +++ b/api/datastore/mock.go @@ -2,12 +2,14 @@ package datastore import ( "context" - - "github.com/jmoiron/sqlx" + "sort" + "strings" + "time" "github.com/fnproject/fn/api/datastore/internal/datastoreutil" "github.com/fnproject/fn/api/logs" "github.com/fnproject/fn/api/models" + "github.com/jmoiron/sqlx" ) type mock struct { @@ -37,8 +39,27 @@ func (m *mock) GetApp(ctx context.Context, appName string) (app *models.App, err return nil, models.ErrAppsNotFound } +type sortA []*models.App + +func (s sortA) Len() int { return len(s) } +func (s sortA) Less(i, j int) bool { return strings.Compare(s[i].Name, s[j].Name) < 0 } +func (s sortA) Swap(i, j int) { s[i], s[j] = s[j], s[i] } + func (m *mock) GetApps(ctx context.Context, appFilter *models.AppFilter) ([]*models.App, error) { - return m.Apps, nil + // sort them all first for cursoring (this is for testing, n is small & mock is not concurrent..) + sort.Sort(sortA(m.Apps)) + + var apps []*models.App + for _, a := range m.Apps { + if len(apps) == appFilter.PerPage { + break + } + if strings.Compare(appFilter.Cursor, a.Name) < 0 { + apps = append(apps, a) + } + } + + return apps, nil } func (m *mock) InsertApp(ctx context.Context, app *models.App) (*models.App, error) { @@ -80,16 +101,26 @@ func (m *mock) GetRoute(ctx context.Context, appName, routePath string) (*models return nil, models.ErrRoutesNotFound } -func (m *mock) GetRoutes(ctx context.Context, routeFilter *models.RouteFilter) (routes []*models.Route, err error) { - for _, r := range m.Routes { - routes = append(routes, r) - } - return -} +type sortR []*models.Route + +func (s sortR) Len() int { return len(s) } +func (s sortR) Less(i, j int) bool { return strings.Compare(s[i].Path, s[j].Path) < 0 } +func (s sortR) Swap(i, j int) { s[i], s[j] = s[j], s[i] } func (m *mock) GetRoutesByApp(ctx context.Context, appName string, routeFilter *models.RouteFilter) (routes []*models.Route, err error) { + // sort them all first for cursoring (this is for testing, n is small & mock is not concurrent..) + sort.Sort(sortR(m.Routes)) + for _, r := range m.Routes { - if r.AppName == appName && (routeFilter.Path == "" || r.Path == routeFilter.Path) && (routeFilter.AppName == "" || r.AppName == routeFilter.AppName) { + if len(routes) == routeFilter.PerPage { + break + } + + if r.AppName == appName && + //strings.HasPrefix(r.Path, routeFilter.PathPrefix) && // TODO + (routeFilter.Image == "" || routeFilter.Image == r.Image) && + strings.Compare(routeFilter.Cursor, r.Path) < 0 { + routes = append(routes, r) } } @@ -147,7 +178,7 @@ func (m *mock) InsertCall(ctx context.Context, call *models.Call) error { func (m *mock) GetCall(ctx context.Context, appName, callID string) (*models.Call, error) { for _, t := range m.Calls { - if t.ID == callID { + if t.ID == callID && t.AppName == appName { return t, nil } } @@ -155,8 +186,34 @@ func (m *mock) GetCall(ctx context.Context, appName, callID string) (*models.Cal return nil, models.ErrCallNotFound } +type sortC []*models.Call + +func (s sortC) Len() int { return len(s) } +func (s sortC) Less(i, j int) bool { return strings.Compare(s[i].ID, s[j].ID) < 0 } +func (s sortC) Swap(i, j int) { s[i], s[j] = s[j], s[i] } + func (m *mock) GetCalls(ctx context.Context, filter *models.CallFilter) ([]*models.Call, error) { - return m.Calls, nil + // sort them all first for cursoring (this is for testing, n is small & mock is not concurrent..) + // calls are in DESC order so use sort.Reverse + sort.Sort(sort.Reverse(sortC(m.Calls))) + + var calls []*models.Call + for _, c := range m.Calls { + if len(calls) == filter.PerPage { + break + } + + if (filter.AppName == "" || c.AppName == filter.AppName) && + (filter.Path == "" || filter.Path == c.Path) && + (time.Time(filter.FromTime).IsZero() || time.Time(filter.FromTime).Before(time.Time(c.CreatedAt))) && + (time.Time(filter.ToTime).IsZero() || time.Time(c.CreatedAt).Before(time.Time(filter.ToTime))) && + (filter.Cursor == "" || strings.Compare(filter.Cursor, c.ID) > 0) { + + calls = append(calls, c) + } + } + + return calls, nil } func (m *mock) batchDeleteCalls(ctx context.Context, appName string) error { diff --git a/api/datastore/mock_test.go b/api/datastore/mock_test.go index b3442da35..7e48ea04f 100644 --- a/api/datastore/mock_test.go +++ b/api/datastore/mock_test.go @@ -7,5 +7,5 @@ import ( ) func TestDatastore(t *testing.T) { - datastoretest.Test(t, NewMock()) + datastoretest.Test(t, NewMock) } diff --git a/api/datastore/sql/sql.go b/api/datastore/sql/sql.go index a24b3fe3f..3a6b5f637 100644 --- a/api/datastore/sql/sql.go +++ b/api/datastore/sql/sql.go @@ -12,6 +12,7 @@ import ( "os" "path/filepath" "strings" + "time" "github.com/fnproject/fn/api/models" "github.com/go-sql-driver/mysql" @@ -471,54 +472,23 @@ func (ds *sqlStore) GetRoute(ctx context.Context, appName, routePath string) (*m return &route, nil } -// GetRoutes retrieves an array of routes according to a specific filter. -func (ds *sqlStore) GetRoutes(ctx context.Context, filter *models.RouteFilter) ([]*models.Route, error) { - res := []*models.Route{} - query, args := buildFilterRouteQuery(filter) - query = fmt.Sprintf("%s %s", routeSelector, query) - query = ds.db.Rebind(query) - rows, err := ds.db.QueryContext(ctx, query, args...) - // todo: check for no rows so we don't respond with a sql 500 err - if err != nil { - return nil, err - } - defer rows.Close() - - for rows.Next() { - var route models.Route - err := scanRoute(rows, &route) - if err != nil { - continue - } - res = append(res, &route) - - } - if err := rows.Err(); err != nil { - return nil, err - } - return res, nil -} - -/* -GetRoutesByApp retrieves a route with a specific app name. -*/ +// GetRoutesByApp retrieves a route with a specific app name. func (ds *sqlStore) GetRoutesByApp(ctx context.Context, appName string, filter *models.RouteFilter) ([]*models.Route, error) { res := []*models.Route{} - var filterQuery string - var args []interface{} if filter == nil { - filterQuery = "WHERE app_name = ?" - args = []interface{}{appName} - } else { - filter.AppName = appName - filterQuery, args = buildFilterRouteQuery(filter) + filter = new(models.RouteFilter) } + filter.AppName = appName + filterQuery, args := buildFilterRouteQuery(filter) + query := fmt.Sprintf("%s %s", routeSelector, filterQuery) query = ds.db.Rebind(query) rows, err := ds.db.QueryContext(ctx, query, args...) - // todo: check for no rows so we don't respond with a sql 500 err if err != nil { + if err == sql.ErrNoRows { + return res, nil // no error for empty list + } return nil, err } defer rows.Close() @@ -533,7 +503,9 @@ func (ds *sqlStore) GetRoutesByApp(ctx context.Context, appName string, filter * } if err := rows.Err(); err != nil { - return nil, err + if err == sql.ErrNoRows { + return res, nil // no error for empty list + } } return res, nil @@ -739,26 +711,52 @@ func buildFilterRouteQuery(filter *models.RouteFilter) (string, []interface{}) { if val != "" { args = append(args, val) if len(args) == 1 { - fmt.Fprintf(&b, `WHERE %s?`, colOp) + fmt.Fprintf(&b, `WHERE %s`, colOp) } else { - fmt.Fprintf(&b, ` AND %s?`, colOp) + fmt.Fprintf(&b, ` AND %s`, colOp) } } } - where("path=", filter.Path) - where("app_name=", filter.AppName) - where("image=", filter.Image) + where("app_name=? ", filter.AppName) + where("image=?", filter.Image) + where("path>?", filter.Cursor) + // where("path LIKE ?%", filter.PathPrefix) TODO needs escaping + + fmt.Fprintf(&b, ` ORDER BY path ASC`) // TODO assert this is indexed + fmt.Fprintf(&b, ` LIMIT ?`) + args = append(args, filter.PerPage) return b.String(), args } func buildFilterAppQuery(filter *models.AppFilter) (string, []interface{}) { - if filter == nil || filter.Name == "" { + if filter == nil { return "", nil } - return "WHERE name LIKE ?", []interface{}{filter.Name} + var b bytes.Buffer + var args []interface{} + + where := func(colOp, val string) { + if val != "" { + args = append(args, val) + if len(args) == 1 { + fmt.Fprintf(&b, `WHERE %s`, colOp) + } else { + fmt.Fprintf(&b, ` AND %s`, colOp) + } + } + } + + // where("name LIKE ?%", filter.Name) // TODO needs escaping? + where("name>?", filter.Cursor) + + fmt.Fprintf(&b, ` ORDER BY name ASC`) // TODO assert this is indexed + fmt.Fprintf(&b, ` LIMIT ?`) + args = append(args, filter.PerPage) + + return b.String(), args } func buildFilterCallQuery(filter *models.CallFilter) (string, []interface{}) { @@ -779,11 +777,19 @@ func buildFilterCallQuery(filter *models.CallFilter) (string, []interface{}) { } } - where("app_name=", filter.AppName) - - if filter.Path != "" { - where("path=", filter.Path) + where("id<", filter.Cursor) + if !time.Time(filter.ToTime).IsZero() { + where("created_at<", filter.ToTime.String()) } + if !time.Time(filter.FromTime).IsZero() { + where("created_at>", filter.FromTime.String()) + } + where("app_name=", filter.AppName) + where("path=", filter.Path) + + fmt.Fprintf(&b, ` ORDER BY id DESC`) // TODO assert this is indexed + fmt.Fprintf(&b, ` LIMIT ?`) + args = append(args, filter.PerPage) return b.String(), args } diff --git a/api/datastore/sql/sql_test.go b/api/datastore/sql/sql_test.go new file mode 100644 index 000000000..bf238e27c --- /dev/null +++ b/api/datastore/sql/sql_test.go @@ -0,0 +1,29 @@ +package sql + +import ( + "net/url" + "os" + "testing" + + "github.com/fnproject/fn/api/datastore/internal/datastoretest" + "github.com/fnproject/fn/api/datastore/internal/datastoreutil" + "github.com/fnproject/fn/api/models" +) + +func TestDatastore(t *testing.T) { + defer os.RemoveAll("sqlite_test_dir") + u, err := url.Parse("sqlite3://sqlite_test_dir") + if err != nil { + t.Fatal(err) + } + f := func() models.Datastore { + os.RemoveAll("sqlite_test_dir") + ds, err := New(u) + if err != nil { + t.Fatal(err) + } + // we don't want to test the validator, really + return datastoreutil.NewValidator(ds) + } + datastoretest.Test(t, f) +} diff --git a/api/models/app.go b/api/models/app.go index 36d9773bc..54c01d0bd 100644 --- a/api/models/app.go +++ b/api/models/app.go @@ -55,6 +55,7 @@ func (a *App) UpdateConfig(patch Config) { } type AppFilter struct { - // An SQL LIKE query. Empty does not filter. - Name string + Name string // prefix query TODO implemented + PerPage int + Cursor string } diff --git a/api/models/call.go b/api/models/call.go index 8a33cb3d6..5cd27d2ee 100644 --- a/api/models/call.go +++ b/api/models/call.go @@ -1,7 +1,7 @@ package models import ( - strfmt "github.com/go-openapi/strfmt" + "github.com/go-openapi/strfmt" ) const ( @@ -130,6 +130,10 @@ type Call struct { } type CallFilter struct { - Path string - AppName string + Path string // match + AppName string // match + FromTime strfmt.DateTime + ToTime strfmt.DateTime + Cursor string + PerPage int } diff --git a/api/models/datastore.go b/api/models/datastore.go index b77ba6b48..2322b8cdd 100644 --- a/api/models/datastore.go +++ b/api/models/datastore.go @@ -36,9 +36,6 @@ type Datastore interface { // Returns ErrRoutesNotFound when no matching route is found. GetRoute(ctx context.Context, appName, routePath string) (*Route, error) - // GetRoutes gets a slice of Routes, optionally filtered by filter. - GetRoutes(ctx context.Context, filter *RouteFilter) ([]*Route, error) - // GetRoutesByApp gets a slice of routes for a appName, optionally filtering on filter (filter.AppName is ignored). // Returns ErrDatastoreEmptyAppName if appName is empty. GetRoutesByApp(ctx context.Context, appName string, filter *RouteFilter) ([]*Route, error) diff --git a/api/models/error.go b/api/models/error.go index eb9b40350..6cb64d27c 100644 --- a/api/models/error.go +++ b/api/models/error.go @@ -96,51 +96,59 @@ var ( code: http.StatusConflict, error: errors.New("Could not update route - path is immutable"), } - ErrRoutesValidationFoundDynamicURL = err{ + ErrFoundDynamicURL = err{ code: http.StatusBadRequest, error: errors.New("Dynamic URL is not allowed"), } - ErrRoutesValidationInvalidPath = err{ + ErrInvalidPath = err{ code: http.StatusBadRequest, error: errors.New("Invalid Path format"), } - ErrRoutesValidationInvalidType = err{ + ErrInvalidType = err{ code: http.StatusBadRequest, error: errors.New("Invalid route Type"), } - ErrRoutesValidationInvalidFormat = err{ + ErrInvalidFormat = err{ code: http.StatusBadRequest, error: errors.New("Invalid route Format"), } - ErrRoutesValidationMissingAppName = err{ + ErrMissingAppName = err{ code: http.StatusBadRequest, error: errors.New("Missing route AppName"), } - ErrRoutesValidationMissingImage = err{ + ErrMissingImage = err{ code: http.StatusBadRequest, error: errors.New("Missing route Image"), } - ErrRoutesValidationMissingName = err{ + ErrMissingName = err{ code: http.StatusBadRequest, error: errors.New("Missing route Name"), } - ErrRoutesValidationMissingPath = err{ + ErrMissingPath = err{ code: http.StatusBadRequest, error: errors.New("Missing route Path"), } - ErrRoutesValidationMissingType = err{ + ErrMissingType = err{ code: http.StatusBadRequest, error: errors.New("Missing route Type"), } - ErrRoutesValidationPathMalformed = err{ + ErrPathMalformed = err{ code: http.StatusBadRequest, error: errors.New("Path malformed"), } - ErrRoutesValidationNegativeTimeout = err{ + ErrInvalidToTime = err{ + code: http.StatusBadRequest, + error: errors.New("to_time is not an epoch time"), + } + ErrInvalidFromTime = err{ + code: http.StatusBadRequest, + error: errors.New("from_time is not an epoch time"), + } + ErrNegativeTimeout = err{ code: http.StatusBadRequest, error: errors.New("Negative timeout"), } - ErrRoutesValidationNegativeIdleTimeout = err{ + ErrNegativeIdleTimeout = err{ code: http.StatusBadRequest, error: errors.New("Negative idle timeout"), } diff --git a/api/models/route.go b/api/models/route.go index 16262e7e7..e287ff8c1 100644 --- a/api/models/route.go +++ b/api/models/route.go @@ -63,51 +63,51 @@ func (r *Route) SetDefaults() { func (r *Route) Validate(skipZero bool) error { if !skipZero { if r.AppName == "" { - return ErrRoutesValidationMissingAppName + return ErrMissingAppName } if r.Path == "" { - return ErrRoutesValidationMissingPath + return ErrMissingPath } if r.Image == "" { - return ErrRoutesValidationMissingImage + return ErrMissingImage } } if !skipZero || r.Path != "" { u, err := url.Parse(r.Path) if err != nil { - return ErrRoutesValidationPathMalformed + return ErrPathMalformed } if strings.Contains(u.Path, ":") { - return ErrRoutesValidationFoundDynamicURL + return ErrFoundDynamicURL } if !path.IsAbs(u.Path) { - return ErrRoutesValidationInvalidPath + return ErrInvalidPath } } if !skipZero || r.Type != "" { if r.Type != TypeAsync && r.Type != TypeSync { - return ErrRoutesValidationInvalidType + return ErrInvalidType } } if !skipZero || r.Format != "" { if r.Format != FormatDefault && r.Format != FormatHTTP { - return ErrRoutesValidationInvalidFormat + return ErrInvalidFormat } } if r.Timeout < 0 { - return ErrRoutesValidationNegativeTimeout + return ErrNegativeTimeout } if r.IdleTimeout < 0 { - return ErrRoutesValidationNegativeIdleTimeout + return ErrNegativeIdleTimeout } return nil @@ -168,9 +168,11 @@ func (r *Route) Update(new *Route) { } } -//TODO are these sql LIKE queries? or strict matches? type RouteFilter struct { - Path string - AppName string - Image string + PathPrefix string // this is prefix match TODO + AppName string // this is exact match (important for security) + Image string // this is exact match + + Cursor string + PerPage int } diff --git a/api/server/apps_list.go b/api/server/apps_list.go index cbf19d8a8..453e801bd 100644 --- a/api/server/apps_list.go +++ b/api/server/apps_list.go @@ -1,6 +1,7 @@ package server import ( + "encoding/base64" "net/http" "github.com/fnproject/fn/api/models" @@ -10,13 +11,24 @@ import ( func (s *Server) handleAppList(c *gin.Context) { ctx := c.Request.Context() - filter := &models.AppFilter{} + var filter models.AppFilter + filter.Cursor, filter.PerPage = pageParams(c, true) - apps, err := s.Datastore.GetApps(ctx, filter) + apps, err := s.Datastore.GetApps(ctx, &filter) if err != nil { handleErrorResponse(c, err) return } - c.JSON(http.StatusOK, appsResponse{"Successfully listed applications", apps}) + var nextCursor string + if len(apps) > 0 && len(apps) == filter.PerPage { + last := []byte(apps[len(apps)-1].Name) + nextCursor = base64.RawURLEncoding.EncodeToString(last) + } + + c.JSON(http.StatusOK, appsResponse{ + Message: "Successfully listed applications", + NextCursor: nextCursor, + Apps: apps, + }) } diff --git a/api/server/apps_test.go b/api/server/apps_test.go index 18f508164..a8b6834ad 100644 --- a/api/server/apps_test.go +++ b/api/server/apps_test.go @@ -2,6 +2,8 @@ package server import ( "bytes" + "encoding/base64" + "encoding/json" "log" "net/http" "strings" @@ -120,17 +122,36 @@ func TestAppList(t *testing.T) { rnr, cancel := testRunner(t) defer cancel() - ds := datastore.NewMock() + ds := datastore.NewMockInit( + []*models.App{ + {Name: "myapp"}, + {Name: "myapp2"}, + {Name: "myapp3"}, + }, + nil, // no routes + nil, // no calls + ) fnl := logs.NewMock() srv := testServer(ds, &mqs.Mock{}, fnl, rnr) + a1b := base64.RawURLEncoding.EncodeToString([]byte("myapp")) + a2b := base64.RawURLEncoding.EncodeToString([]byte("myapp2")) + a3b := base64.RawURLEncoding.EncodeToString([]byte("myapp3")) + for i, test := range []struct { path string body string expectedCode int expectedError error + expectedLen int + nextCursor string }{ - {"/v1/apps", "", http.StatusOK, nil}, + {"/v1/apps?per_page", "", http.StatusOK, nil, 3, ""}, + {"/v1/apps?per_page=1", "", http.StatusOK, nil, 1, a1b}, + {"/v1/apps?per_page=1&cursor=" + a1b, "", http.StatusOK, nil, 1, a2b}, + {"/v1/apps?per_page=1&cursor=" + a2b, "", http.StatusOK, nil, 1, a3b}, + {"/v1/apps?per_page=100&cursor=" + a2b, "", http.StatusOK, nil, 1, ""}, // cursor is empty if per_page > len(results) + {"/v1/apps?per_page=1&cursor=" + a3b, "", http.StatusOK, nil, 0, ""}, // cursor could point to empty page } { _, rec := routerRequest(t, srv.Router, "GET", test.path, nil) @@ -148,6 +169,20 @@ func TestAppList(t *testing.T) { t.Errorf("Test %d: Expected error message to have `%s`", i, test.expectedError.Error()) } + } else { + // normal path + + var resp appsResponse + err := json.NewDecoder(rec.Body).Decode(&resp) + if err != nil { + t.Errorf("Test %d: Expected response body to be a valid json object. err: %v", i, err) + } + if len(resp.Apps) != test.expectedLen { + t.Errorf("Test %d: Expected apps length to be %d, but got %d", i, test.expectedLen, len(resp.Apps)) + } + if resp.NextCursor != test.nextCursor { + t.Errorf("Test %d: Expected next_cursor to be %s, but got %s", i, test.nextCursor, resp.NextCursor) + } } } } diff --git a/api/server/call_get.go b/api/server/call_get.go index c07d648fa..a23e479f1 100644 --- a/api/server/call_get.go +++ b/api/server/call_get.go @@ -18,5 +18,5 @@ func (s *Server) handleCallGet(c *gin.Context) { return } - c.JSON(http.StatusOK, fnCallResponse{"Successfully loaded call", callObj}) + c.JSON(http.StatusOK, callResponse{"Successfully loaded call", callObj}) } diff --git a/api/server/call_list.go b/api/server/call_list.go index 868d7c691..b61ac12ea 100644 --- a/api/server/call_list.go +++ b/api/server/call_list.go @@ -2,45 +2,80 @@ package server import ( "net/http" + "strconv" + "time" "github.com/fnproject/fn/api" "github.com/fnproject/fn/api/models" "github.com/gin-gonic/gin" + "github.com/go-openapi/strfmt" ) func (s *Server) handleCallList(c *gin.Context) { ctx := c.Request.Context() - name, ok := c.Get(api.AppName) - appName, conv := name.(string) - if ok && conv && appName == "" { - handleErrorResponse(c, models.ErrRoutesValidationMissingAppName) - return - } + appName := c.MustGet(api.AppName).(string) - filter := models.CallFilter{AppName: appName, Path: c.Query(api.CRoute)} + // TODO api.CRoute needs to be escaped probably, since it has '/' a lot + filter := models.CallFilter{AppName: appName, Path: c.Query("path")} + filter.Cursor, filter.PerPage = pageParams(c, false) // ids are url safe - calls, err := s.Datastore.GetCalls(ctx, &filter) + var err error + filter.FromTime, filter.ToTime, err = timeParams(c) if err != nil { handleErrorResponse(c, err) return } - if len(calls) == 0 { - _, err = s.Datastore.GetApp(c, appName) - if err != nil { - handleErrorResponse(c, err) - return - } + calls, err := s.Datastore.GetCalls(ctx, &filter) - if filter.Path != "" { - _, err = s.Datastore.GetRoute(c, appName, filter.Path) - if err != nil { - handleErrorResponse(c, err) - return - } - } + if len(calls) == 0 { + // TODO this should be done in front of this handler to even get here... + _, err = s.Datastore.GetApp(c, appName) } - c.JSON(http.StatusOK, fnCallsResponse{"Successfully listed calls", calls}) + if err != nil { + handleErrorResponse(c, err) + return + } + + var nextCursor string + if len(calls) > 0 && len(calls) == filter.PerPage { + nextCursor = calls[len(calls)-1].ID + // don't base64, IDs are url safe + } + + c.JSON(http.StatusOK, callsResponse{ + Message: "Successfully listed calls", + NextCursor: nextCursor, + Calls: calls, + }) +} + +// "" gets parsed to a zero time, which is fine (ignored in query) +func timeParams(c *gin.Context) (fromTime, toTime strfmt.DateTime, err error) { + fromStr := c.Query("from_time") + toStr := c.Query("to_time") + var ok bool + if fromStr != "" { + fromTime, ok = strToTime(fromStr) + if !ok { + return fromTime, toTime, models.ErrInvalidFromTime + } + } + if toStr != "" { + toTime, ok = strToTime(toStr) + if !ok { + return fromTime, toTime, models.ErrInvalidToTime + } + } + return fromTime, toTime, nil +} + +func strToTime(str string) (strfmt.DateTime, bool) { + sec, err := strconv.ParseInt(str, 10, 64) + if err != nil { + return strfmt.DateTime(time.Time{}), false + } + return strfmt.DateTime(time.Unix(sec, 0)), true } diff --git a/api/server/call_logs.go b/api/server/call_logs.go index 0f9ef83ae..d4b3eee0b 100644 --- a/api/server/call_logs.go +++ b/api/server/call_logs.go @@ -24,7 +24,7 @@ func (s *Server) handleCallLogGet(c *gin.Context) { return } - c.JSON(http.StatusOK, fnCallLogResponse{"Successfully loaded call", callObj}) + c.JSON(http.StatusOK, callLogResponse{"Successfully loaded call", callObj}) } func (s *Server) handleCallLogDelete(c *gin.Context) { diff --git a/api/server/calls_test.go b/api/server/calls_test.go new file mode 100644 index 000000000..53228e1fb --- /dev/null +++ b/api/server/calls_test.go @@ -0,0 +1,190 @@ +package server + +import ( + "encoding/json" + "fmt" + "net/http" + "strings" + "testing" + "time" + + "github.com/fnproject/fn/api/datastore" + "github.com/fnproject/fn/api/id" + "github.com/fnproject/fn/api/logs" + "github.com/fnproject/fn/api/models" + "github.com/fnproject/fn/api/mqs" + "github.com/go-openapi/strfmt" +) + +func TestCallGet(t *testing.T) { + buf := setLogBuffer() + + call := &models.Call{ + ID: id.New().String(), + AppName: "myapp", + Path: "/thisisatest", + Image: "fnproject/hello", + // Delay: 0, + Type: "sync", + Format: "default", + // Payload: TODO, + Priority: new(int32), // TODO this is crucial, apparently + Timeout: 30, + IdleTimeout: 30, + Memory: 256, + BaseEnv: map[string]string{"YO": "DAWG"}, + EnvVars: map[string]string{"YO": "DAWG"}, + CreatedAt: strfmt.DateTime(time.Now()), + URL: "http://localhost:8080/r/myapp/thisisatest", + Method: "GET", + } + + rnr, cancel := testRunner(t) + defer cancel() + ds := datastore.NewMockInit( + []*models.App{ + {Name: call.AppName}, + }, + nil, + []*models.Call{call}, + ) + fnl := logs.NewMock() + srv := testServer(ds, &mqs.Mock{}, fnl, rnr) + + for i, test := range []struct { + path string + body string + expectedCode int + expectedError error + }{ + {"/v1/apps//calls/" + call.ID, "", http.StatusBadRequest, models.ErrMissingAppName}, + {"/v1/apps/nodawg/calls/" + call.ID, "", http.StatusNotFound, models.ErrCallNotFound}, // TODO a little weird + {"/v1/apps/myapp/calls/" + call.ID[:3], "", http.StatusNotFound, models.ErrCallNotFound}, + {"/v1/apps/myapp/calls/" + call.ID, "", http.StatusOK, nil}, + } { + _, rec := routerRequest(t, srv.Router, "GET", test.path, nil) + + if rec.Code != test.expectedCode { + t.Log(buf.String()) + t.Errorf("Test %d: Expected status code to be %d but was %d", + i, test.expectedCode, rec.Code) + } + + if test.expectedError != nil { + resp := getErrorResponse(t, rec) + + if !strings.Contains(resp.Error.Message, test.expectedError.Error()) { + t.Log(buf.String()) + t.Errorf("Test %d: Expected error message to have `%s`", + i, test.expectedError.Error()) + } + } + // TODO json parse the body and assert fields + } +} + +func TestCallList(t *testing.T) { + buf := setLogBuffer() + + call := &models.Call{ + ID: id.New().String(), + AppName: "myapp", + Path: "/thisisatest", + Image: "fnproject/hello", + // Delay: 0, + Type: "sync", + Format: "default", + // Payload: TODO, + Priority: new(int32), // TODO this is crucial, apparently + Timeout: 30, + IdleTimeout: 30, + Memory: 256, + BaseEnv: map[string]string{"YO": "DAWG"}, + EnvVars: map[string]string{"YO": "DAWG"}, + CreatedAt: strfmt.DateTime(time.Now()), + URL: "http://localhost:8080/r/myapp/thisisatest", + Method: "GET", + } + c2 := *call + c3 := *call + c2.ID = id.New().String() + c2.CreatedAt = strfmt.DateTime(time.Now().Add(100 * time.Second)) + c2.Path = "test2" + c3.ID = id.New().String() + c3.CreatedAt = strfmt.DateTime(time.Now().Add(200 * time.Second)) + c3.Path = "/test3" + + rnr, cancel := testRunner(t) + defer cancel() + ds := datastore.NewMockInit( + []*models.App{ + {Name: call.AppName}, + }, + nil, + []*models.Call{call, &c2, &c3}, + ) + fnl := logs.NewMock() + srv := testServer(ds, &mqs.Mock{}, fnl, rnr) + + // add / sub 1 second b/c unix time will lop off millis and mess up our comparisons + rangeTest := fmt.Sprintf("from_time=%d&to_time=%d", + time.Time(call.CreatedAt).Add(1*time.Second).Unix(), + time.Time(c3.CreatedAt).Add(-1*time.Second).Unix(), + ) + + for i, test := range []struct { + path string + body string + expectedCode int + expectedError error + expectedLen int + nextCursor string + }{ + {"/v1/apps//calls", "", http.StatusBadRequest, models.ErrMissingAppName, 0, ""}, + {"/v1/apps/nodawg/calls", "", http.StatusNotFound, models.ErrAppsNotFound, 0, ""}, + {"/v1/apps/myapp/calls", "", http.StatusOK, nil, 3, ""}, + {"/v1/apps/myapp/calls?per_page=1", "", http.StatusOK, nil, 1, c3.ID}, + {"/v1/apps/myapp/calls?per_page=1&cursor=" + c3.ID, "", http.StatusOK, nil, 1, c2.ID}, + {"/v1/apps/myapp/calls?per_page=1&cursor=" + c2.ID, "", http.StatusOK, nil, 1, call.ID}, + {"/v1/apps/myapp/calls?per_page=100&cursor=" + c2.ID, "", http.StatusOK, nil, 1, ""}, // cursor is empty if per_page > len(results) + {"/v1/apps/myapp/calls?per_page=1&cursor=" + call.ID, "", http.StatusOK, nil, 0, ""}, // cursor could point to empty page + {"/v1/apps/myapp/calls?" + rangeTest, "", http.StatusOK, nil, 1, ""}, + {"/v1/apps/myapp/calls?from_time=xyz", "", http.StatusBadRequest, models.ErrInvalidFromTime, 0, ""}, + {"/v1/apps/myapp/calls?to_time=xyz", "", http.StatusBadRequest, models.ErrInvalidToTime, 0, ""}, + + // TODO path isn't url safe w/ '/', so this is weird. hack in for tests + {"/v1/apps/myapp/calls?path=test2", "", http.StatusOK, nil, 1, ""}, + } { + _, rec := routerRequest(t, srv.Router, "GET", test.path, nil) + + if rec.Code != test.expectedCode { + t.Log(buf.String()) + t.Errorf("Test %d: Expected status code to be %d but was %d", + i, test.expectedCode, rec.Code) + } + + if test.expectedError != nil { + resp := getErrorResponse(t, rec) + + if resp.Error == nil || !strings.Contains(resp.Error.Message, test.expectedError.Error()) { + t.Log(buf.String()) + t.Errorf("Test %d: Expected error message to have `%s`, got: `%s`", + i, test.expectedError.Error(), resp.Error) + } + } else { + // normal path + + var resp callsResponse + err := json.NewDecoder(rec.Body).Decode(&resp) + if err != nil { + t.Errorf("Test %d: Expected response body to be a valid json object. err: %v", i, err) + } + if len(resp.Calls) != test.expectedLen { + t.Fatalf("Test %d: Expected apps length to be %d, but got %d", i, test.expectedLen, len(resp.Calls)) + } + if resp.NextCursor != test.nextCursor { + t.Errorf("Test %d: Expected next_cursor to be %s, but got %s", i, test.nextCursor, resp.NextCursor) + } + } + } +} diff --git a/api/server/routes_create_update.go b/api/server/routes_create_update.go index 5bab63813..5e65b4dd9 100644 --- a/api/server/routes_create_update.go +++ b/api/server/routes_create_update.go @@ -163,7 +163,7 @@ func bindRoute(c *gin.Context, method string, wroute *models.RouteWrapper) error } if method == http.MethodPost { if wroute.Route.Path == "" { - return models.ErrRoutesValidationMissingPath + return models.ErrMissingPath } } return nil diff --git a/api/server/routes_list.go b/api/server/routes_list.go index 4e2add242..2aa2c9ab8 100644 --- a/api/server/routes_list.go +++ b/api/server/routes_list.go @@ -1,6 +1,7 @@ package server import ( + "encoding/base64" "net/http" "github.com/fnproject/fn/api" @@ -11,24 +12,20 @@ import ( func (s *Server) handleRouteList(c *gin.Context) { ctx := c.Request.Context() - filter := &models.RouteFilter{} + appName := c.MustGet(api.AppName).(string) - if img := c.Query("image"); img != "" { - filter.Image = img - } + var filter models.RouteFilter + filter.Image = c.Query("image") + // filter.PathPrefix = c.Query("path_prefix") TODO not hooked up + filter.Cursor, filter.PerPage = pageParams(c, true) - var routes []*models.Route - var err error - appName, exists := c.Get(api.AppName) - name, ok := appName.(string) - if exists && ok && name != "" { - routes, err = s.Datastore.GetRoutesByApp(ctx, name, filter) - // if there are no routes for the app, check if the app exists to return 404 if it does not - if len(routes) == 0 { - _, err = s.Datastore.GetApp(ctx, name) - } - } else { - routes, err = s.Datastore.GetRoutes(ctx, filter) + routes, err := s.Datastore.GetRoutesByApp(ctx, appName, &filter) + + // if there are no routes for the app, check if the app exists to return + // 404 if it does not + // TODO this should be done in front of this handler to even get here... + if err == nil && len(routes) == 0 { + _, err = s.Datastore.GetApp(ctx, appName) } if err != nil { @@ -36,5 +33,15 @@ func (s *Server) handleRouteList(c *gin.Context) { return } - c.JSON(http.StatusOK, routesResponse{"Successfully listed routes", routes}) + var nextCursor string + if len(routes) > 0 && len(routes) == filter.PerPage { + last := []byte(routes[len(routes)-1].Path) + nextCursor = base64.RawURLEncoding.EncodeToString(last) + } + + c.JSON(http.StatusOK, routesResponse{ + Message: "Successfully listed routes", + NextCursor: nextCursor, + Routes: routes, + }) } diff --git a/api/server/routes_test.go b/api/server/routes_test.go index bee59f435..b26441348 100644 --- a/api/server/routes_test.go +++ b/api/server/routes_test.go @@ -2,6 +2,8 @@ package server import ( "bytes" + "encoding/base64" + "encoding/json" "net/http" "strings" "testing" @@ -59,10 +61,10 @@ func TestRouteCreate(t *testing.T) { {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", ``, http.StatusBadRequest, models.ErrInvalidJSON}, {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "type": "sync" }`, http.StatusBadRequest, models.ErrRoutesMissingNew}, {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "path": "/myroute", "type": "sync" }`, http.StatusBadRequest, models.ErrRoutesMissingNew}, - {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { } }`, http.StatusBadRequest, models.ErrRoutesValidationMissingPath}, - {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "path": "/myroute", "type": "sync" } }`, http.StatusBadRequest, models.ErrRoutesValidationMissingImage}, - {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "image": "fnproject/hello", "type": "sync" } }`, http.StatusBadRequest, models.ErrRoutesValidationMissingPath}, - {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "image": "fnproject/hello", "path": "myroute", "type": "sync" } }`, http.StatusBadRequest, models.ErrRoutesValidationInvalidPath}, + {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { } }`, http.StatusBadRequest, models.ErrMissingPath}, + {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "path": "/myroute", "type": "sync" } }`, http.StatusBadRequest, models.ErrMissingImage}, + {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "image": "fnproject/hello", "type": "sync" } }`, http.StatusBadRequest, models.ErrMissingPath}, + {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "image": "fnproject/hello", "path": "myroute", "type": "sync" } }`, http.StatusBadRequest, models.ErrInvalidPath}, {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/$/routes", `{ "route": { "image": "fnproject/hello", "path": "/myroute", "type": "sync" } }`, http.StatusBadRequest, models.ErrAppsValidationInvalidName}, {datastore.NewMockInit(nil, []*models.Route{ @@ -87,13 +89,13 @@ func TestRoutePut(t *testing.T) { // errors (NOTE: this route doesn't exist yet) {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ }`, http.StatusBadRequest, models.ErrRoutesMissingNew}, {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "path": "/myroute", "type": "sync" }`, http.StatusBadRequest, models.ErrRoutesMissingNew}, - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "type": "sync" } }`, http.StatusBadRequest, models.ErrRoutesValidationMissingImage}, - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "path": "/myroute", "type": "sync" } }`, http.StatusBadRequest, models.ErrRoutesValidationMissingImage}, + {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "type": "sync" } }`, http.StatusBadRequest, models.ErrMissingImage}, + {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "path": "/myroute", "type": "sync" } }`, http.StatusBadRequest, models.ErrMissingImage}, {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "fnproject/hello", "path": "myroute", "type": "sync" } }`, http.StatusConflict, models.ErrRoutesPathImmutable}, {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "fnproject/hello", "path": "diffRoute", "type": "sync" } }`, http.StatusConflict, models.ErrRoutesPathImmutable}, {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/$/routes/myroute", `{ "route": { "image": "fnproject/hello", "path": "/myroute", "type": "sync" } }`, http.StatusBadRequest, models.ErrAppsValidationInvalidName}, - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "fnproject/hello", "path": "/myroute", "type": "invalid-type" } }`, http.StatusBadRequest, models.ErrRoutesValidationInvalidType}, - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "fnproject/hello", "path": "/myroute", "format": "invalid-format", "type": "sync" } }`, http.StatusBadRequest, models.ErrRoutesValidationInvalidFormat}, + {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "fnproject/hello", "path": "/myroute", "type": "invalid-type" } }`, http.StatusBadRequest, models.ErrInvalidType}, + {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "fnproject/hello", "path": "/myroute", "format": "invalid-format", "type": "sync" } }`, http.StatusBadRequest, models.ErrInvalidFormat}, // success {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "fnproject/hello", "path": "/myroute", "type": "sync" } }`, http.StatusOK, nil}, @@ -149,18 +151,53 @@ func TestRouteList(t *testing.T) { rnr, cancel := testRunner(t) defer cancel() - ds := datastore.NewMock() + ds := datastore.NewMockInit( + []*models.App{ + {Name: "myapp"}, + }, + []*models.Route{ + { + AppName: "myapp", + Path: "/myroute", + }, + { + AppName: "myapp", + Path: "/myroute1", + }, + { + AppName: "myapp", + Path: "/myroute2", + Image: "fnproject/hello", + }, + }, + nil, // no calls + ) fnl := logs.NewMock() + r1b := base64.RawURLEncoding.EncodeToString([]byte("/myroute")) + r2b := base64.RawURLEncoding.EncodeToString([]byte("/myroute1")) + r3b := base64.RawURLEncoding.EncodeToString([]byte("/myroute2")) + srv := testServer(ds, &mqs.Mock{}, fnl, rnr) for i, test := range []struct { - path string - body string + path string + body string + expectedCode int expectedError error + expectedLen int + nextCursor string }{ - {"/v1/apps/a/routes", "", http.StatusNotFound, models.ErrAppsNotFound}, + {"/v1/apps//routes", "", http.StatusBadRequest, models.ErrMissingAppName, 0, ""}, + {"/v1/apps/a/routes", "", http.StatusNotFound, models.ErrAppsNotFound, 0, ""}, + {"/v1/apps/myapp/routes", "", http.StatusOK, nil, 3, ""}, + {"/v1/apps/myapp/routes?per_page=1", "", http.StatusOK, nil, 1, r1b}, + {"/v1/apps/myapp/routes?per_page=1&cursor=" + r1b, "", http.StatusOK, nil, 1, r2b}, + {"/v1/apps/myapp/routes?per_page=1&cursor=" + r2b, "", http.StatusOK, nil, 1, r3b}, + {"/v1/apps/myapp/routes?per_page=100&cursor=" + r2b, "", http.StatusOK, nil, 1, ""}, // cursor is empty if per_page > len(results) + {"/v1/apps/myapp/routes?per_page=1&cursor=" + r3b, "", http.StatusOK, nil, 0, ""}, // cursor could point to empty page + {"/v1/apps/myapp/routes?image=fnproject/hello", "", http.StatusOK, nil, 1, ""}, } { _, rec := routerRequest(t, srv.Router, "GET", test.path, nil) @@ -178,6 +215,20 @@ func TestRouteList(t *testing.T) { t.Errorf("Test %d: Expected error message to have `%s`", i, test.expectedError.Error()) } + } else { + // normal path + + var resp routesResponse + err := json.NewDecoder(rec.Body).Decode(&resp) + if err != nil { + t.Errorf("Test %d: Expected response body to be a valid json object. err: %v", i, err) + } + if len(resp.Routes) != test.expectedLen { + t.Errorf("Test %d: Expected route length to be %d, but got %d", i, test.expectedLen, len(resp.Routes)) + } + if resp.NextCursor != test.nextCursor { + t.Errorf("Test %d: Expected next_cursor to be %s, but got %s", i, test.nextCursor, resp.NextCursor) + } } } } @@ -228,8 +279,8 @@ func TestRouteUpdate(t *testing.T) { // errors {datastore.NewMock(), logs.NewMock(), http.MethodPatch, "/v1/apps/a/routes/myroute/do", ``, http.StatusBadRequest, models.ErrInvalidJSON}, {datastore.NewMock(), logs.NewMock(), http.MethodPatch, "/v1/apps/a/routes/myroute/do", `{}`, http.StatusBadRequest, models.ErrRoutesMissingNew}, - {datastore.NewMock(), logs.NewMock(), http.MethodPatch, "/v1/apps/a/routes/myroute/do", `{ "route": { "type": "invalid-type" } }`, http.StatusBadRequest, models.ErrRoutesValidationInvalidType}, - {datastore.NewMock(), logs.NewMock(), http.MethodPatch, "/v1/apps/a/routes/myroute/do", `{ "route": { "format": "invalid-format" } }`, http.StatusBadRequest, models.ErrRoutesValidationInvalidFormat}, + {datastore.NewMock(), logs.NewMock(), http.MethodPatch, "/v1/apps/a/routes/myroute/do", `{ "route": { "type": "invalid-type" } }`, http.StatusBadRequest, models.ErrInvalidType}, + {datastore.NewMock(), logs.NewMock(), http.MethodPatch, "/v1/apps/a/routes/myroute/do", `{ "route": { "format": "invalid-format" } }`, http.StatusBadRequest, models.ErrInvalidFormat}, // success {datastore.NewMockInit(nil, diff --git a/api/server/server.go b/api/server/server.go index 7010b811a..395364c91 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -3,12 +3,14 @@ package server import ( "bytes" "context" + "encoding/base64" "errors" "fmt" "net" "net/http" "os" "path" + "strconv" "github.com/fnproject/fn/api" "github.com/fnproject/fn/api/agent" @@ -210,6 +212,16 @@ func loggerWrap(c *gin.Context) { c.Next() } +func appWrap(c *gin.Context) { + appName := c.GetString(api.AppName) + if appName == "" { + handleErrorResponse(c, models.ErrMissingAppName) + c.Abort() + return + } + c.Next() +} + func (s *Server) handleRunnerRequest(c *gin.Context) { s.handleRequest(c) } @@ -269,18 +281,20 @@ func (s *Server) bindHandlers(ctx context.Context) { engine.GET("/version", handleVersion) engine.GET("/stats", s.handleStats) - v1 := engine.Group("/v1") - v1.Use(s.middlewareWrapperFunc(ctx)) { + v1 := engine.Group("/v1") + v1.Use(s.middlewareWrapperFunc(ctx)) v1.GET("/apps", s.handleAppList) v1.POST("/apps", s.handleAppCreate) - v1.GET("/apps/:app", s.handleAppGet) - v1.PATCH("/apps/:app", s.handleAppUpdate) - v1.DELETE("/apps/:app", s.handleAppDelete) - - apps := v1.Group("/apps/:app") { + apps := v1.Group("/apps/:app") + apps.Use(appWrap) + + apps.GET("", s.handleAppGet) + apps.PATCH("", s.handleAppUpdate) + apps.DELETE("", s.handleAppDelete) + apps.GET("/routes", s.handleRouteList) apps.POST("/routes", s.handleRoutesPostPutPatch) apps.GET("/routes/*route", s.handleRouteGet) @@ -293,12 +307,15 @@ func (s *Server) bindHandlers(ctx context.Context) { apps.GET("/calls/:call", s.handleCallGet) apps.GET("/calls/:call/log", s.handleCallLogGet) apps.DELETE("/calls/:call/log", s.handleCallLogDelete) - } } - engine.Any("/r/:app", s.handleRunnerRequest) - engine.Any("/r/:app/*route", s.handleRunnerRequest) + { + runner := engine.Group("/r") + runner.Use(appWrap) + runner.Any("/:app", s.handleRunnerRequest) + runner.Any("/:app/*route", s.handleRunnerRequest) + } engine.NoRoute(func(c *gin.Context) { logrus.Debugln("not found", c.Request.URL.Path) @@ -306,14 +323,34 @@ func (s *Server) bindHandlers(ctx context.Context) { }) } +// returns the unescaped ?cursor and ?perPage values +// pageParams clamps 0 < ?perPage <= 100 and defaults to 30 if 0 +// ignores parsing errors and falls back to defaults. +func pageParams(c *gin.Context, base64d bool) (cursor string, perPage int) { + cursor = c.Query("cursor") + if base64d { + cbytes, _ := base64.RawURLEncoding.DecodeString(cursor) + cursor = string(cbytes) + } + + perPage, _ = strconv.Atoi(c.Query("per_page")) + if perPage > 100 { + perPage = 100 + } else if perPage <= 0 { + perPage = 30 + } + return cursor, perPage +} + type appResponse struct { Message string `json:"message"` App *models.App `json:"app"` } type appsResponse struct { - Message string `json:"message"` - Apps []*models.App `json:"apps"` + Message string `json:"message"` + NextCursor string `json:"next_cursor"` + Apps []*models.App `json:"apps"` } type routeResponse struct { @@ -322,21 +359,23 @@ type routeResponse struct { } type routesResponse struct { - Message string `json:"message"` - Routes []*models.Route `json:"routes"` + Message string `json:"message"` + NextCursor string `json:"next_cursor"` + Routes []*models.Route `json:"routes"` } -type fnCallResponse struct { +type callResponse struct { Message string `json:"message"` Call *models.Call `json:"call"` } -type fnCallsResponse struct { - Message string `json:"message"` - Calls []*models.Call `json:"calls"` +type callsResponse struct { + Message string `json:"message"` + NextCursor string `json:"next_cursor"` + Calls []*models.Call `json:"calls"` } -type fnCallLogResponse struct { +type callLogResponse struct { Message string `json:"message"` Log *models.CallLog `json:"log"` } diff --git a/docs/definitions.md b/docs/definitions.md index 13d1dbf8a..6718e4c3d 100644 --- a/docs/definitions.md +++ b/docs/definitions.md @@ -243,3 +243,46 @@ Server will replay with following JSON response: ] } ``` + +### Pagination + +The fn api utilizes 'cursoring' to paginate large result sets on endpoints +that list resources. The parameters are read from query parameters on incoming +requests, and a cursor will be returned to the user if they receive a full +page of data to use to retrieve the next page. We'll walk through with a +concrete example in just a minute. + +To begin paging through a results set, a user should provide a `?cursor` with an +empty string or omit the cursor query parameter altogether. A user may specify +how many results per page they would like to receive with the `?per_page` +query parameter, which defaults to 30 and has a max of 100. After calling a +list endpoint, a user may receive a `response.next_cursor` value in the +response, next to the list of resources. If `next_cursor` is an empty string, +then there is no further data to retrieve and the user may stop paging. If +`next_cursor` is a non-empty string, the user may provide it in the next +request's `?cursor` parameter to receive the next page. + +briefly, what this means, is user code should look similar to this: + +``` +req = "http://my.fn.com/v1/apps/" +cursor = "" + +for { + req_with_cursor = req + "?" + cursor + resp = call_http(req_with_cursor) + do_things_with_apps(resp["apps"]) + + if resp["next_cursor"] == "" { + break + } + cursor = resp["next_cursor"] +} + +# done! +``` + +client libraries will have variables for each of these variables in their +respective languages to make this a bit easier, but may the for be with +you. + diff --git a/docs/swagger.yml b/docs/swagger.yml index 23ae573b9..3ef7c4449 100644 --- a/docs/swagger.yml +++ b/docs/swagger.yml @@ -19,9 +19,20 @@ paths: /apps: get: summary: "Get all app names." - description: "Get a list of all the apps in the system." + description: "Get a list of all the apps in the system, returned in alphabetical order." tags: - Apps + parameters: + - name: cursor + description: Cursor from previous response.next_cursor to begin results after, if any. + required: false + type: string + in: query + - name: per_page + description: Number of results to return, defaults to 30. Max of 100. + required: false + type: int + in: query responses: 200: description: List of apps. @@ -182,7 +193,7 @@ paths: get: summary: Get route list by app name. - description: This will list routes for a particular app. + description: This will list routes for a particular app, returned in alphabetical order. tags: - Routes parameters: @@ -191,6 +202,21 @@ paths: description: Name of app for this set of routes. required: true type: string + - name: image + description: Route image to match, exact. + required: false + type: string + in: query + - name: cursor + description: Cursor from previous response.next_cursor to begin results after, if any. + required: false + type: string + in: query + - name: per_page + description: Number of results to return, defaults to 30. Max of 100. + required: false + type: int + in: query responses: 200: description: Route information @@ -424,7 +450,7 @@ paths: /apps/{app}/calls: get: summary: Get app-bound calls. - description: Get app-bound calls can filter to route-bound calls. + description: Get app-bound calls can filter to route-bound calls, results returned in created_at, descending order (newest first). tags: - Call parameters: @@ -433,11 +459,31 @@ paths: required: true type: string in: path - - name: route - description: App route. + - name: path + description: Route path to match, exact. required: false type: string in: query + - name: cursor + description: Cursor from previous response.next_cursor to begin results after, if any. + required: false + type: string + in: query + - name: per_page + description: Number of results to return, defaults to 30. Max of 100. + required: false + type: int + in: query + - name: from_time + description: Unix timestamp in seconds, of call.created_at to begin the results at, default 0. + required: false + type: int + in: query + - name: to_time + description: Unix timestamp in seconds, of call.created_at to end the results at, defaults to latest. + required: false + type: int + in: query responses: 200: description: Calls found @@ -524,6 +570,10 @@ definitions: required: - routes properties: + next_cursor: + type: string + description: cursor to send with subsequent request to receive the next page, if non-empty + readOnly: true routes: type: array items: @@ -548,6 +598,10 @@ definitions: required: - apps properties: + next_cursor: + type: string + description: cursor to send with subsequent request to receive the next page, if non-empty + readOnly: true apps: type: array items: @@ -570,6 +624,10 @@ definitions: required: - calls properties: + next_cursor: + type: string + description: cursor to send with subsequent request to receive the next page, if non-empty + readOnly: true calls: type: array items: diff --git a/test/fn-api-tests/calls_test.go b/test/fn-api-tests/calls_test.go index 461e3430d..3b6ca1fad 100644 --- a/test/fn-api-tests/calls_test.go +++ b/test/fn-api-tests/calls_test.go @@ -26,24 +26,6 @@ func TestCalls(t *testing.T) { } }) - t.Run("list-calls-for-missing-route", func(t *testing.T) { - t.Parallel() - s := SetupDefaultSuite() - CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) - - cfg := &call.GetAppsAppCallsParams{ - App: s.AppName, - Route: &s.RoutePath, - Context: s.Context, - } - _, err := s.Client.Call.GetAppsAppCalls(cfg) - if err == nil { - t.Errorf("Must fail with missing route error, but got %s", err) - } - - DeleteApp(t, s.Context, s.Client, s.AppName) - }) - t.Run("get-dummy-call", func(t *testing.T) { t.Parallel() s := SetupDefaultSuite()