From 9633e34300dda31fbefea5105bada7df5bad5e0b Mon Sep 17 00:00:00 2001 From: Travis Turner Date: Sun, 1 Jul 2018 17:59:18 -0500 Subject: [PATCH] address feedback in PR --- api.go | 16 +++++----- http/error.go | 6 +--- http/handler.go | 79 +++++++++++++++++++++---------------------------- index_test.go | 11 +++++-- pilosa.go | 15 ++++++++++ 5 files changed, 66 insertions(+), 61 deletions(-) diff --git a/api.go b/api.go index 659fdd34f..68fecfd89 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, NotFoundError{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, NotFoundError{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 NotFoundError{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, NotFoundError{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, NotFoundError{ErrIndexNotFound} + return nil, nil, NewNotFoundError(ErrIndexNotFound) } // Retrieve field. diff --git a/http/error.go b/http/error.go index 777540423..90fac3206 100644 --- a/http/error.go +++ b/http/error.go @@ -14,8 +14,6 @@ package http -import "bytes" - // Error defines a standard application error. type Error struct { // Machine-readable error code. @@ -27,7 +25,5 @@ type Error struct { // 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() + return e.Message } diff --git a/http/handler.go b/http/handler.go index 425fa9873..d8067c378 100644 --- a/http/handler.go +++ b/http/handler.go @@ -578,19 +578,14 @@ func (h *Handler) handlePostIndex(w http.ResponseWriter, r *http.Request) { resp := successResponse{} - 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 - }() + // Decode request. + var req postIndexRequest + err := json.NewDecoder(r.Body).Decode(&req) + if err != nil && err != io.EOF { + resp.write(w, err) + return + } + _, err = h.API.CreateIndex(r.Context(), indexName, req.Options) resp.write(w, err) } @@ -647,42 +642,34 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { resp := successResponse{} - 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 - } + // Decode request. + var req postFieldRequest + dec := json.NewDecoder(r.Body) + dec.DisallowUnknownFields() + err := dec.Decode(&req) + if err != nil && err != io.EOF { + resp.write(w, err) + return + } - // Validate field options. - if err := req.Options.validate(); err != nil { - return err - } + // Validate field options. + if err := req.Options.validate(); err != nil { + resp.write(w, err) + 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 { - return err - } - return nil - }() + // 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) resp.write(w, err) } diff --git a/index_test.go b/index_test.go index bfb54c680..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. @@ -196,8 +197,8 @@ func TestIndex_DeleteField(t *testing.T) { // Delete again to make sure it errors. err := index.DeleteField("f") - if err == nil || err.Error() != pilosa.ErrFieldNotFound.Error() { - t.Fatal(err) + if !isNotFoundError(err) { + t.Fatalf("expected 'field not found' error, got: %#v", err) } } @@ -215,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 b98107558..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. @@ -90,12 +95,22 @@ 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}$`)