Plugins: Use error plane for api/ds/query (#54750)

* plugin client returns error base

* fix api test

* add plugin client test

* add fallback err

* fix linting

* wip

* replace bad query

* template is an error

* failing test of templated error

* add one test passing

* fix failing test

* move test

* rename ErrBadQuery to ErrQueryValidationFailure

* tidy diff

* Change to one error per specific error kind

* last err + fix test

* fix imports

* more tests

* keep req vars together

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

pkg/api/metrics.go

@@ -10,11 +10,8 @@ import (
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/response"
 	"github.com/grafana/grafana/pkg/models"
-	"github.com/grafana/grafana/pkg/plugins/backendplugin"
 	"github.com/grafana/grafana/pkg/services/datasources"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
-	"github.com/grafana/grafana/pkg/services/query"
-	"github.com/grafana/grafana/pkg/util"
 	"github.com/grafana/grafana/pkg/web"
 )
 
@@ -31,16 +28,7 @@ func (hs *HTTPServer) handleQueryMetricsError(err error) *response.NormalRespons
 		return response.Error(http.StatusInternalServerError, fmt.Sprint("Secrets Plugin error: ", err.Error()), err)
 	}
 
-	var badQuery query.ErrBadQuery
-	if errors.As(err, &badQuery) {
-		return response.Error(http.StatusBadRequest, util.Capitalize(badQuery.Message), err)
-	}
-
-	if errors.Is(err, backendplugin.ErrPluginNotRegistered) {
-		return response.Error(http.StatusNotFound, "Plugin not found", err)
-	}
-
-	return response.Error(http.StatusInternalServerError, "Query data error", err)
+	return response.ErrOrFallback(http.StatusInternalServerError, "Query data error", err)
 }
 
 // QueryMetricsV2 returns query metrics.

pkg/api/metrics_test.go

@@ -4,42 +4,32 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"io"
 	"net/http"
 	"strings"
 	"testing"
 
 	"github.com/grafana/grafana-plugin-sdk-go/backend"
 	"github.com/stretchr/testify/require"
-
-	"github.com/grafana/grafana/pkg/services/datasources"
-	"github.com/grafana/grafana/pkg/services/featuremgmt"
-	"github.com/grafana/grafana/pkg/services/org"
-	"github.com/grafana/grafana/pkg/services/quota/quotatest"
-	"github.com/grafana/grafana/pkg/services/user"
-	"github.com/grafana/grafana/pkg/web/webtest"
-
 	"golang.org/x/oauth2"
 
+	"github.com/grafana/grafana/pkg/plugins"
+	"github.com/grafana/grafana/pkg/plugins/backendplugin"
+	pluginClient "github.com/grafana/grafana/pkg/plugins/manager/client"
+	"github.com/grafana/grafana/pkg/plugins/manager/registry"
+	"github.com/grafana/grafana/pkg/services/datasources"
 	fakeDatasources "github.com/grafana/grafana/pkg/services/datasources/fakes"
+	"github.com/grafana/grafana/pkg/services/featuremgmt"
+	"github.com/grafana/grafana/pkg/services/org"
 	"github.com/grafana/grafana/pkg/services/query"
+	"github.com/grafana/grafana/pkg/services/quota/quotatest"
+	"github.com/grafana/grafana/pkg/services/user"
+	"github.com/grafana/grafana/pkg/util/errutil"
+	"github.com/grafana/grafana/pkg/web/webtest"
 )
 
-var queryDatasourceInput = `{
-"from": "",
-		"to": "",
-		"queries": [
-			{
-				"datasource": {
-					"type": "datasource",
-					"uid": "grafana"
-				},
-				"queryType": "randomWalk",
-				"refId": "A"
-			}
-		]
-	}`
-
 type fakePluginRequestValidator struct {
 	err error
 }
@@ -98,7 +88,7 @@ func TestAPIEndpoint_Metrics_QueryMetricsV2(t *testing.T) {
 	})
 
 	t.Run("Status code is 400 when data source response has an error and feature toggle is disabled", func(t *testing.T) {
-		req := serverFeatureDisabled.NewPostRequest("/api/ds/query", strings.NewReader(queryDatasourceInput))
+		req := serverFeatureDisabled.NewPostRequest("/api/ds/query", strings.NewReader(reqValid))
 		webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, OrgRole: org.RoleViewer})
 		resp, err := serverFeatureDisabled.SendJSON(req)
 		require.NoError(t, err)
@@ -107,7 +97,7 @@ func TestAPIEndpoint_Metrics_QueryMetricsV2(t *testing.T) {
 	})
 
 	t.Run("Status code is 207 when data source response has an error and feature toggle is enabled", func(t *testing.T) {
-		req := serverFeatureEnabled.NewPostRequest("/api/ds/query", strings.NewReader(queryDatasourceInput))
+		req := serverFeatureEnabled.NewPostRequest("/api/ds/query", strings.NewReader(reqValid))
 		webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, OrgRole: org.RoleViewer})
 		resp, err := serverFeatureEnabled.SendJSON(req)
 		require.NoError(t, err)
@@ -141,7 +131,7 @@ func TestAPIEndpoint_Metrics_PluginDecryptionFailure(t *testing.T) {
 	})
 
 	t.Run("Status code is 500 and a secrets plugin error is returned if there is a problem getting secrets from the remote plugin", func(t *testing.T) {
-		req := httpServer.NewPostRequest("/api/ds/query", strings.NewReader(queryDatasourceInput))
+		req := httpServer.NewPostRequest("/api/ds/query", strings.NewReader(reqValid))
 		webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, OrgRole: org.RoleViewer})
 		resp, err := httpServer.SendJSON(req)
 		require.NoError(t, err)
@@ -157,3 +147,168 @@ func TestAPIEndpoint_Metrics_PluginDecryptionFailure(t *testing.T) {
 		require.Contains(t, resObj.Message, "Secrets Plugin error:")
 	})
 }
+
+var reqValid = `{
+	"from": "",
+	"to": "",
+	"queries": [
+		{
+			"datasource": {
+				"type": "datasource",
+				"uid": "grafana"
+			},
+			"queryType": "randomWalk",
+			"refId": "A"
+		}
+	]
+}`
+
+var reqNoQueries = `{
+	"from": "",
+	"to": "",
+	"queries": []
+}`
+
+var reqQueryWithInvalidDatasourceID = `{
+	"from": "",
+	"to": "",
+	"queries": [
+		{
+			"queryType": "randomWalk",
+			"refId": "A"
+		}
+	]
+}`
+
+var reqDatasourceByUidNotFound = `{
+	"from": "",
+	"to": "",
+	"queries": [
+		{
+			"datasource": {
+				"type": "datasource",
+				"uid": "not-found"
+			},
+			"queryType": "randomWalk",
+			"refId": "A"
+		}
+	]
+}`
+
+var reqDatasourceByIdNotFound = `{
+	"from": "",
+	"to": "",
+	"queries": [
+		{
+			"datasourceId": 1,
+			"queryType": "randomWalk",
+			"refId": "A"
+		}
+	]
+}`
+
+func TestDataSourceQueryError(t *testing.T) {
+	tcs := []struct {
+		request        string
+		clientErr      error
+		expectedStatus int
+		expectedBody   string
+	}{
+		{
+			request:        reqValid,
+			clientErr:      backendplugin.ErrPluginUnavailable,
+			expectedStatus: http.StatusInternalServerError,
+			expectedBody:   `{"message":"Internal server error","messageId":"plugin.unavailable","statusCode":500,"traceID":""}`,
+		},
+		{
+			request:        reqValid,
+			clientErr:      backendplugin.ErrMethodNotImplemented,
+			expectedStatus: http.StatusNotImplemented,
+			expectedBody:   `{"message":"Not implemented","messageId":"plugin.notImplemented","statusCode":501,"traceID":""}`,
+		},
+		{
+			request:        reqValid,
+			clientErr:      errors.New("surprise surprise"),
+			expectedStatus: errutil.StatusInternal.HTTPStatus(),
+			expectedBody:   `{"message":"An error occurred within the plugin","messageId":"plugin.downstreamError","statusCode":500,"traceID":""}`,
+		},
+		{
+			request:        reqNoQueries,
+			expectedStatus: http.StatusBadRequest,
+			expectedBody:   `{"message":"No queries found","messageId":"query.noQueries","statusCode":400,"traceID":""}`,
+		},
+		{
+			request:        reqQueryWithInvalidDatasourceID,
+			expectedStatus: http.StatusBadRequest,
+			expectedBody:   `{"message":"Query does not contain a valid data source identifier","messageId":"query.invalidDatasourceId","statusCode":400,"traceID":""}`,
+		},
+		{
+			request:        reqDatasourceByUidNotFound,
+			expectedStatus: http.StatusNotFound,
+			expectedBody:   `{"error":"data source not found","message":"Data source not found","traceID":""}`,
+		},
+		{
+			request:        reqDatasourceByIdNotFound,
+			expectedStatus: http.StatusNotFound,
+			expectedBody:   `{"error":"data source not found","message":"Data source not found","traceID":""}`,
+		},
+	}
+
+	for _, tc := range tcs {
+		t.Run(fmt.Sprintf("Plugin client error %q should propagate to API", tc.clientErr), func(t *testing.T) {
+			p := &plugins.Plugin{
+				JSONData: plugins.JSONData{
+					ID: "grafana",
+				},
+			}
+			p.RegisterClient(&fakePluginBackend{
+				qdr: func(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
+					return nil, tc.clientErr
+				},
+			})
+			srv := SetupAPITestServer(t, func(hs *HTTPServer) {
+				r := registry.NewInMemory()
+				err := r.Add(context.Background(), p)
+				require.NoError(t, err)
+				hs.queryDataService = query.ProvideService(
+					nil,
+					&fakeDatasources.FakeCacheService{},
+					nil,
+					&fakePluginRequestValidator{},
+					&fakeDatasources.FakeDataSourceService{},
+					pluginClient.ProvideService(r),
+					&fakeOAuthTokenService{},
+				)
+				hs.QuotaService = quotatest.NewQuotaServiceFake()
+			})
+			req := srv.NewPostRequest("/api/ds/query", strings.NewReader(tc.request))
+			webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, OrgRole: org.RoleViewer})
+			resp, err := srv.SendJSON(req)
+			require.NoError(t, err)
+
+			require.Equal(t, tc.expectedStatus, resp.StatusCode)
+			require.Equal(t, tc.expectedStatus, resp.StatusCode)
+			body, err := io.ReadAll(resp.Body)
+			require.NoError(t, err)
+			require.Equal(t, tc.expectedBody, string(body))
+			require.NoError(t, resp.Body.Close())
+		})
+	}
+}
+
+type fakePluginBackend struct {
+	qdr backend.QueryDataHandlerFunc
+
+	backendplugin.Plugin
+}
+
+func (f *fakePluginBackend) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
+	if f.qdr != nil {
+		return f.qdr(ctx, req)
+	}
+	return backend.NewQueryDataResponse(), nil
+}
+
+func (f *fakePluginBackend) IsDecommissioned() bool {
+	return false
+}

pkg/api/plugins_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/grafana/grafana-plugin-sdk-go/backend"
+
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/log/logtest"
@@ -319,11 +320,10 @@ func Test_GetPluginAssets(t *testing.T) {
 }
 
 func TestMakePluginResourceRequest(t *testing.T) {
-	pluginClient := &fakePluginClient{}
 	hs := HTTPServer{
 		Cfg:          setting.NewCfg(),
 		log:          log.New(),
-		pluginClient: pluginClient,
+		pluginClient: &fakePluginClient{},
 	}
 	req := httptest.NewRequest(http.MethodGet, "/", nil)
 	resp := httptest.NewRecorder()