Merge pull request #78 from kaleido-io/more-ut

Improved input validation; more unit tests

Makefile

@@ -1,15 +1,18 @@
 VGO=go
 BINARY_NAME=fabconnect
 GOFILES := $(shell find cmd internal -name '*.go' -print)
-TESTED_INTERNALS := $(shell go list ./internal/... | grep -v rest/test)
+TESTED_INTERNALS := $(shell go list ./internal/... | grep -v test | grep -v kafka)
+TESTED_CMD := $(shell go list ./cmd/...)
+COVERPKG_INTERNALS = $(shell go list ./internal/... | grep -v test | grep -v kafka | tr "\n" ",")
+COVERPKG_CMD = $(shell go list ./cmd/... | tr "\n" ",")
 # Expect that FireFly compiles with CGO disabled
 CGO_ENABLED=0
 GOGC=30
 .DELETE_ON_ERROR:
 
 all: build test go-mod-tidy
 test: deps lint
-	$(VGO) test $(TESTED_INTERNALS) ./cmd/... -cover -coverprofile=coverage.txt -covermode=atomic -timeout=10s
+	$(VGO) test $(TESTED_INTERNALS) $(TESTED_CMD) -cover -coverpkg $(COVERPKG_INTERNALS)$(COVERPKG_CMD) -coverprofile=coverage.txt -covermode=atomic -timeout=10s
 coverage.html:
 	$(VGO) tool cover -html=coverage.txt
 coverage: test coverage.html
@@ -34,9 +37,11 @@ mockery: .ALWAYS
 	go get github.com/vektra/mockery/cmd/mockery
 mocks: mockery ${GOFILES}
 	$(eval MOCKERY := $(shell go list -f '{{.Target}}' github.com/vektra/mockery/cmd/mockery))
+	${MOCKERY} --case underscore --dir internal/events --name SubscriptionManager --output mocks/events --outpkg mockevents
 	${MOCKERY} --case underscore --dir internal/fabric/client --name RPCClient --output mocks/fabric/client --outpkg mockfabric
 	${MOCKERY} --case underscore --dir internal/fabric/client --name IdentityClient --output mocks/fabric/client --outpkg mockfabric
 	${MOCKERY} --case underscore --dir internal/kvstore --name KVStore --output mocks/kvstore --outpkg mockkvstore
+	${MOCKERY} --case underscore --dir internal/kvstore --name KVIterator --output mocks/kvstore --outpkg mockkvstore
 	${MOCKERY} --case underscore --dir internal/rest/async --name AsyncDispatcher --output mocks/rest/async --outpkg mockasync
 	${MOCKERY} --case underscore --dir internal/rest/receipt --name ReceiptStore --output mocks/rest/receipt --outpkg mockreceipt
 	${MOCKERY} --case underscore --dir internal/rest/receipt/api --name ReceiptStorePersistence --output mocks/rest/receipt/api --outpkg mockreceiptapi

cmd/fabconnect.go

@@ -63,45 +63,61 @@ type cmdConfig struct {
 	Filename   string
 }
 
-var rootConfig = cmdConfig{}
-var restGatewayConf = conf.RESTGatewayConf{}
-var restGateway *rest.RESTGateway
-
-var rootCmd = &cobra.Command{
-	Use:   "fabconnect",
-	Short: "Connectivity Bridge for Hyperledger Fabric permissioned chains",
-	PreRunE: func(cmd *cobra.Command, args []string) error {
-		err := viper.Unmarshal(&restGatewayConf)
-		if err != nil {
-			return err
-		}
-
-		// allow tests to assign a mock
-		if restGateway == nil {
-			restGateway = rest.NewRESTGateway(&restGatewayConf)
-		}
-		err = restGateway.ValidateConf()
-		if err != nil {
-			return err
-		}
-
-		initLogging(rootConfig.DebugLevel)
+var rootConfig = &cmdConfig{}
+
+func newRootCmd() (*cobra.Command, *conf.RESTGatewayConf) {
+	restGatewayConf := &conf.RESTGatewayConf{}
+	var restGateway *rest.RESTGateway
+
+	rootCmd := &cobra.Command{
+		Use:   "fabconnect",
+		Short: "Connectivity Bridge for Hyperledger Fabric permissioned chains",
+		PreRunE: func(cmd *cobra.Command, args []string) error {
+			err := viper.Unmarshal(&restGatewayConf)
+			if err != nil {
+				return err
+			}
+
+			// allow tests to assign a mock
+			if restGateway == nil {
+				restGateway = rest.NewRESTGateway(restGatewayConf)
+			}
+			err = restGateway.ValidateConf()
+			if err != nil {
+				return err
+			}
+
+			err = restGateway.Init()
+			if err != nil {
+				return err
+			}
+
+			initLogging(rootConfig.DebugLevel)
+
+			if rootConfig.DebugPort > 0 {
+				go func() {
+					log.Debugf("Debug HTTP endpoint listening on localhost:%d: %s", rootConfig.DebugPort, http.ListenAndServe(fmt.Sprintf("localhost:%d", rootConfig.DebugPort), nil))
+				}()
+			}
+
+			return nil
+		},
+		RunE: func(cmd *cobra.Command, args []string) error {
+			err := startServer(restGatewayConf, restGateway)
+			if err != nil {
+				return err
+			}
+			return nil
+		},
+	}
 
-		if rootConfig.DebugPort > 0 {
-			go func() {
-				log.Debugf("Debug HTTP endpoint listening on localhost:%d: %s", rootConfig.DebugPort, http.ListenAndServe(fmt.Sprintf("localhost:%d", rootConfig.DebugPort), nil))
-			}()
-		}
+	rootCmd.Flags().IntVarP(&rootConfig.DebugLevel, "debug", "d", 1, "0=error, 1=info, 2=debug")
+	rootCmd.Flags().IntVarP(&rootConfig.DebugPort, "debugPort", "Z", 6060, "Port for pprof HTTP endpoints (localhost only)")
+	rootCmd.Flags().BoolVarP(&rootConfig.PrintYAML, "print-yaml-confg", "Y", false, "Print YAML config snippet and exit")
+	rootCmd.Flags().StringVarP(&rootConfig.Filename, "configfile", "f", "", "Configuration file, must be one of .yml, .yaml, or .json")
+	conf.CobraInit(rootCmd, restGatewayConf)
 
-		return nil
-	},
-	RunE: func(cmd *cobra.Command, args []string) error {
-		err := startServer()
-		if err != nil {
-			return err
-		}
-		return nil
-	},
+	return rootCmd, restGatewayConf
 }
 
 func init() {
@@ -111,12 +127,6 @@ func init() {
 	viper.SetEnvPrefix("FC")
 	viper.AutomaticEnv()
 	viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
-
-	rootCmd.Flags().IntVarP(&rootConfig.DebugLevel, "debug", "d", 1, "0=error, 1=info, 2=debug")
-	rootCmd.Flags().IntVarP(&rootConfig.DebugPort, "debugPort", "Z", 6060, "Port for pprof HTTP endpoints (localhost only)")
-	rootCmd.Flags().BoolVarP(&rootConfig.PrintYAML, "print-yaml-confg", "Y", false, "Print YAML config snippet and exit")
-	rootCmd.Flags().StringVarP(&rootConfig.Filename, "configfile", "f", "", "Configuration file, must be one of .yml, .yaml, or .json")
-	conf.CobraInit(rootCmd, &restGatewayConf)
 }
 
 func initConfig() {
@@ -132,7 +142,7 @@ func initConfig() {
 	}
 }
 
-func startServer() error {
+func startServer(restGatewayConf *conf.RESTGatewayConf, restGateway *rest.RESTGateway) error {
 
 	if rootConfig.PrintYAML {
 		a, err := marshalToYAML(rootConfig)
@@ -146,10 +156,6 @@ func startServer() error {
 		print(fmt.Sprintf("# Full YAML configuration processed from supplied file\n%s\n%s\n", string(a), string(b)))
 	}
 
-	err := restGateway.Init()
-	if err != nil {
-		return err
-	}
 	serverDone := make(chan bool)
 	go func(done chan bool) {
 		log.Info("Starting REST gateway")
@@ -181,6 +187,7 @@ func marshalToYAML(conf interface{}) (yamlBytes []byte, err error) {
 
 // Execute is called by the main method of the package
 func Execute() int {
+	rootCmd, _ := newRootCmd()
 	if err := rootCmd.Execute(); err != nil {
 		fmt.Println(err)
 		return 1

cmd/fabconnect_test.go

@@ -21,16 +21,17 @@ import (
 	"regexp"
 	"testing"
 
-	"github.com/hyperledger/firefly-fabconnect/internal/conf"
 	"github.com/hyperledger/firefly-fabconnect/internal/rest/test"
 	"github.com/spf13/cobra"
-	"github.com/spf13/viper"
 
 	"github.com/stretchr/testify/assert"
 )
 
 var tmpdir string
-var testConfig *conf.RESTGatewayConf
+
+func runNothing(cmd *cobra.Command, args []string) error {
+	return nil
+}
 
 func TestMain(m *testing.M) {
 	setup()
@@ -40,7 +41,6 @@ func TestMain(m *testing.M) {
 }
 
 func setup() {
-	tmpdir, testConfig = test.Setup()
 	os.Setenv("FC_HTTP_PORT", "8002")
 	os.Setenv("FC_MAXINFLIGHT", "60")
 }
@@ -49,14 +49,10 @@ func teardown() {
 	test.Teardown(tmpdir)
 }
 
-func runNothing(cmd *cobra.Command, args []string) error {
-	return nil
-}
-
 func TestMissingConfigFile(t *testing.T) {
 	assert := assert.New(t)
 
-	restGateway = nil
+	rootCmd, _ := newRootCmd()
 	args := []string{}
 	rootCmd.SetArgs(args)
 	err := rootCmd.Execute()
@@ -66,34 +62,38 @@ func TestMissingConfigFile(t *testing.T) {
 func TestBadConfigFile(t *testing.T) {
 	assert := assert.New(t)
 
-	restGateway = nil
+	tmpdir, _ := test.Setup()
+	rootCmd, _ := newRootCmd()
 	args := []string{
 		"-f", path.Join(tmpdir, "config-bad.json"),
 	}
 	rootCmd.SetArgs(args)
 	err := rootCmd.Execute()
 	assert.Regexp(regexp.MustCompile(`User credentials store creation failed`), err)
+	test.Teardown(tmpdir)
 }
 
 func TestStartServerError(t *testing.T) {
 	assert := assert.New(t)
 
-	restGateway = nil
+	tmpdir, _ := test.Setup()
+	rootCmd, _ := newRootCmd()
 	args := []string{
 		"-Y",
 		"-f", path.Join(tmpdir, "config.json"),
 		"-r", "/bad-path",
 	}
 	rootCmd.SetArgs(args)
 	err := rootCmd.Execute()
-	assert.Regexp(regexp.MustCompile(`User credentials store creation failed. Path: User credentials store path is empty`), err)
+	assert.Regexp(regexp.MustCompile(`User credentials store creation failed. User credentials store path is empty`), err)
+	test.Teardown(tmpdir)
 }
 
 func TestMaxWaitTimeTooSmallWarns(t *testing.T) {
 	assert := assert.New(t)
 
-	restGateway = nil
-	restGatewayConf.MaxTXWaitTime = 0
+	tmpdir, _ := test.Setup()
+	rootCmd, restGatewayConf := newRootCmd()
 	rootCmd.RunE = runNothing
 	args := []string{
 		"-f", path.Join(tmpdir, "config.json"),
@@ -109,12 +109,13 @@ func TestMaxWaitTimeTooSmallWarns(t *testing.T) {
 	assert.Equal(8002, restGatewayConf.HTTP.Port)
 	// test that settings in the config file that are not overriden
 	assert.Equal("192.168.0.100", restGatewayConf.HTTP.LocalAddr)
+	test.Teardown(tmpdir)
 }
 
 func TestEnvVarOverride(t *testing.T) {
 	assert := assert.New(t)
 
-	restGateway = nil
+	rootCmd, restGatewayConf := newRootCmd()
 	rootCmd.RunE = runNothing
 	_ = rootCmd.Execute()
 	assert.Equal(8002, restGatewayConf.HTTP.Port)
@@ -124,8 +125,7 @@ func TestEnvVarOverride(t *testing.T) {
 func TestCmdArgsOverride(t *testing.T) {
 	assert := assert.New(t)
 
-	restGateway = nil
-	restGatewayConf.Events.PollingIntervalSec = 0
+	rootCmd, restGatewayConf := newRootCmd()
 	rootCmd.RunE = runNothing
 	args := []string{
 		"-P", "8001",
@@ -140,23 +140,25 @@ func TestCmdArgsOverride(t *testing.T) {
 func TestDefaultsInConfigFile(t *testing.T) {
 	assert := assert.New(t)
 
-	restGateway = nil
-	restGatewayConf.HTTP.Port = 0
+	tmpdir, _ := test.Setup()
+	rootCmd, restGatewayConf := newRootCmd()
 	rootCmd.RunE = runNothing
-	viper.Reset()
 	args := []string{
 		"-f", path.Join(tmpdir, "config.json"),
 	}
 	rootCmd.SetArgs(args)
+	os.Unsetenv("FC_HTTP_PORT")
 	err := rootCmd.Execute()
 	assert.NoError(err)
 	assert.Equal(3000, restGatewayConf.HTTP.Port)
+	test.Teardown(tmpdir)
 }
 
 func TestMissingKafkaTopic(t *testing.T) {
 	assert := assert.New(t)
 
-	restGateway = nil
+	tmpdir, _ := test.Setup()
+	rootCmd, _ := newRootCmd()
 	rootCmd.RunE = runNothing
 	args := []string{
 		"-P", "8001",
@@ -167,27 +169,30 @@ func TestMissingKafkaTopic(t *testing.T) {
 	rootCmd.SetArgs(args)
 	err := rootCmd.Execute()
 	assert.EqualError(err, "No output topic specified for bridge to send events to")
+	test.Teardown(tmpdir)
 }
 
 func TestCmdLaunch(t *testing.T) {
 	assert := assert.New(t)
 
-	restGateway = nil
-	restGatewayConf.Kafka.Brokers = []string{}
-	rootCmd.RunE = runNothing
+	tmpdir, _ := test.Setup()
+	rootCmd, _ := newRootCmd()
 	args := []string{
 		"-P", "8001",
 		"-f", path.Join(tmpdir, "config.json"),
+		"-Y",
 	}
 	rootCmd.SetArgs(args)
 	err := rootCmd.Execute()
 	assert.Nil(err)
+	test.Teardown(tmpdir)
 }
 
 func TestKafkaSuccess(t *testing.T) {
 	assert := assert.New(t)
 
-	restGateway = nil
+	tmpdir, _ := test.Setup()
+	rootCmd, restGatewayConf := newRootCmd()
 	rootCmd.RunE = runNothing
 	args := []string{
 		"-P", "8001",
@@ -200,4 +205,5 @@ func TestKafkaSuccess(t *testing.T) {
 	err := rootCmd.Execute()
 	assert.Nil(err)
 	assert.Equal([]string{"broker1", "broker2"}, restGatewayConf.Kafka.Brokers)
+	test.Teardown(tmpdir)
 }

internal/errors/errors.go

@@ -170,8 +170,6 @@ const (
 
 	// EventStreamsDBLoad failed to init DB
 	EventStreamsDBLoad = "Failed to open DB at %s: %s"
-	// EventStreamsNoID attempt to create an event stream/sub without an ID
-	EventStreamsNoID = "No ID"
 	// EventStreamsInvalidActionType unknown action type
 	EventStreamsInvalidActionType = "Unknown action type '%s'"
 	// EventStreamsWebhookNoURL attempt to create a Webhook event stream without a URL

internal/events/api/event.go

@@ -21,8 +21,8 @@ const (
 	BlockType_Config                 = "config"          // corresponds to blocks containing channel configurations and updates
 	EventPayloadType_Bytes           = "bytes"           // default data type of the event payload, no special processing is done before returning to the subscribing client
 	EventPayloadType_String          = "string"          // event payload will be an UTF-8 encoded string
-	EventPayloadType_StringifiedJSON = "stringifiedJSON" // event payload will be a structured map with UTF-8 encoded string values
-	EventPayloadType_JSON            = "json"            // equivalent to "stringifiedJSON"
+	EventPayloadType_JSON            = "json"            // event payload will be a structured map with UTF-8 encoded string values
+	EventPayloadType_StringifiedJSON = "stringifiedJSON" // equivalent to "json" (deprecated)
 )
 
 // persistedFilter is the part of the filter we record to storage
@@ -37,8 +37,6 @@ type persistedFilter struct {
 	BlockType   string `json:"blockType,omitempty"`
 	ChaincodeId string `json:"chaincodeId,omitempty"`
 	EventFilter string `json:"eventFilter,omitempty"`
-	Filter      string `json:"filter,omitempty"`
-	ToBlock     uint64 `json:"toBlock,omitempty"`
 }
 
 // SubscriptionInfo is the persisted data for the subscription