optimise migrate api for console on cli (#2895)

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())
+	g.Use(allowCors())
-
-	// My Router struct
-	router := &cRouter{
-		r,
-	}
 
 	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) {
+func (r *cRouter) setRoutes(migrationDir, metadataFile string, logger *logrus.Logger) {
-	apis := router.Group("/apis")
+	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("migrate", t)
-
-		c.Set("dbpath", host)
 		c.Next()
 	}
 }

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",

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
 	}

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
 	}

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
 	}

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
 	}

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")
 	}

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
 	}

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")
 		}

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
 	}

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
+	// Get migrate instance
-	sourcePtr, ok := c.Get("filedir")
+	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)
+	t := migratePtr.(*migrate.Migrate)
-
-	// Get Logger
-	loggerPtr, ok := c.Get("logger")
-	if !ok {
-		return
-	}
-	logger := loggerPtr.(*logrus.Logger)
 
 	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":

cli/migrate/api/migrate.go

@@ -32,14 +32,12 @@ type Request struct {
 }
 
 func MigrateAPI(c *gin.Context) {
-	// Get File url
+	migratePtr, ok := c.Get("migrate")
-	sourcePtr, ok := c.Get("filedir")
 	if !ok {
 		return
 	}
-
+	// Get File url
-	// Get hasuradb url
+	sourcePtr, ok := c.Get("filedir")
-	databasePtr, ok := c.Get("dbpath")
 	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

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
+	// Get migrate instance
-	sourcePtr, ok := c.Get("filedir")
+	migratePtr, ok := c.Get("migrate")
 	if !ok {
 		return
 	}
 
-	sourceURL := sourcePtr.(*url.URL)
+	t := migratePtr.(*migrate.Migrate)
-
-	// 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
-	}
 
 	// Switch on request method
 	switch c.Request.Method {

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.

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
 }
 

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
 }
 

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.

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 {
+	err = nf.Scan()
-		if !fi.IsDir() {
+	if err != nil {
-			m, err := source.DefaultParse(fi.Name(), p)
+		return nil, err
-			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
-			}
-		}
 	}
 	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}

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