Simplify the APIs to return http status codes

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)
 		}
 	}
 

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)
+	_, err = serv.Register(d1)
-	assert.NoError(t, err)
+	assert.Error(t, err)
-	assert.False(t, r3.Success)
 }
 
 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)
 }

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) {
+func HandleRegister(s *Server, vars map[string]string, b io.ReadCloser) (interface{}, error) {
-	var response = rove.RegisterResponse{
+	var response = rove.RegisterResponse{}
-		Success: false,
-	}
 
 	// 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, &reg, 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) {
+func HandleCommand(s *Server, vars map[string]string, b io.ReadCloser) (interface{}, error) {
-	var response = rove.CommandResponse{
+	var response = rove.CommandResponse{}
-		Success: false,
-	}
 
 	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) {
+func HandleRadar(s *Server, vars map[string]string, b io.ReadCloser) (interface{}, error) {
-	var response = rove.RadarResponse{
+	var response = rove.RadarResponse{}
-		Success: false,
-	}
 
 	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) {
+func HandleRover(s *Server, vars map[string]string, b io.ReadCloser) (interface{}, error) {
-	var response = rove.RoverResponse{
+	var response = rove.RoverResponse{}
-		Success: false,
-	}
 
 	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 {
+	} else if attrib, err := s.world.RoverAttributes(id); err != nil {
-		response.Error = fmt.Sprintf("Error getting radar from rover: %s", err)
+		return nil, fmt.Errorf("error getting rover attributes: %s", err)
 
 	} else {
-		response.Attributes = attribs
+		response.Attributes = attrib
-		response.Success = true
 	}
 
 	log.Printf("rover response \taccount:%s\tresponse:%+v\n", id, response)

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)
+	err = json.NewDecoder(response.Body).Decode(&status)
-
+	assert.NoError(t, err)
-	if status.Success != true {
-		t.Errorf("got false for /register: %s", status.Error)
-	}
 }
 
 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)
+	err = json.NewDecoder(response.Body).Decode(&status)
-
+	assert.NoError(t, err)
-	if status.Success != true {
-		t.Errorf("got false for /command: %s", status.Error)
-	}
 
 	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)
+	err = json.NewDecoder(response.Body).Decode(&status)
-
+	assert.NoError(t, err)
-	if status.Success != true {
-		t.Errorf("got false for /radar: %s", status.Error)
-	}
 
 	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 {
+	if attribs != status.Attributes {
-		t.Errorf("got false for /rover: %s", status.Error)
-	} else if attribs != status.Attributes {
 		t.Errorf("Missmatched attributes: %+v, !=%+v", attribs, status.Attributes)
 	}
 }

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)
 

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)
 		}

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"`
+	// Placeholder for future information
-	Error   string `json:"error,omitempty"`
 }
 
 // ==============================
@@ -60,8 +59,7 @@ type CommandData struct {
 
 // CommandResponse is the response to be sent back
 type CommandResponse struct {
-	Success bool   `json:"success"`
+	// Placeholder for future information
-	Error   string `json:"error,omitempty"`
 }
 
 // ================
@@ -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"`
 }

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)

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'
+              $ref: '#/definitions/rover-attributes'