From a2ed1dfb2da68299452996910acc00e71f6811f1 Mon Sep 17 00:00:00 2001 From: Reed Allman Date: Wed, 28 Feb 2018 17:04:00 -0800 Subject: [PATCH] push down app listeners to a datastore (#742) * push down app listeners to a datastore fnext.NewDatastore returns a datastore that wraps the appropriate methods for AppListener in a Datastore implementation. this is more future proof than needing to wrap every call of GetApp/UpdateApp/etc with the listeners, there are a few places where this can happen and it seems like the AppListener behavior is supposed to wrap the datastore, not just the front end methods surrounding CRUD ops on an app. the hairy case that came up was when fiddling with the create/update route business. this changes the FireBeforeApp* ops to be an AppListener implementation itself rather than having the Server itself expose certain methods to fire off the app listeners, now they're on the datastore itself, which the server can return the instance of. small change to BeforeAppDelete/AfterAppDelete -- we were passing in a half baked struct with only the name filled in and not filling in the fields anywhere. this is mostly just misleading, we could fill in the app, but we weren't and don't really want to, it's more to notify of an app deletion event so that an extension can behave accordingly instead of letting a user inspect the app. i know of 3 extensions and the changes required to update are very small. cleans up all the front end implementations FireBefore/FireAfter. this seems potentially less flexible than previous version if we do want to allow users some way to call the database methods without using the extensions, but that's exactly the trade off, as far as the AppListener's are described it seems heavily implied that this should be the case. mostly a feeler, for the above reasons, but this was kind of odorous so just went for it. we do need to lock in the extension api stuff. * hand em an app that's been smokin the reefer --- api/server/app_listeners.go | 61 ++++++++----------- api/server/apps_create.go | 12 ---- api/server/apps_delete.go | 17 +----- api/server/apps_get.go | 11 ---- api/server/apps_list.go | 11 ---- api/server/apps_update.go | 12 ---- api/server/routes_create_update.go | 12 ---- api/server/server.go | 6 +- fnext/datastore.go | 95 ++++++++++++++++++++++++++++++ 9 files changed, 128 insertions(+), 109 deletions(-) create mode 100644 fnext/datastore.go diff --git a/api/server/app_listeners.go b/api/server/app_listeners.go index fa84dbdef..c7052781b 100644 --- a/api/server/app_listeners.go +++ b/api/server/app_listeners.go @@ -7,14 +7,17 @@ import ( "github.com/fnproject/fn/fnext" ) -// AddAppListener adds a listener that will be notified on App created. +type appListeners []fnext.AppListener + +var _ fnext.AppListener = new(appListeners) + +// AddAppListener adds an AppListener for the server to use. func (s *Server) AddAppListener(listener fnext.AppListener) { - s.appListeners = append(s.appListeners, listener) + *s.appListeners = append(*s.appListeners, listener) } -// FireBeforeAppCreate is used to call all the server's Listeners BeforeAppCreate functions. -func (s *Server) FireBeforeAppCreate(ctx context.Context, app *models.App) error { - for _, l := range s.appListeners { +func (a *appListeners) BeforeAppCreate(ctx context.Context, app *models.App) error { + for _, l := range *a { err := l.BeforeAppCreate(ctx, app) if err != nil { return err @@ -23,9 +26,8 @@ func (s *Server) FireBeforeAppCreate(ctx context.Context, app *models.App) error return nil } -// FireAfterAppCreate is used to call all the server's Listeners AfterAppCreate functions. -func (s *Server) FireAfterAppCreate(ctx context.Context, app *models.App) error { - for _, l := range s.appListeners { +func (a *appListeners) AfterAppCreate(ctx context.Context, app *models.App) error { + for _, l := range *a { err := l.AfterAppCreate(ctx, app) if err != nil { return err @@ -34,9 +36,8 @@ func (s *Server) FireAfterAppCreate(ctx context.Context, app *models.App) error return nil } -// FireBeforeAppUpdate is used to call all the server's Listeners BeforeAppUpdate functions. -func (s *Server) FireBeforeAppUpdate(ctx context.Context, app *models.App) error { - for _, l := range s.appListeners { +func (a *appListeners) BeforeAppUpdate(ctx context.Context, app *models.App) error { + for _, l := range *a { err := l.BeforeAppUpdate(ctx, app) if err != nil { return err @@ -45,9 +46,8 @@ func (s *Server) FireBeforeAppUpdate(ctx context.Context, app *models.App) error return nil } -// FireAfterAppUpdate is used to call all the server's Listeners AfterAppUpdate functions. -func (s *Server) FireAfterAppUpdate(ctx context.Context, app *models.App) error { - for _, l := range s.appListeners { +func (a *appListeners) AfterAppUpdate(ctx context.Context, app *models.App) error { + for _, l := range *a { err := l.AfterAppUpdate(ctx, app) if err != nil { return err @@ -56,9 +56,8 @@ func (s *Server) FireAfterAppUpdate(ctx context.Context, app *models.App) error return nil } -// FireBeforeAppDelete is used to call all the server's Listeners BeforeAppDelete functions. -func (s *Server) FireBeforeAppDelete(ctx context.Context, app *models.App) error { - for _, l := range s.appListeners { +func (a *appListeners) BeforeAppDelete(ctx context.Context, app *models.App) error { + for _, l := range *a { err := l.BeforeAppDelete(ctx, app) if err != nil { return err @@ -67,9 +66,8 @@ func (s *Server) FireBeforeAppDelete(ctx context.Context, app *models.App) error return nil } -// FireAfterAppDelete is used to call all the server's Listeners AfterAppDelete functions. -func (s *Server) FireAfterAppDelete(ctx context.Context, app *models.App) error { - for _, l := range s.appListeners { +func (a *appListeners) AfterAppDelete(ctx context.Context, app *models.App) error { + for _, l := range *a { err := l.AfterAppDelete(ctx, app) if err != nil { return err @@ -78,12 +76,8 @@ func (s *Server) FireAfterAppDelete(ctx context.Context, app *models.App) error return nil } -// FireBeforeAppGet runs AppListener's BeforeAppGet method. -// todo: All of these listener methods could/should return the 2nd param rather than modifying in place. For instance, -// if a listener were to change the appName here (maybe prefix it or something for the database), it wouldn't be reflected anywhere else. -// If this returned appName, then keep passing along the returned appName, it would work. -func (s *Server) FireBeforeAppGet(ctx context.Context, appName string) error { - for _, l := range s.appListeners { +func (a *appListeners) BeforeAppGet(ctx context.Context, appName string) error { + for _, l := range *a { err := l.BeforeAppGet(ctx, appName) if err != nil { return err @@ -92,9 +86,8 @@ func (s *Server) FireBeforeAppGet(ctx context.Context, appName string) error { return nil } -// FireAfterAppGet runs AppListener's AfterAppGet method. -func (s *Server) FireAfterAppGet(ctx context.Context, app *models.App) error { - for _, l := range s.appListeners { +func (a *appListeners) AfterAppGet(ctx context.Context, app *models.App) error { + for _, l := range *a { err := l.AfterAppGet(ctx, app) if err != nil { return err @@ -103,9 +96,8 @@ func (s *Server) FireAfterAppGet(ctx context.Context, app *models.App) error { return nil } -// FireBeforeAppsList runs AppListener's BeforeAppsList method. -func (s *Server) FireBeforeAppsList(ctx context.Context, filter *models.AppFilter) error { - for _, l := range s.appListeners { +func (a *appListeners) BeforeAppsList(ctx context.Context, filter *models.AppFilter) error { + for _, l := range *a { err := l.BeforeAppsList(ctx, filter) if err != nil { return err @@ -114,9 +106,8 @@ func (s *Server) FireBeforeAppsList(ctx context.Context, filter *models.AppFilte return nil } -// FireAfterAppsList runs AppListener's AfterAppsList method. -func (s *Server) FireAfterAppsList(ctx context.Context, apps []*models.App) error { - for _, l := range s.appListeners { +func (a *appListeners) AfterAppsList(ctx context.Context, apps []*models.App) error { + for _, l := range *a { err := l.AfterAppsList(ctx, apps) if err != nil { return err diff --git a/api/server/apps_create.go b/api/server/apps_create.go index aa7d702da..5f5ff5102 100644 --- a/api/server/apps_create.go +++ b/api/server/apps_create.go @@ -28,23 +28,11 @@ func (s *Server) handleAppCreate(c *gin.Context) { return } - err = s.FireBeforeAppCreate(ctx, app) - if err != nil { - handleErrorResponse(c, err) - return - } - app, err = s.datastore.InsertApp(ctx, app) if err != nil { handleErrorResponse(c, err) return } - err = s.FireAfterAppCreate(ctx, app) - if err != nil { - handleErrorResponse(c, err) - return - } - c.JSON(http.StatusOK, appResponse{"App successfully created", app}) } diff --git a/api/server/apps_delete.go b/api/server/apps_delete.go index e1116505d..d73fc670b 100644 --- a/api/server/apps_delete.go +++ b/api/server/apps_delete.go @@ -4,31 +4,18 @@ import ( "net/http" "github.com/fnproject/fn/api" - "github.com/fnproject/fn/api/common" - "github.com/fnproject/fn/api/models" "github.com/gin-gonic/gin" ) func (s *Server) handleAppDelete(c *gin.Context) { ctx := c.Request.Context() - log := common.Logger(ctx) - app := &models.App{Name: c.MustGet(api.AppName).(string)} - - err := s.FireBeforeAppDelete(ctx, app) - - err = s.datastore.RemoveApp(ctx, app.Name) + appName := c.MustGet(api.AppName).(string) + err := s.datastore.RemoveApp(ctx, appName) if err != nil { handleErrorResponse(c, err) return } - err = s.FireAfterAppDelete(ctx, app) - if err != nil { - log.WithError(err).Error("error firing after app delete") - handleErrorResponse(c, err) - return - } - c.JSON(http.StatusOK, gin.H{"message": "App deleted"}) } diff --git a/api/server/apps_get.go b/api/server/apps_get.go index 5f8eed367..1b1c027d7 100644 --- a/api/server/apps_get.go +++ b/api/server/apps_get.go @@ -12,22 +12,11 @@ func (s *Server) handleAppGet(c *gin.Context) { appName := c.MustGet(api.AppName).(string) - err := s.FireBeforeAppGet(ctx, appName) - if err != nil { - handleErrorResponse(c, err) - return - } - app, err := s.datastore.GetApp(ctx, appName) if err != nil { handleErrorResponse(c, err) return } - err = s.FireAfterAppGet(ctx, app) - if err != nil { - handleErrorResponse(c, err) - return - } c.JSON(http.StatusOK, appResponse{"Successfully loaded app", app}) } diff --git a/api/server/apps_list.go b/api/server/apps_list.go index 04097aac6..3d430e2fa 100644 --- a/api/server/apps_list.go +++ b/api/server/apps_list.go @@ -14,22 +14,11 @@ func (s *Server) handleAppList(c *gin.Context) { filter := &models.AppFilter{} filter.Cursor, filter.PerPage = pageParams(c, true) - err := s.FireBeforeAppsList(ctx, filter) - if err != nil { - handleErrorResponse(c, err) - return - } - apps, err := s.datastore.GetApps(ctx, filter) if err != nil { handleErrorResponse(c, err) return } - err = s.FireAfterAppsList(ctx, apps) - if err != nil { - handleErrorResponse(c, err) - return - } var nextCursor string if len(apps) > 0 && len(apps) == filter.PerPage { diff --git a/api/server/apps_update.go b/api/server/apps_update.go index 9c82b8fa3..def73ffe1 100644 --- a/api/server/apps_update.go +++ b/api/server/apps_update.go @@ -35,23 +35,11 @@ func (s *Server) handleAppUpdate(c *gin.Context) { wapp.App.Name = c.MustGet(api.AppName).(string) - err = s.FireBeforeAppUpdate(ctx, wapp.App) - if err != nil { - handleErrorResponse(c, err) - return - } - app, err := s.datastore.UpdateApp(ctx, wapp.App) if err != nil { handleErrorResponse(c, err) return } - err = s.FireAfterAppUpdate(ctx, wapp.App) - if err != nil { - handleErrorResponse(c, err) - return - } - c.JSON(http.StatusOK, appResponse{"App successfully updated", app}) } diff --git a/api/server/routes_create_update.go b/api/server/routes_create_update.go index 8c2e2c79c..159dda997 100644 --- a/api/server/routes_create_update.go +++ b/api/server/routes_create_update.go @@ -109,22 +109,10 @@ func (s *Server) ensureApp(ctx context.Context, wroute *models.RouteWrapper, met } else if app == nil { // Create a new application newapp := &models.App{Name: wroute.Route.AppName} - - err = s.FireBeforeAppCreate(ctx, newapp) - if err != nil { - return err - } - _, err = s.datastore.InsertApp(ctx, newapp) if err != nil { return err } - - err = s.FireAfterAppCreate(ctx, newapp) - if err != nil { - return err - } - } return nil } diff --git a/api/server/server.go b/api/server/server.go index 21d7578f4..c7dba764c 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -76,7 +76,7 @@ type Server struct { mq models.MessageQueue logstore models.LogStore nodeType ServerNodeType - appListeners []fnext.AppListener + appListeners *appListeners rootMiddlewares []fnext.Middleware apiMiddlewares []fnext.Middleware } @@ -279,6 +279,10 @@ func New(ctx context.Context, opts ...ServerOption) *Server { s.Router.Use(loggerWrap, traceWrap, panicWrap) // TODO should be opts optionalCorsWrap(s.Router) // TODO should be an opt s.bindHandlers(ctx) + + s.appListeners = new(appListeners) + s.datastore = fnext.NewDatastore(s.datastore, s.appListeners) + return s } diff --git a/fnext/datastore.go b/fnext/datastore.go new file mode 100644 index 000000000..38e708f71 --- /dev/null +++ b/fnext/datastore.go @@ -0,0 +1,95 @@ +package fnext + +import ( + "context" + + "github.com/fnproject/fn/api/models" +) + +func NewDatastore(ds models.Datastore, al AppListener) models.Datastore { + return &extds{ + Datastore: ds, + al: al, + } +} + +type extds struct { + models.Datastore + al AppListener +} + +func (e *extds) GetApp(ctx context.Context, appName string) (*models.App, error) { + err := e.al.BeforeAppGet(ctx, appName) + if err != nil { + return nil, err + } + + app, err := e.Datastore.GetApp(ctx, appName) + if err != nil { + return nil, err + } + + err = e.al.AfterAppGet(ctx, app) + return app, err +} + +func (e *extds) InsertApp(ctx context.Context, app *models.App) (*models.App, error) { + err := e.al.BeforeAppCreate(ctx, app) + if err != nil { + return nil, err + } + + app, err = e.Datastore.InsertApp(ctx, app) + if err != nil { + return nil, err + } + + err = e.al.AfterAppCreate(ctx, app) + return app, err +} + +func (e *extds) UpdateApp(ctx context.Context, app *models.App) (*models.App, error) { + err := e.al.BeforeAppUpdate(ctx, app) + if err != nil { + return nil, err + } + + app, err = e.Datastore.UpdateApp(ctx, app) + if err != nil { + return nil, err + } + + err = e.al.AfterAppUpdate(ctx, app) + return app, err +} + +func (e *extds) RemoveApp(ctx context.Context, appName string) error { + var app models.App + app.Name = appName + err := e.al.BeforeAppDelete(ctx, &app) + if err != nil { + return err + } + + err = e.Datastore.RemoveApp(ctx, appName) + if err != nil { + return err + } + + return e.al.AfterAppDelete(ctx, &app) +} + +func (e *extds) GetApps(ctx context.Context, filter *models.AppFilter) ([]*models.App, error) { + err := e.al.BeforeAppsList(ctx, filter) + if err != nil { + return nil, err + } + + apps, err := e.Datastore.GetApps(ctx, filter) + if err != nil { + return nil, err + } + + err = e.al.AfterAppsList(ctx, apps) + return apps, err +}