From d11bafb868989d9ec8bda29158e22caa788535b3 Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Tue, 15 Aug 2017 20:39:21 +0300 Subject: [PATCH 1/3] Fix route update procedure API impact: We need to drop default value for type because it brought this type of bugs. Starting this patch users should specify route type through CLI or func.yml Closes: #222 --- api/server/routes_create_update.go | 4 +++- api/server/routes_test.go | 34 +++++++++++++++--------------- api/server/server_test.go | 4 ++-- test/fn-api-tests/routes_test.go | 12 +++++++++++ 4 files changed, 34 insertions(+), 20 deletions(-) diff --git a/api/server/routes_create_update.go b/api/server/routes_create_update.go index c13898031..ea163e6a6 100644 --- a/api/server/routes_create_update.go +++ b/api/server/routes_create_update.go @@ -110,7 +110,9 @@ func (s *Server) bindAndValidate(c *gin.Context, method string, wroute *models.R wroute.Route.Path = p } - wroute.Route.SetDefaults() + if method != http.MethodPatch { + wroute.Route.SetDefaults() + } return wroute.Validate(method == http.MethodPatch) } diff --git a/api/server/routes_test.go b/api/server/routes_test.go index e448cdec1..e1cba37ea 100644 --- a/api/server/routes_test.go +++ b/api/server/routes_test.go @@ -57,13 +57,13 @@ func TestRouteCreate(t *testing.T) { for i, test := range []routeTestCase{ // errors {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", ``, http.StatusBadRequest, models.ErrInvalidJSON}, - {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ }`, http.StatusBadRequest, models.ErrRoutesMissingNew}, - {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "path": "/myroute" }`, http.StatusBadRequest, models.ErrRoutesMissingNew}, + {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" } }`, http.StatusBadRequest, models.ErrRoutesValidationMissingImage}, - {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "image": "funcy/hello" } }`, http.StatusBadRequest, models.ErrRoutesValidationMissingPath}, - {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "image": "funcy/hello", "path": "myroute" } }`, http.StatusBadRequest, models.ErrRoutesValidationInvalidPath}, - {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/$/routes", `{ "route": { "image": "funcy/hello", "path": "/myroute" } }`, http.StatusBadRequest, models.ErrAppsValidationInvalidName}, + {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": "funcy/hello", "type": "sync" } }`, http.StatusBadRequest, models.ErrRoutesValidationMissingPath}, + {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "image": "funcy/hello", "path": "myroute", "type": "sync" } }`, http.StatusBadRequest, models.ErrRoutesValidationInvalidPath}, + {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/$/routes", `{ "route": { "image": "funcy/hello", "path": "/myroute", "type": "sync" } }`, http.StatusBadRequest, models.ErrAppsValidationInvalidName}, {datastore.NewMockInit(nil, []*models.Route{ { @@ -71,10 +71,10 @@ func TestRouteCreate(t *testing.T) { Path: "/myroute", }, }, nil, nil, - ), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "image": "funcy/hello", "path": "/myroute" } }`, http.StatusConflict, models.ErrRoutesAlreadyExists}, + ), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "image": "funcy/hello", "path": "/myroute", "type": "sync" } }`, http.StatusConflict, models.ErrRoutesAlreadyExists}, // success - {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "image": "funcy/hello", "path": "/myroute" } }`, http.StatusOK, nil}, + {datastore.NewMock(), logs.NewMock(), http.MethodPost, "/v1/apps/a/routes", `{ "route": { "image": "funcy/hello", "path": "/myroute", "type": "sync" } }`, http.StatusOK, nil}, } { test.run(t, i, buf) } @@ -86,18 +86,18 @@ func TestRoutePut(t *testing.T) { for i, test := range []routeTestCase{ // 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" }`, http.StatusBadRequest, models.ErrRoutesMissingNew}, - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { } }`, http.StatusBadRequest, models.ErrRoutesValidationMissingImage}, - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "path": "/myroute" } }`, http.StatusBadRequest, models.ErrRoutesValidationMissingImage}, - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "funcy/hello", "path": "myroute" } }`, http.StatusConflict, models.ErrRoutesPathImmutable}, - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "funcy/hello", "path": "diffRoute" } }`, http.StatusConflict, models.ErrRoutesPathImmutable}, - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/$/routes/myroute", `{ "route": { "image": "funcy/hello", "path": "/myroute" } }`, http.StatusBadRequest, models.ErrAppsValidationInvalidName}, + {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": { "image": "funcy/hello", "path": "myroute", "type": "sync" } }`, http.StatusConflict, models.ErrRoutesPathImmutable}, + {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "funcy/hello", "path": "diffRoute", "type": "sync" } }`, http.StatusConflict, models.ErrRoutesPathImmutable}, + {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/$/routes/myroute", `{ "route": { "image": "funcy/hello", "path": "/myroute", "type": "sync" } }`, http.StatusBadRequest, models.ErrAppsValidationInvalidName}, {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "funcy/hello", "path": "/myroute", "type": "invalid-type" } }`, http.StatusBadRequest, models.ErrRoutesValidationInvalidType}, - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "funcy/hello", "path": "/myroute", "format": "invalid-format" } }`, http.StatusBadRequest, models.ErrRoutesValidationInvalidFormat}, + {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "funcy/hello", "path": "/myroute", "format": "invalid-format", "type": "sync" } }`, http.StatusBadRequest, models.ErrRoutesValidationInvalidFormat}, // success - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "funcy/hello", "path": "/myroute" } }`, http.StatusOK, nil}, - {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "funcy/hello" } }`, http.StatusOK, nil}, + {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "funcy/hello", "path": "/myroute", "type": "sync" } }`, http.StatusOK, nil}, + {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "funcy/hello", "type": "sync" } }`, http.StatusOK, nil}, } { test.run(t, i, buf) test.ds = datastore.NewMockInit(nil, diff --git a/api/server/server_test.go b/api/server/server_test.go index 3b95e7fd9..2e962af3c 100644 --- a/api/server/server_test.go +++ b/api/server/server_test.go @@ -116,8 +116,8 @@ func TestFullStack(t *testing.T) { {"list apps", "GET", "/v1/apps", ``, http.StatusOK, 0}, {"get app", "GET", "/v1/apps/myapp", ``, http.StatusOK, 0}, // NOTE: cache is lazy, loads when a request comes in for the route, not when added - {"add myroute", "POST", "/v1/apps/myapp/routes", `{ "route": { "name": "myroute", "path": "/myroute", "image": "funcy/hello" } }`, http.StatusOK, 0}, - {"add myroute2", "POST", "/v1/apps/myapp/routes", `{ "route": { "name": "myroute2", "path": "/myroute2", "image": "funcy/error" } }`, http.StatusOK, 0}, + {"add myroute", "POST", "/v1/apps/myapp/routes", `{ "route": { "name": "myroute", "path": "/myroute", "image": "funcy/hello", "type": "sync" } }`, http.StatusOK, 0}, + {"add myroute2", "POST", "/v1/apps/myapp/routes", `{ "route": { "name": "myroute2", "path": "/myroute2", "image": "funcy/error", "type": "sync" } }`, http.StatusOK, 0}, {"get myroute", "GET", "/v1/apps/myapp/routes/myroute", ``, http.StatusOK, 0}, {"get myroute2", "GET", "/v1/apps/myapp/routes/myroute2", ``, http.StatusOK, 0}, {"get all routes", "GET", "/v1/apps/myapp/routes", ``, http.StatusOK, 0}, diff --git a/test/fn-api-tests/routes_test.go b/test/fn-api-tests/routes_test.go index 19b5b1446..527c7c403 100644 --- a/test/fn-api-tests/routes_test.go +++ b/test/fn-api-tests/routes_test.go @@ -12,6 +12,18 @@ func TestRoutes(t *testing.T) { newRouteType := "sync" newRoutePath := id.New().String() + t.Run("create-route-with-empty-type", func(t *testing.T) { + t.Parallel() + s := SetupDefaultSuite() + CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) + _, err := createRoute(s.Context, s.Client, s.AppName, s.RoutePath, s.Image, "", + s.RouteConfig, s.RouteHeaders) + if err == nil { + t.Errorf("Should fail with Invalid route Type.") + } + DeleteApp(t, s.Context, s.Client, s.AppName) + }) + t.Run("create-route", func(t *testing.T) { t.Parallel() s := SetupDefaultSuite() From 62d650f0a5d2996019a425efc6ed920260b08cde Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Wed, 16 Aug 2017 13:55:30 +0300 Subject: [PATCH 2/3] Rewrite HTTP handler for routes HTTP POST/PUT/PATCH methods - adding tests for HTTP PUT - more tests for HTTP PATCH --- api/server/routes_create_update.go | 138 +++++++++++++++++++---------- api/server/routes_test.go | 9 -- api/server/server.go | 6 +- test/fn-api-tests/apps_api.go | 12 +++ test/fn-api-tests/apps_test.go | 10 +-- test/fn-api-tests/calls_test.go | 6 +- test/fn-api-tests/exec_test.go | 18 ++-- test/fn-api-tests/routes_api.go | 40 ++++++++- test/fn-api-tests/routes_test.go | 80 +++++++++++++++-- 9 files changed, 226 insertions(+), 93 deletions(-) diff --git a/api/server/routes_create_update.go b/api/server/routes_create_update.go index ea163e6a6..3da8c674f 100644 --- a/api/server/routes_create_update.go +++ b/api/server/routes_create_update.go @@ -11,6 +11,8 @@ import ( "github.com/gin-gonic/gin" ) +var bad routeResponse + /* handleRouteCreateOrUpdate is used to handle POST PUT and PATCH for routes. Post will only create route if its not there and create app if its not. create only @@ -22,41 +24,102 @@ import ( update only Patch accepts partial updates / skips validation of zero values. */ -func (s *Server) handleRouteCreateOrUpdate(c *gin.Context) { +func (s *Server) handleRoutesPostPutPatch(c *gin.Context) { ctx := c.Request.Context() method := strings.ToUpper(c.Request.Method) var wroute models.RouteWrapper - - err := s.bindAndValidate(c, method, &wroute) + err := bindRoute(c, method, &wroute) if err != nil { handleErrorResponse(c, err) return } - - // Create the app if it does not exist. - err = s.ensureApp(ctx, &wroute, method) - if err != nil { - handleErrorResponse(c, err) - return + if method != http.MethodPatch { + err = s.ensureApp(ctx, &wroute, method) + if err != nil { + handleErrorResponse(c, err) + return + } } - - resp, err := s.updateOrInsertRoute(ctx, method, wroute) + resp, err := s.ensureRoute(ctx, method, &wroute) if err != nil { handleErrorResponse(c, err) return } s.cachedelete(resp.Route.AppName, resp.Route.Path) - c.JSON(http.StatusOK, resp) } +func (s *Server) submitRoute(ctx context.Context, wroute *models.RouteWrapper) error { + err := s.setDefaultsAndValidate(wroute) + if err != nil { + return err + } + r, err := s.Datastore.InsertRoute(ctx, wroute.Route) + if err != nil { + return err + } + wroute.Route = r + return nil +} + +func (s *Server) changeRoute(ctx context.Context, wroute *models.RouteWrapper) error { + err := wroute.Validate(true) + if err != nil { + return err + } + r, err := s.Datastore.UpdateRoute(ctx, wroute.Route) + if err != nil { + return err + } + wroute.Route = r + return nil +} + +// ensureApp will only execute if it is on put +func (s *Server) ensureRoute(ctx context.Context, method string, wroute *models.RouteWrapper) (routeResponse, error) { + resp := routeResponse{"Route successfully created", nil} + up := routeResponse{"Route successfully updated", nil} + + switch method { + case http.MethodPost: + err := s.submitRoute(ctx, wroute) + if err != nil { + return bad, err + } + resp.Route = wroute.Route + return resp, nil + case http.MethodPut: + _, err := s.Datastore.GetRoute(ctx, wroute.Route.AppName, wroute.Route.Path) + if err != nil && err == models.ErrRoutesNotFound { + err := s.submitRoute(ctx, wroute) + if err != nil { + return bad, err + } + resp.Route = wroute.Route + return resp, nil + } else { + err := s.changeRoute(ctx, wroute) + if err != nil { + return bad, err + } + up.Route = wroute.Route + return up, nil + } + case http.MethodPatch: + err := s.changeRoute(ctx, wroute) + if err != nil { + return bad, err + } + up.Route = wroute.Route + return up, nil + } + return bad, nil +} + // ensureApp will only execute if it is on post or put. Patch is not allowed to create apps. func (s *Server) ensureApp(ctx context.Context, wroute *models.RouteWrapper, method string) error { - if !(method == http.MethodPost || method == http.MethodPut) { - return nil - } app, err := s.Datastore.GetApp(ctx, wroute.Route.AppName) if err != nil && err != models.ErrAppsNotFound { return err @@ -86,11 +149,8 @@ func (s *Server) ensureApp(ctx context.Context, wroute *models.RouteWrapper, met return nil } -/* bindAndValidate binds the RouteWrapper to the json from the request and validates that it is correct. -If it is a put or patch it makes sure that the path in the url matches the provideed one in the body. -Defaults are set and if patch skipZero is true for validating the RouteWrapper -*/ -func (s *Server) bindAndValidate(c *gin.Context, method string, wroute *models.RouteWrapper) error { +// bindRoute binds the RouteWrapper to the json from the request. +func bindRoute(c *gin.Context, method string, wroute *models.RouteWrapper) error { err := c.BindJSON(wroute) if err != nil { return models.ErrInvalidJSON @@ -109,35 +169,15 @@ func (s *Server) bindAndValidate(c *gin.Context, method string, wroute *models.R } wroute.Route.Path = p } - - if method != http.MethodPatch { - wroute.Route.SetDefaults() - } - - return wroute.Validate(method == http.MethodPatch) -} - -// updateOrInsertRoute will either update or insert the route respective the method. -func (s *Server) updateOrInsertRoute(ctx context.Context, method string, wroute models.RouteWrapper) (routeResponse, error) { - var route *models.Route - var err error - resp := routeResponse{"Route successfully created", nil} - up := routeResponse{"Route successfully updated", nil} - - switch method { - case http.MethodPost: - route, err = s.Datastore.InsertRoute(ctx, wroute.Route) - case http.MethodPut: - route, err = s.Datastore.UpdateRoute(ctx, wroute.Route) - if err == models.ErrRoutesNotFound { - // try insert then - route, err = s.Datastore.InsertRoute(ctx, wroute.Route) + if method == http.MethodPost { + if wroute.Route.Path == "" { + return models.ErrRoutesValidationMissingPath } - case http.MethodPatch: - // When patching if there is an error around the app we will return one and the update fails. - route, err = s.Datastore.UpdateRoute(ctx, wroute.Route) - resp = up } - resp.Route = route - return resp, err + return nil +} + +func (s *Server) setDefaultsAndValidate(wroute *models.RouteWrapper) error { + wroute.Route.SetDefaults() + return wroute.Validate(false) } diff --git a/api/server/routes_test.go b/api/server/routes_test.go index e1cba37ea..df94b494c 100644 --- a/api/server/routes_test.go +++ b/api/server/routes_test.go @@ -100,15 +100,6 @@ func TestRoutePut(t *testing.T) { {datastore.NewMock(), logs.NewMock(), http.MethodPut, "/v1/apps/a/routes/myroute", `{ "route": { "image": "funcy/hello", "type": "sync" } }`, http.StatusOK, nil}, } { test.run(t, i, buf) - test.ds = datastore.NewMockInit(nil, - []*models.Route{ - { - AppName: "a", - Path: "/myroute", - }, - }, nil, nil, - ) - test.run(t, i, buf) } } diff --git a/api/server/server.go b/api/server/server.go index 57827b42e..534e6a89d 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -365,10 +365,10 @@ func (s *Server) bindHandlers(ctx context.Context) { apps := v1.Group("/apps/:app") { apps.GET("/routes", s.handleRouteList) - apps.POST("/routes", s.handleRouteCreateOrUpdate) + apps.POST("/routes", s.handleRoutesPostPutPatch) apps.GET("/routes/*route", s.handleRouteGet) - apps.PATCH("/routes/*route", s.handleRouteCreateOrUpdate) - apps.PUT("/routes/*route", s.handleRouteCreateOrUpdate) + apps.PATCH("/routes/*route", s.handleRoutesPostPutPatch) + apps.PUT("/routes/*route", s.handleRoutesPostPutPatch) apps.DELETE("/routes/*route", s.handleRouteDelete) apps.GET("/calls", s.handleCallList) diff --git a/test/fn-api-tests/apps_api.go b/test/fn-api-tests/apps_api.go index 8ed33437b..1b82f7a3f 100644 --- a/test/fn-api-tests/apps_api.go +++ b/test/fn-api-tests/apps_api.go @@ -84,6 +84,7 @@ func CreateUpdateApp(t *testing.T, ctx context.Context, fnclient *client.Functio }, Context: ctx, } + cfg.WithTimeout(time.Second * 60) appPayload, err := fnclient.Apps.PatchAppsApp(cfg) CheckAppResponseError(t, err) return appPayload @@ -98,3 +99,14 @@ func DeleteApp(t *testing.T, ctx context.Context, fnclient *client.Functions, ap _, err := fnclient.Apps.DeleteAppsApp(cfg) CheckAppResponseError(t, err) } + +func GetApp(t *testing.T, ctx context.Context, fnclient *client.Functions, appName string) *models.App { + cfg := &apps.GetAppsAppParams{ + App: appName, + Context: ctx, + } + cfg.WithTimeout(time.Second * 60) + app, err := fnclient.Apps.GetAppsApp(cfg) + CheckAppResponseError(t, err) + return app.Payload.App +} diff --git a/test/fn-api-tests/apps_test.go b/test/fn-api-tests/apps_test.go index cec2c31ed..75a88c6f7 100644 --- a/test/fn-api-tests/apps_test.go +++ b/test/fn-api-tests/apps_test.go @@ -41,14 +41,8 @@ func TestApps(t *testing.T) { t.Parallel() s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{"A": "a"}) - cfg := &apps.GetAppsAppParams{ - Context: s.Context, - App: s.AppName, - } - appPayload, err := s.Client.Apps.GetAppsApp(cfg) - CheckAppResponseError(t, err) - appBody := appPayload.Payload.App - val, ok := appBody.Config["A"] + app := GetApp(t, s.Context, s.Client, s.AppName) + val, ok := app.Config["A"] if !ok { t.Error("Error during app config inspect: config map misses required entity `A` with value `a`.") } diff --git a/test/fn-api-tests/calls_test.go b/test/fn-api-tests/calls_test.go index febdac0e3..339c075d7 100644 --- a/test/fn-api-tests/calls_test.go +++ b/test/fn-api-tests/calls_test.go @@ -49,7 +49,7 @@ func TestCalls(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) cfg := &call.GetAppsAppCallsCallParams{ Call: "dummy", @@ -71,7 +71,7 @@ func TestCalls(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) u := url.URL{ Scheme: "http", @@ -106,7 +106,7 @@ func TestCalls(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) u := url.URL{ Scheme: "http", diff --git a/test/fn-api-tests/exec_test.go b/test/fn-api-tests/exec_test.go index edd9a9031..24b37aaf3 100644 --- a/test/fn-api-tests/exec_test.go +++ b/test/fn-api-tests/exec_test.go @@ -57,7 +57,7 @@ func TestRouteExecutions(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, "sync", - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) u := url.URL{ Scheme: "http", @@ -84,7 +84,7 @@ func TestRouteExecutions(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, "sync", - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) u := url.URL{ Scheme: "http", @@ -115,7 +115,7 @@ func TestRouteExecutions(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, "sync", - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) u := url.URL{ Scheme: "http", @@ -141,7 +141,7 @@ func TestRouteExecutions(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, "sync", - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) u := url.URL{ Scheme: "http", @@ -202,7 +202,7 @@ func TestRouteExecutions(t *testing.T) { CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, routePath, image, routeType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) u := url.URL{ Scheme: "http", @@ -253,7 +253,7 @@ func TestRouteExecutions(t *testing.T) { CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, routePath, image, routeType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) u := url.URL{ Scheme: "http", @@ -298,7 +298,7 @@ func TestRouteExecutions(t *testing.T) { image := "denismakogon/os.environ" routeType := "sync" CreateRoute(t, s.Context, s.Client, s.AppName, routePath, image, routeType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) u := url.URL{ Scheme: "http", @@ -330,7 +330,7 @@ func TestRouteExecutions(t *testing.T) { CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, routePath, image, routeType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) u := url.URL{ Scheme: "http", @@ -372,7 +372,7 @@ func TestRouteExecutions(t *testing.T) { CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, routePath, image, routeType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) size := 1 * 1024 * 1024 * 1024 u := url.URL{ diff --git a/test/fn-api-tests/routes_api.go b/test/fn-api-tests/routes_api.go index 9645104fc..4bc16d856 100644 --- a/test/fn-api-tests/routes_api.go +++ b/test/fn-api-tests/routes_api.go @@ -55,19 +55,28 @@ func CheckRouteResponseError(t *testing.T, err error) { msg := err.(*routes.PatchAppsAppRoutesRouteDefault).Payload.Error.Message code := err.(*routes.PatchAppsAppRoutesRouteDefault).Code() t.Errorf("Unexpected error occurred: %v. Status code: %v", msg, code) + case *routes.PutAppsAppRoutesRouteBadRequest: + msg := err.(*routes.PutAppsAppRoutesRouteBadRequest).Payload.Error.Message + t.Errorf("Unexpected error occurred: %v.", msg) + case *routes.PutAppsAppRoutesRouteDefault: + msg := err.(*routes.PutAppsAppRoutesRouteDefault).Payload.Error.Message + code := err.(*routes.PutAppsAppRoutesRouteDefault).Code() + t.Errorf("Unexpected error occurred: %v. Status code: %v", msg, code) default: t.Errorf("Unable to determine type of error: %s", err) } } } -func assertRouteFields(t *testing.T, routeObject *models.Route, path, image, routeType string) { +func assertRouteFields(t *testing.T, routeObject *models.Route, path, image, routeType, routeFormat string) { rPath := routeObject.Path rImage := routeObject.Image rType := routeObject.Type rTimeout := *routeObject.Timeout rIdleTimeout := *routeObject.IDLETimeout + rFormat := routeObject.Format + if rPath != path { t.Errorf("Route path mismatch. Expected: %v. Actual: %v", path, rPath) } @@ -83,6 +92,9 @@ func assertRouteFields(t *testing.T, routeObject *models.Route, path, image, rou if rIdleTimeout == 0 { t.Error("Route idle timeout should have default value of 30 seconds, but got 0 seconds") } + if rFormat != routeFormat { + t.Errorf("Route format mismatch. Expected: %v. Actual: %v", routeFormat, rFormat) + } } @@ -105,11 +117,11 @@ func createRoute(ctx context.Context, fnclient *client.Functions, appName, image } -func CreateRoute(t *testing.T, ctx context.Context, fnclient *client.Functions, appName, routePath, image, routeType string, routeConfig map[string]string, headers map[string][]string) { +func CreateRoute(t *testing.T, ctx context.Context, fnclient *client.Functions, appName, routePath, image, routeType, routeFormat string, routeConfig map[string]string, headers map[string][]string) { routeResponse, err := createRoute(ctx, fnclient, appName, image, routePath, routeType, routeConfig, headers) CheckRouteResponseError(t, err) - assertRouteFields(t, routeResponse.Payload.Route, routePath, image, routeType) + assertRouteFields(t, routeResponse.Payload.Route, routePath, image, routeType, routeFormat) } func deleteRoute(ctx context.Context, fnclient *client.Functions, appName, routePath string) (*routes.DeleteAppsAppRoutesRouteOK, error) { @@ -218,3 +230,25 @@ func assertContainsRoute(routeModels []*models.Route, expectedRoute string) bool } return false } + +func DeployRoute(t *testing.T, ctx context.Context, fnclient *client.Functions, appName, routePath, image, routeType, routeFormat string, routeConfig map[string]string, headers map[string][]string) *models.Route { + cfg := &routes.PutAppsAppRoutesRouteParams{ + App: appName, + Context: ctx, + Route: routePath, + Body: &models.RouteWrapper{ + Route: &models.Route{ + Config: routeConfig, + Headers: headers, + Image: image, + Path: routePath, + Type: routeType, + Format: routeFormat, + }, + }, + } + cfg.WithTimeout(time.Second * 60) + route, err := fnclient.Routes.PutAppsAppRoutesRoute(cfg) + CheckRouteResponseError(t, err) + return route.Payload.Route +} diff --git a/test/fn-api-tests/routes_test.go b/test/fn-api-tests/routes_test.go index 527c7c403..0e72ff150 100644 --- a/test/fn-api-tests/routes_test.go +++ b/test/fn-api-tests/routes_test.go @@ -11,7 +11,6 @@ import ( func TestRoutes(t *testing.T) { newRouteType := "sync" newRoutePath := id.New().String() - t.Run("create-route-with-empty-type", func(t *testing.T) { t.Parallel() s := SetupDefaultSuite() @@ -29,7 +28,7 @@ func TestRoutes(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) DeleteRoute(t, s.Context, s.Client, s.AppName, s.RoutePath) DeleteApp(t, s.Context, s.Client, s.AppName) }) @@ -39,7 +38,7 @@ func TestRoutes(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) if !assertContainsRoute(ListRoutes(t, s.Context, s.Client, s.AppName), s.RoutePath) { t.Errorf("Unable to find corresponding route `%v` in list", s.RoutePath) } @@ -52,7 +51,7 @@ func TestRoutes(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) rObjects := []*models.Route{GetRoute(t, s.Context, s.Client, s.AppName, s.RoutePath)} if !assertContainsRoute(rObjects, s.RoutePath) { @@ -68,7 +67,7 @@ func TestRoutes(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) routeResp, err := UpdateRoute( t, s.Context, s.Client, @@ -77,7 +76,31 @@ func TestRoutes(t *testing.T) { s.Memory, s.RouteConfig, s.RouteHeaders, "") CheckRouteResponseError(t, err) - assertRouteFields(t, routeResp.Payload.Route, s.RoutePath, s.Image, newRouteType) + assertRouteFields(t, routeResp.Payload.Route, s.RoutePath, s.Image, newRouteType, s.Format) + + DeleteRoute(t, s.Context, s.Client, s.AppName, s.RoutePath) + DeleteApp(t, s.Context, s.Client, s.AppName) + }) + + t.Run("patch-route-with-config", func(t *testing.T) { + t.Parallel() + s := SetupDefaultSuite() + CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) + CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, + s.Format, s.RouteConfig, s.RouteHeaders) + + newRouteConf := map[string]string{ + "A": "a", + } + + routeResp, err := UpdateRoute( + t, s.Context, s.Client, + s.AppName, s.RoutePath, + s.Image, s.RouteType, s.Format, + s.Memory, newRouteConf, s.RouteHeaders, "") + + CheckRouteResponseError(t, err) + assertRouteFields(t, routeResp.Payload.Route, s.RoutePath, s.Image, s.RouteType, s.Format) DeleteRoute(t, s.Context, s.Client, s.AppName, s.RoutePath) DeleteApp(t, s.Context, s.Client, s.AppName) @@ -88,7 +111,7 @@ func TestRoutes(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) _, err := UpdateRoute( t, s.Context, s.Client, @@ -108,7 +131,7 @@ func TestRoutes(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) _, err := createRoute(s.Context, s.Client, s.AppName, s.Image, s.RoutePath, newRouteType, s.RouteConfig, s.RouteHeaders) if err == nil { @@ -124,7 +147,7 @@ func TestRoutes(t *testing.T) { s := SetupDefaultSuite() CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, - s.RouteConfig, s.RouteHeaders) + s.Format, s.RouteConfig, s.RouteHeaders) DeleteRoute(t, s.Context, s.Client, s.AppName, s.RoutePath) DeleteApp(t, s.Context, s.Client, s.AppName) @@ -141,4 +164,43 @@ func TestRoutes(t *testing.T) { } DeleteApp(t, s.Context, s.Client, s.AppName) }) + + t.Run("deploy-route-without-existing-app", func(T *testing.T) { + t.Parallel() + s := SetupDefaultSuite() + DeployRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, s.Format, s.RouteConfig, s.RouteHeaders) + GetApp(t, s.Context, s.Client, s.AppName) + GetRoute(t, s.Context, s.Client, s.AppName, s.RoutePath) + DeleteRoute(t, s.Context, s.Client, s.AppName, s.RoutePath) + DeleteApp(t, s.Context, s.Client, s.AppName) + }) + + t.Run("deploy-route-with-existing-app", func(T *testing.T) { + s := SetupDefaultSuite() + CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) + DeployRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, s.Format, s.RouteConfig, s.RouteHeaders) + GetApp(t, s.Context, s.Client, s.AppName) + GetRoute(t, s.Context, s.Client, s.AppName, s.RoutePath) + DeleteRoute(t, s.Context, s.Client, s.AppName, s.RoutePath) + DeleteApp(t, s.Context, s.Client, s.AppName) + }) + + t.Run("deploy-update-with-existing-route-and-app", func(T *testing.T) { + newRouteType := "sync" + s := SetupDefaultSuite() + CreateApp(t, s.Context, s.Client, s.AppName, map[string]string{}) + CreateRoute(t, s.Context, s.Client, s.AppName, s.RoutePath, s.Image, s.RouteType, + s.Format, s.RouteConfig, s.RouteHeaders) + + updatedRoute := DeployRoute( + t, s.Context, s.Client, + s.AppName, s.RoutePath, + s.Image, newRouteType, + s.Format, s.RouteConfig, s.RouteHeaders) + assertRouteFields(t, updatedRoute, s.RoutePath, s.Image, newRouteType, s.Format) + + DeleteRoute(t, s.Context, s.Client, s.AppName, s.RoutePath) + DeleteApp(t, s.Context, s.Client, s.AppName) + }) + } From 1f0f9c7a4612750489904a5f9a99fe3a048d0141 Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Thu, 17 Aug 2017 10:36:05 +0300 Subject: [PATCH 3/3] Addressing comments --- api/server/routes_create_update.go | 27 ++++++++++----------------- test/fn-api-tests/apps_api.go | 9 ++++----- test/fn-api-tests/routes_api.go | 12 +++++------- test/fn-api-tests/utils.go | 5 +++-- 4 files changed, 22 insertions(+), 31 deletions(-) diff --git a/api/server/routes_create_update.go b/api/server/routes_create_update.go index 3da8c674f..6f5899539 100644 --- a/api/server/routes_create_update.go +++ b/api/server/routes_create_update.go @@ -11,8 +11,6 @@ import ( "github.com/gin-gonic/gin" ) -var bad routeResponse - /* handleRouteCreateOrUpdate is used to handle POST PUT and PATCH for routes. Post will only create route if its not there and create app if its not. create only @@ -79,43 +77,38 @@ func (s *Server) changeRoute(ctx context.Context, wroute *models.RouteWrapper) e // ensureApp will only execute if it is on put func (s *Server) ensureRoute(ctx context.Context, method string, wroute *models.RouteWrapper) (routeResponse, error) { - resp := routeResponse{"Route successfully created", nil} - up := routeResponse{"Route successfully updated", nil} + bad := new(routeResponse) switch method { case http.MethodPost: err := s.submitRoute(ctx, wroute) if err != nil { - return bad, err + return *bad, err } - resp.Route = wroute.Route - return resp, nil + return routeResponse{"Route successfully created", wroute.Route}, nil case http.MethodPut: _, err := s.Datastore.GetRoute(ctx, wroute.Route.AppName, wroute.Route.Path) if err != nil && err == models.ErrRoutesNotFound { err := s.submitRoute(ctx, wroute) if err != nil { - return bad, err + return *bad, err } - resp.Route = wroute.Route - return resp, nil + return routeResponse{"Route successfully created", wroute.Route}, nil } else { err := s.changeRoute(ctx, wroute) if err != nil { - return bad, err + return *bad, err } - up.Route = wroute.Route - return up, nil + return routeResponse{"Route successfully updated", wroute.Route}, nil } case http.MethodPatch: err := s.changeRoute(ctx, wroute) if err != nil { - return bad, err + return *bad, err } - up.Route = wroute.Route - return up, nil + return routeResponse{"Route successfully updated", wroute.Route}, nil } - return bad, nil + return *bad, nil } // ensureApp will only execute if it is on post or put. Patch is not allowed to create apps. diff --git a/test/fn-api-tests/apps_api.go b/test/fn-api-tests/apps_api.go index 1b82f7a3f..bf927c891 100644 --- a/test/fn-api-tests/apps_api.go +++ b/test/fn-api-tests/apps_api.go @@ -4,7 +4,6 @@ import ( "context" "strings" "testing" - "time" "github.com/funcy/functions_go/client" "github.com/funcy/functions_go/client/apps" @@ -59,7 +58,7 @@ func CreateAppNoAssert(ctx context.Context, fnclient *client.Functions, appName }, Context: ctx, } - cfg.WithTimeout(time.Second * 60) + return fnclient.Apps.PostApps(cfg) } @@ -84,7 +83,7 @@ func CreateUpdateApp(t *testing.T, ctx context.Context, fnclient *client.Functio }, Context: ctx, } - cfg.WithTimeout(time.Second * 60) + appPayload, err := fnclient.Apps.PatchAppsApp(cfg) CheckAppResponseError(t, err) return appPayload @@ -95,7 +94,7 @@ func DeleteApp(t *testing.T, ctx context.Context, fnclient *client.Functions, ap App: appName, Context: ctx, } - cfg.WithTimeout(time.Second * 60) + _, err := fnclient.Apps.DeleteAppsApp(cfg) CheckAppResponseError(t, err) } @@ -105,7 +104,7 @@ func GetApp(t *testing.T, ctx context.Context, fnclient *client.Functions, appNa App: appName, Context: ctx, } - cfg.WithTimeout(time.Second * 60) + app, err := fnclient.Apps.GetAppsApp(cfg) CheckAppResponseError(t, err) return app.Payload.App diff --git a/test/fn-api-tests/routes_api.go b/test/fn-api-tests/routes_api.go index 4bc16d856..9a99666b4 100644 --- a/test/fn-api-tests/routes_api.go +++ b/test/fn-api-tests/routes_api.go @@ -3,7 +3,6 @@ package tests import ( "context" "testing" - "time" "github.com/funcy/functions_go/client" "github.com/funcy/functions_go/client/routes" @@ -112,7 +111,7 @@ func createRoute(ctx context.Context, fnclient *client.Functions, appName, image }, Context: ctx, } - cfg.WithTimeout(time.Second * 60) + return fnclient.Routes.PostAppsAppRoutes(cfg) } @@ -130,7 +129,7 @@ func deleteRoute(ctx context.Context, fnclient *client.Functions, appName, route Route: routePath, Context: ctx, } - cfg.WithTimeout(time.Second * 60) + return fnclient.Routes.DeleteAppsAppRoutesRoute(cfg) } @@ -144,7 +143,7 @@ func ListRoutes(t *testing.T, ctx context.Context, fnclient *client.Functions, a App: appName, Context: ctx, } - cfg.WithTimeout(time.Second * 60) + routesResponse, err := fnclient.Routes.GetAppsAppRoutes(cfg) CheckRouteResponseError(t, err) return routesResponse.Payload.Routes @@ -156,7 +155,7 @@ func GetRoute(t *testing.T, ctx context.Context, fnclient *client.Functions, app Route: routePath, Context: ctx, } - cfg.WithTimeout(time.Second * 60) + routeResponse, err := fnclient.Routes.GetAppsAppRoutesRoute(cfg) CheckRouteResponseError(t, err) return routeResponse.Payload.Route @@ -217,7 +216,6 @@ func UpdateRoute(t *testing.T, ctx context.Context, fnclient *client.Functions, }, Route: routePath, } - cfg.WithTimeout(time.Second * 60) return fnclient.Routes.PatchAppsAppRoutesRoute(cfg) } @@ -247,7 +245,7 @@ func DeployRoute(t *testing.T, ctx context.Context, fnclient *client.Functions, }, }, } - cfg.WithTimeout(time.Second * 60) + route, err := fnclient.Routes.PutAppsAppRoutesRoute(cfg) CheckRouteResponseError(t, err) return route.Payload.Route diff --git a/test/fn-api-tests/utils.go b/test/fn-api-tests/utils.go index b8a4de063..2b9da4276 100644 --- a/test/fn-api-tests/utils.go +++ b/test/fn-api-tests/utils.go @@ -118,8 +118,9 @@ func RandStringBytes(n int) string { } func SetupDefaultSuite() *SuiteSetup { + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) ss := &SuiteSetup{ - Context: context.Background(), + Context: ctx, Client: APIClient(), AppName: RandStringBytes(10), RoutePath: "/" + RandStringBytes(10), @@ -128,7 +129,7 @@ func SetupDefaultSuite() *SuiteSetup { RouteType: "async", RouteConfig: map[string]string{}, RouteHeaders: map[string][]string{}, - Cancel: func() {}, + Cancel: cancel, } _, ok := ss.Client.Version.GetVersion(nil)