From 5f3294f4a0ba5902c84bbc6971729f2101dba10f Mon Sep 17 00:00:00 2001 From: Aravind Shankar Date: Wed, 18 Sep 2019 11:06:16 +0530 Subject: [PATCH] optimise migrate api for console on cli (#2895) --- cli/commands/console.go | 42 +++++++++-------- cli/commands/console_test.go | 4 +- cli/commands/metadata_apply.go | 2 +- cli/commands/metadata_clear.go | 2 +- cli/commands/metadata_export.go | 2 +- cli/commands/metadata_reload.go | 2 +- cli/commands/migrate.go | 4 +- cli/commands/migrate_apply.go | 2 +- cli/commands/migrate_create.go | 2 +- cli/commands/migrate_status.go | 2 +- cli/migrate/api/metadata.go | 34 ++------------ cli/migrate/api/migrate.go | 43 ++++++----------- cli/migrate/api/settings.go | 35 ++------------ cli/migrate/database/driver.go | 2 + cli/migrate/database/hasuradb/hasuradb.go | 12 ++++- cli/migrate/migrate.go | 13 +++--- cli/migrate/source/driver.go | 2 + cli/migrate/source/file/file.go | 56 +++++++++++++---------- cli/migrate/source/stub/stub.go | 4 ++ 19 files changed, 111 insertions(+), 154 deletions(-) diff --git a/cli/commands/console.go b/cli/commands/console.go index 5c0d7d1b..70169271 100644 --- a/cli/commands/console.go +++ b/cli/commands/console.go @@ -3,7 +3,6 @@ package commands import ( "fmt" "net/http" - "net/url" "sync" "github.com/fatih/color" @@ -11,9 +10,9 @@ import ( "github.com/gin-contrib/static" "github.com/gin-gonic/gin" "github.com/hasura/graphql-engine/cli" + "github.com/hasura/graphql-engine/cli/migrate" "github.com/hasura/graphql-engine/cli/migrate/api" "github.com/hasura/graphql-engine/cli/util" - "github.com/hasura/graphql-engine/cli/version" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/skratchdot/open-golang/open" @@ -85,14 +84,9 @@ func (o *consoleOptions) run() error { gin.SetMode(gin.ReleaseMode) // An Engine instance with the Logger and Recovery middleware already attached. - r := gin.New() + g := gin.New() - r.Use(allowCors()) - - // My Router struct - router := &cRouter{ - r, - } + g.Use(allowCors()) if o.EC.Version == nil { return errors.New("cannot validate version, object is nil") @@ -103,7 +97,18 @@ func (o *consoleOptions) run() error { return err } - router.setRoutes(o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.MigrationDir, metadataPath, o.EC.Logger, o.EC.Version) + t, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version, false) + if err != nil { + return err + } + + // My Router struct + r := &cRouter{ + g, + t, + } + + r.setRoutes(o.EC.MigrationDir, metadataPath, o.EC.Logger) consoleTemplateVersion := o.EC.Version.GetConsoleTemplateVersion() consoleAssetsVersion := o.EC.Version.GetConsoleAssetsVersion() @@ -134,7 +139,7 @@ func (o *consoleOptions) run() error { o.WG = wg wg.Add(1) go func() { - err = router.Run(o.Address + ":" + o.APIPort) + err = r.router.Run(o.Address + ":" + o.APIPort) if err != nil { o.EC.Logger.WithError(err).Errorf("error listening on port %s", o.APIPort) } @@ -171,15 +176,16 @@ func (o *consoleOptions) run() error { } type cRouter struct { - *gin.Engine + router *gin.Engine + migrate *migrate.Migrate } -func (router *cRouter) setRoutes(nurl *url.URL, adminSecret, migrationDir, metadataFile string, logger *logrus.Logger, v *version.Version) { - apis := router.Group("/apis") +func (r *cRouter) setRoutes(migrationDir, metadataFile string, logger *logrus.Logger) { + apis := r.router.Group("/apis") { apis.Use(setLogger(logger)) apis.Use(setFilePath(migrationDir)) - apis.Use(setDataPath(nurl, getAdminSecretHeaderName(v), adminSecret)) + apis.Use(setMigrate(r.migrate)) // Migrate api endpoints and middleware migrateAPIs := apis.Group("/migrate") { @@ -198,11 +204,9 @@ func (router *cRouter) setRoutes(nurl *url.URL, adminSecret, migrationDir, metad } } -func setDataPath(nurl *url.URL, adminSecretHeader, adminSecret string) gin.HandlerFunc { +func setMigrate(t *migrate.Migrate) gin.HandlerFunc { return func(c *gin.Context) { - host := getDataPath(nurl, adminSecretHeader, adminSecret) - - c.Set("dbpath", host) + c.Set("migrate", t) c.Next() } } diff --git a/cli/commands/console_test.go b/cli/commands/console_test.go index f2630f75..b54e8b4d 100644 --- a/cli/commands/console_test.go +++ b/cli/commands/console_test.go @@ -1,6 +1,7 @@ package commands import ( + "os" "testing" "time" @@ -19,7 +20,7 @@ func TestConsoleCmd(t *testing.T) { ec.Spinner = spinner.New(spinner.CharSets[7], 100*time.Millisecond) ec.ServerConfig = &cli.ServerConfig{ Endpoint: "http://localhost:8080", - AdminSecret: "", + AdminSecret: os.Getenv("HASURA_GRAPHQL_TEST_ADMIN_SECRET"), } ec.MetadataFile = []string{"metadata.yaml"} @@ -33,7 +34,6 @@ func TestConsoleCmd(t *testing.T) { if err != nil { t.Fatalf("prepare failed: %v", err) } - opts := &consoleOptions{ EC: ec, APIPort: "9693", diff --git a/cli/commands/metadata_apply.go b/cli/commands/metadata_apply.go index f7fe3580..027f0a20 100644 --- a/cli/commands/metadata_apply.go +++ b/cli/commands/metadata_apply.go @@ -57,7 +57,7 @@ type metadataApplyOptions struct { } func (o *metadataApplyOptions) run() error { - migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version) + migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version, true) if err != nil { return err } diff --git a/cli/commands/metadata_clear.go b/cli/commands/metadata_clear.go index ea691df9..052d3997 100644 --- a/cli/commands/metadata_clear.go +++ b/cli/commands/metadata_clear.go @@ -61,7 +61,7 @@ type metadataClearOptions struct { } func (o *metadataClearOptions) run() error { - migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version) + migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version, true) if err != nil { return err } diff --git a/cli/commands/metadata_export.go b/cli/commands/metadata_export.go index 5cf3496f..5eddcbe9 100644 --- a/cli/commands/metadata_export.go +++ b/cli/commands/metadata_export.go @@ -64,7 +64,7 @@ type metadataExportOptions struct { } func (o *metadataExportOptions) run() error { - migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version) + migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version, true) if err != nil { return err } diff --git a/cli/commands/metadata_reload.go b/cli/commands/metadata_reload.go index c4e8993b..19d21257 100644 --- a/cli/commands/metadata_reload.go +++ b/cli/commands/metadata_reload.go @@ -57,7 +57,7 @@ type metadataReloadOptions struct { } func (o *metadataReloadOptions) run() error { - migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version) + migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version, true) if err != nil { return err } diff --git a/cli/commands/migrate.go b/cli/commands/migrate.go index 9992978c..d5b5d114 100644 --- a/cli/commands/migrate.go +++ b/cli/commands/migrate.go @@ -34,10 +34,10 @@ func NewMigrateCmd(ec *cli.ExecutionContext) *cobra.Command { return migrateCmd } -func newMigrate(dir string, db *url.URL, adminSecretValue string, logger *logrus.Logger, v *version.Version) (*migrate.Migrate, error) { +func newMigrate(dir string, db *url.URL, adminSecretValue string, logger *logrus.Logger, v *version.Version, isCmd bool) (*migrate.Migrate, error) { dbURL := getDataPath(db, getAdminSecretHeaderName(v), adminSecretValue) fileURL := getFilePath(dir) - t, err := migrate.New(fileURL.String(), dbURL.String(), true, logger) + t, err := migrate.New(fileURL.String(), dbURL.String(), isCmd, logger) if err != nil { return nil, errors.Wrap(err, "cannot create migrate instance") } diff --git a/cli/commands/migrate_apply.go b/cli/commands/migrate_apply.go index 37cd6b8e..b46aad68 100644 --- a/cli/commands/migrate_apply.go +++ b/cli/commands/migrate_apply.go @@ -71,7 +71,7 @@ func (o *migrateApplyOptions) run() error { return errors.Wrap(err, "error validating flags") } - migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version) + migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version, true) if err != nil { return err } diff --git a/cli/commands/migrate_create.go b/cli/commands/migrate_create.go index 163ed5c7..96ad62db 100644 --- a/cli/commands/migrate_create.go +++ b/cli/commands/migrate_create.go @@ -109,7 +109,7 @@ func (o *migrateCreateOptions) run() (version int64, err error) { var migrateDrv *migrate.Migrate if o.sqlServer || o.metaDataServer { - migrateDrv, err = newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version) + migrateDrv, err = newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version, true) if err != nil { return 0, errors.Wrap(err, "cannot create migrate instance") } diff --git a/cli/commands/migrate_status.go b/cli/commands/migrate_status.go index ee6b5bad..587f2e2b 100644 --- a/cli/commands/migrate_status.go +++ b/cli/commands/migrate_status.go @@ -58,7 +58,7 @@ type migrateStatusOptions struct { } func (o *migrateStatusOptions) run() (*migrate.Status, error) { - migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version) + migrateDrv, err := newMigrate(o.EC.MigrationDir, o.EC.ServerConfig.ParsedEndpoint, o.EC.ServerConfig.AdminSecret, o.EC.Logger, o.EC.Version, true) if err != nil { return nil, err } diff --git a/cli/migrate/api/metadata.go b/cli/migrate/api/metadata.go index ceb86901..0c1f5658 100644 --- a/cli/migrate/api/metadata.go +++ b/cli/migrate/api/metadata.go @@ -4,39 +4,22 @@ import ( "encoding/json" "io/ioutil" "net/http" - "net/url" "strings" "github.com/ghodss/yaml" "github.com/gin-gonic/gin" "github.com/hasura/graphql-engine/cli/migrate" - "github.com/sirupsen/logrus" ) func MetadataAPI(c *gin.Context) { - // Get File url - sourcePtr, ok := c.Get("filedir") - if !ok { - return - } - - sourceURL := sourcePtr.(*url.URL) - - // Get hasuradb url - databasePtr, ok := c.Get("dbpath") + // Get migrate instance + migratePtr, ok := c.Get("migrate") if !ok { return } // Convert to url.URL - databaseURL := databasePtr.(*url.URL) - - // Get Logger - loggerPtr, ok := c.Get("logger") - if !ok { - return - } - logger := loggerPtr.(*logrus.Logger) + t := migratePtr.(*migrate.Migrate) metadataFilePtr, ok := c.Get("metadataFile") if !ok { @@ -44,17 +27,6 @@ func MetadataAPI(c *gin.Context) { } metadataFile := metadataFilePtr.(string) - // Create new migrate - t, err := migrate.New(sourceURL.String(), databaseURL.String(), false, logger) - if err != nil { - if strings.HasPrefix(err.Error(), DataAPIError) { - c.JSON(http.StatusInternalServerError, &Response{Code: "data_api_error", Message: err.Error()}) - return - } - c.JSON(http.StatusInternalServerError, &Response{Code: "internal_error", Message: err.Error()}) - return - } - // Switch on request method switch c.Request.Method { case "GET": diff --git a/cli/migrate/api/migrate.go b/cli/migrate/api/migrate.go index 299f7cb3..35075779 100644 --- a/cli/migrate/api/migrate.go +++ b/cli/migrate/api/migrate.go @@ -32,14 +32,12 @@ type Request struct { } func MigrateAPI(c *gin.Context) { - // Get File url - sourcePtr, ok := c.Get("filedir") + migratePtr, ok := c.Get("migrate") if !ok { return } - - // Get hasuradb url - databasePtr, ok := c.Get("dbpath") + // Get File url + sourcePtr, ok := c.Get("filedir") if !ok { return } @@ -51,21 +49,10 @@ func MigrateAPI(c *gin.Context) { } // Convert to url.URL - databaseURL := databasePtr.(*url.URL) + t := migratePtr.(*migrate.Migrate) sourceURL := sourcePtr.(*url.URL) logger := loggerPtr.(*logrus.Logger) - // Create new migrate - t, err := migrate.New(sourceURL.String(), databaseURL.String(), false, logger) - if err != nil { - if strings.HasPrefix(err.Error(), DataAPIError) { - c.JSON(http.StatusInternalServerError, &Response{Code: "data_api_error", Message: err.Error()}) - return - } - c.JSON(http.StatusInternalServerError, &Response{Code: "internal_error", Message: err.Error()}) - return - } - // Switch on request method switch c.Request.Method { case "POST": @@ -82,7 +69,7 @@ func MigrateAPI(c *gin.Context) { timestamp := startTime.UnixNano() / int64(time.Millisecond) createOptions := cmd.New(timestamp, request.Name, sourceURL.Path) - err = createOptions.SetMetaUp(request.Up) + err := createOptions.SetMetaUp(request.Up) if err != nil { c.JSON(http.StatusInternalServerError, &Response{Code: "create_file_error", Message: err.Error()}) return @@ -99,25 +86,23 @@ func MigrateAPI(c *gin.Context) { return } + defer func() { + if err != nil { + err = createOptions.Delete() + if err != nil { + logger.Debug(err) + } + } + }() + // Rescan file system err = t.ReScan() if err != nil { - deleteErr := createOptions.Delete() - if deleteErr != nil { - c.JSON(http.StatusInternalServerError, &Response{Code: "delete_file_error", Message: deleteErr.Error()}) - return - } c.JSON(http.StatusInternalServerError, &Response{Code: "internal_error", Message: err.Error()}) return } if err = t.Migrate(uint64(timestamp), "up"); err != nil { - deleteErr := createOptions.Delete() - if deleteErr != nil { - c.JSON(http.StatusInternalServerError, &Response{Code: "delete_file_error", Message: deleteErr.Error()}) - return - } - if strings.HasPrefix(err.Error(), DataAPIError) { c.JSON(http.StatusBadRequest, &Response{Code: "data_api_error", Message: strings.TrimPrefix(err.Error(), DataAPIError)}) return diff --git a/cli/migrate/api/settings.go b/cli/migrate/api/settings.go index cd0a0a03..024de042 100644 --- a/cli/migrate/api/settings.go +++ b/cli/migrate/api/settings.go @@ -2,12 +2,10 @@ package api import ( "net/http" - "net/url" "strings" "github.com/gin-gonic/gin" "github.com/hasura/graphql-engine/cli/migrate" - "github.com/sirupsen/logrus" ) type SettingReqeust struct { @@ -16,40 +14,13 @@ type SettingReqeust struct { } func SettingsAPI(c *gin.Context) { - // Get File url - sourcePtr, ok := c.Get("filedir") + // Get migrate instance + migratePtr, ok := c.Get("migrate") if !ok { return } - sourceURL := sourcePtr.(*url.URL) - - // Get hasuradb url - databasePtr, ok := c.Get("dbpath") - if !ok { - return - } - - // Convert to url.URL - databaseURL := databasePtr.(*url.URL) - - // Get Logger - loggerPtr, ok := c.Get("logger") - if !ok { - return - } - logger := loggerPtr.(*logrus.Logger) - - // Create new migrate - t, err := migrate.New(sourceURL.String(), databaseURL.String(), false, logger) - if err != nil { - if strings.HasPrefix(err.Error(), DataAPIError) { - c.JSON(500, &Response{Code: "data_api_error", Message: err.Error()}) - return - } - c.JSON(500, &Response{Code: "internal_error", Message: err.Error()}) - return - } + t := migratePtr.(*migrate.Migrate) // Switch on request method switch c.Request.Method { diff --git a/cli/migrate/database/driver.go b/cli/migrate/database/driver.go index 506e5778..1a63eabe 100644 --- a/cli/migrate/database/driver.go +++ b/cli/migrate/database/driver.go @@ -49,6 +49,8 @@ type Driver interface { // Migrate will call this function only once per instance. Close() error + Scan() error + // Lock should acquire a database lock so that only one migration process // can run at a time. Migrate will call this function before Run is called. // If the implementation can't provide this functionality, return nil. diff --git a/cli/migrate/database/hasuradb/hasuradb.go b/cli/migrate/database/hasuradb/hasuradb.go index 399eb485..f2ee7ef8 100644 --- a/cli/migrate/database/hasuradb/hasuradb.go +++ b/cli/migrate/database/hasuradb/hasuradb.go @@ -78,7 +78,7 @@ func WithInstance(config *Config, logger *log.Logger) (database.Driver, error) { return nil, err } - err := hx.getVersions() + err := hx.Scan() if err != nil { return nil, err } @@ -145,6 +145,11 @@ func (h *HasuraDB) Close() error { return nil } +func (h *HasuraDB) Scan() error { + h.migrations = database.NewMigrations() + return h.getVersions() +} + func (h *HasuraDB) Lock() error { if h.isLocked { return database.ErrLocked @@ -164,6 +169,10 @@ func (h *HasuraDB) UnLock() error { return nil } + defer func() { + h.isLocked = false + }() + if len(h.migrationQuery.Args) == 0 { return nil } @@ -213,7 +222,6 @@ func (h *HasuraDB) UnLock() error { } return horror.Error(h.config.isCMD) } - h.isLocked = false return nil } diff --git a/cli/migrate/migrate.go b/cli/migrate/migrate.go index 8b547e3b..bc57ffb5 100644 --- a/cli/migrate/migrate.go +++ b/cli/migrate/migrate.go @@ -119,6 +119,7 @@ func New(sourceUrl string, databaseUrl string, cmd bool, logger *log.Logger) (*M if logger == nil { logger = log.New() } + m.Logger = logger sourceDrv, err := source.Open(sourceUrl, logger) if err != nil { @@ -150,19 +151,17 @@ func newCommon(cmd bool) *Migrate { } func (m *Migrate) ReScan() error { - sourceDrv, err := source.Open(m.sourceURL, m.Logger) + err := m.sourceDrv.Scan() if err != nil { m.Logger.Debug(err) return err } - m.sourceDrv = sourceDrv - databaseDrv, err := database.Open(m.databaseURL, m.isCMD, m.Logger) + err = m.databaseDrv.Scan() if err != nil { m.Logger.Debug(err) return err } - m.databaseDrv = databaseDrv err = m.calculateStatus() if err != nil { @@ -1096,11 +1095,13 @@ func (m *Migrate) unlock() error { m.isLockedMu.Lock() defer m.isLockedMu.Unlock() + defer func() { + m.isLocked = false + }() + if err := m.databaseDrv.UnLock(); err != nil { return err } - - m.isLocked = false return nil } diff --git a/cli/migrate/source/driver.go b/cli/migrate/source/driver.go index 55d86601..4c3a60cf 100644 --- a/cli/migrate/source/driver.go +++ b/cli/migrate/source/driver.go @@ -40,6 +40,8 @@ type Driver interface { // Migrate will call this function only once per instance. Close() error + Scan() error + // First returns the very first migration version available to the driver. // Migrate will call this function multiple times. // If there is no version available, it must return os.ErrNotExist. diff --git a/cli/migrate/source/file/file.go b/cli/migrate/source/file/file.go index a22bb3c9..ae83da8e 100644 --- a/cli/migrate/source/file/file.go +++ b/cli/migrate/source/file/file.go @@ -59,12 +59,6 @@ func (f *File) Open(url string, logger *log.Logger) (source.Driver, error) { p = strings.TrimPrefix(p, "/") } - // scan directory - files, err := ioutil.ReadDir(p) - if err != nil { - return nil, err - } - nf := &File{ url: url, logger: logger, @@ -72,24 +66,9 @@ func (f *File) Open(url string, logger *log.Logger) (source.Driver, error) { migrations: source.NewMigrations(), } - for _, fi := range files { - if !fi.IsDir() { - m, err := source.DefaultParse(fi.Name(), p) - if err != nil { - continue // ignore files that we can't parse - } - ok, err := source.IsEmptyFile(m, p) - if err != nil { - return nil, err - } - if !ok { - continue - } - err = nf.migrations.Append(m) - if err != nil { - return nil, err - } - } + err = nf.Scan() + if err != nil { + return nil, err } return nf, nil } @@ -99,6 +78,35 @@ func (f *File) Close() error { return nil } +func (f *File) Scan() error { + f.migrations = source.NewMigrations() + files, err := ioutil.ReadDir(f.path) + if err != nil { + return err + } + + for _, fi := range files { + if !fi.IsDir() { + m, err := source.DefaultParse(fi.Name(), f.path) + if err != nil { + continue // ignore files that we can't parse + } + ok, err := source.IsEmptyFile(m, f.path) + if err != nil { + return err + } + if !ok { + continue + } + err = f.migrations.Append(m) + if err != nil { + return err + } + } + } + return nil +} + func (f *File) First() (version uint64, err error) { if v, ok := f.migrations.First(); !ok { return 0, &os.PathError{Op: "first", Path: f.path, Err: os.ErrNotExist} diff --git a/cli/migrate/source/stub/stub.go b/cli/migrate/source/stub/stub.go index 5c22aeb5..72d032b9 100644 --- a/cli/migrate/source/stub/stub.go +++ b/cli/migrate/source/stub/stub.go @@ -41,6 +41,10 @@ func (s *Stub) Close() error { return nil } +func (s *Stub) Scan() error { + return nil +} + func (s *Stub) First() (version uint64, err error) { if v, ok := s.Migrations.First(); !ok { return 0, &os.PathError{Op: "first", Path: s.Url, Err: os.ErrNotExist} // TODO: s.Url can be empty when called with WithInstance