Repository: beatlabs/patron
Pull request #115 (2019-12-06 17:04:39)

HTTP Component Builder
## Which problem is this PR solving?

This PR addresses  #53 [HTTP Component builder pattern](https://github.com/beatlabs/patron/issues/53)

It introduces the builder pattern for sync/http components.


## Short description of the changes

* Introduce a builder 
* Introduce table-driven tests for testing this new functionality
* Use string templates for logging and error reporting
* Builder uses existing defaults for specific properties
* 100% test coverage on new code
* Ensure logic compatibility with other uses of the builder pattern throughout the project, [here](https://github.com/beatlabs/patron/pull/63) and [here](https://github.com/beatlabs/patron/pull/110).


tpaschalis: Things that I'm not 100% clear on, and might require a quick chat.

1) Is the current testing scope wide enough? Or should I include more test cases, where the component is actually being used, and acts like it should?
2) Should the builder have some logic, where it aborts under some conditions? Or is this part of the application logic later on?
3) On the other hand, should I include some default values eg. a default `httpReadTimeout` in case that its builder function has not been used, or is used without arguments. (Personally, not really a fan).
mantzas: > Things that I'm not 100% clear on, and might require a quick chat.
> 
>     1. Is the current testing scope wide enough? Or should I include more test cases, where the component is actually being used, and acts like it should?
> 
>     2. Should the builder have some logic, where it aborts under some conditions? Or is this part of the application logic later on?
> 
>     3. On the other hand, should I include some default values eg. a default `httpReadTimeout` in case that its builder function has not been used, or is used without arguments. (Personally, not really a fan).

1. Testing should be as close to 100% coverage in unit tests
2. All the logic should live in the component that the builder creates.
3. We should use the existing defaults but provider overrrides with the builder.

Hope this helps...

Patch:
diff --git a/service.go b/service.go
index 971eeaf71..28754e6ed 100644
--- a/service.go
+++ b/service.go
@@ -187,27 +187,25 @@ func (s *Service) createHTTPComponent() (Component, error) {
 	port = strconv.FormatInt(portVal, 10)
 	log.Infof("creating default HTTP component at port %s", port)
 
-	options := []http.OptionFunc{
-		http.Port(int(portVal)),
-	}
+	b := http.NewBuilder().WithPort(int(portVal))
 
 	if s.acf != nil {
-		options = append(options, http.AliveCheck(s.acf))
+		b.WithAliveCheckFunc(s.acf)
 	}
 
 	if s.rcf != nil {
-		options = append(options, http.ReadyCheck(s.rcf))
+		b.WithReadyCheckFunc(s.rcf)
 	}
 
 	if s.routes != nil {
-		options = append(options, http.Routes(s.routes))
+		b.WithRoutes(s.routes)
 	}
 
 	if s.middlewares != nil && len(s.middlewares) > 0 {
-		options = append(options, http.Middlewares(s.middlewares...))
+		b.WithMiddlewares(s.middlewares...)
 	}
 
-	cp, err := http.New(options...)
+	cp, err := b.Create()
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to create default HTTP component")
 	}
diff --git a/sync/http/component.go b/sync/http/component.go
index c28fe81f2..e6212e07c 100644
--- a/sync/http/component.go
+++ b/sync/http/component.go
@@ -7,6 +7,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/beatlabs/patron/errors"
 	"github.com/beatlabs/patron/log"
 	"github.com/julienschmidt/httprouter"
 )
@@ -32,7 +33,6 @@ type Component struct {
 	httpPort         int
 	httpReadTimeout  time.Duration
 	httpWriteTimeout time.Duration
-	info             map[string]interface{}
 	sync.Mutex
 	routes      []Route
 	middlewares []MiddlewareFunc
@@ -40,34 +40,6 @@ type Component struct {
 	keyFile     string
 }
 
-// New returns a new component.
-func New(oo ...OptionFunc) (*Component, error) {
-	c := Component{
-		ac:               DefaultAliveCheck,
-		rc:               DefaultReadyCheck,
-		httpPort:         httpPort,
-		httpReadTimeout:  httpReadTimeout,
-		httpWriteTimeout: httpWriteTimeout,
-		routes:           []Route{},
-		middlewares:      []MiddlewareFunc{},
-		info:             make(map[string]interface{}),
-	}
-
-	for _, o := range oo {
-		err := o(&c)
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	c.routes = append(c.routes, aliveCheckRoute(c.ac))
-	c.routes = append(c.routes, readyCheckRoute(c.rc))
-	c.routes = append(c.routes, profilingRoutes()...)
-	c.routes = append(c.routes, metricRoute())
-
-	return &c, nil
-}
-
 // Run starts the HTTP server.
 func (c *Component) Run(ctx context.Context) error {
 	c.Lock()
@@ -121,3 +93,158 @@ func (c *Component) createHTTPServer() *http.Server {
 		Handler:      routerAfterMiddleware,
 	}
 }
+
+const fieldSetMsg = "Setting property '%v' for '%v'"
+
+// Builder gathers all required and optional properties, in order
+// to construct an HTTP component.
+type Builder struct {
+	ac               AliveCheckFunc
+	rc               ReadyCheckFunc
+	httpPort         int
+	httpReadTimeout  time.Duration
+	httpWriteTimeout time.Duration
+	routes           []Route
+	middlewares      []MiddlewareFunc
+	certFile         string
+	keyFile          string
+	errors           []error
+}
+
+// NewBuilder initiates the HTTP component builder chain.
+// The builder instantiates the component using default values for
+// HTTP Port, Alive/Ready check functions and Read/Write timeouts.
+func NewBuilder() *Builder {
+	var errs []error
+	return &Builder{
+		ac:               DefaultAliveCheck,
+		rc:               DefaultReadyCheck,
+		httpPort:         httpPort,
+		httpReadTimeout:  httpReadTimeout,
+		httpWriteTimeout: httpWriteTimeout,
+		errors:           errs,
+	}
+}
+
+// WithSSL sets the filenames for the Certificate and Keyfile, in order to enable SSL.
+func (cb *Builder) WithSSL(c, k string) *Builder {
+	if c == "" || k == "" {
+		cb.errors = append(cb.errors, errors.New("Invalid cert or key provided"))
+	} else {
+		log.Info(fieldSetMsg, "Cert, Key", c+","+k)
+		cb.certFile = c
+		cb.keyFile = k
+	}
+
+	return cb
+}
+
+// WithRoutes adds routes to the HTTP component.
+func (cb *Builder) WithRoutes(rr []Route) *Builder {
+	if len(rr) == 0 {
+		cb.errors = append(cb.errors, errors.New("Empty Routes slice provided"))
+	} else {
+		log.Info(fieldSetMsg, "Routes", rr)
+		cb.routes = append(cb.routes, rr...)
+	}
+
+	return cb
+}
+
+// WithMiddlewares adds middlewares to the HTTP component.
+func (cb *Builder) WithMiddlewares(mm ...MiddlewareFunc) *Builder {
+	if len(mm) == 0 {
+		cb.errors = append(cb.errors, errors.New("Empty list of middlewares provided"))
+	} else {
+		log.Info(fieldSetMsg, "Middlewares", mm)
+		cb.middlewares = append(cb.middlewares, mm...)
+	}
+
+	return cb
+}
+
+// WithReadTimeout sets the Read Timeout for the HTTP component.
+func (cb *Builder) WithReadTimeout(rt time.Duration) *Builder {
+	if rt <= 0*time.Second {
+		cb.errors = append(cb.errors, errors.New("Negative or zero read timeout provided"))
+	} else {
+		log.Infof(fieldSetMsg, "Read Timeout", rt)
+		cb.httpReadTimeout = rt
+	}
+
+	return cb
+}
+
+// WithWriteTimeout sets the Write Timeout for the HTTP component.
+func (cb *Builder) WithWriteTimeout(wt time.Duration) *Builder {
+	if wt <= 0*time.Second {
+		cb.errors = append(cb.errors, errors.New("Negative or zero write timeout provided"))
+	} else {
+		log.Infof(fieldSetMsg, "Write Timeout", wt)
+		cb.httpWriteTimeout = wt
+	}
+
+	return cb
+}
+
+// WithPort sets the port used by the HTTP component.
+func (cb *Builder) WithPort(p int) *Builder {
+	if p <= 0 || p > 65535 {
+		cb.errors = append(cb.errors, errors.New("Invalid HTTP Port provided"))
+	} else {
+		log.Infof(fieldSetMsg, "Port", p)
+		cb.httpPort = p
+	}
+
+	return cb
+}
+
+// WithAliveCheckFunc sets the AliveCheckFunc used by the HTTP component.
+func (cb *Builder) WithAliveCheckFunc(acf AliveCheckFunc) *Builder {
+	if acf == nil {
+		cb.errors = append(cb.errors, errors.New("Nil AliveCheckFunc was provided"))
+	} else {
+		log.Infof(fieldSetMsg, "AliveCheckFunc", acf)
+		cb.ac = acf
+	}
+
+	return cb
+}
+
+// WithReadyCheckFunc sets the ReadyCheckFunc used by the HTTP component.
+func (cb *Builder) WithReadyCheckFunc(rcf ReadyCheckFunc) *Builder {
+	if rcf == nil {
+		cb.errors = append(cb.errors, errors.New("Nil ReadyCheckFunc provided"))
+	} else {
+		log.Infof(fieldSetMsg, "ReadyCheckFunc", rcf)
+		cb.rc = rcf
+	}
+
+	return cb
+}
+
+// Create constructs the HTTP component by applying the gathered properties.
+func (cb *Builder) Create() (*Component, error) {
+	if len(cb.errors) > 0 {
+		return nil, errors.Aggregate(cb.errors...)
+	}
+
+	c := &Component{
+		ac:               cb.ac,
+		rc:               cb.rc,
+		httpPort:         cb.httpPort,
+		httpReadTimeout:  cb.httpReadTimeout,
+		httpWriteTimeout: cb.httpWriteTimeout,
+		routes:           cb.routes,
+		middlewares:      cb.middlewares,
+		certFile:         cb.certFile,
+		keyFile:          cb.keyFile,
+	}
+
+	c.routes = append(c.routes, aliveCheckRoute(c.ac))
+	c.routes = append(c.routes, readyCheckRoute(c.rc))
+	c.routes = append(c.routes, profilingRoutes()...)
+	c.routes = append(c.routes, metricRoute())
+
+	return c, nil
+}
diff --git a/sync/http/option.go b/sync/http/option.go
deleted file mode 100644
index 87fbbb5e2..000000000
--- a/sync/http/option.go
+++ /dev/null
@@ -1,89 +0,0 @@
-package http
-
-import (
-	"time"
-
-	"github.com/beatlabs/patron/errors"
-)
-
-// OptionFunc defines a option func for the HTTP component.
-type OptionFunc func(*Component) error
-
-// Port option for setting the ports of the HTTP component.
-func Port(port int) OptionFunc {
-	return func(s *Component) error {
-		if port <= 0 || port > 65535 {
-			return errors.New("invalid port")
-		}
-		s.httpPort = port
-		return nil
-	}
-}
-
-// Timeouts option for setting the timeouts of the HTTP component.
-func Timeouts(read, write time.Duration) OptionFunc {
-	return func(s *Component) error {
-		s.httpReadTimeout = read
-		s.httpWriteTimeout = write
-		return nil
-	}
-}
-
-// Routes option for setting the routes of the HTTP component.
-func Routes(rr []Route) OptionFunc {
-	return func(s *Component) error {
-		if len(rr) == 0 {
-			return errors.New("routes are empty")
-		}
-		s.routes = append(s.routes, rr...)
-		return nil
-	}
-}
-
-// Middlewares option for setting the routes middlewares of the HTTP component.
-func Middlewares(mm ...MiddlewareFunc) OptionFunc {
-	return func(s *Component) error {
-		if len(mm) == 0 {
-			return errors.New("middlewares are empty")
-		}
-		s.middlewares = append(s.middlewares, mm...)
-		return nil
-	}
-}
-
-// AliveCheck option for setting the liveness check function of the HTTP component.
-func AliveCheck(acf AliveCheckFunc) OptionFunc {
-	return func(s *Component) error {
-		if acf == nil {
-			return errors.New("alive check function is not defined")
-		}
-		s.ac = acf
-		return nil
-	}
-}
-
-// ReadyCheck option for setting the readiness check function of the HTTP component.
-func ReadyCheck(rcf ReadyCheckFunc) OptionFunc {
-	return func(s *Component) error {
-		if rcf == nil {
-			return errors.New("alive check function is not defined")
-		}
-		s.rc = rcf
-		return nil
-	}
-}
-
-// Secure option for securing the default HTTP component.
-func Secure(certFile, keyFile string) OptionFunc {
-	return func(s *Component) error {
-		if certFile == "" {
-			return errors.New("cert file is required")
-		}
-		if keyFile == "" {
-			return errors.New("key file is required")
-		}
-		s.certFile = certFile
-		s.keyFile = keyFile
-		return nil
-	}
-}


Test patch:
diff --git a/sync/http/component_test.go b/sync/http/component_test.go
index fd27fc972..0f54ab39e 100644
--- a/sync/http/component_test.go
+++ b/sync/http/component_test.go
@@ -9,39 +9,15 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func ErrorOption() OptionFunc {
-	return func(s *Component) error {
-		return errors.New("TEST")
-	}
-}
-
-func TestNew(t *testing.T) {
-	tests := []struct {
-		name    string
-		options []OptionFunc
-		wantErr bool
-	}{
-		{"success with no options", []OptionFunc{}, false},
-		{"success with options", []OptionFunc{Port(50000)}, false},
-		{"failed with error option", []OptionFunc{ErrorOption()}, true},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got, err := New(tt.options...)
-			if tt.wantErr {
-				assert.Error(t, err)
-				assert.Nil(t, got)
-			} else {
-				assert.NoError(t, err)
-				assert.NotNil(t, got)
-			}
-		})
-	}
+func TestBuilderWithoutOptions(t *testing.T) {
+	got, err := NewBuilder().Create()
+	assert.NotNil(t, got)
+	assert.NoError(t, err)
 }
 
 func TestComponent_ListenAndServe_DefaultRoutes_Shutdown(t *testing.T) {
 	rr := []Route{NewRoute("/", "GET", nil, true, nil)}
-	s, err := New(Routes(rr), Port(50003))
+	s, err := NewBuilder().WithRoutes(rr).WithPort(50003).Create()
 	assert.NoError(t, err)
 	done := make(chan bool)
 	ctx, cnl := context.WithCancel(context.Background())
@@ -57,7 +33,7 @@ func TestComponent_ListenAndServe_DefaultRoutes_Shutdown(t *testing.T) {
 
 func TestComponent_ListenAndServeTLS_DefaultRoutes_Shutdown(t *testing.T) {
 	rr := []Route{NewRoute("/", "GET", nil, true, nil)}
-	s, err := New(Routes(rr), Secure("testdata/server.pem", "testdata/server.key"), Port(50003))
+	s, err := NewBuilder().WithRoutes(rr).WithSSL("testdata/server.pem", "testdata/server.key").WithPort(50003).Create()
 	assert.NoError(t, err)
 	done := make(chan bool)
 	ctx, cnl := context.WithCancel(context.Background())
@@ -73,7 +49,7 @@ func TestComponent_ListenAndServeTLS_DefaultRoutes_Shutdown(t *testing.T) {
 
 func TestComponent_ListenAndServeTLS_FailsInvalidCerts(t *testing.T) {
 	rr := []Route{NewRoute("/", "GET", nil, true, nil)}
-	s, err := New(Routes(rr), Secure("testdata/server.pem", "testdata/server.pem"))
+	s, err := NewBuilder().WithRoutes(rr).WithSSL("testdata/server.pem", "testdata/server.pem").Create()
 	assert.NoError(t, err)
 	assert.Error(t, s.Run(context.Background()))
 }
@@ -90,3 +66,87 @@ func Test_createHTTPServer(t *testing.T) {
 	assert.Equal(t, 5*time.Second, s.ReadTimeout)
 	assert.Equal(t, 10*time.Second, s.WriteTimeout)
 }
+
+func Test_createHTTPServerUsingBuilder(t *testing.T) {
+
+	var httpBuilderNoErrors = []error{}
+	var httpBuilderAllErrors = []error{
+		errors.New("Nil AliveCheckFunc was provided"),
+		errors.New("Nil ReadyCheckFunc provided"),
+		errors.New("Invalid HTTP Port provided"),
+		errors.New("Negative or zero read timeout provided"),
+		errors.New("Negative or zero write timeout provided"),
+		errors.New("Empty Routes slice provided"),
+		errors.New("Empty list of middlewares provided"),
+		errors.New("Invalid cert or key provided"),
+	}
+
+	tests := map[string]struct {
+		acf      AliveCheckFunc
+		rcf      ReadyCheckFunc
+		p        int
+		rt       time.Duration
+		wt       time.Duration
+		rr       []Route
+		mm       []MiddlewareFunc
+		c        string
+		k        string
+		wantErrs []error
+	}{
+		"success": {
+			acf: DefaultAliveCheck,
+			rcf: DefaultReadyCheck,
+			p:   httpPort,
+			rt:  httpReadTimeout,
+			wt:  httpIdleTimeout,
+			rr: []Route{
+				aliveCheckRoute(DefaultAliveCheck),
+				readyCheckRoute(DefaultReadyCheck),
+				metricRoute(),
+			},
+			mm: []MiddlewareFunc{
+				NewRecoveryMiddleware(),
+				panicMiddleware("error"),
+			},
+			c:        "cert.file",
+			k:        "key.file",
+			wantErrs: httpBuilderNoErrors,
+		},
+		"error in all builder steps": {
+			acf:      nil,
+			rcf:      nil,
+			p:        -1,
+			rt:       -10 * time.Second,
+			wt:       -20 * time.Second,
+			rr:       []Route{},
+			mm:       []MiddlewareFunc{},
+			c:        "",
+			k:        "",
+			wantErrs: httpBuilderAllErrors,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			gotHTTPComponent, gotErrs := NewBuilder().
+				WithAliveCheckFunc(tc.acf).
+				WithReadyCheckFunc(tc.rcf).
+				WithPort(tc.p).
+				WithReadTimeout(tc.rt).
+				WithWriteTimeout(tc.wt).
+				WithRoutes(tc.rr).
+				WithMiddlewares(tc.mm...).
+				WithSSL(tc.c, tc.k).
+				Create()
+
+			if len(tc.wantErrs) > 0 {
+				assert.ObjectsAreEqual(tc.wantErrs, gotErrs)
+				assert.Nil(t, gotHTTPComponent)
+			} else {
+				assert.NotNil(t, gotHTTPComponent)
+				assert.IsType(t, &Component{}, gotHTTPComponent)
+			}
+		})
+	}
+
+}
diff --git a/sync/http/option_test.go b/sync/http/option_test.go
deleted file mode 100644
index 212a59850..000000000
--- a/sync/http/option_test.go
+++ /dev/null
@@ -1,165 +0,0 @@
-package http
-
-import (
-	"net/http"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func TestPort(t *testing.T) {
-	tests := []struct {
-		name    string
-		port    int
-		wantErr bool
-	}{
-		{"success", 30000, false},
-		{"error for port number out of range", -1, true},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			s := Component{}
-			err := Port(tt.port)(&s)
-			if tt.wantErr {
-				assert.Error(t, err)
-			} else {
-				assert.Equal(t, tt.port, s.httpPort)
-				assert.NoError(t, err)
-			}
-		})
-	}
-}
-
-func TestSetRoutes(t *testing.T) {
-	tests := []struct {
-		name    string
-		rr      []Route
-		wantErr bool
-	}{
-		{"success", []Route{NewGetRoute("/", testHandler{}.Process, true)}, false},
-		{"error for no routes", nil, true},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			s := Component{}
-			err := Routes(tt.rr)(&s)
-			if tt.wantErr {
-				assert.Error(t, err)
-			} else {
-				assert.Len(t, s.routes, 1)
-				assert.Equal(t, tt.rr[0].Method, s.routes[0].Method)
-				assert.Equal(t, tt.rr[0].Pattern, s.routes[0].Pattern)
-				assert.NoError(t, err)
-			}
-		})
-	}
-}
-
-func TestSetMiddlewares(t *testing.T) {
-	tests := []struct {
-		name    string
-		mm      []MiddlewareFunc
-		wantErr bool
-	}{
-		{"success", []MiddlewareFunc{func(next http.Handler) http.Handler { return next }}, false},
-		{"error for empty middlewares", []MiddlewareFunc{}, true},
-		{"error for nil middlewares", nil, true},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			s := Component{}
-			err := Middlewares(tt.mm...)(&s)
-			if tt.wantErr {
-				assert.Error(t, err)
-			} else {
-				assert.Len(t, s.middlewares, 1)
-				assert.NoError(t, err)
-			}
-		})
-	}
-}
-
-func TestSetLivenessCheck(t *testing.T) {
-	tests := []struct {
-		name    string
-		acf     AliveCheckFunc
-		wantErr bool
-	}{
-		{"success", func() AliveStatus { return Alive }, false},
-		{"error for no routes", nil, true},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			s := Component{}
-			err := AliveCheck(tt.acf)(&s)
-			if tt.wantErr {
-				assert.Error(t, err)
-				assert.Nil(t, s.ac)
-			} else {
-				assert.NoError(t, err)
-				assert.NotNil(t, s.ac)
-			}
-		})
-	}
-}
-
-func TestSetReadinessCheck(t *testing.T) {
-	tests := []struct {
-		name    string
-		rcf     ReadyCheckFunc
-		wantErr bool
-	}{
-		{"success", func() ReadyStatus { return Ready }, false},
-		{"error for no routes", nil, true},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			s := Component{}
-			err := ReadyCheck(tt.rcf)(&s)
-			if tt.wantErr {
-				assert.Error(t, err)
-				assert.Nil(t, s.rc)
-			} else {
-				assert.NoError(t, err)
-				assert.NotNil(t, s.rc)
-			}
-		})
-	}
-}
-
-func TestSetSecure(t *testing.T) {
-	tests := []struct {
-		name     string
-		certFile string
-		keyFile  string
-		wantErr  bool
-	}{
-		{"success", "certFile", "keyFile", false},
-		{"failed missing cert file", "", "keyFile", true},
-		{"failed missing key file", "certFile", "", true},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			s := Component{}
-			err := Secure(tt.certFile, tt.keyFile)(&s)
-			if tt.wantErr {
-				assert.Error(t, err)
-				assert.Empty(t, s.certFile)
-				assert.Empty(t, s.keyFile)
-			} else {
-				assert.NoError(t, err)
-				assert.NotEmpty(t, s.certFile)
-				assert.NotEmpty(t, s.keyFile)
-			}
-		})
-	}
-}
-
-func TestTimeouts(t *testing.T) {
-	c := Component{}
-	err := Timeouts(2*time.Second, 3*time.Second)(&c)
-	assert.NoError(t, err)
-	assert.Equal(t, 2*time.Second, c.httpReadTimeout)
-	assert.Equal(t, 3*time.Second, c.httpWriteTimeout)
-}