If a filter file is not found and is in config, throw an error (#788)

* When the filter file is missing throw a fatal

* Ignore path when file is not specified

* Fix build failure on ci

* Remove logs

* Fix propagate and handle errors

* Fix build error

* Actually check filter file

* Check for errors in the test suite as well

* Improve test coverage

* Add more tests

* Simplify NewFilter function

* Address review comments

pkg/module/filter.go

@@ -29,16 +29,15 @@ type Filter struct {
 //   -
 //   + github.com/a
 // will exclude all items from communication except github.com/a
-func NewFilter(filterFilePath string) *Filter {
-	rn := newRule(Default)
-	modFilter := Filter{
-		filePath: filterFilePath,
+func NewFilter(filterFilePath string) (*Filter, error) {
+	// Do not return an error if the file path is empty
+	// Do not attempt to parse it as well.
+	if filterFilePath == "" {
+		return nil, nil
 	}
-	modFilter.root = rn
 
-	modFilter.initFromConfig()
+	return initFromConfig(filterFilePath)
 
-	return &modFilter
 }
 
 // AddRule adds rule for specified path
@@ -116,13 +115,18 @@ func (f *Filter) getAssociatedRule(path ...string) FilterRule {
 	return f.root.rule
 }
 
-func (f *Filter) initFromConfig() {
-	lines, err := getConfigLines(f.filePath)
-
-	if err != nil || len(lines) == 0 {
-		return
+func initFromConfig(filePath string) (*Filter, error) {
+	lines, err := getConfigLines(filePath)
+	if err != nil {
+		return nil, err
 	}
 
+	rn := newRule(Default)
+	f := &Filter{
+		filePath: filePath,
+	}
+	f.root = rn
+
 	for _, line := range lines {
 		split := strings.Split(strings.TrimSpace(line), " ")
 		if len(split) > 2 {
@@ -151,6 +155,7 @@ func (f *Filter) initFromConfig() {
 		path := strings.TrimSpace(split[1])
 		f.AddRule(path, rule)
 	}
+	return f, nil
 }
 
 func getPathSegments(path string) []string {
@@ -179,7 +184,6 @@ func getConfigLines(filterFile string) ([]string, error) {
 	if err != nil {
 		return nil, errors.E(op, err)
 	}
-	defer f.Close()
 
 	scanner := bufio.NewScanner(f)
 
@@ -193,6 +197,5 @@ func getConfigLines(filterFile string) ([]string, error) {
 
 		lines = append(lines, line)
 	}
-
-	return lines, nil
+	return lines, f.Close()
 }

pkg/module/filter_test.go

@@ -1,10 +1,11 @@
 package module
 
 import (
+	"io/ioutil"
+	"os"
 	"path/filepath"
 	"testing"
 
-	"github.com/gomods/athens/pkg/config"
 	"github.com/stretchr/testify/suite"
 )
 
@@ -20,10 +21,33 @@ func Test_Filter(t *testing.T) {
 	suite.Run(t, new(FilterTests))
 }
 
+func (t *FilterTests) Test_NewFilter() {
+	r := t.Require()
+	mf, err := NewFilter("")
+	r.NoError(err, "When a file name is empty string return no error")
+	r.Nil(nil, mf)
+
+	mf, err = NewFilter("nofile")
+	r.Nil(mf)
+	r.Error(err)
+
+	filter := tempFilterFile(t.T())
+	defer os.Remove(filter)
+
+	mf, err = NewFilter(filter)
+	r.Equal(filter, mf.filePath)
+	r.NoError(err)
+
+}
+
 func (t *FilterTests) Test_IgnoreSimple() {
 	r := t.Require()
-	conf := GetConfLogErr(testConfigFile, t.T())
-	f := NewFilter(conf.FilterFile)
+
+	filter := tempFilterFile(t.T())
+	defer os.Remove(filter)
+
+	f, err := NewFilter(filter)
+	r.NoError(err)
 	f.AddRule("github.com/a/b", Exclude)
 
 	r.Equal(Include, f.Rule("github.com/a"))
@@ -35,8 +59,12 @@ func (t *FilterTests) Test_IgnoreSimple() {
 
 func (t *FilterTests) Test_IgnoreParentAllowChildren() {
 	r := t.Require()
-	conf := GetConfLogErr(testConfigFile, t.T())
-	f := NewFilter(conf.FilterFile)
+
+	filter := tempFilterFile(t.T())
+	defer os.Remove(filter)
+
+	f, err := NewFilter(filter)
+	r.NoError(err)
 	f.AddRule("github.com/a/b", Exclude)
 	f.AddRule("github.com/a/b/c", Include)
 
@@ -50,8 +78,11 @@ func (t *FilterTests) Test_IgnoreParentAllowChildren() {
 func (t *FilterTests) Test_OnlyAllowed() {
 	r := t.Require()
 
-	conf := GetConfLogErr(testConfigFile, t.T())
-	f := NewFilter(conf.FilterFile)
+	filter := tempFilterFile(t.T())
+	defer os.Remove(filter)
+
+	f, err := NewFilter(filter)
+	r.NoError(err)
 	f.AddRule("github.com/a/b", Include)
 	f.AddRule("", Exclude)
 
@@ -65,8 +96,11 @@ func (t *FilterTests) Test_OnlyAllowed() {
 func (t *FilterTests) Test_Direct() {
 	r := t.Require()
 
-	conf := GetConfLogErr(testConfigFile, t.T())
-	f := NewFilter(conf.FilterFile)
+	filter := tempFilterFile(t.T())
+	defer os.Remove(filter)
+
+	f, err := NewFilter(filter)
+	r.NoError(err)
 	f.AddRule("github.com/a/b/c", Exclude)
 	f.AddRule("github.com/a/b", Direct)
 	f.AddRule("github.com/a", Include)
@@ -77,13 +111,10 @@ func (t *FilterTests) Test_Direct() {
 	r.Equal(Exclude, f.Rule("github.com/a/b/c/d"))
 }
 
-// GetConfLogErr is similar to GetConf, except it logs a failure for the calling test
-// if any errors are encountered
-func GetConfLogErr(path string, t *testing.T) *config.Config {
-	c, err := config.GetConf(path)
+func tempFilterFile(t *testing.T) (path string) {
+	filter, err := ioutil.TempFile(os.TempDir(), "filter-")
 	if err != nil {
-		t.Fatalf("Unable to parse config file: %s", err.Error())
-		return nil
+		t.FailNow()
 	}
-	return c
+	return filter.Name()
 }