Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default status code for queries is now 200 #2186

Merged
merged 3 commits into from
May 11, 2015
Merged

Default status code for queries is now 200 #2186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4114031
WIP: Make `200 OK` the default status code for queries.
toddboom Apr 7, 2015
f6f66a7
Remove unused test methods.
toddboom Apr 7, 2015
ea5a321
Fix tests to expect 200 response code, handle client errors gracefully.
toddboom Apr 8, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 12 additions & 27 deletions httpd/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -501,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
}

Expand All @@ -532,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)
Expand Down Expand Up @@ -839,19 +837,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()})
Expand Down
30 changes: 15 additions & 15 deletions httpd/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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)
}
}
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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)
}
}
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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')"}`
Expand All @@ -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"}`
Expand Down
12 changes: 12 additions & 0 deletions influxdb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down