diff --git a/scripts/docker-integration-tests/coordinator_noop/test.sh b/scripts/docker-integration-tests/coordinator_noop/test.sh index e6e244b11a..9a3a746c4f 100755 --- a/scripts/docker-integration-tests/coordinator_noop/test.sh +++ b/scripts/docker-integration-tests/coordinator_noop/test.sh @@ -43,7 +43,7 @@ if ! curl -vvvsSf localhost:7201/api/v1/services/m3coordinator/placement; then exit 1 fi -QUERY_EXP='{"error":"operation not valid for noop client"}' +QUERY_EXP='{"status":"error","error":"operation not valid for noop client"}' RES=$(curl "localhost:7201/m3query/api/v1/query_range?start=$(date '+%s')&end=$(date '+%s')&step=10&query=foo") if [[ "$RES" != "$QUERY_EXP" ]]; then echo "Expected resp '$QUERY_EXP', GOT '$RES'" diff --git a/src/cmd/tools/dtest/docker/harness/query_api_test.go b/src/cmd/tools/dtest/docker/harness/query_api_test.go index 5ef751b2ec..9ec0907219 100644 --- a/src/cmd/tools/dtest/docker/harness/query_api_test.go +++ b/src/cmd/tools/dtest/docker/harness/query_api_test.go @@ -21,6 +21,7 @@ package harness import ( + "encoding/json" "fmt" "strings" "testing" @@ -145,6 +146,24 @@ func verifyResponse(expectedStatus int) resources.ResponseVerifier { return fmt.Errorf("expected json content type, got %v", contentType) } + errorResponse := struct { + Status string `json:"status,omitempty"` + Error string `json:"error,omitempty"` + }{} + + err = json.Unmarshal([]byte(resp), &errorResponse) + if err != nil { + return fmt.Errorf("failed unmarshalling response: %w", err) + } + + if errorResponse.Status != "error" { + return fmt.Errorf("expected body to contain status 'error', got %v", errorResponse.Status) + } + + if errorResponse.Error == "" { + return fmt.Errorf("expected body to contain error message") + } + return nil } } diff --git a/src/query/api/v1/handler/database/create_test.go b/src/query/api/v1/handler/database/create_test.go index a313faf858..7c8f76add1 100644 --- a/src/query/api/v1/handler/database/create_test.go +++ b/src/query/api/v1/handler/database/create_test.go @@ -1132,7 +1132,8 @@ func TestClusterTypeMissingHostnames(t *testing.T) { assert.Equal(t, xtest.MustPrettyJSONMap(t, xjson.Map{ - "error": "missing required field", + "status": "error", + "error": "missing required field", }, ), xtest.MustPrettyJSONString(t, string(body))) @@ -1167,7 +1168,8 @@ func TestBadType(t *testing.T) { assert.Equal(t, xtest.MustPrettyJSONMap(t, xjson.Map{ - "error": "invalid database type", + "status": "error", + "error": "invalid database type", }, ), xtest.MustPrettyJSONString(t, string(body))) diff --git a/src/query/api/v1/handler/namespace/add_test.go b/src/query/api/v1/handler/namespace/add_test.go index 0f26d83596..74f1bf7acd 100644 --- a/src/query/api/v1/handler/namespace/add_test.go +++ b/src/query/api/v1/handler/namespace/add_test.go @@ -100,7 +100,9 @@ func TestNamespaceAddHandler(t *testing.T) { body, err := ioutil.ReadAll(resp.Body) assert.NoError(t, err) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - assert.Equal(t, "{\"error\":\"bad namespace metadata: retention options must be set\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"bad namespace metadata: retention options must be set"}`, + string(body)) // Test good case. Note: there is no way to tell the difference between a boolean // being false and it not being set by a user. diff --git a/src/query/api/v1/handler/namespace/delete_test.go b/src/query/api/v1/handler/namespace/delete_test.go index 08fd5bb1b0..268b9dcbe2 100644 --- a/src/query/api/v1/handler/namespace/delete_test.go +++ b/src/query/api/v1/handler/namespace/delete_test.go @@ -56,7 +56,9 @@ func TestNamespaceDeleteHandlerNotFound(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) assert.Equal(t, http.StatusNotFound, resp.StatusCode) - assert.Equal(t, "{\"error\":\"unable to find a namespace with specified name\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"unable to find a namespace with specified name"}`, + string(body)) } func TestNamespaceDeleteHandlerDeleteAll(t *testing.T) { diff --git a/src/query/api/v1/handler/namespace/schema_test.go b/src/query/api/v1/handler/namespace/schema_test.go index 80165d2e5d..e5ceaeefd6 100644 --- a/src/query/api/v1/handler/namespace/schema_test.go +++ b/src/query/api/v1/handler/namespace/schema_test.go @@ -158,7 +158,7 @@ func TestSchemaDeploy_KVKeyNotFound(t *testing.T) { body, err := ioutil.ReadAll(resp.Body) assert.NoError(t, err) assert.Equal(t, http.StatusNotFound, resp.StatusCode) - assert.Equal(t, "{\"error\":\"namespace is not found\"}\n", string(body)) + assert.JSONEq(t, `{"status":"error","error":"namespace is not found"}`, string(body)) } func TestSchemaDeploy(t *testing.T) { @@ -250,7 +250,7 @@ func TestSchemaDeploy_NamespaceNotFound(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) assert.Equal(t, http.StatusNotFound, resp.StatusCode) - assert.Equal(t, "{\"error\":\"namespace is not found\"}\n", string(body)) + assert.JSONEq(t, `{"status":"error","error":"namespace is not found"}`, string(body)) } func TestSchemaReset(t *testing.T) { diff --git a/src/query/api/v1/handler/namespace/update_test.go b/src/query/api/v1/handler/namespace/update_test.go index 38ba2282ab..30f4aa4261 100644 --- a/src/query/api/v1/handler/namespace/update_test.go +++ b/src/query/api/v1/handler/namespace/update_test.go @@ -103,7 +103,9 @@ func TestNamespaceUpdateHandler(t *testing.T) { body, err := ioutil.ReadAll(resp.Body) assert.NoError(t, err) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - assert.Equal(t, "{\"error\":\"unable to validate update request: update options cannot be empty\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"unable to validate update request: update options cannot be empty"}`, + string(body)) // Test good case. Note: there is no way to tell the difference between a boolean // being false and it not being set by a user. diff --git a/src/query/api/v1/handler/placement/add_test.go b/src/query/api/v1/handler/placement/add_test.go index 2d66ddc6f7..7526fa4236 100644 --- a/src/query/api/v1/handler/placement/add_test.go +++ b/src/query/api/v1/handler/placement/add_test.go @@ -78,7 +78,9 @@ func TestPlacementAddHandler_Force(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) - assert.Equal(t, "{\"error\":\"no new instances found in the valid zone\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"no new instances found in the valid zone"}`, + string(body)) assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) // Test add success @@ -137,7 +139,9 @@ func TestPlacementAddHandler_SafeErr_NoNewInstance(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) - assert.Equal(t, "{\"error\":\"no new instances found in the valid zone\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"no new instances found in the valid zone"}`, + string(body)) }) } @@ -174,8 +178,8 @@ func TestPlacementAddHandler_SafeErr_NotAllAvailable(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - assert.Equal(t, - `{"error":"instances do not have all shards available: [A, B]"}`+"\n", + assert.JSONEq(t, + `{"status":"error","error":"instances do not have all shards available: [A, B]"}`, string(body)) }) } @@ -243,7 +247,7 @@ func TestPlacementAddHandler_SafeOK(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) require.Equal(t, http.StatusInternalServerError, resp.StatusCode) - require.Equal(t, `{"error":"test err"}`+"\n", string(body)) + require.JSONEq(t, `{"status":"error","error":"test err"}`, string(body)) w = httptest.NewRecorder() if serviceName == handleroptions.M3AggregatorServiceName { diff --git a/src/query/api/v1/handler/placement/delete_test.go b/src/query/api/v1/handler/placement/delete_test.go index 01402b4514..b8c5027d88 100644 --- a/src/query/api/v1/handler/placement/delete_test.go +++ b/src/query/api/v1/handler/placement/delete_test.go @@ -134,7 +134,7 @@ func TestPlacementDeleteHandler_Force(t *testing.T) { body, err = ioutil.ReadAll(resp.Body) require.NoError(t, err) require.Equal(t, http.StatusNotFound, resp.StatusCode) - require.Equal(t, "{\"error\":\"instance not found: nope\"}\n", string(body)) + require.JSONEq(t, `{"status":"error","error":"instance not found: nope"}`, string(body)) }) } @@ -267,7 +267,9 @@ func testDeleteHandlerSafe(t *testing.T, serviceName string) { body, err = ioutil.ReadAll(resp.Body) require.NoError(t, err) require.Equal(t, http.StatusBadRequest, resp.StatusCode) - require.Equal(t, `{"error":"instances do not have all shards available: [host2]"}`+"\n", string(body)) + require.JSONEq(t, + `{"status":"error","error":"instances do not have all shards available: [host2]"}`, + string(body)) } // Test OK diff --git a/src/query/api/v1/handler/placement/init_test.go b/src/query/api/v1/handler/placement/init_test.go index 3bec2ce906..eef51e58c5 100644 --- a/src/query/api/v1/handler/placement/init_test.go +++ b/src/query/api/v1/handler/placement/init_test.go @@ -135,7 +135,9 @@ func TestPlacementInitHandler(t *testing.T) { body, err = ioutil.ReadAll(resp.Body) require.NoError(t, err) assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) - assert.Equal(t, "{\"error\":\"unable to build initial placement\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"unable to build initial placement"}`, + string(body)) // Test error response w = httptest.NewRecorder() diff --git a/src/query/api/v1/handler/placement/replace_test.go b/src/query/api/v1/handler/placement/replace_test.go index e45ff85a9d..a4d44ef7a7 100644 --- a/src/query/api/v1/handler/placement/replace_test.go +++ b/src/query/api/v1/handler/placement/replace_test.go @@ -91,7 +91,7 @@ func testPlacementReplaceHandlerForce(t *testing.T, serviceName string) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) - assert.Equal(t, `{"error":"test"}`+"\n", string(body)) + assert.JSONEq(t, `{"status":"error","error":"test"}`, string(body)) assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) w = httptest.NewRecorder() @@ -136,7 +136,9 @@ func testPlacementReplaceHandlerSafeErr(t *testing.T, serviceName string) { assert.Equal(t, http.StatusOK, resp.StatusCode) default: assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - assert.Equal(t, `{"error":"instances do not have all shards available: [A, B]"}`+"\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"instances do not have all shards available: [A, B]"}`, + string(body)) } } diff --git a/src/query/api/v1/handler/prometheus/native/list_tags_test.go b/src/query/api/v1/handler/prometheus/native/list_tags_test.go index 0ecc6dfa04..f33ef27b18 100644 --- a/src/query/api/v1/handler/prometheus/native/list_tags_test.go +++ b/src/query/api/v1/handler/prometheus/native/list_tags_test.go @@ -22,7 +22,6 @@ package native import ( "errors" - "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -179,9 +178,6 @@ func TestListErrorTags(t *testing.T) { r, err := ioutil.ReadAll(body) require.NoError(t, err) - ex := `{"error":"err"}` - // NB: error handler adds a newline to the output. - ex = fmt.Sprintf("%s\n", ex) - require.Equal(t, ex, string(r)) + require.JSONEq(t, `{"status":"error","error":"err"}`, string(r)) } } diff --git a/src/query/api/v1/handler/prometheus/remote/tag_values_test.go b/src/query/api/v1/handler/prometheus/remote/tag_values_test.go index 89a799f39c..120d158fdf 100644 --- a/src/query/api/v1/handler/prometheus/remote/tag_values_test.go +++ b/src/query/api/v1/handler/prometheus/remote/tag_values_test.go @@ -204,6 +204,6 @@ func TestTagValueErrors(t *testing.T) { read, err := ioutil.ReadAll(rr.Body) require.NoError(t, err) - ex := fmt.Sprintf(`{"error":"invalid path with no name present"}%s`, "\n") - assert.Equal(t, ex, string(read)) + ex := `{"status":"error","error":"invalid path with no name present"}` + assert.JSONEq(t, ex, string(read)) } diff --git a/src/query/api/v1/handler/topic/init_test.go b/src/query/api/v1/handler/topic/init_test.go index 78085bec2b..7e33e8d8ad 100644 --- a/src/query/api/v1/handler/topic/init_test.go +++ b/src/query/api/v1/handler/topic/init_test.go @@ -98,5 +98,5 @@ func TestPlacementInitHandler(t *testing.T) { body, err = ioutil.ReadAll(resp.Body) require.NoError(t, err) require.Equal(t, http.StatusInternalServerError, resp.StatusCode) - require.Equal(t, "{\"error\":\"init error\"}\n", string(body)) + require.JSONEq(t, `{"status":"error","error":"init error"}`, string(body)) } diff --git a/src/query/generated/assets/openapi/assets.go b/src/query/generated/assets/openapi/assets.go index 2f4409dffe..4f5e461d9c 100644 --- a/src/query/generated/assets/openapi/assets.go +++ b/src/query/generated/assets/openapi/assets.go @@ -239,7 +239,7 @@ d8dUBmQZxiF/+uI1I7E8TMF6pCyWxDpY7WPA8pmKZsl6ouawOaOS+Sj+BQAA//8by2IcfAIAAA== "/spec.yml": { local: "openapi/spec.yml", - size: 22779, + size: 22816, modtime: 12345, compressed: ` H4sIAAAAAAAC/+xcX2/bOBJ/96fgqvdw+5A4bXp7gN+cOJsaSNMgyS5wuzhgaXIkc1ciVXKYNF3cdz9Q @@ -275,9 +275,9 @@ FK7RbZ4sol8Ss+4Ap7xuumWQuqSNN3CGMCpMVkXya0sNwl+VbOpjH0EEc2wKGkgeKyGxQZmpzirVmuY3 ewhRM8KSEBcUN8bb/WU/7VRvaaw0tkld973D953B5wlf4fR5n7FsEZ6XdTxB7W4eriXOIEVoIJu0HGHu PMcoqxlMm+K3qJvXVCqzbeF0Onx/axUr2wthy9sJ0kbpzSPiTa+n99Px1fS36fWlt7w4/nU8vRqfXV1k V64uxr8uJCper9gLkW5V1tZWRraJVZpBq7Yl94WMb86L1sRe1eXk2gg3SbtWorZdKj7H7xCtEOiDkME0 -O2nvHrbq5UYlF5zitwimrWv0syMrfwzZZce6kq9MSOXJ8Tabo+tm2kguDhpO4FZlblHDQsVo6OWvsNAa -3KKRftb1VjiUqWzBNxzFNLDa2VIuz9F7gtkHlWLrrGhLq8Rjs4vwJQaGwO+SH351SEt6EHMD+oOyehue -/KD224ZSzjUYs2O/84oN7f5DXP0kZ5uS0PagpuJRaOcDhoKO/wcAAP//ae6OF/tYAAA= +O2nvHrbq5UYlF5zitwimrWv0syMrfwzZwTBXOK1pYoeV0kqZyuPlbXZQ183cklwcNBzTrWrhotCFitHQ +y19hoTW4Rbf9rIuycHJT2advOK9poL6zpVyeyPeExQ8qBeBZ0ZZWicdmF+FLDAyB3yW/DuuQljQq5gb0 +B2X1NmT6Qe23V6WcazBmx6boFbve/Ye4+nHPNiWh7WlOxfPSzqcQBR3/DwAA//8beZKQIFkAAA== `, }, diff --git a/src/query/generated/assets/openapi/spec.yml b/src/query/generated/assets/openapi/spec.yml index b7fe173fb4..7a0194e5d9 100644 --- a/src/query/generated/assets/openapi/spec.yml +++ b/src/query/generated/assets/openapi/spec.yml @@ -856,6 +856,8 @@ definitions: GenericError: type: "object" properties: + status: + type: "string" error: type: "string" DatabaseCreateRequest: diff --git a/src/query/util/logging/log_test.go b/src/query/util/logging/log_test.go index 7d0ccca73e..00fab9f63c 100644 --- a/src/query/util/logging/log_test.go +++ b/src/query/util/logging/log_test.go @@ -144,7 +144,7 @@ func TestPanicErrorResponder(t *testing.T) { assert.Equal(t, 500, writer.status) require.Equal(t, 1, len(writer.written)) - assert.Equal(t, "{\"error\":\"caught panic: beef\"}\n", writer.written[0]) + assert.JSONEq(t, `{"status":"error","error":"caught panic: beef"}`, writer.written[0]) assertNoErrorLogs(t, stderr) b, err := ioutil.ReadAll(stdout) @@ -323,7 +323,7 @@ func TestWithResponseTimeAndPanicErrorLoggingFunc(t *testing.T) { assert.Equal(t, 500, writer.status) require.Equal(t, 1, len(writer.written)) - assert.Equal(t, "{\"error\":\"caught panic: err\"}\n", writer.written[0]) + assert.JSONEq(t, `{"status":"error","error":"caught panic: err"}`, writer.written[0]) assertNoErrorLogs(t, stderr) diff --git a/src/x/net/http/errors.go b/src/x/net/http/errors.go index 331e23e4d3..082fce1b5d 100644 --- a/src/x/net/http/errors.go +++ b/src/x/net/http/errors.go @@ -68,7 +68,8 @@ func (e errorWithCode) Code() int { // ErrorResponse is a generic response for an HTTP error. type ErrorResponse struct { - Error string `json:"error"` + Status string `json:"status"` + Error string `json:"error"` } type options struct { @@ -96,7 +97,7 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) { if o.response == nil { w.Header().Set(HeaderContentType, ContentTypeJSON) w.WriteHeader(statusCode) - json.NewEncoder(w).Encode(ErrorResponse{Error: err.Error()}) + json.NewEncoder(w).Encode(ErrorResponse{Status: "error", Error: err.Error()}) //nolint:errcheck } else { w.WriteHeader(statusCode) w.Write(o.response)