From 7dd1f50a75f07e28e6d60809935f21080c04232f Mon Sep 17 00:00:00 2001 From: Travis Turner Date: Fri, 29 Jun 2018 14:42:52 -0500 Subject: [PATCH] consolidate http errors into a shared response type --- api.go | 10 +- holder.go | 2 +- http/client.go | 4 +- http/error.go | 33 ++++++ http/handler.go | 239 ++++++++++++++++++++++------------------- index.go | 2 +- pilosa.go | 18 ++++ server/handler_test.go | 4 +- 8 files changed, 189 insertions(+), 123 deletions(-) create mode 100644 http/error.go diff --git a/api.go b/api.go index 0f19ef19d..659fdd34f 100644 --- a/api.go +++ b/api.go @@ -205,7 +205,7 @@ func (api *API) Index(ctx context.Context, indexName string) (*Index, error) { index := api.Holder.Index(indexName) if index == nil { - return nil, ErrIndexNotFound + return nil, NotFoundError{ErrIndexNotFound} } return index, nil } @@ -253,7 +253,7 @@ func (api *API) CreateField(ctx context.Context, indexName string, fieldName str // Find index. index := api.Holder.Index(indexName) if index == nil { - return nil, ErrIndexNotFound + return nil, NotFoundError{ErrIndexNotFound} } // Create field. @@ -288,7 +288,7 @@ func (api *API) DeleteField(ctx context.Context, indexName string, fieldName str // Find index. index := api.Holder.Index(indexName) if index == nil { - return ErrIndexNotFound + return NotFoundError{ErrIndexNotFound} } // Delete field from the index. @@ -575,7 +575,7 @@ func (api *API) IndexAttrDiff(ctx context.Context, indexName string, blocks []At // Retrieve index from holder. index := api.Holder.Index(indexName) if index == nil { - return nil, ErrIndexNotFound + return nil, NotFoundError{ErrIndexNotFound} } // Retrieve local blocks. @@ -717,7 +717,7 @@ func (api *API) indexField(indexName string, fieldName string, shard uint64) (*I index := api.Holder.Index(indexName) if index == nil { api.server.logger.Printf("fragment error: index=%s, field=%s, shard=%d, err=%s", indexName, fieldName, shard, ErrIndexNotFound.Error()) - return nil, nil, ErrIndexNotFound + return nil, nil, NotFoundError{ErrIndexNotFound} } // Retrieve field. diff --git a/holder.go b/holder.go index eb6eacd57..2d0e592b9 100644 --- a/holder.go +++ b/holder.go @@ -304,7 +304,7 @@ func (h *Holder) CreateIndex(name string, opt IndexOptions) (*Index, error) { // Ensure index doesn't already exist. if h.indexes[name] != nil { - return nil, ErrIndexExists + return nil, ConflictError{ErrIndexExists} } return h.createIndex(name, opt) } diff --git a/http/client.go b/http/client.go index 345c572c1..67b9135a5 100644 --- a/http/client.go +++ b/http/client.go @@ -328,7 +328,7 @@ func (c *InternalClient) ImportK(ctx context.Context, index, field string, colum func (c *InternalClient) EnsureIndex(ctx context.Context, name string, options pilosa.IndexOptions) error { err := c.CreateIndex(ctx, name, options) - if err == nil || err == pilosa.ErrIndexExists { + if err == nil || errors.Cause(err) == pilosa.ErrIndexExists { return nil } return err @@ -336,7 +336,7 @@ func (c *InternalClient) EnsureIndex(ctx context.Context, name string, options p func (c *InternalClient) EnsureField(ctx context.Context, indexName string, fieldName string) error { err := c.CreateField(ctx, indexName, fieldName) - if err == nil || err == pilosa.ErrFieldExists { + if err == nil || errors.Cause(err) == pilosa.ErrFieldExists { return nil } return err diff --git a/http/error.go b/http/error.go new file mode 100644 index 000000000..777540423 --- /dev/null +++ b/http/error.go @@ -0,0 +1,33 @@ +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package http + +import "bytes" + +// Error defines a standard application error. +type Error struct { + // Machine-readable error code. + Code string `json:"code,omitempty"` + + // Human-readable message. + Message string `json:"message"` +} + +// Error returns the string representation of the error message. +func (e *Error) Error() string { + var buf bytes.Buffer + buf.WriteString(e.Message) + return buf.String() +} diff --git a/http/handler.go b/http/handler.go index 4cc7099ba..425fa9873 100644 --- a/http/handler.go +++ b/http/handler.go @@ -279,6 +279,62 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } +// successResponse is a general success/error struct for http responses. +type successResponse struct { + Success bool `json:"success"` + Error *Error `json:"error,omitempty"` +} + +// check determines success or failure based on the error. +// It also returns the corresponding http status code. +func (r *successResponse) check(err error) (statusCode int) { + if err == nil { + r.Success = true + return + } + + cause := errors.Cause(err) + + // Determine HTTP status code based on the error type. + switch cause.(type) { + case pilosa.BadRequestError: + statusCode = http.StatusBadRequest + case pilosa.ConflictError: + statusCode = http.StatusConflict + case pilosa.NotFoundError: + statusCode = http.StatusNotFound + default: + statusCode = http.StatusInternalServerError + } + + r.Success = false + r.Error = &Error{Message: cause.Error()} + + return +} + +// write sends a response to the http.ResponseWriter based on the success +// status and the error. +func (r *successResponse) write(w http.ResponseWriter, err error) { + // Apply the error and get the status code. + statusCode := r.check(err) + + // Marshal the json response. + msg, err := json.Marshal(r) + if err != nil { + http.Error(w, string(msg), http.StatusInternalServerError) + return + } + + // Write the response. + if statusCode == 0 { + w.Write(msg) + w.Write([]byte("\n")) + } else { + http.Error(w, string(msg), statusCode) + } +} + func (h *Handler) handleHome(w http.ResponseWriter, r *http.Request) { http.Error(w, "Welcome. Pilosa is running. Visit https://www.pilosa.com/docs/ for more information.", http.StatusNotFound) } @@ -498,30 +554,20 @@ func foundItem(items []string, item string) bool { return false } -type postIndexResponse struct{} - // handleDeleteIndex handles DELETE /index request. func (h *Handler) handleDeleteIndex(w http.ResponseWriter, r *http.Request) { if !validHeaderAcceptJSON(r.Header) { http.Error(w, "JSON only acceptable response", http.StatusNotAcceptable) return } + indexName := mux.Vars(r)["index"] + + resp := successResponse{} err := h.API.DeleteIndex(r.Context(), indexName) - if err != nil { - h.Logger.Printf("problem deleting index: %s", err) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - // Encode response. - if err := json.NewEncoder(w).Encode(deleteIndexResponse{}); err != nil { - h.Logger.Printf("response encoding error: %s", err) - } + resp.write(w, err) } -type deleteIndexResponse struct{} - // handlePostIndex handles POST /index request. func (h *Handler) handlePostIndex(w http.ResponseWriter, r *http.Request) { if !validHeaderAcceptJSON(r.Header) { @@ -530,30 +576,23 @@ func (h *Handler) handlePostIndex(w http.ResponseWriter, r *http.Request) { } indexName := mux.Vars(r)["index"] - // Decode request. - var req postIndexRequest - err := json.NewDecoder(r.Body).Decode(&req) - if err == io.EOF { - // If no data was provided (EOF), we still create the index - // with default values. - } else if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } + resp := successResponse{} - _, err = h.API.CreateIndex(r.Context(), indexName, req.Options) - if errors.Cause(err) == pilosa.ErrIndexExists { - http.Error(w, err.Error(), http.StatusConflict) - return - } else if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } + err := func() error { + // Decode request. + var req postIndexRequest + err := json.NewDecoder(r.Body).Decode(&req) + if err == io.EOF { + // If no data was provided (EOF), we still create the index + // with default values. + } else if err != nil { + return err + } + _, err = h.API.CreateIndex(r.Context(), indexName, req.Options) + return err + }() - // Encode response. - if err := json.NewEncoder(w).Encode(postIndexResponse{}); err != nil { - h.Logger.Printf("response encoding error: %s", err) - } + resp.write(w, err) } // handlePostIndexAttrDiff handles POST /index/attr/diff requests. @@ -606,60 +645,51 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { indexName := mux.Vars(r)["index"] fieldName := mux.Vars(r)["field"] - // Decode request. - var req postFieldRequest - dec := json.NewDecoder(r.Body) - dec.DisallowUnknownFields() - err := dec.Decode(&req) - if err == io.EOF { - // If no data was provided (EOF), we still create the field - // with default values. - } else if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } + resp := successResponse{} - // Validate field options. - if err := req.Options.validate(); err != nil { - http.Error(w, err.Error(), http.StatusNotAcceptable) - return - } - - // Convert json options into functional options. - var fos pilosa.FieldOption - switch req.Options.Type { - case pilosa.FieldTypeSet: - fos = pilosa.OptFieldTypeSet(*req.Options.CacheType, *req.Options.CacheSize) - case pilosa.FieldTypeInt: - fos = pilosa.OptFieldTypeInt(*req.Options.Min, *req.Options.Max) - case pilosa.FieldTypeTime: - fos = pilosa.OptFieldTypeTime(*req.Options.TimeQuantum) - } - - _, err = h.API.CreateField(r.Context(), indexName, fieldName, fos) - if err != nil { - switch errors.Cause(err) { - case pilosa.ErrIndexNotFound: - http.Error(w, err.Error(), http.StatusNotFound) - case pilosa.ErrFieldExists: - http.Error(w, err.Error(), http.StatusConflict) - default: - http.Error(w, err.Error(), http.StatusInternalServerError) + err := func() error { + // Decode request. + var req postFieldRequest + dec := json.NewDecoder(r.Body) + dec.DisallowUnknownFields() + err := dec.Decode(&req) + if err == io.EOF { + // If no data was provided (EOF), we still create the field + // with default values. + } else if err != nil { + return err } - return - } - // Encode response. - if err := json.NewEncoder(w).Encode(postFieldResponse{}); err != nil { - h.Logger.Printf("response encoding error: %s", err) - } + + // Validate field options. + if err := req.Options.validate(); err != nil { + return err + } + + // Convert json options into functional options. + var fos pilosa.FieldOption + switch req.Options.Type { + case pilosa.FieldTypeSet: + fos = pilosa.OptFieldTypeSet(*req.Options.CacheType, *req.Options.CacheSize) + case pilosa.FieldTypeInt: + fos = pilosa.OptFieldTypeInt(*req.Options.Min, *req.Options.Max) + case pilosa.FieldTypeTime: + fos = pilosa.OptFieldTypeTime(*req.Options.TimeQuantum) + } + + _, err = h.API.CreateField(r.Context(), indexName, fieldName, fos) + if err != nil { + return err + } + return nil + }() + + resp.write(w, err) } type postFieldRequest struct { Options fieldOptions `json:"options"` } -type postFieldResponse struct{} - // fieldOptions tracks pilosa.FieldOptions. It is made up of pointers to values, // and used for input validation. type fieldOptions struct { @@ -692,35 +722,35 @@ func (o *fieldOptions) validate() error { o.CacheSize = &defaultCacheSize } if o.Min != nil { - return errors.New("min does not apply to field type set") + return pilosa.NewBadRequestError(errors.New("min does not apply to field type set")) } else if o.Max != nil { - return errors.New("max does not apply to field type set") + return pilosa.NewBadRequestError(errors.New("max does not apply to field type set")) } else if o.TimeQuantum != nil { - return errors.New("timeQuantum does not apply to field type set") + return pilosa.NewBadRequestError(errors.New("timeQuantum does not apply to field type set")) } case pilosa.FieldTypeInt: if o.CacheType != nil { - return errors.New("cacheType does not apply to field type int") + return pilosa.NewBadRequestError(errors.New("cacheType does not apply to field type int")) } else if o.CacheSize != nil { - return errors.New("cacheSize does not apply to field type int") + return pilosa.NewBadRequestError(errors.New("cacheSize does not apply to field type int")) } else if o.Min == nil { - return errors.New("min is required for field type int") + return pilosa.NewBadRequestError(errors.New("min is required for field type int")) } else if o.Max == nil { - return errors.New("max is required for field type int") + return pilosa.NewBadRequestError(errors.New("max is required for field type int")) } else if o.TimeQuantum != nil { - return errors.New("timeQuantum does not apply to field type int") + return pilosa.NewBadRequestError(errors.New("timeQuantum does not apply to field type int")) } case pilosa.FieldTypeTime: if o.CacheType != nil { - return errors.New("cacheType does not apply to field type time") + return pilosa.NewBadRequestError(errors.New("cacheType does not apply to field type time")) } else if o.CacheSize != nil { - return errors.New("cacheSize does not apply to field type time") + return pilosa.NewBadRequestError(errors.New("cacheSize does not apply to field type time")) } else if o.Min != nil { - return errors.New("min does not apply to field type time") + return pilosa.NewBadRequestError(errors.New("min does not apply to field type time")) } else if o.Max != nil { - return errors.New("max does not apply to field type time") + return pilosa.NewBadRequestError(errors.New("max does not apply to field type time")) } else if o.TimeQuantum == nil { - return errors.New("timeQuantum is required for field type time") + return pilosa.NewBadRequestError(errors.New("timeQuantum is required for field type time")) } default: return errors.Errorf("invalid field type: %s", o.Type) @@ -738,26 +768,11 @@ func (h *Handler) handleDeleteField(w http.ResponseWriter, r *http.Request) { indexName := mux.Vars(r)["index"] fieldName := mux.Vars(r)["field"] + resp := successResponse{} err := h.API.DeleteField(r.Context(), indexName, fieldName) - if err != nil { - if errors.Cause(err) == pilosa.ErrIndexNotFound { - if err := json.NewEncoder(w).Encode(deleteIndexResponse{}); err != nil { - h.Logger.Printf("response encoding error: %s", err) - } - return - } - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - // Encode response. - if err := json.NewEncoder(w).Encode(deleteFieldResponse{}); err != nil { - h.Logger.Printf("response encoding error: %s", err) - } + resp.write(w, err) } -type deleteFieldResponse struct{} - // handlePostFieldAttrDiff handles POST /field/attr/diff requests. func (h *Handler) handlePostFieldAttrDiff(w http.ResponseWriter, r *http.Request) { if !validHeaderAcceptJSON(r.Header) { diff --git a/index.go b/index.go index ae6141b90..e57afe60e 100644 --- a/index.go +++ b/index.go @@ -276,7 +276,7 @@ func (i *Index) CreateField(name string, opt FieldOptions) (*Field, error) { // Ensure field doesn't already exist. if i.fields[name] != nil { - return nil, ErrFieldExists + return nil, ConflictError{ErrFieldExists} } return i.createField(name, opt) } diff --git a/pilosa.go b/pilosa.go index 9505bf513..b98107558 100644 --- a/pilosa.go +++ b/pilosa.go @@ -78,6 +78,24 @@ type BadRequestError struct { error } +// NewBadRequestError returns err wrapped in a BadRequestError. +func NewBadRequestError(err error) BadRequestError { + return BadRequestError{err} +} + +// ConflictError wraps an error value to signify that a conflict with an +// existing resource occurred such that in an HTTP scenario, http.StatusConflict +// would be returned. +type ConflictError struct { + error +} + +// NotFoundError wraps an error value to signify that a resource was not found +// such that in an HTTP scenario, http.StatusNotFound would be returned. +type NotFoundError struct { + error +} + // Regular expression to validate index and field names. var nameRegexp = regexp.MustCompile(`^[a-z][a-z0-9_-]{0,63}$`) diff --git a/server/handler_test.go b/server/handler_test.go index 169065971..7f5d55be6 100644 --- a/server/handler_test.go +++ b/server/handler_test.go @@ -390,7 +390,7 @@ func TestHandler_Endpoints(t *testing.T) { h.ServeHTTP(w, test.MustNewHTTPRequest("DELETE", "/index/i", strings.NewReader(""))) if w.Code != gohttp.StatusOK { t.Fatalf("unexpected status code: %d, body: %s", w.Code, w.Body.String()) - } else if w.Body.String() != "{}\n" { + } else if w.Body.String() != `{"success":true}`+"\n" { t.Fatalf("unexpected response body: %s", w.Body.String()) } // Verify index is gone. @@ -408,7 +408,7 @@ func TestHandler_Endpoints(t *testing.T) { h.ServeHTTP(w, test.MustNewHTTPRequest("DELETE", "/index/i/field/f1", strings.NewReader(""))) if w.Code != gohttp.StatusOK { t.Fatalf("unexpected status code: %d, body: %s", w.Code, w.Body.String()) - } else if body := w.Body.String(); body != `{}`+"\n" { + } else if body := w.Body.String(); body != `{"success":true}`+"\n" { t.Fatalf("unexpected body: %s", body) } else if f := hldr.Index("i").Field("f1"); f != nil { t.Fatal("expected nil field")