From 81d9fef76a64cba2ad1fefabae9918b54a84ca36 Mon Sep 17 00:00:00 2001 From: Jacob Kozol Date: Mon, 14 Oct 2019 14:39:42 +0200 Subject: [PATCH] api: update api error responses to equal lorax's In order to maintain parity with lorax the api needs to reply with an error message equivalent to that used by lorax. Error messages are now returned inside an error object that contains an id, message, and optional status code. For some routes, there are errors when no url parameters are passed. The httprouter was using named parameters of the form /:param which does not match for empty parameters. Now, it has been updated to use the catch-all parameters of the form /*param. This change allows the case of no parameters. However, parameters will now include a "/" as their first character. This needs to be removed from the string in the route handler. In order to provide the proper error message for /modules/list/, searching for the modules needed to be updated. The requested modules and known packages are iterated over and if there is a match the module is added to the response. Also, the found module is dropped from the list of requested modules. If this list is not empty after searching all of the modules then an error is returned containing the name of the non-existant module. --- internal/weldr/api.go | 272 ++++++++++++++++++++++++++++--------- internal/weldr/api_test.go | 14 +- 2 files changed, 218 insertions(+), 68 deletions(-) diff --git a/internal/weldr/api.go b/internal/weldr/api.go index afec0a695..94046ac5b 100644 --- a/internal/weldr/api.go +++ b/internal/weldr/api.go @@ -46,18 +46,18 @@ func New(repo rpmmd.RepoConfig, packages rpmmd.PackageList, logger *log.Logger, api.router.GET("/api/status", api.statusHandler) api.router.GET("/api/v0/projects/source/list", api.sourceListHandler) - api.router.GET("/api/v0/projects/source/info/:sources", api.sourceInfoHandler) + api.router.GET("/api/v0/projects/source/info/*sources", api.sourceInfoHandler) api.router.GET("/api/v0/modules/list", api.modulesListAllHandler) api.router.GET("/api/v0/modules/list/:modules", api.modulesListHandler) // these are the same, except that modules/info also includes dependencies - api.router.GET("/api/v0/modules/info/:modules", api.modulesInfoHandler) - api.router.GET("/api/v0/projects/info/:modules", api.modulesInfoHandler) + api.router.GET("/api/v0/modules/info/*modules", api.modulesInfoHandler) + api.router.GET("/api/v0/projects/info/*modules", api.modulesInfoHandler) api.router.GET("/api/v0/blueprints/list", api.blueprintsListHandler) - api.router.GET("/api/v0/blueprints/info/:blueprints", api.blueprintsInfoHandler) - api.router.GET("/api/v0/blueprints/depsolve/:blueprints", api.blueprintsDepsolveHandler) + api.router.GET("/api/v0/blueprints/info/*blueprints", api.blueprintsInfoHandler) + api.router.GET("/api/v0/blueprints/depsolve/*blueprints", api.blueprintsDepsolveHandler) api.router.GET("/api/v0/blueprints/diff/:blueprint/:from/:to", api.blueprintsDiffHandler) api.router.POST("/api/v0/blueprints/new", api.blueprintsNewHandler) api.router.POST("/api/v0/blueprints/workspace", api.blueprintsWorkspaceHandler) @@ -70,7 +70,7 @@ func New(repo rpmmd.RepoConfig, packages rpmmd.PackageList, logger *log.Logger, api.router.GET("/api/v0/compose/status/:uuids", api.composeStatusHandler) api.router.GET("/api/v0/compose/finished", api.composeFinishedHandler) api.router.GET("/api/v0/compose/failed", api.composeFailedHandler) - api.router.GET("/api/v0/compose/image/:id", api.composeImageHandler) + api.router.GET("/api/v0/compose/image/:uuid", api.composeImageHandler) return api } @@ -112,10 +112,16 @@ func statusResponseOK(writer http.ResponseWriter) { json.NewEncoder(writer).Encode(reply{true}) } -func statusResponseError(writer http.ResponseWriter, code int, errors ...string) { +type responseError struct { + Code int `json:"code,omitempty"` + ID string `json:"id"` + Msg string `json:"msg"` +} + +func statusResponseError(writer http.ResponseWriter, code int, errors ...responseError) { type reply struct { - Status bool `json:"status"` - Errors []string `json:"errors,omitempty"` + Status bool `json:"status"` + Errors []responseError `json:"errors,omitempty"` } writer.WriteHeader(code) @@ -168,13 +174,29 @@ func (api *API) sourceInfoHandler(writer http.ResponseWriter, request *http.Requ } type reply struct { Sources map[string]sourceConfig `json:"sources"` - Errors []string `json:"errors"` + Errors []responseError `json:"errors"` } - // we only have one repository names := strings.Split(params.ByName("sources"), ",") - if names[0] != api.repo.Id && names[0] != "*" { - statusResponseError(writer, http.StatusBadRequest, "repository not found: "+names[0]) + + if names[0] == "/" { + errors := responseError{ + Code: http.StatusNotFound, + ID: "HTTPError", + Msg: "Not Found", + } + statusResponseError(writer, http.StatusNotFound, errors) + return + } + + // Remove the leading / from the names + sourceName := names[0][1:] + if sourceName != api.repo.Id && sourceName != "*" { + errors := responseError{ + ID: "UnknownSource", + Msg: fmt.Sprintf("%s is not a valid source", sourceName), + } + statusResponseError(writer, http.StatusBadRequest, errors) return } @@ -199,7 +221,7 @@ func (api *API) sourceInfoHandler(writer http.ResponseWriter, request *http.Requ json.NewEncoder(writer).Encode(reply{ Sources: map[string]sourceConfig{cfg.ID: cfg}, - Errors: []string{}, + Errors: []responseError{}, }) } @@ -218,7 +240,11 @@ type modulesListReply struct { func (api *API) modulesListAllHandler(writer http.ResponseWriter, request *http.Request, _ httprouter.Params) { offset, limit, err := parseOffsetAndLimit(request.URL.Query()) if err != nil { - statusResponseError(writer, http.StatusBadRequest, "BadRequest: "+err.Error()) + errors := responseError{ + ID: "BadLimitOrOffset", + Msg: fmt.Sprintf("BadRequest: %s", err.Error()), + } + statusResponseError(writer, http.StatusBadRequest, errors) return } @@ -248,7 +274,12 @@ func (api *API) modulesListHandler(writer http.ResponseWriter, request *http.Req offset, limit, err := parseOffsetAndLimit(request.URL.Query()) if err != nil { - statusResponseError(writer, http.StatusBadRequest, "BadRequest: "+err.Error()) + errors := responseError{ + ID: "BadLimitOrOffset", + Msg: fmt.Sprintf("BadRequest: %s", err.Error()), + } + + statusResponseError(writer, http.StatusBadRequest, errors) return } @@ -261,18 +292,33 @@ func (api *API) modulesListHandler(writer http.ResponseWriter, request *http.Req modules := make([]modulesListModule, 0) total := uint(0) end := offset + limit - - for _, pkg := range api.packages { - for _, name := range names { + for i, name := range names { + for _, pkg := range api.packages { if strings.Contains(pkg.Name, name) { total++ if total > offset && total < end { modules = append(modules, modulesListModule{pkg.Name, "rpm"}) + // this removes names that have been found from the list of names + if len(names) < i - 1 { + names = append(names[:i], names[i+1:]...) + } else { + names = names[:i] + } } } } } + // if a name remains in the list it was not found + if len(names) > 0 { + errors := responseError{ + ID: "UnknownModule", + Msg: fmt.Sprintf("one of the requested modules does not exist: ['%s']", names[0]), + } + statusResponseError(writer, http.StatusBadRequest, errors) + return + } + json.NewEncoder(writer).Encode(modulesListReply{ Total: total, Offset: offset, @@ -312,16 +358,30 @@ func (api *API) modulesInfoHandler(writer http.ResponseWriter, request *http.Req modulesRequested := strings.HasPrefix(request.URL.Path, "/api/v0/modules") names := strings.Split(params.ByName("modules"), ",") - if names[0] == "" { - statusResponseError(writer, http.StatusNotFound) + if names[0] == "/" { + errors := responseError{ + Code: http.StatusNotFound, + ID: "HTTPError", + Msg: "Not Found", + } + statusResponseError(writer, http.StatusNotFound, errors) return } projects := make([]project, 0) - for _, name := range names { + for i, name := range names { + // remove leading / from first name + if i == 0 { + name = name[1:] + } + first, n := api.packages.Search(name) if n == 0 { - statusResponseError(writer, http.StatusNotFound) + errors := responseError{ + ID: "UnknownModule", + Msg: fmt.Sprintf("one of the requested modules does not exist: %s", name), + } + statusResponseError(writer, http.StatusBadRequest, errors) return } @@ -370,7 +430,11 @@ func (api *API) blueprintsListHandler(writer http.ResponseWriter, request *http. offset, limit, err := parseOffsetAndLimit(request.URL.Query()) if err != nil { - statusResponseError(writer, http.StatusBadRequest, "BadRequest: "+err.Error()) + errors := responseError{ + ID: "BadLimitOrOffset", + Msg: fmt.Sprintf("BadRequest: %s", err.Error()), + } + statusResponseError(writer, http.StatusBadRequest, errors) return } @@ -395,22 +459,36 @@ func (api *API) blueprintsInfoHandler(writer http.ResponseWriter, request *http. type reply struct { Blueprints []blueprint.Blueprint `json:"blueprints"` Changes []change `json:"changes"` - Errors []string `json:"errors"` + Errors []responseError `json:"errors"` } names := strings.Split(params.ByName("blueprints"), ",") - if names[0] == "" { - statusResponseError(writer, http.StatusNotFound) + if names[0] == "/" { + errors := responseError{ + Code: http.StatusNotFound, + ID: "HTTPError", + Msg: "Not Found", + } + statusResponseError(writer, http.StatusNotFound, errors) return } blueprints := []blueprint.Blueprint{} changes := []change{} - for _, name := range names { + + for i, name := range names { + // remove leading / from first name + if i == 0 { + name = name[1:] + } var blueprint blueprint.Blueprint var changed bool if !api.store.GetBlueprint(name, &blueprint, &changed) { - statusResponseError(writer, http.StatusNotFound) + errors := responseError{ + ID: "UnknownBlueprint", + Msg: fmt.Sprintf("%s: ", name), + } + statusResponseError(writer, http.StatusBadRequest, errors) return } blueprints = append(blueprints, blueprint) @@ -420,7 +498,7 @@ func (api *API) blueprintsInfoHandler(writer http.ResponseWriter, request *http. json.NewEncoder(writer).Encode(reply{ Blueprints: blueprints, Changes: changes, - Errors: []string{}, + Errors: []responseError{}, }) } @@ -430,21 +508,34 @@ func (api *API) blueprintsDepsolveHandler(writer http.ResponseWriter, request *h Dependencies []rpmmd.PackageSpec `json:"dependencies"` } type reply struct { - Blueprints []entry `json:"blueprints"` - Errors []string `json:"errors"` + Blueprints []entry `json:"blueprints"` + Errors []responseError `json:"errors"` } names := strings.Split(params.ByName("blueprints"), ",") - if names[0] == "" { - statusResponseError(writer, http.StatusNotFound) + if names[0] == "/" { + errors := responseError{ + Code: http.StatusNotFound, + ID: "HTTPError", + Msg: "Not Found", + } + statusResponseError(writer, http.StatusNotFound, errors) return } blueprints := []entry{} - for _, name := range names { + for i, name := range names { + // remove leading / from first name + if i == 0 { + name = name[1:] + } var blueprint blueprint.Blueprint if !api.store.GetBlueprint(name, &blueprint, nil) { - statusResponseError(writer, http.StatusNotFound) + errors := responseError{ + ID: "UnknownBlueprint", + Msg: fmt.Sprintf("%s: blueprint not found", name), + } + statusResponseError(writer, http.StatusBadRequest, errors) return } @@ -466,7 +557,7 @@ func (api *API) blueprintsDepsolveHandler(writer http.ResponseWriter, request *h json.NewEncoder(writer).Encode(reply{ Blueprints: blueprints, - Errors: []string{}, + Errors: []responseError{}, }) } @@ -485,25 +576,43 @@ func (api *API) blueprintsDiffHandler(writer http.ResponseWriter, request *http. } name := params.ByName("blueprint") - if len(name) == 0 { - statusResponseError(writer, http.StatusNotFound, "no blueprint name given") - return - } fromCommit := params.ByName("from") - if len(fromCommit) == 0 || fromCommit != "NEWEST" { - statusResponseError(writer, http.StatusNotFound, "invalid from commit ID given") + toCommit := params.ByName("to") + + if len(name) == 0 || len(fromCommit) == 0 || len(toCommit) == 0 { + errors := responseError{ + Code: http.StatusNotFound, + ID: "HTTPError", + Msg: "Not Found", + } + statusResponseError(writer, http.StatusNotFound, errors) return } - toCommit := params.ByName("to") - if len(toCommit) == 0 || toCommit != "WORKSPACE" { - statusResponseError(writer, http.StatusNotFound, "invalid to commit ID given") + if fromCommit != "NEWEST" { + errors := responseError{ + ID: "UnknownCommit", + Msg: fmt.Sprintf("ggit-error: revspec '%s' not found (-3)", fromCommit), + } + statusResponseError(writer, http.StatusBadRequest, errors) + return + } + if toCommit != "WORKSPACE" { + errors := responseError{ + ID: "UnknownCommit", + Msg: fmt.Sprintf("ggit-error: revspec '%s' not found (-3)", toCommit), + } + statusResponseError(writer, http.StatusBadRequest, errors) return } // Fetch old and new blueprint details from store and return error if not found var oldBlueprint, newBlueprint blueprint.Blueprint if !api.store.GetBlueprintCommitted(name, &oldBlueprint) || !api.store.GetBlueprint(name, &newBlueprint, nil) { - statusResponseError(writer, http.StatusNotFound) + errors := responseError{ + ID: "UnknownBlueprint", + Msg: fmt.Sprintf("Unknown blueprint name: %s", name), + } + statusResponseError(writer, http.StatusNotFound, errors) return } @@ -541,14 +650,22 @@ func (api *API) blueprintsDiffHandler(writer http.ResponseWriter, request *http. func (api *API) blueprintsNewHandler(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, "blueprint must be json") + errors := responseError{ + ID: "BlueprintsError", + Msg: "'blueprint must be json'", + } + statusResponseError(writer, http.StatusBadRequest, errors) return } var blueprint blueprint.Blueprint err := json.NewDecoder(request.Body).Decode(&blueprint) if err != nil { - statusResponseError(writer, http.StatusBadRequest, "invalid blueprint: "+err.Error()) + errors := responseError{ + ID: "BlueprintsError", + Msg: "400 Bad Request: The browser (or proxy) sent a request that this server could not understand.", + } + statusResponseError(writer, http.StatusBadRequest, errors) return } @@ -560,14 +677,22 @@ func (api *API) blueprintsNewHandler(writer http.ResponseWriter, request *http.R func (api *API) blueprintsWorkspaceHandler(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, "blueprint must be json") + errors := responseError{ + ID: "BlueprintsError", + Msg: "'blueprint must be json'", + } + statusResponseError(writer, http.StatusBadRequest, errors) return } var blueprint blueprint.Blueprint err := json.NewDecoder(request.Body).Decode(&blueprint) if err != nil { - statusResponseError(writer, http.StatusBadRequest, "invalid blueprint: "+err.Error()) + errors := responseError{ + ID: "BlueprintsError", + Msg: "400 Bad Request: The browser (or proxy) sent a request that this server could not understand.", + } + statusResponseError(writer, http.StatusBadRequest, errors) return } @@ -602,14 +727,23 @@ func (api *API) composeHandler(writer http.ResponseWriter, httpRequest *http.Req contentType := httpRequest.Header["Content-Type"] if len(contentType) != 1 || contentType[0] != "application/json" { - statusResponseError(writer, http.StatusUnsupportedMediaType, "blueprint must be json") + errors := responseError{ + ID: "MissingPost", + Msg: "blueprint must be json", + } + statusResponseError(writer, http.StatusBadRequest, errors) return } var cr ComposeRequest err := json.NewDecoder(httpRequest.Body).Decode(&cr) if err != nil { - statusResponseError(writer, http.StatusBadRequest, "invalid request format: "+err.Error()) + errors := responseError{ + Code: http.StatusNotFound, + ID: "HTTPError", + Msg: "Not Found", + } + statusResponseError(writer, http.StatusNotFound, errors) return } @@ -625,7 +759,11 @@ func (api *API) composeHandler(writer http.ResponseWriter, httpRequest *http.Req if found { api.store.PushCompose(reply.BuildID, &bp, cr.ComposeType) } else { - statusResponseError(writer, http.StatusBadRequest, "blueprint does not exist") + errors := responseError{ + ID: "UnknownBlueprint", + Msg: fmt.Sprintf("Unknown blueprint name: %s", cr.BlueprintName), + } + statusResponseError(writer, http.StatusBadRequest, errors) return } @@ -680,7 +818,11 @@ func (api *API) composeStatusHandler(writer http.ResponseWriter, request *http.R for _, uuidString := range uuidStrings { id, err := uuid.Parse(uuidString) if err != nil { - statusResponseError(writer, http.StatusBadRequest, "invalid UUID") + errors := responseError{ + ID: "UnknownUUID", + Msg: fmt.Sprintf("%s is not a valid build uuid", uuidString), + } + statusResponseError(writer, http.StatusBadRequest, errors) return } uuids = append(uuids, id) @@ -691,16 +833,24 @@ func (api *API) composeStatusHandler(writer http.ResponseWriter, request *http.R } func (api *API) composeImageHandler(writer http.ResponseWriter, request *http.Request, params httprouter.Params) { - idString := params.ByName("id") - id, err := uuid.Parse(idString) + uuidString := params.ByName("uuid") + uuid, err := uuid.Parse(uuidString) if err != nil { - statusResponseError(writer, http.StatusBadRequest, "invalid UUID") + errors := responseError{ + ID: "UnknownUUID", + Msg: fmt.Sprintf("%s is not a valid build uuid", uuidString), + } + statusResponseError(writer, http.StatusBadRequest, errors) return } - image, err := api.store.GetImage(id) + image, err := api.store.GetImage(uuid) if err != nil { - statusResponseError(writer, http.StatusNotFound, "image for compose not found") + errors := responseError{ + ID: "BuildMissingFile", + Msg: fmt.Sprintf("Build %s is missing image file %s", uuidString, image.Name), + } + statusResponseError(writer, http.StatusBadRequest, errors) return } @@ -710,7 +860,7 @@ func (api *API) composeImageHandler(writer http.ResponseWriter, request *http.Re return } - writer.Header().Set("Content-Disposition", "attachment; filename="+id.String()+"-"+image.Name) + writer.Header().Set("Content-Disposition", "attachment; filename="+uuid.String()+"-"+image.Name) writer.Header().Set("Content-Type", image.Mime) writer.Header().Set("Content-Length", fmt.Sprintf("%d", stat.Size())) diff --git a/internal/weldr/api_test.go b/internal/weldr/api_test.go index 9f034bd55..c11ed1ed0 100644 --- a/internal/weldr/api_test.go +++ b/internal/weldr/api_test.go @@ -128,8 +128,8 @@ func TestBasic(t *testing.T) { {"/api/v0/projects/source/list", http.StatusOK, `{"sources":["test"]}`}, {"/api/v0/projects/source/info", http.StatusNotFound, ``}, - {"/api/v0/projects/source/info/", http.StatusNotFound, ``}, - {"/api/v0/projects/source/info/foo", http.StatusBadRequest, `{"status":false,"errors":["repository not found: foo"]}`}, + {"/api/v0/projects/source/info/", http.StatusNotFound, `{"errors":[{"code":404,"id":"HTTPError","msg":"Not Found"}],"status":false}`}, + {"/api/v0/projects/source/info/foo", http.StatusBadRequest, `{"errors":[{"id":"UnknownSource","msg":"foo is not a valid source"}],"status":false}`}, {"/api/v0/projects/source/info/test", http.StatusOK, `{"sources":{"test":{"id":"test","name":"Test","type":"yum-baseurl","url":"http://example.com/test/os","check_gpg":true,"check_ssl":true,"system":true}},"errors":[]}`}, {"/api/v0/projects/source/info/*", http.StatusOK, `{"sources":{"test":{"id":"test","name":"Test","type":"yum-baseurl","url":"http://example.com/test/os","check_gpg":true,"check_ssl":true,"system":true}},"errors":[]}`}, @@ -139,17 +139,17 @@ func TestBasic(t *testing.T) { {"/api/v0/modules/list?limit=1", http.StatusOK, `{"total":2,"offset":0,"limit":1,"modules":[{"name":"package1","group_type":"rpm"}]}`}, {"/api/v0/modules/list?limit=0", http.StatusOK, `{"total":2,"offset":0,"limit":0,"modules":[]}`}, {"/api/v0/modules/list?offset=10&limit=10", http.StatusOK, `{"total":2,"offset":10,"limit":10,"modules":[]}`}, - {"/api/v0/modules/list/foo", http.StatusOK, `{"total":0,"offset":0,"limit":20,"modules":[]}`}, // returns empty list instead of an error for unknown packages + {"/api/v0/modules/list/foo", http.StatusBadRequest, `{"errors":[{"id":"UnknownModule","msg":"one of the requested modules does not exist: ['foo']"}],"status":false}`}, // returns empty list instead of an error for unknown packages {"/api/v0/modules/list/package2", http.StatusOK, `{"total":1,"offset":0,"limit":20,"modules":[{"name":"package2","group_type":"rpm"}]}`}, {"/api/v0/modules/list/*package2*", http.StatusOK, `{"total":1,"offset":0,"limit":20,"modules":[{"name":"package2","group_type":"rpm"}]}`}, {"/api/v0/modules/list/*package*", http.StatusOK, `{"total":2,"offset":0,"limit":20,"modules":[{"name":"package1","group_type":"rpm"},{"name":"package2","group_type":"rpm"}]}`}, {"/api/v0/modules/info", http.StatusNotFound, ``}, - {"/api/v0/modules/info/", http.StatusNotFound, ``}, + {"/api/v0/modules/info/", http.StatusNotFound, `{"errors":[{"code":404,"id":"HTTPError","msg":"Not Found"}],"status":false}`}, {"/api/v0/blueprints/list", http.StatusOK, `{"total":0,"offset":0,"limit":0,"blueprints":[]}`}, - {"/api/v0/blueprints/info/", http.StatusNotFound, ``}, - {"/api/v0/blueprints/info/foo", http.StatusNotFound, `{"status":false}`}, + {"/api/v0/blueprints/info/", http.StatusNotFound, `{"errors":[{"code":404,"id":"HTTPError","msg":"Not Found"}],"status":false}`}, + {"/api/v0/blueprints/info/foo", http.StatusBadRequest, `{"errors":[{"id":"UnknownBlueprint","msg":"foo: "}],"status":false}`}, } for _, c := range cases { @@ -192,7 +192,7 @@ func TestCompose(t *testing.T) { http.StatusOK, `{"status":true}`) testRoute(t, api, "POST", "/api/v0/compose", `{"blueprint_name": "http-server","compose_type": "tar","branch": "master"}`, - http.StatusBadRequest, `{"status":false,"errors":["blueprint does not exist"]}`) + http.StatusBadRequest, `{"errors":[{"id":"UnknownBlueprint","msg":"Unknown blueprint name: http-server"}],"status":false}`) testRoute(t, api, "POST", "/api/v0/compose", `{"blueprint_name": "test","compose_type": "tar","branch": "master"}`, http.StatusOK, `*`)