From 6cfc9444f3fde7d8875294dd0feb7843fabcc849 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Fri, 12 Jun 2020 18:58:38 +0100 Subject: [PATCH] Simplify the APIs to return http status codes --- cmd/rove-accountant/internal/accounts.go | 2 +- cmd/rove-server/internal/api_test.go | 29 +++----- cmd/rove-server/internal/routes.go | 85 ++++++++++-------------- cmd/rove-server/internal/routes_test.go | 31 +++------ cmd/rove-server/internal/server.go | 17 ++++- cmd/rove/main.go | 16 +---- pkg/rove/api.go | 12 +--- pkg/rove/http.go | 13 +++- swagger.yml | 79 +++++++++++----------- 9 files changed, 128 insertions(+), 156 deletions(-) diff --git a/cmd/rove-accountant/internal/accounts.go b/cmd/rove-accountant/internal/accounts.go index 5aecb47..01e29dc 100644 --- a/cmd/rove-accountant/internal/accounts.go +++ b/cmd/rove-accountant/internal/accounts.go @@ -42,7 +42,7 @@ func (a *Accountant) RegisterAccount(name string) (acc Account, err error) { // Verify this acount isn't already registered for _, a := range a.Accounts { if a.Name == acc.Name { - return Account{}, fmt.Errorf("account name already registered") + return Account{}, fmt.Errorf("account name already registered: %s", a.Name) } } diff --git a/cmd/rove-server/internal/api_test.go b/cmd/rove-server/internal/api_test.go index 7b99d07..146d61b 100644 --- a/cmd/rove-server/internal/api_test.go +++ b/cmd/rove-server/internal/api_test.go @@ -35,29 +35,25 @@ func TestServer_Register(t *testing.T) { d1 := rove.RegisterData{ Name: uuid.New().String(), } - r1, err := serv.Register(d1) + _, err := serv.Register(d1) assert.NoError(t, err) - assert.True(t, r1.Success) d2 := rove.RegisterData{ Name: uuid.New().String(), } - r2, err := serv.Register(d2) + _, err = serv.Register(d2) assert.NoError(t, err) - assert.True(t, r2.Success) - r3, err := serv.Register(d1) - assert.NoError(t, err) - assert.False(t, r3.Success) + _, err = serv.Register(d1) + assert.Error(t, err) } func TestServer_Command(t *testing.T) { d1 := rove.RegisterData{ Name: uuid.New().String(), } - r1, err := serv.Register(d1) + _, err := serv.Register(d1) assert.NoError(t, err) - assert.True(t, r1.Success) c := rove.CommandData{ Commands: []game.Command{ @@ -68,33 +64,28 @@ func TestServer_Command(t *testing.T) { }, }, } - r3, err := serv.Command(d1.Name, c) + _, err = serv.Command(d1.Name, c) assert.NoError(t, err) - assert.True(t, r3.Success) } func TestServer_Radar(t *testing.T) { d1 := rove.RegisterData{ Name: uuid.New().String(), } - r1, err := serv.Register(d1) + _, err := serv.Register(d1) assert.NoError(t, err) - assert.True(t, r1.Success) - r3, err := serv.Radar(d1.Name) + _, err = serv.Radar(d1.Name) assert.NoError(t, err) - assert.True(t, r3.Success) } func TestServer_Rover(t *testing.T) { d1 := rove.RegisterData{ Name: uuid.New().String(), } - r1, err := serv.Register(d1) + _, err := serv.Register(d1) assert.NoError(t, err) - assert.True(t, r1.Success) - r3, err := serv.Rover(d1.Name) + _, err = serv.Rover(d1.Name) assert.NoError(t, err) - assert.True(t, r3.Success) } diff --git a/cmd/rove-server/internal/routes.go b/cmd/rove-server/internal/routes.go index 12fbd7a..41258c0 100644 --- a/cmd/rove-server/internal/routes.go +++ b/cmd/rove-server/internal/routes.go @@ -17,7 +17,7 @@ import ( ) // Handler describes a function that handles any incoming request and can respond -type Handler func(*Server, map[string]string, io.ReadCloser, io.Writer) (interface{}, error) +type Handler func(*Server, map[string]string, io.ReadCloser) (interface{}, error) // Route defines the information for a single path->function route type Route struct { @@ -56,7 +56,7 @@ var Routes = []Route{ } // HandleStatus handles the /status request -func HandleStatus(s *Server, vars map[string]string, b io.ReadCloser, w io.Writer) (interface{}, error) { +func HandleStatus(s *Server, vars map[string]string, b io.ReadCloser) (interface{}, error) { // Simply return the current server status response := rove.StatusResponse{ @@ -74,40 +74,35 @@ func HandleStatus(s *Server, vars map[string]string, b io.ReadCloser, w io.Write } // HandleRegister handles /register endpoint -func HandleRegister(s *Server, vars map[string]string, b io.ReadCloser, w io.Writer) (interface{}, error) { - var response = rove.RegisterResponse{ - Success: false, - } +func HandleRegister(s *Server, vars map[string]string, b io.ReadCloser) (interface{}, error) { + var response = rove.RegisterResponse{} // Decode the registration info, verify it and register the account var data rove.RegisterData err := json.NewDecoder(b).Decode(&data) if err != nil { log.Printf("Failed to decode json: %s\n", err) - response.Error = err.Error() + return BadRequestError{Error: err.Error()}, nil } else if len(data.Name) == 0 { - response.Error = "Cannot register empty name" + return BadRequestError{Error: "cannot register empty name"}, nil } ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() reg := accounts.RegisterInfo{Name: data.Name} if acc, err := s.accountant.Register(ctx, ®, grpc.WaitForReady(true)); err != nil { - response.Error = err.Error() + return nil, fmt.Errorf("gRPC failed to contact accountant: %s", err) } else if !acc.Success { - response.Error = acc.Error + return BadRequestError{Error: acc.Error}, nil } else if _, _, err := s.SpawnRoverForAccount(data.Name); err != nil { - response.Error = err.Error() + return nil, fmt.Errorf("failed to spawn rover for account: %s", err) } else if err := s.SaveWorld(); err != nil { - response.Error = fmt.Sprintf("Internal server error when saving world: %s", err) + return nil, fmt.Errorf("internal server error when saving world: %s", err) - } else { - // Save out the new accounts - response.Success = true } log.Printf("register response:%+v\n", response) @@ -115,10 +110,8 @@ func HandleRegister(s *Server, vars map[string]string, b io.ReadCloser, w io.Wri } // HandleSpawn will spawn the player entity for the associated account -func HandleCommand(s *Server, vars map[string]string, b io.ReadCloser, w io.Writer) (interface{}, error) { - var response = rove.CommandResponse{ - Success: false, - } +func HandleCommand(s *Server, vars map[string]string, b io.ReadCloser) (interface{}, error) { + var response = rove.CommandResponse{} id := vars["account"] @@ -126,7 +119,7 @@ func HandleCommand(s *Server, vars map[string]string, b io.ReadCloser, w io.Writ var data rove.CommandData if err := json.NewDecoder(b).Decode(&data); err != nil { log.Printf("Failed to decode json: %s\n", err) - response.Error = err.Error() + return BadRequestError{Error: err.Error()}, nil } @@ -134,22 +127,20 @@ func HandleCommand(s *Server, vars map[string]string, b io.ReadCloser, w io.Writ defer cancel() key := accounts.DataKey{Account: id, Key: "rover"} if len(id) == 0 { - response.Error = "No account ID provided" + return BadRequestError{Error: "no account ID provided"}, nil } else if resp, err := s.accountant.GetValue(ctx, &key); err != nil { - response.Error = fmt.Sprintf("Provided account has no rover: %s", err) + return nil, fmt.Errorf("gRPC failed to contact accountant: %s", err) } else if !resp.Success { - response.Error = resp.Error + return BadRequestError{Error: resp.Error}, nil } else if id, err := uuid.Parse(resp.Value); err != nil { - response.Error = fmt.Sprintf("Account had invalid rover id: %s", err) + return nil, fmt.Errorf("account had invalid rover ID: %s", resp.Value) } else if err := s.world.Enqueue(id, data.Commands...); err != nil { - response.Error = fmt.Sprintf("Failed to execute commands: %s", err) + return BadRequestError{Error: err.Error()}, nil - } else { - response.Success = true } log.Printf("command response \taccount:%s\tresponse:%+v\n", id, response) @@ -157,37 +148,34 @@ func HandleCommand(s *Server, vars map[string]string, b io.ReadCloser, w io.Writ } // HandleRadar handles the radar request -func HandleRadar(s *Server, vars map[string]string, b io.ReadCloser, w io.Writer) (interface{}, error) { - var response = rove.RadarResponse{ - Success: false, - } +func HandleRadar(s *Server, vars map[string]string, b io.ReadCloser) (interface{}, error) { + var response = rove.RadarResponse{} ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() id := vars["account"] key := accounts.DataKey{Account: id, Key: "rover"} if len(id) == 0 { - response.Error = "No account ID provided" + return BadRequestError{Error: "no account ID provided"}, nil } else if resp, err := s.accountant.GetValue(ctx, &key); err != nil { - response.Error = fmt.Sprintf("Provided account has no rover: %s", err) + return nil, fmt.Errorf("gRPC failed to contact accountant: %s", err) } else if !resp.Success { - response.Error = resp.Error + return BadRequestError{Error: resp.Error}, nil } else if id, err := uuid.Parse(resp.Value); err != nil { - response.Error = fmt.Sprintf("Account had invalid rover id: %s", err) + return nil, fmt.Errorf("account had invalid rover ID: %s", resp.Value) } else if attrib, err := s.world.RoverAttributes(id); err != nil { - response.Error = fmt.Sprintf("Error getting rover attributes: %s", err) + return nil, fmt.Errorf("error getting rover attributes: %s", err) } else if radar, err := s.world.RadarFromRover(id); err != nil { - response.Error = fmt.Sprintf("Error getting radar from rover: %s", err) + return nil, fmt.Errorf("error getting radar from rover: %s", err) } else { response.Tiles = radar response.Range = attrib.Range - response.Success = true } log.Printf("radar response \taccount:%s\tresponse:%+v\n", id, response) @@ -195,33 +183,30 @@ func HandleRadar(s *Server, vars map[string]string, b io.ReadCloser, w io.Writer } // HandleRover handles the rover request -func HandleRover(s *Server, vars map[string]string, b io.ReadCloser, w io.Writer) (interface{}, error) { - var response = rove.RoverResponse{ - Success: false, - } +func HandleRover(s *Server, vars map[string]string, b io.ReadCloser) (interface{}, error) { + var response = rove.RoverResponse{} ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() id := vars["account"] key := accounts.DataKey{Account: id, Key: "rover"} if len(id) == 0 { - response.Error = "No account ID provided" + return BadRequestError{Error: "no account ID provided"}, nil } else if resp, err := s.accountant.GetValue(ctx, &key); err != nil { - response.Error = fmt.Sprintf("Provided account has no rover: %s", err) + return nil, fmt.Errorf("gRPC failed to contact accountant: %s", err) } else if !resp.Success { - response.Error = resp.Error + return BadRequestError{Error: resp.Error}, nil } else if id, err := uuid.Parse(resp.Value); err != nil { - response.Error = fmt.Sprintf("Account had invalid rover id: %s", err) + return nil, fmt.Errorf("account had invalid rover ID: %s", resp.Value) - } else if attribs, err := s.world.RoverAttributes(id); err != nil { - response.Error = fmt.Sprintf("Error getting radar from rover: %s", err) + } else if attrib, err := s.world.RoverAttributes(id); err != nil { + return nil, fmt.Errorf("error getting rover attributes: %s", err) } else { - response.Attributes = attribs - response.Success = true + response.Attributes = attrib } log.Printf("rover response \taccount:%s\tresponse:%+v\n", id, response) diff --git a/cmd/rove-server/internal/routes_test.go b/cmd/rove-server/internal/routes_test.go index e34b983..04a1bac 100644 --- a/cmd/rove-server/internal/routes_test.go +++ b/cmd/rove-server/internal/routes_test.go @@ -31,7 +31,8 @@ func TestHandleStatus(t *testing.T) { assert.Equal(t, http.StatusOK, response.Code) var status rove.StatusResponse - json.NewDecoder(response.Body).Decode(&status) + err := json.NewDecoder(response.Body).Decode(&status) + assert.NoError(t, err) if status.Ready != true { t.Errorf("got false for /status") @@ -58,11 +59,8 @@ func TestHandleRegister(t *testing.T) { assert.Equal(t, http.StatusOK, response.Code) var status rove.RegisterResponse - json.NewDecoder(response.Body).Decode(&status) - - if status.Success != true { - t.Errorf("got false for /register: %s", status.Error) - } + err = json.NewDecoder(response.Body).Decode(&status) + assert.NoError(t, err) } func TestHandleCommand(t *testing.T) { @@ -104,11 +102,8 @@ func TestHandleCommand(t *testing.T) { assert.Equal(t, http.StatusOK, response.Code) var status rove.CommandResponse - json.NewDecoder(response.Body).Decode(&status) - - if status.Success != true { - t.Errorf("got false for /command: %s", status.Error) - } + err = json.NewDecoder(response.Body).Decode(&status) + assert.NoError(t, err) attrib, err := s.world.RoverAttributes(inst) assert.NoError(t, err, "Couldn't get rover attribs") @@ -157,11 +152,8 @@ func TestHandleRadar(t *testing.T) { assert.Equal(t, http.StatusOK, response.Code) var status rove.RadarResponse - json.NewDecoder(response.Body).Decode(&status) - - if status.Success != true { - t.Errorf("got false for /radar: %s", status.Error) - } + err = json.NewDecoder(response.Body).Decode(&status) + assert.NoError(t, err) scope := attrib.Range*2 + 1 radarOrigin := vector.Vector{X: -attrib.Range, Y: -attrib.Range} @@ -201,11 +193,10 @@ func TestHandleRover(t *testing.T) { assert.Equal(t, http.StatusOK, response.Code) var status rove.RoverResponse - json.NewDecoder(response.Body).Decode(&status) + err = json.NewDecoder(response.Body).Decode(&status) + assert.NoError(t, err) - if status.Success != true { - t.Errorf("got false for /rover: %s", status.Error) - } else if attribs != status.Attributes { + if attribs != status.Attributes { t.Errorf("Missmatched attributes: %+v, !=%+v", attribs, status.Attributes) } } diff --git a/cmd/rove-server/internal/server.go b/cmd/rove-server/internal/server.go index b1d2277..00f696b 100644 --- a/cmd/rove-server/internal/server.go +++ b/cmd/rove-server/internal/server.go @@ -248,6 +248,11 @@ func (s *Server) LoadWorld() error { return nil } +// used as the type for the return struct +type BadRequestError struct { + Error string `json:"error"` +} + // wrapHandler wraps a request handler in http checks func (s *Server) wrapHandler(method string, handler Handler) func(w http.ResponseWriter, r *http.Request) { return func(w http.ResponseWriter, r *http.Request) { @@ -259,12 +264,20 @@ func (s *Server) wrapHandler(method string, handler Handler) func(w http.Respons // Verify the method, call the handler, and encode the return if r.Method != method { w.WriteHeader(http.StatusMethodNotAllowed) + return + } - } else if val, err := handler(s, vars, r.Body, w); err != nil { + val, err := handler(s, vars, r.Body) + if err != nil { log.Printf("Failed to handle http request: %s", err) w.WriteHeader(http.StatusInternalServerError) + return - } else if err := json.NewEncoder(w).Encode(val); err != nil { + } else if _, ok := val.(BadRequestError); ok { + w.WriteHeader(http.StatusBadRequest) + } + + if err := json.NewEncoder(w).Encode(val); err != nil { log.Printf("Failed to encode reply to json: %s", err) w.WriteHeader(http.StatusInternalServerError) diff --git a/cmd/rove/main.go b/cmd/rove/main.go index ca7a54f..527faf9 100644 --- a/cmd/rove/main.go +++ b/cmd/rove/main.go @@ -117,14 +117,11 @@ func InnerMain(command string) error { d := rove.RegisterData{ Name: *name, } - response, err := server.Register(d) + _, err := server.Register(d) switch { case err != nil: return err - case !response.Success: - return fmt.Errorf("server returned failure: %s", response.Error) - default: fmt.Printf("Registered account with id: %s\n", *name) config.Accounts[config.Host] = *name @@ -145,14 +142,11 @@ func InnerMain(command string) error { return err } - response, err := server.Command(account, d) + _, err := server.Command(account, d) switch { case err != nil: return err - case !response.Success: - return fmt.Errorf("server returned failure: %s", response.Error) - default: fmt.Printf("Request succeeded\n") } @@ -167,9 +161,6 @@ func InnerMain(command string) error { case err != nil: return err - case !response.Success: - return fmt.Errorf("server returned failure: %s", response.Error) - default: // Print out the radar game.PrintTiles(response.Tiles) @@ -185,9 +176,6 @@ func InnerMain(command string) error { case err != nil: return err - case !response.Success: - return fmt.Errorf("server returned failure: %s", response.Error) - default: fmt.Printf("attributes: %+v\n", response.Attributes) } diff --git a/pkg/rove/api.go b/pkg/rove/api.go index 007b8d1..4c258bc 100644 --- a/pkg/rove/api.go +++ b/pkg/rove/api.go @@ -40,8 +40,7 @@ type RegisterData struct { // RegisterResponse describes the response to a register request type RegisterResponse struct { - Success bool `json:"success"` - Error string `json:"error,omitempty"` + // Placeholder for future information } // ============================== @@ -60,8 +59,7 @@ type CommandData struct { // CommandResponse is the response to be sent back type CommandResponse struct { - Success bool `json:"success"` - Error string `json:"error,omitempty"` + // Placeholder for future information } // ================ @@ -75,9 +73,6 @@ func (s Server) Radar(account string) (r RadarResponse, err error) { // RadarResponse describes the response to a /radar call type RadarResponse struct { - Success bool `json:"success"` - Error string `json:"error,omitempty"` - // The set of positions for nearby non-empty tiles Range int `json:"range"` Tiles []atlas.Tile `json:"tiles"` @@ -94,9 +89,6 @@ func (s Server) Rover(account string) (r RoverResponse, err error) { // RoverResponse includes information about the rover in question type RoverResponse struct { - Success bool `json:"success"` - Error string `json:"error,omitempty"` - // The current position of this rover Attributes game.RoverAttributes `json:"attributes"` } diff --git a/pkg/rove/http.go b/pkg/rove/http.go index e415762..37c16b9 100644 --- a/pkg/rove/http.go +++ b/pkg/rove/http.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "io/ioutil" "net/http" "net/url" ) @@ -22,7 +23,11 @@ func (s Server) Get(path string, out interface{}) error { return err } else if resp.StatusCode != http.StatusOK { - return fmt.Errorf("http.Get returned status %d: %s", resp.StatusCode, resp.Status) + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("failed to read response body to code %d", resp.StatusCode) + } + return fmt.Errorf("http returned status %d: %s", resp.StatusCode, string(body)) } else { return json.NewDecoder(resp.Body).Decode(out) @@ -56,7 +61,11 @@ func (s Server) Post(path string, in, out interface{}) error { return err } else if resp.StatusCode != http.StatusOK { - return fmt.Errorf("http returned status %d", resp.StatusCode) + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("failed to read response body to code %d", resp.StatusCode) + } + return fmt.Errorf("http returned status %d: %s", resp.StatusCode, string(body)) } else { return json.NewDecoder(resp.Body).Decode(out) diff --git a/swagger.yml b/swagger.yml index 03b0fad..3dcabef 100644 --- a/swagger.yml +++ b/swagger.yml @@ -45,9 +45,15 @@ paths: $ref: '#/definitions/register-data' responses: "200": - description: "Successfully attempted to create account, check success value in body" + description: "Successfully created account" + "400": + description: "Bad request, typically due to duplicate name" schema: - $ref: '#/definitions/register-response' + $ref: '#/definitions/error' + "500": + description: "Server encountered error, please report this as a bug" + schema: + $ref: '#/definitions/error' /{account}/commands: post: @@ -67,9 +73,15 @@ paths: $ref: '#/definitions/commands-data' responses: "200": - description: "Recieved commands, check success value in body" + description: "Successfully queued commands" + "400": + description: "Bad request, typically due to duplicate name" schema: - $ref: '#/definitions/commands-response' + $ref: '#/definitions/error' + "500": + description: "Server encountered error, please report this as a bug" + schema: + $ref: '#/definitions/error' /{account}/radar: get: @@ -85,9 +97,17 @@ paths: type: string responses: "200": - description: "Recieved request, check success value in body" + description: "Successfully returned rover radar" schema: $ref: '#/definitions/radar-response' + "400": + description: "Bad request, typically due to duplicate name" + schema: + $ref: '#/definitions/error' + "500": + description: "Server encountered error, please report this as a bug" + schema: + $ref: '#/definitions/error' /{account}/rover: get: @@ -103,13 +123,20 @@ paths: type: string responses: "200": - description: "Recieved request, check success value in body" + description: "Successfully returned rover information" schema: $ref: '#/definitions/rover-response' + "400": + description: "Bad request, typically due to duplicate name" + schema: + $ref: '#/definitions/error' + "500": + description: "Server encountered error, please report this as a bug" + schema: + $ref: '#/definitions/error' definitions: status: - properties: ready: type: boolean @@ -124,6 +151,12 @@ definitions: type: string example: "15:30:00" + error: + properties: + error: + type: string + example: "account not found" + register-data: properties: name: @@ -132,15 +165,6 @@ definitions: required: - name - register-response: - properties: - success: - type: boolean - example: false - error: - type: string - example: "account with name already existed" - command: properties: command: @@ -166,23 +190,8 @@ definitions: type: string example: "account with name already existed" - commands-response: - properties: - success: - type: boolean - example: false - error: - type: string - example: "invalid bearing 'SN'" - radar-response: properties: - success: - type: boolean - example: false - error: - type: string - example: "unknown account" range: type: integer example: 5 @@ -219,14 +228,8 @@ definitions: rover-response: properties: - success: - type: boolean - example: false - error: - type: string - example: "unknown account" attributes: type: object properties: schema: - $ref: '#/definitions/rover-attributes' \ No newline at end of file + $ref: '#/definitions/rover-attributes'