diff --git a/api.go b/api.go index 5b073d1ae..a5d581b5b 100644 --- a/api.go +++ b/api.go @@ -89,7 +89,7 @@ func (api *API) validate(f apiMethod) error { if _, ok := validAPIMethods[state][f]; ok { return nil } - return ApiMethodNotAllowedError{errors.Errorf("api method %s not allowed in state %s", f, state)} + return NewApiMethodNotAllowedError(errors.Errorf("api method %s not allowed in state %s", f, state)) } // Query parses a PQL query out of the request and executes it. @@ -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, NewNotFoundError(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, NewNotFoundError(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 NewNotFoundError(ErrIndexNotFound) } // Delete field from the index. @@ -416,11 +416,11 @@ func (api *API) FragmentBlockData(ctx context.Context, body io.Reader) ([]byte, reqBytes, err := ioutil.ReadAll(body) if err != nil { - return nil, BadRequestError{errors.Wrap(err, "read body error")} + return nil, NewBadRequestError(errors.Wrap(err, "read body error")) } var req internal.BlockDataRequest if err := proto.Unmarshal(reqBytes, &req); err != nil { - return nil, BadRequestError{errors.Wrap(err, "unmarshal body error")} + return nil, NewBadRequestError(errors.Wrap(err, "unmarshal body error")) } // Retrieve fragment from holder. @@ -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, NewNotFoundError(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, NewNotFoundError(ErrIndexNotFound) } // Retrieve field. diff --git a/holder.go b/holder.go index 5d125b0fd..f050ad896 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, NewConflictError(ErrIndexExists) } return h.createIndex(name, opt) } @@ -371,10 +371,10 @@ func (h *Holder) DeleteIndex(name string) error { h.mu.Lock() defer h.mu.Unlock() - // Ignore if index doesn't exist. + // Confirm index exists. index := h.index(name) if index == nil { - return nil + return NewNotFoundError(ErrIndexNotFound) } // Close index. 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..90fac3206 --- /dev/null +++ b/http/error.go @@ -0,0 +1,29 @@ +// 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 + +// 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 { + return e.Message +} diff --git a/http/handler.go b/http/handler.go index 4cc7099ba..d8067c378 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,18 @@ func (h *Handler) handlePostIndex(w http.ResponseWriter, r *http.Request) { } indexName := mux.Vars(r)["index"] + resp := successResponse{} + // 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) + if err != nil && err != io.EOF { + resp.write(w, err) return } - _, 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 - } - // 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,22 +640,21 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { indexName := mux.Vars(r)["index"] fieldName := mux.Vars(r)["field"] + resp := successResponse{} + // 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) + if err != nil && err != io.EOF { + resp.write(w, err) return } // Validate field options. if err := req.Options.validate(); err != nil { - http.Error(w, err.Error(), http.StatusNotAcceptable) + resp.write(w, err) return } @@ -637,29 +670,13 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { } _, 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) - } - return - } - // Encode response. - if err := json.NewEncoder(w).Encode(postFieldResponse{}); err != nil { - h.Logger.Printf("response encoding error: %s", err) - } + 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 +709,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 +755,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 19e28c760..c1ae5de9e 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, NewConflictError(ErrFieldExists) } return i.createField(name, opt) } @@ -346,10 +346,10 @@ func (i *Index) DeleteField(name string) error { i.mu.Lock() defer i.mu.Unlock() - // Ignore if field doesn't exist. + // Confirm field exists. f := i.field(name) if f == nil { - return nil + return NewNotFoundError(ErrFieldNotFound) } // Close field. diff --git a/index_test.go b/index_test.go index d1a740a4f..5490e2978 100644 --- a/index_test.go +++ b/index_test.go @@ -21,6 +21,7 @@ import ( "github.com/pilosa/pilosa" "github.com/pilosa/pilosa/test" + "github.com/pkg/errors" ) // ShardWidth is a helper reference to use when testing. @@ -194,13 +195,14 @@ func TestIndex_DeleteField(t *testing.T) { t.Fatal("expected nil field") } - // Delete again to make sure it doesn't error. - if err := index.DeleteField("f"); err != nil { - t.Fatal(err) + // Delete again to make sure it errors. + err := index.DeleteField("f") + if !isNotFoundError(err) { + t.Fatalf("expected 'field not found' error, got: %#v", err) } } -// Ensure index can delete a field. +// Ensure index can validate its name. func TestIndex_InvalidName(t *testing.T) { path, err := ioutil.TempDir("", "pilosa-index-") if err != nil { @@ -214,3 +216,9 @@ func TestIndex_InvalidName(t *testing.T) { t.Fatalf("unexpected index name %v", index) } } + +func isNotFoundError(err error) bool { + root := errors.Cause(err) + _, ok := root.(pilosa.NotFoundError) + return ok +} diff --git a/pilosa.go b/pilosa.go index 9505bf513..ebc2be438 100644 --- a/pilosa.go +++ b/pilosa.go @@ -71,6 +71,11 @@ type ApiMethodNotAllowedError struct { error } +// NewApiMethodNotAllowedError returns err wrapped in an ApiMethodNotAllowedError. +func NewApiMethodNotAllowedError(err error) ApiMethodNotAllowedError { + return ApiMethodNotAllowedError{err} +} + // BadRequestError wraps an error value to signify that a request could not be // read, decoded, or parsed such that in an HTTP scenario, http.StatusBadRequest // would be returned. @@ -78,6 +83,34 @@ 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 +} + +// NewConflictError returns err wrapped in a ConflictError. +func NewConflictError(err error) ConflictError { + return ConflictError{err} +} + +// 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 +} + +// NewNotFoundError returns err wrapped in a NotFoundError. +func NewNotFoundError(err error) NotFoundError { + return NotFoundError{err} +} + // 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..00326059e 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") @@ -579,6 +579,88 @@ func TestHandler_Endpoints(t *testing.T) { t.Fatal("CORS header not present") } }) + + t.Run("index handlers", func(t *testing.T) { + // create index + w := httptest.NewRecorder() + r := test.MustNewHTTPRequest("POST", "/index/idx1", strings.NewReader("")) + h.ServeHTTP(w, r) + if w.Code != gohttp.StatusOK { + t.Fatalf("unexpected status code: %d", w.Code) + } else if w.Body.String() != `{"success":true}`+"\n" { + t.Fatalf("unexpected body: %q", w.Body.String()) + } + + // create index again + w = httptest.NewRecorder() + r = test.MustNewHTTPRequest("POST", "/index/idx1", strings.NewReader("")) + h.ServeHTTP(w, r) + if w.Code != gohttp.StatusConflict { + t.Fatalf("unexpected status code: %d", w.Code) + } else if w.Body.String() != `{"success":false,"error":{"message":"index already exists"}}`+"\n" { + t.Fatalf("unexpected body: %q", w.Body.String()) + } + + // create field + w = httptest.NewRecorder() + r = test.MustNewHTTPRequest("POST", "/index/idx1/field/fld1", strings.NewReader("")) + h.ServeHTTP(w, r) + if w.Code != gohttp.StatusOK { + t.Fatalf("unexpected status code: %d", w.Code) + } else if w.Body.String() != `{"success":true}`+"\n" { + t.Fatalf("unexpected body: %q", w.Body.String()) + } + + // create field again + w = httptest.NewRecorder() + r = test.MustNewHTTPRequest("POST", "/index/idx1/field/fld1", strings.NewReader("")) + h.ServeHTTP(w, r) + if w.Code != gohttp.StatusConflict { + t.Fatalf("unexpected status code: %d", w.Code) + } else if w.Body.String() != `{"success":false,"error":{"message":"field already exists"}}`+"\n" { + t.Fatalf("unexpected body: %q", w.Body.String()) + } + + // delete field + w = httptest.NewRecorder() + r = test.MustNewHTTPRequest("DELETE", "/index/idx1/field/fld1", strings.NewReader("")) + h.ServeHTTP(w, r) + if w.Code != gohttp.StatusOK { + t.Fatalf("unexpected status code: %d", w.Code) + } else if w.Body.String() != `{"success":true}`+"\n" { + t.Fatalf("unexpected body: %q", w.Body.String()) + } + + // delete field again + w = httptest.NewRecorder() + r = test.MustNewHTTPRequest("DELETE", "/index/idx1/field/fld1", strings.NewReader("")) + h.ServeHTTP(w, r) + if w.Code != gohttp.StatusNotFound { + t.Fatalf("unexpected status code: %d", w.Code) + } else if w.Body.String() != `{"success":false,"error":{"message":"field not found"}}`+"\n" { + t.Fatalf("unexpected body: %q", w.Body.String()) + } + + // delete index + w = httptest.NewRecorder() + r = test.MustNewHTTPRequest("DELETE", "/index/idx1", strings.NewReader("")) + h.ServeHTTP(w, r) + if w.Code != gohttp.StatusOK { + t.Fatalf("unexpected status code: %d", w.Code) + } else if w.Body.String() != `{"success":true}`+"\n" { + t.Fatalf("unexpected body: %q", w.Body.String()) + } + + // delete index again + w = httptest.NewRecorder() + r = test.MustNewHTTPRequest("DELETE", "/index/idx1", strings.NewReader("")) + h.ServeHTTP(w, r) + if w.Code != gohttp.StatusNotFound { + t.Fatalf("unexpected status code: %d", w.Code) + } else if w.Body.String() != `{"success":false,"error":{"message":"index not found"}}`+"\n" { + t.Fatalf("unexpected body: %q", w.Body.String()) + } + }) } func mustJSONDecode(t *testing.T, r io.Reader) (ret map[string]interface{}) {