[query] Add missing "status" field to PromQL error responses (#2933)

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'"

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
 	}
 }

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)))

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.

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) {

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) {

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.

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 {

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

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()

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))
 	}
 }
 

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))
 	}
 }

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))
 }

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))
 }

src/query/generated/assets/openapi/spec.yml

@@ -856,6 +856,8 @@ definitions:
   GenericError:
     type: "object"
     properties:
+      status:
+        type: "string"
       error:
         type: "string"
   DatabaseCreateRequest:

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)
 

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)