Improved migration creation + Windows fix (#352)

* CLI: Improve migration creation.

* Validates migration version on creation, to avoid creation of duplicated versions
* Uses `os.OpenFile` with `O_CREATE|O_EXCL` to create files to avoid file collisions
* Uses `filepath.Join` to concatenate paths, making `cleanPath()` not necessary
* Prints generated filenames
* Fixes #238
* Supersedes #250

* CLI: change `createCmd` to return error and accept `print` parameter

* feat: print out an abs path

Better for Windows OS when specify -dir as /subdir, user will see C:/subdir/0001_name.up.sql rather than /subdir/0001_name.up.sql

* test: fixed abs path test fail for OS Windows

abs path tests fails because filepath.IsAbs() treats `/subdir` path as invalid abs path at windows when drive letter is not present

* feat: print absolute path for created files

Better for Windows OS systems where `/path` can be interpreted in different ways depending on working dir

* test: corrected tests for OS Windows

OS Windows has different interpretation of `/path`, it depends on working dir.
If working dir D:\test it interprets `/path` as `D:\path`

* test: fixed `dir invalid` test

Linux OS has less restriction on a filepath than Windows, so path invalid in windows is perfectly valid for Linux. The only invalid dir name in Linux is one ending with null string terminator (\000)

* refac(cli): *Cmd() now returns an error and not uses log.fatalErr(err)

* docs: added godoc, migarate usage updated

* refac(cli): code refactored

* refac: removed unnecessary path covert

* docs: comment added

* test: fixed code review issue, noErrorExpected var removed

#352 (comment)

* docs: fixed godoc

Co-authored-by: Kiyoshi '13k' Murata <[email protected]>

Author: r3code

internal/cli/commands.go

@@ -3,184 +3,223 @@ package cli
 import (
 	"errors"
 	"fmt"
-	"github.com/golang-migrate/migrate/v4"
-	_ "github.com/golang-migrate/migrate/v4/database/stub" // TODO remove again
-	_ "github.com/golang-migrate/migrate/v4/source/file"
 	"os"
 	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
+
+	"github.com/golang-migrate/migrate/v4"
+	_ "github.com/golang-migrate/migrate/v4/database/stub" // TODO remove again
+	_ "github.com/golang-migrate/migrate/v4/source/file"
 )
 
-func nextSeq(matches []string, dir string, seqDigits int) (string, error) {
+var (
+	errInvalidSequenceWidth     = errors.New("Digits must be positive")
+	errIncompatibleSeqAndFormat = errors.New("The seq and format options are mutually exclusive")
+	errInvalidTimeFormat        = errors.New("Time format may not be empty")
+)
+
+func nextSeqVersion(matches []string, seqDigits int) (string, error) {
 	if seqDigits <= 0 {
-		return "", errors.New("Digits must be positive")
+		return "", errInvalidSequenceWidth
 	}
 
-	nextSeq := 1
+	nextSeq := uint64(1)
+
 	if len(matches) > 0 {
 		filename := matches[len(matches)-1]
-		matchSeqStr := strings.TrimPrefix(filename, dir)
+		matchSeqStr := filepath.Base(filename)
 		idx := strings.Index(matchSeqStr, "_")
+
 		if idx < 1 { // Using 1 instead of 0 since there should be at least 1 digit
-			return "", errors.New("Malformed migration filename: " + filename)
+			return "", fmt.Errorf("Malformed migration filename: %s", filename)
 		}
-		matchSeqStr = matchSeqStr[0:idx]
+
 		var err error
-		nextSeq, err = strconv.Atoi(matchSeqStr)
+		matchSeqStr = matchSeqStr[0:idx]
+		nextSeq, err = strconv.ParseUint(matchSeqStr, 10, 64)
+
 		if err != nil {
 			return "", err
 		}
+
 		nextSeq++
 	}
-	if nextSeq <= 0 {
-		return "", errors.New("Next sequence number must be positive")
-	}
 
-	nextSeqStr := strconv.Itoa(nextSeq)
-	if len(nextSeqStr) > seqDigits {
-		return "", fmt.Errorf("Next sequence number %s too large. At most %d digits are allowed", nextSeqStr, seqDigits)
-	}
-	padding := seqDigits - len(nextSeqStr)
-	if padding > 0 {
-		nextSeqStr = strings.Repeat("0", padding) + nextSeqStr
+	version := fmt.Sprintf("%0[2]*[1]d", nextSeq, seqDigits)
+
+	if len(version) > seqDigits {
+		return "", fmt.Errorf("Next sequence number %s too large. At most %d digits are allowed", version, seqDigits)
 	}
-	return nextSeqStr, nil
+
+	return version, nil
 }
 
-// cleanDir normalizes the provided directory
-func cleanDir(dir string) string {
-	dir = filepath.Clean(dir)
-	switch dir {
-	case ".":
-		return ""
-	case "/":
-		return dir
+func timeVersion(startTime time.Time, format string) (version string, err error) {
+	switch format {
+	case "":
+		err = errInvalidTimeFormat
+	case "unix":
+		version = strconv.FormatInt(startTime.Unix(), 10)
+	case "unixNano":
+		version = strconv.FormatInt(startTime.UnixNano(), 10)
 	default:
-		return dir + "/"
+		version = startTime.Format(format)
 	}
+
+	return
 }
 
 // createCmd (meant to be called via a CLI command) creates a new migration
-func createCmd(dir string, startTime time.Time, format string, name string, ext string, seq bool, seqDigits int) {
-	dir = cleanDir(dir)
-	var base string
+func createCmd(dir string, startTime time.Time, format string, name string, ext string, seq bool, seqDigits int, print bool) error {
 	if seq && format != defaultTimeFormat {
-		log.fatalErr(errors.New("The seq and format options are mutually exclusive"))
+		return errIncompatibleSeqAndFormat
 	}
+
+	var version string
+	var err error
+
+	dir = filepath.Clean(dir)
+	ext = "." + strings.TrimPrefix(ext, ".")
+
 	if seq {
-		if seqDigits <= 0 {
-			log.fatalErr(errors.New("Digits must be positive"))
-		}
-		matches, err := filepath.Glob(dir + "*" + ext)
+		matches, err := filepath.Glob(filepath.Join(dir, "*"+ext))
+
 		if err != nil {
-			log.fatalErr(err)
+			return err
 		}
-		nextSeqStr, err := nextSeq(matches, dir, seqDigits)
+
+		version, err = nextSeqVersion(matches, seqDigits)
+
 		if err != nil {
-			log.fatalErr(err)
+			return err
 		}
-		base = fmt.Sprintf("%v%v_%v.", dir, nextSeqStr, name)
 	} else {
-		switch format {
-		case "":
-			log.fatal("Time format may not be empty")
-		case "unix":
-			base = fmt.Sprintf("%v%v_%v.", dir, startTime.Unix(), name)
-		case "unixNano":
-			base = fmt.Sprintf("%v%v_%v.", dir, startTime.UnixNano(), name)
-		default:
-			base = fmt.Sprintf("%v%v_%v.", dir, startTime.Format(format), name)
+		version, err = timeVersion(startTime, format)
+
+		if err != nil {
+			return err
 		}
 	}
 
-	if err := os.MkdirAll(dir, os.ModePerm); err != nil {
-		log.fatalErr(err)
+	versionGlob := filepath.Join(dir, version+"_*"+ext)
+	matches, err := filepath.Glob(versionGlob)
+
+	if err != nil {
+		return err
+	}
+
+	if len(matches) > 0 {
+		return fmt.Errorf("duplicate migration version: %s", version)
+	}
+
+	if err = os.MkdirAll(dir, os.ModePerm); err != nil {
+		return err
+	}
+
+	for _, direction := range []string{"up", "down"} {
+		basename := fmt.Sprintf("%s_%s.%s%s", version, name, direction, ext)
+		filename := filepath.Join(dir, basename)
+
+		if err = createFile(filename); err != nil {
+			return err
+		}
+
+		if print {
+			absPath, _ := filepath.Abs(filename)
+			log.Println(absPath)
+		}
 	}
 
-	createFile(base + "up" + ext)
-	createFile(base + "down" + ext)
+	return nil
 }
 
-func createFile(fname string) {
-	if _, err := os.Create(fname); err != nil {
-		log.fatalErr(err)
+func createFile(filename string) error {
+	// create exclusive (fails if file already exists)
+	// os.Create() specifies 0666 as the FileMode, so we're doing the same
+	f, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)
+
+	if err != nil {
+		return err
 	}
+
+	return f.Close()
 }
 
-func gotoCmd(m *migrate.Migrate, v uint) {
+func gotoCmd(m *migrate.Migrate, v uint) error {
 	if err := m.Migrate(v); err != nil {
 		if err != migrate.ErrNoChange {
-			log.fatalErr(err)
-		} else {
-			log.Println(err)
+			return err
 		}
+		log.Println(err)
 	}
+	return nil
 }
 
-func upCmd(m *migrate.Migrate, limit int) {
+func upCmd(m *migrate.Migrate, limit int) error {
 	if limit >= 0 {
 		if err := m.Steps(limit); err != nil {
 			if err != migrate.ErrNoChange {
-				log.fatalErr(err)
-			} else {
-				log.Println(err)
+				return err
 			}
+			log.Println(err)
 		}
 	} else {
 		if err := m.Up(); err != nil {
 			if err != migrate.ErrNoChange {
-				log.fatalErr(err)
-			} else {
-				log.Println(err)
+				return err
 			}
+			log.Println(err)
 		}
 	}
+	return nil
 }
 
-func downCmd(m *migrate.Migrate, limit int) {
+func downCmd(m *migrate.Migrate, limit int) error {
 	if limit >= 0 {
 		if err := m.Steps(-limit); err != nil {
 			if err != migrate.ErrNoChange {
-				log.fatalErr(err)
-			} else {
-				log.Println(err)
+				return err
 			}
+			log.Println(err)
 		}
 	} else {
 		if err := m.Down(); err != nil {
 			if err != migrate.ErrNoChange {
-				log.fatalErr(err)
-			} else {
-				log.Println(err)
+				return err
 			}
+			log.Println(err)
 		}
 	}
+	return nil
 }
 
-func dropCmd(m *migrate.Migrate) {
+func dropCmd(m *migrate.Migrate) error {
 	if err := m.Drop(); err != nil {
-		log.fatalErr(err)
+		return err
 	}
+	return nil
 }
 
-func forceCmd(m *migrate.Migrate, v int) {
+func forceCmd(m *migrate.Migrate, v int) error {
 	if err := m.Force(v); err != nil {
-		log.fatalErr(err)
+		return err
 	}
+	return nil
 }
 
-func versionCmd(m *migrate.Migrate) {
+func versionCmd(m *migrate.Migrate) error {
 	v, dirty, err := m.Version()
 	if err != nil {
-		log.fatalErr(err)
+		return err
 	}
 	if dirty {
 		log.Printf("%v (dirty)\n", v)
 	} else {
 		log.Println(v)
 	}
+	return nil
 }
 
 // numDownMigrationsFromArgs returns an int for number of migrations to apply