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

@@ -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()
+```
+

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

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
 	}
 

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

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
+}

source/httpfs/testdata/duplicates/1_foobar.up.sql

@@ -0,0 +1 @@
+1 up

source/httpfs/testdata/duplicates/1_foobaz.up.sql

@@ -0,0 +1 @@
+1 up