From c7bacb831beed5beee5e944b3cde4e512401132b Mon Sep 17 00:00:00 2001 From: John Weldon Date: Sat, 5 Sep 2015 17:40:56 -0700 Subject: [PATCH 1/3] Test to demonstrate issue #45 Added a unit test to demonstrate the problem in issue #45 The introduced makeFiles() function could be used to simplify the other tests too, but I didn't touch more than necessary for this commit. --- file/file_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/file/file_test.go b/file/file_test.go index 482c1a9..1d8f41b 100644 --- a/file/file_test.go +++ b/file/file_test.go @@ -209,3 +209,41 @@ func TestFiles(t *testing.T) { } } + +func TestDuplicateFiles(t *testing.T) { + dups := []string{ + "001_migration.up.sql", + "001_duplicate.up.sql", + } + + root, cleanFn, err := makeFiles("TestDuplicateFiles", dups...) + defer cleanFn() + + if err != nil { + t.Fatal(err) + } + + _, err = ReadMigrationFiles(root, FilenameRegex("sql")) + if err == nil { + t.Fatal("Expected duplicate migration file error") + } +} + +func makeFiles(testname string, names ...string) (root string, cleanup func(), err error) { + cleanup = func() {} + root, err = ioutil.TempDir("/tmp", testname) + if err != nil { + return + } + cleanup = func() { os.RemoveAll(root) } + if err = ioutil.WriteFile(path.Join(root, "nonsense.txt"), nil, 0755); err != nil { + return + } + + for _, name := range names { + if err = ioutil.WriteFile(path.Join(root, name), nil, 0755); err != nil { + return + } + } + return +} From ca486013969b7e1058fef293a45dcccb023d3337 Mon Sep 17 00:00:00 2001 From: John Weldon Date: Sat, 5 Sep 2015 17:41:11 -0700 Subject: [PATCH 2/3] Proposed fix for #45 This is the bare minimum proposed fix for #45 By using a nested map the way I do in this PR we could also simplify the logic further down where we look for matching direction.Up or direction.Down migrations. --- file/file.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/file/file.go b/file/file.go index 57addb3..54992a4 100644 --- a/file/file.go +++ b/file/file.go @@ -163,9 +163,18 @@ func ReadMigrationFiles(path string, filenameRegex *regexp.Regexp) (files Migrat d direction.Direction } tmpFiles := make([]*tmpFile, 0) + tmpFileMap := map[uint64]map[direction.Direction]tmpFile{} for _, file := range ioFiles { version, name, d, err := parseFilenameSchema(file.Name(), filenameRegex) if err == nil { + if _, ok := tmpFileMap[version]; !ok { + tmpFileMap[version] = map[direction.Direction]tmpFile{} + } + if existing, ok := tmpFileMap[version][d]; !ok { + tmpFileMap[version][d] = tmpFile{version: version, name: name, filename: file.Name(), d: d} + } else { + return nil, fmt.Errorf("duplicate migration file version %d : %q and %q", version, existing.filename, file.Name()) + } tmpFiles = append(tmpFiles, &tmpFile{version, name, file.Name(), d}) } } From 07fedf844530da99dc7fa5aa1ab03e3b1efbda31 Mon Sep 17 00:00:00 2001 From: John Weldon Date: Mon, 14 Sep 2015 16:41:19 -0700 Subject: [PATCH 3/3] Add some docstring explanation to makeFiles --- file/file_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/file/file_test.go b/file/file_test.go index 1d8f41b..b9ddeab 100644 --- a/file/file_test.go +++ b/file/file_test.go @@ -229,6 +229,10 @@ func TestDuplicateFiles(t *testing.T) { } } +// makeFiles takes an identifier, and a list of file names and uses them to create a temporary +// directory populated with files named with the names passed in. makeFiles returns the root +// directory name, and a func suitable for a defer cleanup to remove the temporary files after +// the calling function exits. func makeFiles(testname string, names ...string) (root string, cleanup func(), err error) { cleanup = func() {} root, err = ioutil.TempDir("/tmp", testname)