QueryService: Return application/json and better errors (#84234)

pkg/registry/apis/query/errors.go

@@ -0,0 +1,79 @@
+package query
+
+import (
+	"errors"
+	"fmt"
+
+	"github.com/grafana/grafana/pkg/util/errutil"
+)
+
+var QueryError = errutil.BadRequest("query.error").MustTemplate(
+	"failed to execute query [{{ .Public.refId }}]: {{ .Error }}",
+	errutil.WithPublic(
+		"failed to execute query [{{ .Public.refId }}]: {{ .Public.error }}",
+	))
+
+func MakeQueryError(refID, err error) error {
+	var pErr error
+	var utilErr errutil.Error
+	// See if this is grafana error, if so, grab public message
+	if errors.As(err, &utilErr) {
+		pErr = utilErr.Public()
+	} else {
+		pErr = err
+	}
+
+	data := errutil.TemplateData{
+		Public: map[string]any{
+			"refId": refID,
+			"error": pErr.Error(),
+		},
+		Error: err,
+	}
+
+	return QueryError.Build(data)
+}
+
+func MakePublicQueryError(refID, err string) error {
+	data := errutil.TemplateData{
+		Public: map[string]any{
+			"refId": refID,
+			"error": err,
+		},
+	}
+	return QueryError.Build(data)
+}
+
+var depErrStr = "did not execute expression [{{ .Public.refId }}] due to a failure to of the dependent expression or query [{{.Public.depRefId}}]"
+
+var dependencyError = errutil.BadRequest("sse.dependencyError").MustTemplate(
+	depErrStr,
+	errutil.WithPublic(depErrStr))
+
+func makeDependencyError(refID, depRefID string) error {
+	data := errutil.TemplateData{
+		Public: map[string]interface{}{
+			"refId":    refID,
+			"depRefId": depRefID,
+		},
+		Error: fmt.Errorf("did not execute expression %v due to a failure to of the dependent expression or query %v", refID, depRefID),
+	}
+
+	return dependencyError.Build(data)
+}
+
+var cyclicErrStr = "cyclic reference in expression [{{ .Public.refId }}]"
+
+var cyclicErr = errutil.BadRequest("sse.cyclic").MustTemplate(
+	cyclicErrStr,
+	errutil.WithPublic(cyclicErrStr))
+
+func makeCyclicError(refID string) error {
+	data := errutil.TemplateData{
+		Public: map[string]interface{}{
+			"refId": refID,
+		},
+		Error: fmt.Errorf("cyclic reference in %s", refID),
+	}
+	return cyclicErr.Build(data)
+}

pkg/registry/apis/query/errors_test.go

@@ -0,0 +1,21 @@
+package query_test
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/grafana/pkg/expr"
+	"github.com/grafana/grafana/pkg/util/errutil"
+)
+
+func TestQueryErrorType(t *testing.T) {
+	qet := expr.QueryError
+	utilError := errutil.Error{}
+	qe := expr.MakeQueryError("A", "", fmt.Errorf("not work"))
+
+	require.True(t, errors.Is(qe, qet))
+	require.True(t, errors.As(qe, &utilError))
+}

pkg/registry/apis/query/parser.go

@@ -81,11 +81,11 @@ func (p *queryParser) parseRequest(ctx context.Context, input *query.QueryDataRe
 	for _, q := range input.Queries {
 		_, found := queryRefIDs[q.RefID]
 		if found {
-			return rsp, fmt.Errorf("multiple queries found for refId: %s", q.RefID)
+			return rsp, MakePublicQueryError(q.RefID, "multiple queries with same refId")
 		}
 		_, found = expressions[q.RefID]
 		if found {
-			return rsp, fmt.Errorf("multiple queries found for refId: %s", q.RefID)
+			return rsp, MakePublicQueryError(q.RefID, "multiple queries with same refId")
 		}
 
 		ds, err := p.getValidDataSourceRef(ctx, q.Datasource, q.DatasourceID)
@@ -161,7 +161,7 @@ func (p *queryParser) parseRequest(ctx context.Context, input *query.QueryDataRe
 				if !ok {
 					target, ok = expressions[refId]
 					if !ok {
-						return rsp, fmt.Errorf("expression [%s] is missing variable [%s]", exp.RefID, refId)
+						return rsp, makeDependencyError(exp.RefID, refId)
 					}
 				}
 				// Do not hide queries used in variables
@@ -169,7 +169,7 @@ func (p *queryParser) parseRequest(ctx context.Context, input *query.QueryDataRe
 					q.Hide = false
 				}
 				if target.ID() == exp.ID() {
-					return rsp, fmt.Errorf("expression [%s] can not depend on itself", exp.RefID)
+					return rsp, makeCyclicError(refId)
 				}
 				dg.SetEdge(dg.NewEdge(target, exp))
 			}
@@ -178,7 +178,7 @@ func (p *queryParser) parseRequest(ctx context.Context, input *query.QueryDataRe
 		// Add the sorted expressions
 		sortedNodes, err := topo.SortStabilized(dg, nil)
 		if err != nil {
-			return rsp, fmt.Errorf("cyclic references in query")
+			return rsp, makeCyclicError("")
 		}
 		for _, v := range sortedNodes {
 			if v.ID() > 0 {

pkg/registry/apis/query/parser_test.go

@@ -75,39 +75,41 @@ func TestQuerySplitting(t *testing.T) {
 				continue
 			}
 
-			fpath := path.Join("testdata", file.Name())
+			t.Run(file.Name(), func(t *testing.T) {
-			// nolint:gosec
+				fpath := path.Join("testdata", file.Name())
-			body, err := os.ReadFile(fpath)
+				// nolint:gosec
-			require.NoError(t, err)
+				body, err := os.ReadFile(fpath)
-			harness := &parserTestObject{}
+				require.NoError(t, err)
-			err = json.Unmarshal(body, harness)
+				harness := &parserTestObject{}
-			require.NoError(t, err)
+				err = json.Unmarshal(body, harness)
+				require.NoError(t, err)
 
-			changed := false
+				changed := false
-			parsed, err := parser.parseRequest(ctx, &harness.Request)
+				parsed, err := parser.parseRequest(ctx, &harness.Request)
-			if err != nil {
-				if !assert.Equal(t, harness.Error, err.Error(), "File %s", file) {
-					changed = true
-				}
-			} else {
-				x, _ := json.Marshal(parsed)
-				y, _ := json.Marshal(harness.Expect)
-				if !assert.JSONEq(t, string(y), string(x), "File %s", file) {
-					changed = true
-				}
-			}
-
-			if changed {
-				harness.Error = ""
-				harness.Expect = parsed
 				if err != nil {
-					harness.Error = err.Error()
+					if !assert.Equal(t, harness.Error, err.Error(), "File %s", file) {
+						changed = true
+					}
+				} else {
+					x, _ := json.Marshal(parsed)
+					y, _ := json.Marshal(harness.Expect)
+					if !assert.JSONEq(t, string(y), string(x), "File %s", file) {
+						changed = true
+					}
 				}
-				jj, err := json.MarshalIndent(harness, "", "  ")
+
-				require.NoError(t, err)
+				if changed {
-				err = os.WriteFile(fpath, jj, 0600)
+					harness.Error = ""
-				require.NoError(t, err)
+					harness.Expect = parsed
-			}
+					if err != nil {
+						harness.Error = err.Error()
+					}
+					jj, err := json.MarshalIndent(harness, "", "  ")
+					require.NoError(t, err)
+					err = os.WriteFile(fpath, jj, 0600)
+					require.NoError(t, err)
+				}
+			})
 		}
 	})
 }

pkg/registry/apis/query/query.go

@@ -46,10 +46,7 @@ func (b *QueryAPIBuilder) doQuery(w http.ResponseWriter, r *http.Request) {
 				errutil.WithPublicMessage(err.Error())), w)
 			return
 		}
-		errhttp.Write(ctx, errutil.BadRequest(
+		errhttp.Write(ctx, err, w)
-			"query.parse",
-			errutil.WithPublicMessage("Error parsing query")).
-			Errorf("error parsing: %w", err), w)
 		return
 	}
 
@@ -63,6 +60,7 @@ func (b *QueryAPIBuilder) doQuery(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(query.GetResponseCode(rsp))
 	_ = json.NewEncoder(w).Encode(rsp)
 }
@@ -112,7 +110,7 @@ func (b *QueryAPIBuilder) handleQuerySingleDatasource(ctx context.Context, req d
 		return &backend.QueryDataResponse{}, nil
 	}
 
-	// headers?
+	// Add user headers... here or in client.QueryData
 	client, err := b.client.GetDataSourceClient(ctx, v0alpha1.DataSourceRef{
 		Type: req.PluginId,
 		UID:  req.UID,
@@ -121,9 +119,8 @@ func (b *QueryAPIBuilder) handleQuerySingleDatasource(ctx context.Context, req d
 		return nil, err
 	}
 
-	// headers?
 	_, rsp, err := client.QueryData(ctx, *req.Request)
-	if err == nil {
+	if err == nil && rsp != nil {
 		for _, q := range req.Request.Queries {
 			if q.ResultAssertions != nil {
 				result, ok := rsp.Responses[q.RefID]

pkg/registry/apis/query/testdata/cyclic-references.json

@@ -25,5 +25,5 @@
     ]
   },
   "expect": {},
-  "error": "cyclic references in query"
+  "error": "[sse.cyclic] cyclic reference in expression []"
 }

pkg/registry/apis/query/testdata/self-reference.json

@@ -16,5 +16,5 @@
     ]
   },
   "expect": {},
-  "error": "expression [A] can not depend on itself"
+  "error": "[sse.cyclic] cyclic reference in expression [A]"
 }

pkg/tests/apis/query/query_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"net/http"
 	"testing"
 
 	"github.com/grafana/grafana-plugin-sdk-go/backend"
@@ -48,50 +49,25 @@ func TestIntegrationSimpleQuery(t *testing.T) {
 			Version: "v0alpha1",
 		})
 
-		q1 := data.DataQuery{
-			CommonQueryProperties: data.CommonQueryProperties{
-				RefID: "X",
-				Datasource: &data.DataSourceRef{
-					Type: "grafana-testdata-datasource",
-					UID:  ds.UID,
-				},
-			},
-		}
-		q1.Set("scenarioId", "csv_content")
-		q1.Set("csvContent", "a\n1")
-
-		q2 := data.DataQuery{
-			CommonQueryProperties: data.CommonQueryProperties{
-				RefID: "Y",
-				Datasource: &data.DataSourceRef{
-					UID: "__expr__",
-				},
-			},
-		}
-		q2.Set("type", "math")
-		q2.Set("expression", "$X + 2")
-
 		body, err := json.Marshal(&data.QueryDataRequest{
 			Queries: []data.DataQuery{
-				q1, q2,
+				data.NewDataQuery(map[string]any{
-				// https://github.com/grafana/grafana-plugin-sdk-go/pull/921
+					"refId": "X",
-				// data.NewDataQuery(map[string]any{
+					"datasource": data.DataSourceRef{
-				// 	"refId": "X",
+						Type: "grafana-testdata-datasource",
-				// 	"datasource": data.DataSourceRef{
+						UID:  ds.UID,
-				// 		Type: "grafana-testdata-datasource",
+					},
-				// 		UID:  ds.UID,
+					"scenarioId": "csv_content",
-				// 	},
+					"csvContent": "a\n1",
-				// 	"scenarioId": "csv_content",
+				}),
-				// 	"csvContent": "a\n1",
+				data.NewDataQuery(map[string]any{
-				// }),
+					"refId": "Y",
-				// data.NewDataQuery(map[string]any{
+					"datasource": data.DataSourceRef{
-				// 	"refId": "Y",
+						UID: "__expr__",
-				// 	"datasource": data.DataSourceRef{
+					},
-				// 		UID: "__expr__",
+					"type":       "math",
-				// 	},
+					"expression": "$X + 2",
-				// 	"type":       "math",
+				}),
-				// 	"expression": "$X + 2",
-				// }),
 			},
 		})
 
@@ -108,6 +84,10 @@ func TestIntegrationSimpleQuery(t *testing.T) {
 
 		require.NoError(t, result.Error())
 
+		contentType := "?"
+		result.ContentType(&contentType)
+		require.Equal(t, "application/json", contentType)
+
 		body, err = result.Raw()
 		require.NoError(t, err)
 		fmt.Printf("OUT: %s", string(body))
@@ -126,4 +106,54 @@ func TestIntegrationSimpleQuery(t *testing.T) {
 		require.Equal(t, int64(1), vX)
 		require.Equal(t, float64(3), vY) // 1 + 2, but always float64
 	})
+
+	t.Run("Gets an error with invalid queries", func(t *testing.T) {
+		client := helper.Org1.Admin.RESTClient(t, &schema.GroupVersion{
+			Group:   "query.grafana.app",
+			Version: "v0alpha1",
+		})
+
+		body, err := json.Marshal(&data.QueryDataRequest{
+			Queries: []data.DataQuery{
+				data.NewDataQuery(map[string]any{
+					"refId": "Y",
+					"datasource": data.DataSourceRef{
+						UID: "__expr__",
+					},
+					"type":       "math",
+					"expression": "$X + 2", // invalid X does not exit
+				}),
+			},
+		})
+		require.NoError(t, err)
+
+		result := client.Post().
+			Namespace("default").
+			Suffix("query").
+			SetHeader("Content-type", "application/json").
+			Body(body).
+			Do(context.Background())
+
+		body, err = result.Raw()
+		//fmt.Printf("OUT: %s", string(body))
+
+		require.Error(t, err, "expecting a 400")
+		require.JSONEq(t, `{
+			"status": "Failure",
+			"metadata": {},
+			"message": "did not execute expression [Y] due to a failure to of the dependent expression or query [X]",
+			"reason": "BadRequest",
+			"details": { "group": "query.grafana.app" },
+			"code": 400,
+			"messageId": "sse.dependencyError",
+			"extra": { "depRefId": "X", "refId": "Y" }
+		  }`, string(body))
+
+		statusCode := -1
+		contentType := "?"
+		result.ContentType(&contentType)
+		result.StatusCode(&statusCode)
+		require.Equal(t, "application/json", contentType)
+		require.Equal(t, http.StatusBadRequest, statusCode)
+	})
 }

pkg/util/errutil/errhttp/writer.go

@@ -7,6 +7,9 @@ import (
 	"net/http"
 	"reflect"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apiserver/pkg/endpoints/request"
+
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/util/errutil"
 )
@@ -20,6 +23,14 @@ type ErrorOptions struct {
 	logger   log.Logger
 }
 
+type k8sError struct {
+	metav1.Status `json:",inline"`
+
+	// Internal values that do not have a clean home in the standard Status object
+	MessageID string         `json:"messageId"`
+	Extra     map[string]any `json:"extra,omitempty"`
+}
+
 // Write writes an error to the provided [http.ResponseWriter] with the
 // appropriate HTTP status and JSON payload from [errutil.Error].
 // Write also logs the provided error to either the "request-errors"
@@ -43,10 +54,49 @@ func Write(ctx context.Context, err error, w http.ResponseWriter, opts ...func(E
 
 	logError(ctx, gErr, opt)
 
+	var rsp any
 	pub := gErr.Public()
 	w.Header().Add("Content-Type", "application/json")
 	w.WriteHeader(pub.StatusCode)
-	err = json.NewEncoder(w).Encode(pub)
+	rsp = pub
+
+	// When running in k8s, this will return a v1 status
+	// Typically, k8s handlers should directly support error negotiation, however
+	// when implementing handlers directly this will maintain compatibility with client-go
+	info, ok := request.RequestInfoFrom(ctx)
+	if ok {
+		status := &k8sError{
+			Status: metav1.Status{
+				Status:  metav1.StatusFailure,
+				Code:    int32(pub.StatusCode),
+				Message: pub.Message,
+				Details: &metav1.StatusDetails{
+					Name:  info.Name,
+					Group: info.APIGroup,
+				},
+			},
+			// Add the internal values into
+			MessageID: pub.MessageID,
+			Extra:     pub.Extra,
+		}
+		switch pub.StatusCode {
+		case 400:
+			status.Reason = metav1.StatusReasonBadRequest
+		case 401:
+			status.Reason = metav1.StatusReasonUnauthorized
+		case 403:
+			status.Reason = metav1.StatusReasonForbidden
+		case 404:
+			status.Reason = metav1.StatusReasonNotFound
+		case 500: // many reasons things could map here
+			status.Reason = metav1.StatusReasonInternalError
+		case 504:
+			status.Reason = metav1.StatusReasonTimeout
+		}
+		rsp = status
+	}
+
+	err = json.NewEncoder(w).Encode(rsp)
 	if err != nil {
 		defaultLogger.FromContext(ctx).Error("error while writing error", "error", err)
 	}

pkg/util/errutil/errhttp/writer_test.go

@@ -7,15 +7,39 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"k8s.io/apiserver/pkg/endpoints/request"
 
 	"github.com/grafana/grafana/pkg/util/errutil"
 )
 
 func TestWrite(t *testing.T) {
-	ctx := context.Background()
+	// Error without k8s context
+	recorder := doError(t, context.Background())
+	assert.Equal(t, http.StatusGatewayTimeout, recorder.Code)
+	assert.JSONEq(t, `{"message": "Timeout", "messageId": "test.thisIsExpected", "statusCode": 504}`, recorder.Body.String())
+
+	// Another request, but within the k8s framework
+	recorder = doError(t, request.WithRequestInfo(context.Background(), &request.RequestInfo{
+		APIGroup: "TestGroup",
+	}))
+	assert.Equal(t, http.StatusGatewayTimeout, recorder.Code)
+	assert.JSONEq(t, `{
+		"status": "Failure",
+		"reason": "Timeout",
+		"metadata": {},
+		"messageId": "test.thisIsExpected",
+		"message": "Timeout",
+		"details": { "group": "TestGroup" },
+		"code": 504
+	  }`, recorder.Body.String())
+}
+
+func doError(t *testing.T, ctx context.Context) *httptest.ResponseRecorder {
+	t.Helper()
+
 	const msgID = "test.thisIsExpected"
 	base := errutil.Timeout(msgID)
-	handler := func(writer http.ResponseWriter, request *http.Request) {
+	handler := func(writer http.ResponseWriter, _ *http.Request) {
 		Write(ctx, base.Errorf("got expected error"), writer)
 	}
 
@@ -23,7 +47,5 @@ func TestWrite(t *testing.T) {
 	recorder := httptest.NewRecorder()
 
 	handler(recorder, req)
-
+	return recorder
-	assert.Equal(t, http.StatusGatewayTimeout, recorder.Code)
-	assert.JSONEq(t, `{"message": "Timeout", "messageId": "test.thisIsExpected", "statusCode": 504}`, recorder.Body.String())
 }