From 47bd2be8823c6f869e3988b6521f193851cc672c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Abdelkader=20Mart=C3=ADnez=20P=C3=A9rez?= Date: Mon, 7 Oct 2019 08:53:48 +0200 Subject: [PATCH] Some fixes suggested by golangci-lint --- internal/client/route_list.go | 1 - internal/client/route_remove.go | 1 - internal/client/set.go | 1 - internal/cmd/get.go | 4 ++-- internal/cmd/validations.go | 1 - internal/server/data/state.go | 6 +++--- internal/server/data/state_test.go | 29 ++++++++++++++--------------- internal/server/model/handler.go | 21 ++++++++++++++++++--- internal/server/model/route.go | 24 ++++++++++++++++++++---- internal/server/user/state.go | 17 ++++++++--------- internal/server/user/state_test.go | 30 +++++++++++++++++++++--------- 11 files changed, 86 insertions(+), 49 deletions(-) diff --git a/internal/client/route_list.go b/internal/client/route_list.go index eae8f6c..bd08260 100644 --- a/internal/client/route_list.go +++ b/internal/client/route_list.go @@ -8,7 +8,6 @@ import ( // ListRoutes queries the kapow! instance for the routes that are registered func ListRoutes(host string, w io.Writer) error { - url := host + "/routes/" return http.Get(url, "", nil, w) } diff --git a/internal/client/route_remove.go b/internal/client/route_remove.go index 72482ee..3749516 100644 --- a/internal/client/route_remove.go +++ b/internal/client/route_remove.go @@ -6,7 +6,6 @@ import ( // RemoveRoute removes a registered route in Kapow! server func RemoveRoute(host, id string) error { - url := host + "/routes/" + id return http.Delete(url, "", nil, nil) } diff --git a/internal/client/set.go b/internal/client/set.go index 5db8ab9..7769428 100644 --- a/internal/client/set.go +++ b/internal/client/set.go @@ -7,7 +7,6 @@ import ( ) func SetData(host, handlerID, path string, r io.Reader) error { - url := host + "/handlers/" + handlerID + path return http.Put(url, "", r, nil) } diff --git a/internal/cmd/get.go b/internal/cmd/get.go index 594c523..f684f3a 100644 --- a/internal/cmd/get.go +++ b/internal/cmd/get.go @@ -12,8 +12,8 @@ import ( // GetCmd is the command line interface for get kapow data operation var GetCmd = &cobra.Command{ Use: "get [flags] resource", - Short: "Retrive a Kapow! resource", - Long: "Retrive a Kapow! resource for the current request", + Short: "Retrieve a Kapow! resource", + Long: "Retrieve a Kapow! resource for the current request", Args: cobra.ExactArgs(1), PreRunE: handlerIDRequired, Run: func(cmd *cobra.Command, args []string) { diff --git a/internal/cmd/validations.go b/internal/cmd/validations.go index 9e855fb..d714912 100644 --- a/internal/cmd/validations.go +++ b/internal/cmd/validations.go @@ -21,5 +21,4 @@ func handlerIDRequired(cmd *cobra.Command, args []string) error { return errors.New("--handler or KAPOW_HANDLER_ID is mandatory") } return nil - } diff --git a/internal/server/data/state.go b/internal/server/data/state.go index 2d44d3a..fa9d584 100644 --- a/internal/server/data/state.go +++ b/internal/server/data/state.go @@ -8,7 +8,7 @@ import ( type safeHandlerMap struct { hs map[string]*model.Handler - m sync.RWMutex + m *sync.RWMutex } var Handlers = New() @@ -16,13 +16,13 @@ var Handlers = New() func New() safeHandlerMap { return safeHandlerMap{ hs: make(map[string]*model.Handler), - m: sync.RWMutex{}, + m: &sync.RWMutex{}, } } func (shm *safeHandlerMap) Add(h *model.Handler) { shm.m.Lock() - shm.hs[h.Id] = h + shm.hs[h.ID] = h shm.m.Unlock() } diff --git a/internal/server/data/state_test.go b/internal/server/data/state_test.go index 3cc9dbb..f4b827f 100644 --- a/internal/server/data/state_test.go +++ b/internal/server/data/state_test.go @@ -27,7 +27,7 @@ func TestPackageHaveASingletonEmptyHandlersList(t *testing.T) { func TestAddAddsANewHandlerToTheMap(t *testing.T) { shm := New() - shm.Add(&model.Handler{Id: "FOO"}) + shm.Add(&model.Handler{ID: "FOO"}) if _, ok := shm.hs["FOO"]; !ok { t.Error("Handler not added to the map") @@ -39,12 +39,12 @@ func TestAddAdquiresMutexBeforeAdding(t *testing.T) { shm.m.Lock() defer shm.m.Unlock() - go shm.Add(&model.Handler{Id: "FOO"}) + go shm.Add(&model.Handler{ID: "FOO"}) time.Sleep(10 * time.Millisecond) if _, ok := shm.hs["FOO"]; ok { - t.Error("Handler added while mutex was adquired") + t.Error("Handler added while mutex was acquired") } } @@ -52,7 +52,7 @@ func TestAddAddsHandlerAfterMutexIsReleased(t *testing.T) { shm := New() shm.m.Lock() - go shm.Add(&model.Handler{Id: "FOO"}) + go shm.Add(&model.Handler{ID: "FOO"}) shm.m.Unlock() time.Sleep(10 * time.Millisecond) @@ -64,7 +64,7 @@ func TestAddAddsHandlerAfterMutexIsReleased(t *testing.T) { func TestRemoveRemovesAHandlerFromTheMap(t *testing.T) { shm := New() - shm.Add(&model.Handler{Id: "FOO"}) + shm.Add(&model.Handler{ID: "FOO"}) shm.Remove("FOO") @@ -75,7 +75,7 @@ func TestRemoveRemovesAHandlerFromTheMap(t *testing.T) { func TestRemoveAdquiresMutexBeforeRemoving(t *testing.T) { shm := New() - shm.Add(&model.Handler{Id: "FOO"}) + shm.Add(&model.Handler{ID: "FOO"}) shm.m.Lock() defer shm.m.Unlock() @@ -85,13 +85,13 @@ func TestRemoveAdquiresMutexBeforeRemoving(t *testing.T) { time.Sleep(10 * time.Millisecond) if _, ok := shm.hs["FOO"]; !ok { - t.Error("Handler was remove while mutex was adquired") + t.Error("Handler was remove while mutex was acquired") } } func TestRemoveRemovesHandlerAfterMutexIsReleased(t *testing.T) { shm := New() - shm.Add(&model.Handler{Id: "FOO"}) + shm.Add(&model.Handler{ID: "FOO"}) shm.m.Lock() go shm.Remove("FOO") @@ -110,12 +110,11 @@ func TestGetReturnFalseWhenHandlerDoesNotExist(t *testing.T) { if _, exists := shm.Get("FOO"); exists { t.Error("Get should return false when handler does not exist") } - } func TestGetReturnTrueWhenHandlerExists(t *testing.T) { shm := New() - shm.Add(&model.Handler{Id: "FOO"}) + shm.Add(&model.Handler{ID: "FOO"}) if _, exists := shm.Get("FOO"); !exists { t.Error("Get should return true when handler do exist") @@ -124,7 +123,7 @@ func TestGetReturnTrueWhenHandlerExists(t *testing.T) { func TestGetReturnExistingHandler(t *testing.T) { shm := New() - expected := &model.Handler{Id: "FOO"} + expected := &model.Handler{ID: "FOO"} shm.Add(expected) if current, _ := shm.Get("FOO"); current != expected { @@ -134,7 +133,7 @@ func TestGetReturnExistingHandler(t *testing.T) { func TestGetWaitsForTheWriterToFinish(t *testing.T) { shm := New() - shm.Add(&model.Handler{Id: "FOO"}) + shm.Add(&model.Handler{ID: "FOO"}) shm.m.Lock() defer shm.m.Unlock() @@ -146,14 +145,14 @@ func TestGetWaitsForTheWriterToFinish(t *testing.T) { select { case <-c: - t.Error("Handler readed while mutex was adquired") + t.Error("Handler readed while mutex was acquired") default: // This default prevents the select from being blocking } } func TestGetNonBlockingReadWithOtherReaders(t *testing.T) { shm := New() - shm.Add(&model.Handler{Id: "FOO"}) + shm.Add(&model.Handler{ID: "FOO"}) shm.m.RLock() defer shm.m.RUnlock() @@ -166,6 +165,6 @@ func TestGetNonBlockingReadWithOtherReaders(t *testing.T) { select { case <-c: default: // This default prevents the select from being blocking - t.Error("Handler couldn't read while mutex was adquired for read") + t.Error("Handler couldn't read while mutex was acquired for read") } } diff --git a/internal/server/model/handler.go b/internal/server/model/handler.go index ec71677..0899934 100644 --- a/internal/server/model/handler.go +++ b/internal/server/model/handler.go @@ -5,10 +5,25 @@ import ( "sync" ) +// Handler represents an open HTTP connection in the User Server. +// +// This struct contains the connection Writer and Request to be managed +// by endpoints of the Data Server. type Handler struct { - Id string - Route *Route + // ID is unique identifier of the request. + ID string + + // Route is a reference to the original route that matched this + // request. + Route *Route + + // Writing is a mutex that prevents two goroutines from writing at + // the same time in the response. Writing sync.Mutex - Writer http.ResponseWriter + + // Request is a pointer to the in-progress request. Request *http.Request + + // Writer is the original http.ResponseWriter of the request. + Writer http.ResponseWriter } diff --git a/internal/server/model/route.go b/internal/server/model/route.go index 12275d4..9a8fc9e 100644 --- a/internal/server/model/route.go +++ b/internal/server/model/route.go @@ -1,9 +1,25 @@ package model +// Route contains the data needed to represent a Kapow! user route. type Route struct { - Id string `json:"id"` - Method string `json:"method"` - Pattern string `json:"url_pattern"` + // ID is the unique identifier of the Route. + ID string `json:"id"` + + // Method is the HTTP method that will match this Route. + Method string `json:"method"` + + // Pattern is the gorilla/mux path pattern that will match this + // Route. + Pattern string `json:"url_pattern"` + + // Entrypoint is the string that will be executed when the Route + // match. + // + // This string will be split according to the shell parsing rules to + // be passed as a list to exec.Command. Entrypoint string `json:"entrypoint"` - Command string `json:"command"` + + // Command is the last argument to be passed to exec.Command when + // executing the Entrypoint + Command string `json:"command"` } diff --git a/internal/server/user/state.go b/internal/server/user/state.go index 726bb46..5c516ab 100644 --- a/internal/server/user/state.go +++ b/internal/server/user/state.go @@ -8,13 +8,16 @@ import ( type safeRouteList struct { rs []model.Route - m sync.RWMutex + m *sync.RWMutex } var Routes = New() func New() safeRouteList { - return safeRouteList{} + return safeRouteList{ + rs: []model.Route{}, + m: &sync.RWMutex{}, + } } func (srl *safeRouteList) Append(r model.Route) { @@ -27,11 +30,7 @@ func (srl *safeRouteList) Snapshot() []model.Route { srl.m.RLock() defer srl.m.RUnlock() - if srl.rs == nil { - return nil - } else { - rs := make([]model.Route, len(srl.rs)) - copy(rs, srl.rs) - return rs - } + rs := make([]model.Route, len(srl.rs)) + copy(rs, srl.rs) + return rs } diff --git a/internal/server/user/state_test.go b/internal/server/user/state_test.go index 3815f36..5704292 100644 --- a/internal/server/user/state_test.go +++ b/internal/server/user/state_test.go @@ -3,10 +3,11 @@ package user import ( - "github.com/BBVA/kapow/internal/server/model" "reflect" "testing" "time" + + "github.com/BBVA/kapow/internal/server/model" ) func TestNewReturnAnEmptyStruct(t *testing.T) { @@ -43,7 +44,7 @@ func TestAppendAdquiresMutexBeforeAdding(t *testing.T) { time.Sleep(10 * time.Millisecond) if len(srl.rs) != 0 { - t.Error("Route added while mutex was adquired") + t.Error("Route added while mutex was acquired") } } @@ -63,7 +64,7 @@ func TestAppendAddsRouteAfterMutexIsReleased(t *testing.T) { func TestSnapshotReturnTheCurrentListOfRoutes(t *testing.T) { srl := New() - srl.Append(model.Route{Id: "FOO"}) + srl.Append(model.Route{ID: "FOO"}) rs := srl.Snapshot() @@ -72,19 +73,30 @@ func TestSnapshotReturnTheCurrentListOfRoutes(t *testing.T) { } } +func TestSnapshotReturnADeepCopyOfTheListWhenIsNil(t *testing.T) { + srl := New() + srl.rs = nil + + rs := srl.Snapshot() + + if len(rs) != 0 { + t.Fatal("Route list copy is not empty") + } +} + func TestSnapshotReturnADeepCopyOfTheListWhenEmpty(t *testing.T) { srl := New() rs := srl.Snapshot() - if rs != nil { + if len(rs) != 0 { t.Fatal("Route list copy is not empty") } } func TestSnapshotReturnADeepCopyOfTheListWhenNonEmpty(t *testing.T) { srl := New() - srl.Append(model.Route{Id: "FOO"}) + srl.Append(model.Route{ID: "FOO"}) rs := srl.Snapshot() @@ -101,7 +113,7 @@ func TestSnapshotReturnADeepCopyOfTheListWhenNonEmpty(t *testing.T) { func TestSnapshotWaitsForTheWriterToFinish(t *testing.T) { srl := New() - srl.Append(model.Route{Id: "FOO"}) + srl.Append(model.Route{ID: "FOO"}) srl.m.Lock() defer srl.m.Unlock() @@ -113,14 +125,14 @@ func TestSnapshotWaitsForTheWriterToFinish(t *testing.T) { select { case <-c: - t.Error("Route list readed while mutex was adquired") + t.Error("Route list readed while mutex was acquired") default: // This default prevents the select from being blocking } } func TestSnapshotNonBlockingReadWithOtherReaders(t *testing.T) { srl := New() - srl.Append(model.Route{Id: "FOO"}) + srl.Append(model.Route{ID: "FOO"}) srl.m.RLock() defer srl.m.RUnlock() @@ -133,6 +145,6 @@ func TestSnapshotNonBlockingReadWithOtherReaders(t *testing.T) { select { case <-c: default: // This default prevents the select from being blocking - t.Error("Route list couldn't be readed while mutex was adquired for read") + t.Error("Route list couldn't be readed while mutex was acquired for read") } }