From 41dfe1d575462cfa03953811cae058570245b935 Mon Sep 17 00:00:00 2001 From: Julius Kriukas Date: Tue, 22 Oct 2019 00:53:36 +0300 Subject: [PATCH] Add httpfs.New() constructor with delayed errors This PR adds a new httpfs source driver constructor New(). It is identical to the WithInstance() constructor but will postpone any errors until the driver is actually being used. This allows clients of this package to have less error checks without loosing correctness. In addition this PR adds more tests, improves test code coverage, and makes golinter happy by removing deferred Close call. --- source/httpfs/README.md | 15 +++- source/httpfs/coverage.out | 45 ++++++++++ source/httpfs/driver.go | 22 ++++- source/httpfs/driver_test.go | 89 ++++++++++++++++--- source/httpfs/faileddriver.go | 41 +++++++++ .../testdata/duplicates/1_foobar.up.sql | 1 + .../testdata/duplicates/1_foobaz.up.sql | 1 + .../testdata/sql/other-files-are-ignored | 0 8 files changed, 197 insertions(+), 17 deletions(-) create mode 100644 source/httpfs/coverage.out create mode 100644 source/httpfs/faileddriver.go create mode 100644 source/httpfs/testdata/duplicates/1_foobar.up.sql create mode 100644 source/httpfs/testdata/duplicates/1_foobaz.up.sql create mode 100644 source/httpfs/testdata/sql/other-files-are-ignored diff --git a/source/httpfs/README.md b/source/httpfs/README.md index 859ac1f0f..8e3f5b1fe 100644 --- a/source/httpfs/README.md +++ b/source/httpfs/README.md @@ -3,9 +3,9 @@ ## Usage To create migration data source from `http.FileSystem` instance use -`WithInstance()` function. Users of this package are responsible for getting -`http.FileSystem` instance. It is not possible to create httpfs instance from -URL. +`WithInstance()` or `New()` functions. Users of this package are responsible for +getting `http.FileSystem` instance. It is not possible to create httpfs instance +from URL. Example of using `http.Dir()` to read migrations from `sql` directory: @@ -14,3 +14,12 @@ Example of using `http.Dir()` to read migrations from `sql` directory: m, err := migrate.NewWithSourceInstance("httpfs", src, "database://url") err = m.Up() ``` + +Using `New()` instead of `WithInstance()` reduces the number of errors that +needs to be handled, example: + +```go + m, err := migrate.NewWithSourceInstance("httpfs", httpfs.New(http.Dir("sql"), ""), "database://url") + err = m.Up() +``` + diff --git a/source/httpfs/coverage.out b/source/httpfs/coverage.out new file mode 100644 index 000000000..6868128cf --- /dev/null +++ b/source/httpfs/coverage.out @@ -0,0 +1,45 @@ +mode: set +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:27.57,29.16 2 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:32.2,32.10 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:29.16,31.3 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:37.75,39.2 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:41.66,43.16 2 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:46.2,49.16 3 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:53.2,54.29 2 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:69.2,73.8 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:43.16,45.3 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:49.16,51.3 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:54.29,55.19 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:59.3,60.17 2 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:64.3,64.20 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:55.19,56.12 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:60.17,61.12 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:64.20,66.4 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:79.58,81.2 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:84.32,86.2 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:89.52,90.45 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:93.2,97.3 1 0 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:90.45,92.3 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:101.67,102.51 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:105.2,109.3 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:102.51,104.3 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:113.67,114.51 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:117.2,121.3 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:114.51,116.3 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:125.87,127.9 2 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:134.2,135.16 2 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:138.2,138.32 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:127.9,133.3 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:135.16,137.3 1 0 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:142.89,144.9 2 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:151.2,152.16 2 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:155.2,155.32 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:144.9,150.3 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/driver.go:152.16,154.3 1 0 +github.com/golang-migrate/migrate/v4/source/httpfs/faileddriver.go:15.64,17.2 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/faileddriver.go:19.38,21.2 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/faileddriver.go:23.58,25.2 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/faileddriver.go:27.73,29.2 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/faileddriver.go:31.73,33.2 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/faileddriver.go:35.93,37.2 1 1 +github.com/golang-migrate/migrate/v4/source/httpfs/faileddriver.go:39.95,41.2 1 1 diff --git a/source/httpfs/driver.go b/source/httpfs/driver.go index 66b10568b..d00244905 100644 --- a/source/httpfs/driver.go +++ b/source/httpfs/driver.go @@ -19,17 +19,37 @@ type driver struct { path string } +// New creates a new migrate source driver from a http.FileSystem instance and a +// relative path to migration files within the virtual FS. It is identical to +// the WithInstance() function except it will delay any errors on fist usage of +// this driver. This reduces the number of error handling branches in clients of +// this package without compromising on correctness. +func New(fs http.FileSystem, path string) source.Driver { + d, err := newDriver(fs, path) + if err != nil { + return &failedDriver{err} + } + return d +} + // WithInstance creates new migrate source driver from a http.FileSystem // instance and a relative path to migration files within the virtual FS. func WithInstance(fs http.FileSystem, path string) (source.Driver, error) { + return newDriver(fs, path) +} + +func newDriver(fs http.FileSystem, path string) (*driver, error) { root, err := fs.Open(path) if err != nil { return nil, err } - defer root.Close() files, err := root.Readdir(0) if err != nil { + _ = root.Close() + return nil, err + } + if err = root.Close(); err != nil { return nil, err } diff --git a/source/httpfs/driver_test.go b/source/httpfs/driver_test.go index f31a27995..e32c589d5 100644 --- a/source/httpfs/driver_test.go +++ b/source/httpfs/driver_test.go @@ -7,22 +7,85 @@ import ( st "github.com/golang-migrate/migrate/v4/source/testing" ) -func TestWithInstance(t *testing.T) { - t.Run("empty path", func(t *testing.T) { - d, err := WithInstance(http.Dir("testdata/sql"), "") - if err != nil { - t.Fatal(err) +func TestWithInstanceAndNew(t *testing.T) { + tests := []struct { + msg string + fs http.FileSystem + path string + ok bool + }{ + { + msg: "valid dir, empty path", + fs: http.Dir("testdata/sql"), + ok: true, + }, + { + msg: "valid dir, non-empty path", + fs: http.Dir("testdata"), + path: "sql", + ok: true, + }, + { + msg: "invalid dir", + fs: http.Dir("does-not-exist"), + }, + { + msg: "file instead of dir", + fs: http.Dir("testdata/sql/1_foobar.up.sql"), + }, + { + msg: "dir with duplicates", + fs: http.Dir("testdata/duplicates"), + }, + } + + for _, test := range tests { + d, err := WithInstance(test.fs, test.path) + if test.ok { + if err != nil { + t.Errorf("%s, WithInstance() returned error %s", test.msg, err) + } + st.Test(t, d) + if err = d.Close(); err != nil { + t.Errorf("%s, WithInstance().Close() returned error %s", test.msg, err) + } + } + if !test.ok { + if err == nil { + t.Errorf("%s, WithInstance() expected error but did not get one", test.msg) + } } - st.Test(t, d) - }) + } - t.Run("with path", func(t *testing.T) { - d, err := WithInstance(http.Dir("testdata"), "sql") - if err != nil { - t.Fatal(err) + for _, test := range tests { + d := New(test.fs, test.path) + if test.ok { + st.Test(t, d) + } + if !test.ok { + if _, err := d.Open(""); err == nil { + t.Errorf("%s, New().Open() expected error but did not get one", test.msg) + } + if err := d.Close(); err == nil { + t.Errorf("%s, New().Close() expected error but did not get one", test.msg) + } + if _, err := d.First(); err == nil { + t.Errorf("%s, New().First() expected error but did not get one", test.msg) + } + if _, err := d.Prev(0); err == nil { + t.Errorf("%s, New().Prev() expected error but did not get one", test.msg) + } + if _, err := d.Next(0); err == nil { + t.Errorf("%s, New().Next() expected error but did not get one", test.msg) + } + if _, _, err := d.ReadUp(0); err == nil { + t.Errorf("%s, New().ReadUp() expected error but did not get one", test.msg) + } + if _, _, err := d.ReadDown(0); err == nil { + t.Errorf("%s, New().ReadDown() expected error but did not get one", test.msg) + } } - st.Test(t, d) - }) + } } diff --git a/source/httpfs/faileddriver.go b/source/httpfs/faileddriver.go new file mode 100644 index 000000000..1e9362002 --- /dev/null +++ b/source/httpfs/faileddriver.go @@ -0,0 +1,41 @@ +package httpfs + +import ( + "io" + + "github.com/golang-migrate/migrate/v4/source" +) + +// failedDriver is a dummy implementation of source.Driver interface that +// always returns underlying error. +type failedDriver struct { + err error +} + +func (d *failedDriver) Open(url string) (source.Driver, error) { + return nil, d.err +} + +func (d *failedDriver) Close() error { + return d.err +} + +func (d *failedDriver) First() (version uint, err error) { + return 0, d.err +} + +func (d *failedDriver) Prev(version uint) (prevVersion uint, err error) { + return 0, d.err +} + +func (d *failedDriver) Next(version uint) (nextVersion uint, err error) { + return 0, d.err +} + +func (d *failedDriver) ReadUp(version uint) (r io.ReadCloser, identifier string, err error) { + return nil, "", d.err +} + +func (d *failedDriver) ReadDown(version uint) (r io.ReadCloser, identifier string, err error) { + return nil, "", d.err +} diff --git a/source/httpfs/testdata/duplicates/1_foobar.up.sql b/source/httpfs/testdata/duplicates/1_foobar.up.sql new file mode 100644 index 000000000..046fd5a5d --- /dev/null +++ b/source/httpfs/testdata/duplicates/1_foobar.up.sql @@ -0,0 +1 @@ +1 up diff --git a/source/httpfs/testdata/duplicates/1_foobaz.up.sql b/source/httpfs/testdata/duplicates/1_foobaz.up.sql new file mode 100644 index 000000000..046fd5a5d --- /dev/null +++ b/source/httpfs/testdata/duplicates/1_foobaz.up.sql @@ -0,0 +1 @@ +1 up diff --git a/source/httpfs/testdata/sql/other-files-are-ignored b/source/httpfs/testdata/sql/other-files-are-ignored new file mode 100644 index 000000000..e69de29bb