jobqueue/api: return errors as JSON
The API is always advertising a content type of application/json, but
not sending JSON on errors.
Change it to send simple JSON objects like this:
{ "message": "something went wrong" }
This can be extended to include more structured information in the
future.
Also return an (for now) empty JSON object from `addJobHandler()`. It
returned nothing before, which is invalid JSON.
Stop testing for the actual error strings in `api_test.go`. These are
meant for humans and can change. Only check what a client could
meaningfully check for, which is only the HTTP status code right now.
This commit is contained in:
Lars Karlitski
Tom Gundersen
parent
ce6d3c511a
commit
8ef4db816d
4 changed files with 78 additions and 75 deletions
|
|
@ -59,34 +59,36 @@ func (api *API) ServeHTTP(writer http.ResponseWriter, request *http.Request) {
|
|||
api.router.ServeHTTP(writer, request)
|
||||
}
|
||||
|
||||
// jsonErrorf() is similar to http.Error(), but returns the message in a json
|
||||
// object with a "message" field.
|
||||
func jsonErrorf(writer http.ResponseWriter, code int, message string, args ...interface{}) {
|
||||
writer.WriteHeader(code)
|
||||
|
||||
// ignore error, because we cannot do anything useful with it
|
||||
_ = json.NewEncoder(writer).Encode(&errorResponse{
|
||||
Message: fmt.Sprintf(message, args...),
|
||||
})
|
||||
}
|
||||
|
||||
func methodNotAllowedHandler(writer http.ResponseWriter, request *http.Request) {
|
||||
writer.WriteHeader(http.StatusMethodNotAllowed)
|
||||
jsonErrorf(writer, http.StatusMethodNotAllowed, "method not allowed")
|
||||
}
|
||||
|
||||
func notFoundHandler(writer http.ResponseWriter, request *http.Request) {
|
||||
writer.WriteHeader(http.StatusNotFound)
|
||||
}
|
||||
|
||||
func statusResponseError(writer http.ResponseWriter, code int, message string) {
|
||||
writer.WriteHeader(code)
|
||||
|
||||
if message != "" {
|
||||
// ignore error, because we cannot do anything useful with it
|
||||
fmt.Fprint(writer, message)
|
||||
}
|
||||
jsonErrorf(writer, http.StatusNotFound, "not found")
|
||||
}
|
||||
|
||||
func (api *API) addJobHandler(writer http.ResponseWriter, request *http.Request, _ httprouter.Params) {
|
||||
contentType := request.Header["Content-Type"]
|
||||
if len(contentType) != 1 || contentType[0] != "application/json" {
|
||||
statusResponseError(writer, http.StatusUnsupportedMediaType, "")
|
||||
jsonErrorf(writer, http.StatusUnsupportedMediaType, "request must contain application/json data")
|
||||
return
|
||||
}
|
||||
|
||||
var body addJobRequest
|
||||
err := json.NewDecoder(request.Body).Decode(&body)
|
||||
if err != nil {
|
||||
statusResponseError(writer, http.StatusBadRequest, "invalid request: "+err.Error())
|
||||
jsonErrorf(writer, http.StatusBadRequest, "%v", err)
|
||||
return
|
||||
}
|
||||
|
||||
|
|
@ -105,59 +107,57 @@ func (api *API) addJobHandler(writer http.ResponseWriter, request *http.Request,
|
|||
func (api *API) updateJobHandler(writer http.ResponseWriter, request *http.Request, params httprouter.Params) {
|
||||
contentType := request.Header["Content-Type"]
|
||||
if len(contentType) != 1 || contentType[0] != "application/json" {
|
||||
statusResponseError(writer, http.StatusUnsupportedMediaType, "")
|
||||
jsonErrorf(writer, http.StatusUnsupportedMediaType, "request must contain application/json data")
|
||||
return
|
||||
}
|
||||
|
||||
id, err := uuid.Parse(params.ByName("job_id"))
|
||||
if err != nil {
|
||||
statusResponseError(writer, http.StatusBadRequest, "invalid compose id: "+err.Error())
|
||||
jsonErrorf(writer, http.StatusBadRequest, "cannot parse compose id: %v", err)
|
||||
return
|
||||
}
|
||||
|
||||
imageBuildId, err := strconv.Atoi(params.ByName("build_id"))
|
||||
|
||||
if err != nil {
|
||||
statusResponseError(writer, http.StatusBadRequest, "invalid image build id: "+err.Error())
|
||||
jsonErrorf(writer, http.StatusBadRequest, "cannot parse image build id: %v", err)
|
||||
return
|
||||
}
|
||||
|
||||
var body updateJobRequest
|
||||
err = json.NewDecoder(request.Body).Decode(&body)
|
||||
if err != nil {
|
||||
statusResponseError(writer, http.StatusBadRequest, "invalid status: "+err.Error())
|
||||
jsonErrorf(writer, http.StatusBadRequest, "cannot parse request body: %v", err)
|
||||
return
|
||||
}
|
||||
|
||||
err = api.store.UpdateImageBuildInCompose(id, imageBuildId, body.Status, body.Result)
|
||||
if err != nil {
|
||||
switch err.(type) {
|
||||
case *store.NotFoundError:
|
||||
statusResponseError(writer, http.StatusNotFound, err.Error())
|
||||
case *store.NotPendingError:
|
||||
statusResponseError(writer, http.StatusNotFound, err.Error())
|
||||
case *store.NotRunningError:
|
||||
statusResponseError(writer, http.StatusBadRequest, err.Error())
|
||||
case *store.InvalidRequestError:
|
||||
statusResponseError(writer, http.StatusBadRequest, err.Error())
|
||||
case *store.NotFoundError, *store.NotPendingError:
|
||||
jsonErrorf(writer, http.StatusNotFound, "%v", err)
|
||||
case *store.NotRunningError, *store.InvalidRequestError:
|
||||
jsonErrorf(writer, http.StatusBadRequest, "%v", err)
|
||||
default:
|
||||
statusResponseError(writer, http.StatusInternalServerError, err.Error())
|
||||
jsonErrorf(writer, http.StatusInternalServerError, "%v", err)
|
||||
}
|
||||
return
|
||||
}
|
||||
|
||||
_ = json.NewEncoder(writer).Encode(updateJobResponse{})
|
||||
}
|
||||
|
||||
func (api *API) addJobImageHandler(writer http.ResponseWriter, request *http.Request, params httprouter.Params) {
|
||||
id, err := uuid.Parse(params.ByName("job_id"))
|
||||
if err != nil {
|
||||
statusResponseError(writer, http.StatusBadRequest, "invalid compose id: "+err.Error())
|
||||
jsonErrorf(writer, http.StatusBadRequest, "cannot parse compose id: %v", err)
|
||||
return
|
||||
}
|
||||
|
||||
imageBuildId, err := strconv.Atoi(params.ByName("build_id"))
|
||||
|
||||
if err != nil {
|
||||
statusResponseError(writer, http.StatusBadRequest, "invalid build id: "+err.Error())
|
||||
jsonErrorf(writer, http.StatusBadRequest, "cannot parse image build id: %v", err)
|
||||
return
|
||||
}
|
||||
|
||||
|
|
@ -166,11 +166,11 @@ func (api *API) addJobImageHandler(writer http.ResponseWriter, request *http.Req
|
|||
if err != nil {
|
||||
switch err.(type) {
|
||||
case *store.NotFoundError:
|
||||
statusResponseError(writer, http.StatusNotFound, err.Error())
|
||||
jsonErrorf(writer, http.StatusNotFound, "%v", err)
|
||||
case *store.NoLocalTargetError:
|
||||
statusResponseError(writer, http.StatusBadRequest, err.Error())
|
||||
jsonErrorf(writer, http.StatusBadRequest, "%v", err)
|
||||
default:
|
||||
statusResponseError(writer, http.StatusInternalServerError, err.Error())
|
||||
jsonErrorf(writer, http.StatusInternalServerError, "%v", err)
|
||||
}
|
||||
return
|
||||
}
|
||||
|
|
|
|||
|
|
@ -15,30 +15,28 @@ import (
|
|||
|
||||
func TestErrors(t *testing.T) {
|
||||
var cases = []struct {
|
||||
Method string
|
||||
Path string
|
||||
Body string
|
||||
ExpectedStatus int
|
||||
ExpectedResponse string
|
||||
Method string
|
||||
Path string
|
||||
Body string
|
||||
ExpectedStatus int
|
||||
}{
|
||||
// Bogus path
|
||||
{"GET", "/foo", ``, http.StatusNotFound, ``},
|
||||
{"GET", "/foo", ``, http.StatusNotFound},
|
||||
// Create job with invalid body
|
||||
{"POST", "/job-queue/v1/jobs", ``, http.StatusBadRequest, `invalid request: EOF`},
|
||||
{"POST", "/job-queue/v1/jobs", ``, http.StatusBadRequest},
|
||||
// Wrong method
|
||||
{"GET", "/job-queue/v1/jobs", ``, http.StatusMethodNotAllowed, ``},
|
||||
{"GET", "/job-queue/v1/jobs", ``, http.StatusMethodNotAllowed},
|
||||
// Update job with invalid ID
|
||||
{"PATCH", "/job-queue/v1/jobs/foo/builds/0", `{"status":"RUNNING"}`, http.StatusBadRequest, `invalid compose id: invalid UUID length: 3`},
|
||||
{"PATCH", "/job-queue/v1/jobs/foo/builds/0", `{"status":"RUNNING"}`, http.StatusBadRequest},
|
||||
// Update job that does not exist, with invalid body
|
||||
{"PATCH", "/job-queue/v1/jobs/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa/builds/0", ``, http.StatusBadRequest, `invalid status: EOF`},
|
||||
{"PATCH", "/job-queue/v1/jobs/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa/builds/0", ``, http.StatusBadRequest},
|
||||
// Update job that does not exist
|
||||
{"PATCH", "/job-queue/v1/jobs/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa/builds/0", `{"status":"RUNNING"}`, http.StatusNotFound, `compose does not exist`},
|
||||
{"PATCH", "/job-queue/v1/jobs/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa/builds/0", `{"status":"RUNNING"}`, http.StatusNotFound},
|
||||
}
|
||||
|
||||
for _, c := range cases {
|
||||
api := jobqueue.New(nil, store.New(nil))
|
||||
|
||||
test.TestNonJsonRoute(t, api, false, c.Method, c.Path, c.Body, c.ExpectedStatus, c.ExpectedResponse)
|
||||
test.TestRoute(t, api, false, c.Method, c.Path, c.Body, c.ExpectedStatus, "{}", "message")
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -64,7 +62,7 @@ func TestCreate(t *testing.T) {
|
|||
`{"compose_id":"`+id.String()+`","image_build_id":0,"manifest":{"sources":{},"pipeline":{}},"targets":[]}`, "created")
|
||||
}
|
||||
|
||||
func testUpdateTransition(t *testing.T, from, to string, expectedStatus int, expectedResponse string) {
|
||||
func testUpdateTransition(t *testing.T, from, to string, expectedStatus int) {
|
||||
distroStruct := fedoratest.New()
|
||||
arch, err := distroStruct.GetArch("x86_64")
|
||||
if err != nil {
|
||||
|
|
@ -92,39 +90,38 @@ func testUpdateTransition(t *testing.T, from, to string, expectedStatus int, exp
|
|||
}
|
||||
}
|
||||
|
||||
test.TestNonJsonRoute(t, api, false, "PATCH", "/job-queue/v1/jobs/"+id.String()+"/builds/0", `{"status":"`+to+`"}`, expectedStatus, expectedResponse)
|
||||
test.TestRoute(t, api, false, "PATCH", "/job-queue/v1/jobs/"+id.String()+"/builds/0", `{"status":"`+to+`"}`, expectedStatus, "{}", "message")
|
||||
}
|
||||
|
||||
func TestUpdate(t *testing.T) {
|
||||
var cases = []struct {
|
||||
From string
|
||||
To string
|
||||
ExpectedStatus int
|
||||
ExpectedResponse string
|
||||
From string
|
||||
To string
|
||||
ExpectedStatus int
|
||||
}{
|
||||
{"VOID", "WAITING", http.StatusNotFound, "compose does not exist"},
|
||||
{"VOID", "RUNNING", http.StatusNotFound, "compose does not exist"},
|
||||
{"VOID", "FINISHED", http.StatusNotFound, "compose does not exist"},
|
||||
{"VOID", "FAILED", http.StatusNotFound, "compose does not exist"},
|
||||
{"WAITING", "WAITING", http.StatusNotFound, "compose has not been popped"},
|
||||
{"WAITING", "RUNNING", http.StatusNotFound, "compose has not been popped"},
|
||||
{"WAITING", "FINISHED", http.StatusNotFound, "compose has not been popped"},
|
||||
{"WAITING", "FAILED", http.StatusNotFound, "compose has not been popped"},
|
||||
{"RUNNING", "WAITING", http.StatusBadRequest, "invalid state transition: image build cannot be moved into waiting state"},
|
||||
{"RUNNING", "RUNNING", http.StatusOK, ""},
|
||||
{"RUNNING", "FINISHED", http.StatusOK, ""},
|
||||
{"RUNNING", "FAILED", http.StatusOK, ""},
|
||||
{"FINISHED", "WAITING", http.StatusBadRequest, "invalid state transition: image build cannot be moved into waiting state"},
|
||||
{"FINISHED", "RUNNING", http.StatusBadRequest, "invalid state transition: only waiting image build can be transitioned into running state"},
|
||||
{"FINISHED", "FINISHED", http.StatusBadRequest, "invalid state transition: only running image build can be transitioned into finished or failed state"},
|
||||
{"FINISHED", "FAILED", http.StatusBadRequest, "invalid state transition: only running image build can be transitioned into finished or failed state"},
|
||||
{"FAILED", "WAITING", http.StatusBadRequest, "invalid state transition: image build cannot be moved into waiting state"},
|
||||
{"FAILED", "RUNNING", http.StatusBadRequest, "invalid state transition: only waiting image build can be transitioned into running state"},
|
||||
{"FAILED", "FINISHED", http.StatusBadRequest, "invalid state transition: only running image build can be transitioned into finished or failed state"},
|
||||
{"FAILED", "FAILED", http.StatusBadRequest, "invalid state transition: only running image build can be transitioned into finished or failed state"},
|
||||
{"VOID", "WAITING", http.StatusNotFound},
|
||||
{"VOID", "RUNNING", http.StatusNotFound},
|
||||
{"VOID", "FINISHED", http.StatusNotFound},
|
||||
{"VOID", "FAILED", http.StatusNotFound},
|
||||
{"WAITING", "WAITING", http.StatusNotFound},
|
||||
{"WAITING", "RUNNING", http.StatusNotFound},
|
||||
{"WAITING", "FINISHED", http.StatusNotFound},
|
||||
{"WAITING", "FAILED", http.StatusNotFound},
|
||||
{"RUNNING", "WAITING", http.StatusBadRequest},
|
||||
{"RUNNING", "RUNNING", http.StatusOK},
|
||||
{"RUNNING", "FINISHED", http.StatusOK},
|
||||
{"RUNNING", "FAILED", http.StatusOK},
|
||||
{"FINISHED", "WAITING", http.StatusBadRequest},
|
||||
{"FINISHED", "RUNNING", http.StatusBadRequest},
|
||||
{"FINISHED", "FINISHED", http.StatusBadRequest},
|
||||
{"FINISHED", "FAILED", http.StatusBadRequest},
|
||||
{"FAILED", "WAITING", http.StatusBadRequest},
|
||||
{"FAILED", "RUNNING", http.StatusBadRequest},
|
||||
{"FAILED", "FINISHED", http.StatusBadRequest},
|
||||
{"FAILED", "FAILED", http.StatusBadRequest},
|
||||
}
|
||||
|
||||
for _, c := range cases {
|
||||
testUpdateTransition(t, c.From, c.To, c.ExpectedStatus, c.ExpectedResponse)
|
||||
testUpdateTransition(t, c.From, c.To, c.ExpectedStatus)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -8,7 +8,6 @@ import (
|
|||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"io/ioutil"
|
||||
"net"
|
||||
"net/http"
|
||||
|
||||
|
|
@ -74,9 +73,9 @@ func (c *Client) AddJob() (*Job, error) {
|
|||
defer response.Body.Close()
|
||||
|
||||
if response.StatusCode != http.StatusCreated {
|
||||
rawR, _ := ioutil.ReadAll(response.Body)
|
||||
r := string(rawR)
|
||||
return nil, fmt.Errorf("couldn't create job, got %d: %s", response.StatusCode, r)
|
||||
var er errorResponse
|
||||
_ = json.NewDecoder(response.Body).Decode(&er)
|
||||
return nil, fmt.Errorf("couldn't create job, got %d: %s", response.StatusCode, er.Message)
|
||||
}
|
||||
|
||||
var jr addJobResponse
|
||||
|
|
|
|||
|
|
@ -8,6 +8,10 @@ import (
|
|||
"github.com/osbuild/osbuild-composer/internal/target"
|
||||
)
|
||||
|
||||
type errorResponse struct {
|
||||
Message string `json:"message"`
|
||||
}
|
||||
|
||||
type addJobRequest struct {
|
||||
}
|
||||
|
||||
|
|
@ -22,3 +26,6 @@ type updateJobRequest struct {
|
|||
Status common.ImageBuildState `json:"status"`
|
||||
Result *common.ComposeResult `json:"result"`
|
||||
}
|
||||
|
||||
type updateJobResponse struct {
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue