Skip to content

Bug fixes / context refactor #32

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.

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

Merged
merged 5 commits into from
Aug 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 7 additions & 4 deletions completion.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@ package sqlds
import (
"context"
"encoding/json"
"errors"
"fmt"
"net/http"

"github.com/grafana/grafana-plugin-sdk-go/backend"
"github.com/pkg/errors"
)

// ErrorNotImplemented is returned if the function is not implemented by the provided Driver (the Completable pointer is nil)
var ErrorNotImplemented = errors.New("not implemented")

// Completable will be used to autocomplete Tables Schemas and Columns for SQL languages
type Completable interface {
Schemas(ctx context.Context) ([]string, error)
Expand Down Expand Up @@ -43,7 +46,7 @@ type columnRequest struct {

func (ds *sqldatasource) getSchemas(rw http.ResponseWriter, req *http.Request) {
if ds.Completable == nil {
handleError(rw, errors.New("not implemented"))
handleError(rw, ErrorNotImplemented)
return
}

Expand All @@ -58,7 +61,7 @@ func (ds *sqldatasource) getSchemas(rw http.ResponseWriter, req *http.Request) {

func (ds *sqldatasource) getTables(rw http.ResponseWriter, req *http.Request) {
if ds.Completable == nil {
handleError(rw, errors.New("not implemented"))
handleError(rw, ErrorNotImplemented)
return
}

Expand All @@ -78,7 +81,7 @@ func (ds *sqldatasource) getTables(rw http.ResponseWriter, req *http.Request) {

func (ds *sqldatasource) getColumns(rw http.ResponseWriter, req *http.Request) {
if ds.Completable == nil {
handleError(rw, errors.New("not implemented"))
handleError(rw, ErrorNotImplemented)
return
}

Expand Down
42 changes: 30 additions & 12 deletions datasource.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,27 @@ package sqlds
import (
"context"
"database/sql"
"errors"
"fmt"
"net/http"
"sync"

"github.com/grafana/grafana-plugin-sdk-go/backend"
"github.com/grafana/grafana-plugin-sdk-go/backend/instancemgmt"
"github.com/grafana/grafana-plugin-sdk-go/backend/resource/httpadapter"
"github.com/grafana/grafana-plugin-sdk-go/data"
"github.com/pkg/errors"
)

type sqldatasource struct {
db *sql.DB
c Driver
settings backend.DataSourceInstanceSettings
Completable

db *sql.DB
c Driver

driverSettings DriverSettings
settings backend.DataSourceInstanceSettings

backend.CallResourceHandler
Completable
CustomRoutes map[string]func(http.ResponseWriter, *http.Request)
}

Expand All @@ -37,7 +41,9 @@ func (ds *sqldatasource) NewDatasource(settings backend.DataSourceInstanceSettin
if err != nil {
return nil, err
}

ds.CallResourceHandler = httpadapter.New(mux)
ds.driverSettings = ds.c.Settings(settings)

return ds, nil
}
Expand Down Expand Up @@ -66,7 +72,7 @@ func (ds *sqldatasource) QueryData(ctx context.Context, req *backend.QueryDataRe
// Execute each query and store the results by query RefID
for _, q := range req.Queries {
go func(query backend.DataQuery) {
frames, err := ds.handleQuery(query)
frames, err := ds.handleQuery(ctx, query)

response.Set(query.RefID, backend.DataResponse{
Frames: frames,
Expand All @@ -83,7 +89,7 @@ func (ds *sqldatasource) QueryData(ctx context.Context, req *backend.QueryDataRe
}

// handleQuery will call query, and attempt to reconnect if the query failed
func (ds *sqldatasource) handleQuery(req backend.DataQuery) (data.Frames, error) {
func (ds *sqldatasource) handleQuery(ctx context.Context, req backend.DataQuery) (data.Frames, error) {
// Convert the backend.DataQuery into a Query object
q, err := GetQuery(req)
if err != nil {
Expand All @@ -93,30 +99,42 @@ func (ds *sqldatasource) handleQuery(req backend.DataQuery) (data.Frames, error)
// Apply supported macros to the query
q.RawSQL, err = interpolate(ds.c, q)
if err != nil {
return nil, errors.WithMessage(err, "Could not apply macros")
return nil, fmt.Errorf("%s: %w", "Could not apply macros", err)
}

// Apply the default FillMode, overwritting it if the query specifies it
fillMode := ds.c.FillMode()
fillMode := ds.driverSettings.FillMode
if q.FillMissing != nil {
fillMode = q.FillMissing
}

if ds.driverSettings.Timeout != 0 {
tctx, cancel := context.WithTimeout(ctx, ds.driverSettings.Timeout)
defer cancel()

ctx = tctx
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it' a bit artificial but can we move lines 93:116 to a prepareQuery method? That way we can easily add tests for new/existing code without having to mock the query execution.


// FIXES:
// * Some datasources (snowflake) expire connections or have an authentication token that expires if not used in 1 or 4 hours.
// Because the datasource driver does not include an option for permanent connections, we retry the connection
// if the query fails. NOTE: this does not include some errors like "ErrNoRows"
res, err := query(ds.db, ds.c.Converters(), fillMode, q)
res, err := query(ctx, ds.db, ds.c.Converters(), fillMode, q)
if err == nil {
return res, nil
}

if errors.Cause(err) == ErrorQuery {
if errors.Is(err, ErrorNoResults) {
return res, nil
}

if errors.Is(err, ErrorQuery) {
ds.db, err = ds.c.Connect(ds.settings)
if err != nil {
return nil, err
}
return query(ds.db, ds.c.Converters(), fillMode, q)

return query(ctx, ds.db, ds.c.Converters(), fillMode, q)
}

return nil, err
Expand Down
9 changes: 8 additions & 1 deletion driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,25 @@ package sqlds

import (
"database/sql"
"time"

"github.com/grafana/grafana-plugin-sdk-go/backend"
"github.com/grafana/grafana-plugin-sdk-go/data"
"github.com/grafana/grafana-plugin-sdk-go/data/sqlutil"
)

type DriverSettings struct {
Timeout time.Duration
FillMode *data.FillMissing
}

// Driver is a simple interface that defines how to connect to a backend SQL datasource
// Plugin creators will need to implement this in order to create a managed datasource
type Driver interface {
// Connect connects to the database. It does not need to call `db.Ping()`
Connect(backend.DataSourceInstanceSettings) (*sql.DB, error)
FillMode() *data.FillMissing
// Settings are read whenever the plugin is initialized, or after the data source settings are updated
Settings(backend.DataSourceInstanceSettings) DriverSettings
Macros() Macros
Converters() []sqlutil.Converter
}
8 changes: 6 additions & 2 deletions errors.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
package sqlds

import "github.com/pkg/errors"
import "errors"

var (
// ErrorBadDatasource ...
// ErrorBadDatasource is returned if the data source could not be asserted to the correct type (this should basically never happen?)
ErrorBadDatasource = errors.New("type assertion to datasource failed")
// ErrorJSON is returned when json.Unmarshal fails
ErrorJSON = errors.New("error unmarshaling query JSON the Query Model")
// ErrorQuery is returned when the query could not complete / execute
ErrorQuery = errors.New("error querying the database")
// ErrorTimeout is returned if the query has timed out
ErrorTimeout = errors.New("query timeout exceeded")
// ErrorNoResults is returned if there were no results returned
ErrorNoResults = errors.New("no results returned from query")
)
1 change: 0 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,5 @@ go 1.15

require (
github.com/grafana/grafana-plugin-sdk-go v0.94.0
github.com/pkg/errors v0.9.1
github.com/stretchr/testify v1.7.0
)
1 change: 0 additions & 1 deletion go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,6 @@ github.com/pierrec/lz4 v1.0.2-0.20190131084431-473cd7ce01a1/go.mod h1:3/3N9NVKO0
github.com/pierrec/lz4 v2.0.5+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi+IEE17M5jbnwPHcY=
github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pkg/profile v1.2.1/go.mod h1:hJw3o1OdXxsrSjjVksARp5W95eeEaEfptyVZyv6JUPA=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
Expand Down
3 changes: 1 addition & 2 deletions macros.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
package sqlds

import (
"errors"
"fmt"
"regexp"
"strings"

"github.com/pkg/errors"
)

var (
Expand Down
11 changes: 9 additions & 2 deletions macros_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import (
"database/sql"
"fmt"
"testing"
"time"

"github.com/grafana/grafana-plugin-sdk-go/backend"
"github.com/grafana/grafana-plugin-sdk-go/data"
"github.com/grafana/grafana-plugin-sdk-go/data/sqlutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand All @@ -17,12 +17,15 @@ type MockDB struct{}
func (h *MockDB) Connect(backend.DataSourceInstanceSettings) (db *sql.DB, err error) {
return
}
func (h *MockDB) FillMode() (mode *data.FillMissing) {

func (h *MockDB) Settings(backend.DataSourceInstanceSettings) (settings DriverSettings) {
return
}

func (h *MockDB) Converters() (sc []sqlutil.Converter) {
return
}

func (h *MockDB) Macros() (macros Macros) {
return map[string]MacroFunc{
"foo": func(query *Query, args []string) (out string, err error) {
Expand All @@ -37,6 +40,10 @@ func (h *MockDB) Macros() (macros Macros) {
}
}

func (h *MockDB) Timeout(backend.DataSourceInstanceSettings) time.Duration {
return time.Minute
}

func TestInterpolate(t *testing.T) {
type test struct {
name string
Expand Down
25 changes: 18 additions & 7 deletions query.go
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
package sqlds

import (
"context"
"database/sql"
"encoding/json"
"fmt"
"time"

"github.com/grafana/grafana-plugin-sdk-go/backend"
"github.com/grafana/grafana-plugin-sdk-go/data"
"github.com/grafana/grafana-plugin-sdk-go/data/sqlutil"
"github.com/pkg/errors"
)

// FormatQueryOption defines how the user has chosen to represent the data
Expand Down Expand Up @@ -91,21 +92,21 @@ func getErrorFrameFromQuery(query *Query) data.Frames {
}

// query sends the query to the sql.DB and converts the rows to a dataframe.
func query(db *sql.DB, converters []sqlutil.Converter, fillMode *data.FillMissing, query *Query) (data.Frames, error) {
func query(ctx context.Context, db *sql.DB, converters []sqlutil.Converter, fillMode *data.FillMissing, query *Query) (data.Frames, error) {
// Query the rows from the database
rows, err := db.Query(query.RawSQL)
rows, err := db.QueryContext(ctx, query.RawSQL)
if err != nil {
return getErrorFrameFromQuery(query), errors.Wrap(ErrorQuery, err.Error())
return getErrorFrameFromQuery(query), fmt.Errorf("%w: %s", ErrorQuery, err.Error())
}

// Check for an error response
if err := rows.Err(); err != nil {
if err == sql.ErrNoRows {
// Should we even response with an error here?
// The panel will simply show "no data"
return getErrorFrameFromQuery(query), errors.WithMessage(err, "No results from query")
return getErrorFrameFromQuery(query), fmt.Errorf("%s: %w", "No results from query", err)
}
return getErrorFrameFromQuery(query), errors.WithMessage(err, "Error response from database")
return getErrorFrameFromQuery(query), fmt.Errorf("%s: %w", "Error response from database", err)
}

defer func() {
Expand All @@ -117,7 +118,7 @@ func query(db *sql.DB, converters []sqlutil.Converter, fillMode *data.FillMissin
// Convert the response to frames
res, err := getFrames(rows, -1, converters, fillMode, query)
if err != nil {
return nil, errors.WithMessage(err, "Could not process SQL results")
return getErrorFrameFromQuery(query), fmt.Errorf("%w: %s", err, "Could not process SQL results")
}

return res, nil
Expand All @@ -139,6 +140,16 @@ func getFrames(rows *sql.Rows, limit int64, converters []sqlutil.Converter, fill
return data.Frames{frame}, nil
}

count, err := frame.RowLen()

if err != nil {
return nil, err
}

if count == 0 {
return nil, ErrorNoResults
}
Comment on lines +143 to +151
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd be also great to have some test for this, since this is the fix of #16


if frame.TimeSeriesSchema().Type == data.TimeSeriesTypeLong {
frame, err := data.LongToWide(frame, fillMode)
if err != nil {
Expand Down