From 411403117671558012873d18cf801e5453278428 Mon Sep 17 00:00:00 2001 From: Todd Persen Date: Tue, 7 Apr 2015 13:07:24 -0700 Subject: [PATCH 1/3] WIP: Make `200 OK` the default status code for queries. --- httpd/handler.go | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/httpd/handler.go b/httpd/handler.go index 0d57a1b59c..4707ed31a0 100644 --- a/httpd/handler.go +++ b/httpd/handler.go @@ -243,21 +243,15 @@ func (h *Handler) serveQuery(w http.ResponseWriter, r *http.Request, user *influ for r := range results { // write the status header based on the first result returned in the channel if !statusWritten { + status := http.StatusOK + if r != nil && r.Err != nil { if isAuthorizationError(r.Err) { - w.WriteHeader(http.StatusUnauthorized) - } else if isMeasurementNotFoundError(r.Err) { - w.WriteHeader(http.StatusOK) - } else if isTagNotFoundError(r.Err) { - w.WriteHeader(http.StatusOK) - } else if isFieldNotFoundError(r.Err) { - w.WriteHeader(http.StatusOK) - } else { - w.WriteHeader(http.StatusInternalServerError) + status = http.StatusUnauthorized } - } else { - w.WriteHeader(http.StatusOK) } + + w.WriteHeader(status) statusWritten = true } From f6f66a77977ada4c43908bdf5da88ec28f5f4a9a Mon Sep 17 00:00:00 2001 From: Todd Persen Date: Tue, 7 Apr 2015 13:09:00 -0700 Subject: [PATCH 2/3] Remove unused test methods. --- httpd/handler.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/httpd/handler.go b/httpd/handler.go index 4707ed31a0..58efd9679e 100644 --- a/httpd/handler.go +++ b/httpd/handler.go @@ -833,19 +833,6 @@ func isAuthorizationError(err error) bool { return ok } -func isMeasurementNotFoundError(err error) bool { - s := err.Error() - return strings.HasPrefix(s, "measurement") && strings.HasSuffix(s, "not found") || strings.Contains(s, "measurement not found") -} - -func isTagNotFoundError(err error) bool { - return (strings.HasPrefix(err.Error(), "unknown field or tag name")) -} - -func isFieldNotFoundError(err error) bool { - return (strings.HasPrefix(err.Error(), "field not found")) -} - // mapError writes an error result after trying to start a mapper func mapError(w http.ResponseWriter, err error) { b, _ := json.Marshal(&influxdb.MapResponse{Err: err.Error()}) From ea5a321cf4fdf4849ff862ef63c04b55e7438de0 Mon Sep 17 00:00:00 2001 From: Todd Persen Date: Tue, 7 Apr 2015 17:14:43 -0700 Subject: [PATCH 3/3] Fix tests to expect 200 response code, handle client errors gracefully. --- httpd/handler.go | 10 +++++++--- httpd/handler_test.go | 30 +++++++++++++++--------------- influxdb.go | 12 ++++++++++++ 3 files changed, 34 insertions(+), 18 deletions(-) diff --git a/httpd/handler.go b/httpd/handler.go index 58efd9679e..d43e1654c7 100644 --- a/httpd/handler.go +++ b/httpd/handler.go @@ -495,12 +495,12 @@ func (h *Handler) serveWrite(w http.ResponseWriter, r *http.Request, user *influ w.WriteHeader(http.StatusOK) return } - writeError(influxdb.Result{Err: err}, http.StatusInternalServerError) + writeError(influxdb.Result{Err: err}, http.StatusBadRequest) return } if bp.Database == "" { - writeError(influxdb.Result{Err: fmt.Errorf("database is required")}, http.StatusInternalServerError) + writeError(influxdb.Result{Err: fmt.Errorf("database is required")}, http.StatusBadRequest) return } @@ -526,7 +526,11 @@ func (h *Handler) serveWrite(w http.ResponseWriter, r *http.Request, user *influ } if index, err := h.server.WriteSeries(bp.Database, bp.RetentionPolicy, points); err != nil { - writeError(influxdb.Result{Err: err}, http.StatusInternalServerError) + if influxdb.IsClientError(err) { + writeError(influxdb.Result{Err: err}, http.StatusBadRequest) + } else { + writeError(influxdb.Result{Err: err}, http.StatusInternalServerError) + } return } else { w.WriteHeader(http.StatusOK) diff --git a/httpd/handler_test.go b/httpd/handler_test.go index 9954b70542..63da23e82f 100644 --- a/httpd/handler_test.go +++ b/httpd/handler_test.go @@ -224,7 +224,7 @@ func TestHandler_CreateDatabase_Conflict(t *testing.T) { defer s.Close() status, body := MustHTTP("GET", s.URL+`/query`, map[string]string{"q": "CREATE DATABASE foo"}, nil, "") - if status != http.StatusInternalServerError { + if status != http.StatusOK { t.Fatalf("unexpected status: %d", status) } else if body != `{"results":[{"error":"database exists"}]}` { t.Fatalf("unexpected body: %s", body) @@ -255,7 +255,7 @@ func TestHandler_DropDatabase_NotFound(t *testing.T) { defer s.Close() status, body := MustHTTP("GET", s.URL+`/query`, map[string]string{"q": "DROP DATABASE bar"}, nil, "") - if status != http.StatusInternalServerError { + if status != http.StatusOK { t.Fatalf("unexpected status: %d", status) } else if !matchRegex(`database not found: bar.*`, body) { t.Fatalf("unexpected body: %s", body) @@ -289,7 +289,7 @@ func TestHandler_RetentionPolicies_DatabaseNotFound(t *testing.T) { status, body := MustHTTP("GET", s.URL+`/query`, map[string]string{"q": "SHOW RETENTION POLICIES foo"}, nil, "") - if status != http.StatusInternalServerError { + if status != http.StatusOK { t.Fatalf("unexpected status: %d", status) } else if !matchRegex(`database not found: foo.*`, body) { t.Fatalf("unexpected body: %s", body) @@ -349,7 +349,7 @@ func TestHandler_CreateRetentionPolicy_DatabaseNotFound(t *testing.T) { query := map[string]string{"q": "CREATE RETENTION POLICY bar ON foo DURATION 1h REPLICATION 1"} status, _ := MustHTTP("GET", s.URL+`/query`, query, nil, "") - if status != http.StatusInternalServerError { + if status != http.StatusOK { t.Fatalf("unexpected status: %d", status) } } @@ -367,7 +367,7 @@ func TestHandler_CreateRetentionPolicy_Conflict(t *testing.T) { status, _ := MustHTTP("GET", s.URL+`/query`, query, nil, "") - if status != http.StatusInternalServerError { + if status != http.StatusOK { t.Fatalf("unexpected status: %d", status) } } @@ -449,7 +449,7 @@ func TestHandler_UpdateRetentionPolicy_DatabaseNotFound(t *testing.T) { status, _ := MustHTTP("GET", s.URL+`/query`, query, nil, "") // Verify response. - if status != http.StatusInternalServerError { + if status != http.StatusOK { t.Fatalf("unexpected status: %d", status) } } @@ -467,7 +467,7 @@ func TestHandler_UpdateRetentionPolicy_NotFound(t *testing.T) { status, _ := MustHTTP("GET", s.URL+`/query`, query, nil, "") // Verify response. - if status != http.StatusInternalServerError { + if status != http.StatusOK { t.Fatalf("unexpected status: %d", status) } } @@ -501,7 +501,7 @@ func TestHandler_DeleteRetentionPolicy_DatabaseNotFound(t *testing.T) { query := map[string]string{"q": "DROP RETENTION POLICY bar ON qux"} status, body := MustHTTP("GET", s.URL+`/query`, query, nil, "") - if status != http.StatusInternalServerError { + if status != http.StatusOK { t.Fatalf("unexpected status: %d", status) } else if !matchRegex(`database not found: .*qux.*`, body) { t.Fatalf("unexpected body: %s", body) @@ -519,7 +519,7 @@ func TestHandler_DeleteRetentionPolicy_NotFound(t *testing.T) { query := map[string]string{"q": "DROP RETENTION POLICY bar ON foo"} status, body := MustHTTP("GET", s.URL+`/query`, query, nil, "") - if status != http.StatusInternalServerError { + if status != http.StatusOK { t.Fatalf("unexpected status: %d", status) } else if body != `{"results":[{"error":"retention policy not found"}]}` { t.Fatalf("unexpected body: %s", body) @@ -821,7 +821,7 @@ func TestHandler_CreateDataNode_InternalServerError(t *testing.T) { defer s.Close() status, body := MustHTTP("POST", s.URL+`/data/data_nodes`, nil, nil, `{"url":""}`) - if status != http.StatusInternalServerError { + if status != http.StatusOK { t.Fatalf("unexpected status: %d, %s", status, body) } else if body != `data node url required` { t.Fatalf("unexpected body: %s", body) @@ -1177,7 +1177,7 @@ func TestHandler_serveWriteSeriesWithNoFields(t *testing.T) { expected := fmt.Sprintf(`{"error":"%s"}`, influxdb.ErrFieldsRequired.Error()) - if status != http.StatusInternalServerError { + if status != http.StatusBadRequest { t.Fatalf("unexpected status: %d", status) } else if body != expected { t.Fatalf("result mismatch:\n\texp=%s\n\tgot=%s\n", expected, body) @@ -1337,8 +1337,8 @@ func TestHandler_serveWriteSeries_invalidJSON(t *testing.T) { status, body := MustHTTP("POST", s.URL+`/write`, nil, nil, `{"database" : foo", "retentionPolicy" : "bar", "points": [{"name": "cpu", "tags": {"host": "server01"},"timestamp": "2009-11-10T23:00:00Z","fields": {"value": 100}}]}`) - if status != http.StatusInternalServerError { - t.Fatalf("unexpected status: expected: %d, actual: %d", http.StatusInternalServerError, status) + if status != http.StatusBadRequest { + t.Fatalf("unexpected status: expected: %d, actual: %d", http.StatusBadRequest, status) } response := `{"error":"invalid character 'o' in literal false (expecting 'a')"}` @@ -1356,8 +1356,8 @@ func TestHandler_serveWriteSeries_noDatabaseSpecified(t *testing.T) { status, body := MustHTTP("POST", s.URL+`/write`, nil, nil, `{}`) - if status != http.StatusInternalServerError { - t.Fatalf("unexpected status: expected: %d, actual: %d", http.StatusInternalServerError, status) + if status != http.StatusBadRequest { + t.Fatalf("unexpected status: expected: %d, actual: %d", http.StatusBadRequest, status) } response := `{"error":"database is required"}` diff --git a/influxdb.go b/influxdb.go index e076254636..5c5eec279c 100644 --- a/influxdb.go +++ b/influxdb.go @@ -189,6 +189,18 @@ func isAuthorizationError(err error) bool { return ok } +// IsClientError indicates whether an error is a known client error. +func IsClientError(err error) bool { + if err == ErrFieldsRequired { + return true + } + if err == ErrFieldTypeConflict { + return true + } + + return false +} + // mustMarshal encodes a value to JSON. // This will panic if an error occurs. This should only be used internally when // an invalid marshal will cause corruption and a panic is appropriate.