Skip to content
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

Let driver implement CallResourceHandler #19

Closed
sunker opened this issue Jun 23, 2021 · 3 comments · Fixed by #21
Closed

Let driver implement CallResourceHandler #19

sunker opened this issue Jun 23, 2021 · 3 comments · Fixed by #21
Labels
enhancement New feature or request

Comments

@sunker
Copy link
Collaborator

sunker commented Jun 23, 2021

Many SQL data sources have the need to load additional data from some api to populate the query editor with meta data. In some cases, the meta data can be retrieved using a normal sql query, i.e by using the QueryData handler which in turn is using the go sql query method. But retrieving meta data using sql queries is not always feasible. Both redshift and athena for example, needs to be able to load meta data that cannot be fetched using a sql statement. Since sqlds acts as the main entry point for the grpc server, there's currently no way for redshift or athena to load meta data, making it hard to use sqlds.

To solve this problem, it should be possible for the driver to implement it's own CallResourceHandler. The resource handler should be optional, and registered upon creation of the sqlds datasource.

The signature in sqlds could be something like this:

GetResourceHandler() backend.CallResourceHandler

And the consumer would create the resource handler doing something like this:

func (s *RedshiftDatasource) GetResourceHandler() backend.CallResourceHandler {
	mux := http.NewServeMux()
	mux.HandleFunc("/some-meta-data-endpoint", func (rw http.ResponseWriter, req *http.Request) {
		rw.Write([]byte("hello"))
	})
	return httpadapter.New(mux)
}

To make this optional and not introduce breaking changes, it should probably not be a part of the driver interface. Maybe we should use type assertions instead.

Thoughts @kminehart @andresmgot @yesoreyeram ?

@sunker sunker added the enhancement New feature or request label Jun 23, 2021
@kminehart
Copy link
Collaborator

kminehart commented Jun 23, 2021

summarizing my thoughts for @andresmgot and @yesoreyeram :

I think you were spot on in this issue: grafana/grafana#36061

The schema is basically the same for all SQL languages. They have tables, columns, functions, keywords and so on. Therefore, it should be possible to build a generic completion item provider for any sql data source. All the user of the provider would do is to pass tables, columns etc based upon the selected schema. Potentially, it can also provide autocomplete for values that are loaded on demand.

I would like something a little less manual for this. An interface or set of functions that are basically 1 level up from the resource handlers, that way we wouldn't have to create a new query editor for each SQL data source. FWIW the next part of this project is to create a query editor that applies to any data source using sqlds.

Something like,

type Completable interface {
    Tables(ctx context.Context, db string) ([]string, error)
    Schemas(ctx context.Context, db string) ([]string, error)
    Columns(ctx context.Context, table string) ([]stirng, error)
}

Though after our discussion, I understand that sqlds can not satisfy every use case. Some sql databases are just weird.

Quoting what I said in slack:

I would be fine with that if we include comments that strongly suggest to limit its usage. I'm worried it will be used as a "catch all" for things that aren't in the sqlds interfaces yet but could be universally applicable

@sunker
Copy link
Collaborator Author

sunker commented Jun 23, 2021

Completely agree.

I would be fine with that if we include comments that strongly suggest to limit its usage. I'm worried it will be used as a "catch all" for things that aren't in the sqlds interfaces yet but could be universally applicable

Yeah true. Let's add support for Completable now and then we can make it possible for the consumer to extend with its own resource handler once there's a need for that. For now, the ability to load Tables, Schemas and Columns is sufficient for redshift and athena.

@andresmgot
Copy link
Collaborator

My +1 for the Completable interface since it will save us from writing boilerplate code. My only concern is that it will be more difficult for future developers to connect the dots between the Completable interface and CallResourceHandler if at some point we need to extend it or make it more verbose. I guess that can be fixed with proper documentation (at least comments in code).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants