adds support for query parameters #625

brunocalza · 2024-05-23T19:59:47Z

Summary

This PR adds support for the new SQL parser that is able to interpret placeholders (?) and replace them with query parameters that come from the request. You can now perform the following queries:
- GET /query?statement="select * from my_table where id = ?"&params=1
- POST --json '{"statement": "SELECT * FROM my_table WHERE counter = ?", "params" : [1], "unwrap": true, "extract": true}' /query
If you want to provide multiple params values, you just repeat the params with new values. For example &params=1&params=2&params=3. And if you want to provide a string, you gotta enclose it &params="3".
This PR depends on add support for query parameters go-sqlparser#388 and add support for params in POST /query docs#198

Context

The context for this PR can be found at ENG-852.

Make sure you understand tablelandnetwork/go-sqlparser#388 and tablelandnetwork/docs#198 before reviewing this one.

Implementation overview

This PR changes a lot of files, but most of the changes are just a side effect of the main changes. Here's an overview on the most important changes:

Controller

The controller needed to be changed to capture the params query parameter. Both GET /query and POST /query were changed. The params is captured as a slice of string ([]string). In the case of the POST, since we have an []interface{} after the JSON is decoded into memory, we try to convert it to []string.

Resolver is now passed to the `GatewayService`

I made a change to make the resolver a dependency of the GatewayService. See the implication of this change here, here and here. The reason for that is that it makes more sense for the GatewayService to attach the parameters values to the resolver, than the storage layer. The storage layer is just responsible for executing the query.

The resolver is not a dependency of `GatewayStore` and is now passed in the `Read` method

We already mentioned how the resolver is passed to a GatewayService instead of GatewayStore. In order to build the final statement, we need to pass the resolver to the Read method. See it here:

The resolver implementation needs to implement `GetBindValues() []sqlparser.Expr`

We must implement the new method of our resolver (remember this? ). For that, we make use of PrepareParams that converts []string that comes from the controller to []sqlparser.Expr and store those values inside the struct. Here's how PrepareParams is used.

Review orientation

Start at internal/router/controllers/controller.go and understand the change made in the controller
Go to cmd/api/main.go and see how the resolver is now passed to the gateway and not to the storage layer then check the changes at internal/gateway/gateway.go
Check the internal/gateway/impl/gateway_store.go file
Check the pkg/parsing/readstatementresolver.go file
Check the changes made in the Go client (pkg/client/v1/readquery.go)
The changes in the other files are just to adjust to the changes explained above

Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>

brunocalza · 2024-05-24T17:28:57Z

Makefile

@@ -80,7 +80,8 @@ lint:
 .PHONY: lint

 # OpenAPI
-SPEC_URL=https://raw.githubusercontent.com/tablelandnetwork/docs/main/specs/validator/tableland-openapi-spec.yaml
+#SPEC_URL=https://raw.githubusercontent.com/tablelandnetwork/docs/main/specs/validator/tableland-openapi-spec.yaml
+SPEC_URL=https://raw.githubusercontent.com/tablelandnetwork/docs/8ec1d2f5137d4372b493cf6c64707ae21b5ba19b/specs/validator/tableland-openapi-spec.yaml


Gotta point back to main after tablelandnetwork/docs#198 is merged

brunocalza · 2024-05-24T17:32:53Z

Makefile

@@ -91,6 +92,7 @@ gen-api-v1:
 		&& mv go/* . \
 		&& rm -rf go main.go Dockerfile README.md api .swagger-codegen .swagger-codegen-ignore *.yaml \
 		&& sed -i 's/\*OneOfTableAttributesValue/interface{}/' model_table_attributes.go \
+		&& sed -i 's/\OneOfQueryParamsItems/interface{}/' model_query.go \


This is a trick we do because the type of the field params in the Post body can be anything. See mode_query.go:L16

brunocalza · 2024-05-24T17:33:46Z

go.mod

@@ -20,7 +20,7 @@ require (
 	github.com/sethvargo/go-limiter v0.7.2
 	github.com/spf13/cobra v1.7.0
 	github.com/stretchr/testify v1.8.2
-	github.com/tablelandnetwork/sqlparser v0.0.0-20230605164749-c0e6862c37f6
+	github.com/tablelandnetwork/sqlparser v0.0.0-20240523182602-af3edf08e3db


This needs to be changed after tablelandnetwork/go-sqlparser#388 is merged

brunocalza · 2024-05-24T17:56:00Z

internal/router/controllers/apiv1/model_query.go

@@ -12,6 +12,8 @@ package apiv1
 type Query struct {
 	// The SQL read query statement
 	Statement string `json:"statement,omitempty"`
+	// The values of query parameters
+	Params []interface{} `json:"params,omitempty"`


this code was generated by running this

brunocalza · 2024-05-24T17:56:31Z

internal/router/controllers/controller.go

@@ -231,9 +231,14 @@ func (c *Controller) GetTableQuery(rw http.ResponseWriter, r *http.Request) {
 	rw.Header().Set("Content-Type", "application/json")

 	stm := r.URL.Query().Get("statement")
+	params := []string{}


capturing the params query param from a GET request

brunocalza · 2024-05-24T17:57:06Z

internal/router/controllers/controller.go

@@ -276,6 +281,7 @@ func (c *Controller) PostTableQuery(rw http.ResponseWriter, r *http.Request) {

 	// setting a default body because these options could be missing from JSON
 	body := &apiv1.Query{
+		Params:  []any{},


the body can contain now a params field

brunocalza · 2024-05-24T17:57:39Z

internal/router/controllers/controller.go

@@ -289,8 +295,31 @@ func (c *Controller) PostTableQuery(rw http.ResponseWriter, r *http.Request) {
 	}
 	_ = r.Body.Close()

+	params := make([]string, len(body.Params))


we try to standardize to a []string, the same as we would get if the request was a GET

brunocalza · 2024-05-24T18:10:32Z

pkg/client/v1/client_test.go

 	require.Len(t, res, 2)                         // it should be 2 because we inserted two rows
 	require.Equal(t, int64(5), res[0].BlockNumber) // the block number should be 5
 }

+func TestQueryParams(t *testing.T) {


added this e2e test

brunocalza · 2024-05-24T18:12:40Z

pkg/client/v1/readquery.go

@@ -61,7 +61,9 @@ func ReadUnwrap() ReadOption {
 var queryURL, _ = url.Parse("/api/v1/query")

 // Read runs a read query with the provided opts and unmarshals the results into target.
-func (c *Client) Read(ctx context.Context, query string, target interface{}, opts ...ReadOption) error {
+func (c *Client) Read(


This go client library now accepts query params. @asutula if you update the go library in your project your code will probably break, you just have to add an empty slice of string to fix it

brunocalza · 2024-05-24T18:18:45Z

pkg/parsing/impl/validator_test.go

@@ -111,6 +111,53 @@ func TestReadRunSQL(t *testing.T) {
 	}
 }

+func TestReadQueryWithParams(t *testing.T) {


added this test just to see if the query is being parsed and resolved correctly

Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>

brunocalza · 2024-05-24T18:59:58Z

internal/router/controllers/controller.go

+	params := make([]string, len(body.Params))
+	for i, p := range body.Params {
+		switch v := p.(type) {
+		case float64:


A JSON Number is converted float64 when decoded

brunocalza · 2024-05-24T19:01:31Z

internal/router/controllers/controller.go

+		case float64:
+			params[i] = fmt.Sprint(v)
+		case string:
+			params[i] = fmt.Sprintf("\"%s\"", v)


We must enclose the string with double quotes so that it behaves similarly with GET /query?statement=...&params="bla" See also how PrepareParams handle this.

avichalp

looks good!

Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>

brunocalza · 2024-05-29T19:58:52Z

@avichalp would you mind approving this again? i've updated with the correct parser version

adds support for query parameters

2f4a7f2

Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>

brunocalza force-pushed the bcalza/queryparams branch from 6eeb081 to 2f4a7f2 Compare May 23, 2024 20:04

brunocalza commented May 24, 2024

View reviewed changes

brunocalza self-assigned this May 24, 2024

fixes to how json value are converted to string

0a43946

Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>

brunocalza commented May 24, 2024

View reviewed changes

brunocalza marked this pull request as ready for review May 24, 2024 19:01

brunocalza requested review from asutula and avichalp as code owners May 24, 2024 19:01

avichalp previously approved these changes May 27, 2024

View reviewed changes

asutula previously approved these changes May 28, 2024

View reviewed changes

updates parser to the correct version [testnetdeploy]

640d45f

Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>

brunocalza dismissed stale reviews from asutula and avichalp via 640d45f May 29, 2024 19:18

brunocalza requested a review from avichalp May 29, 2024 19:58

avichalp approved these changes May 30, 2024

View reviewed changes

brunocalza merged commit 179520f into main May 30, 2024
5 checks passed

brunocalza deleted the bcalza/queryparams branch May 30, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds support for query parameters #625

adds support for query parameters #625

brunocalza commented May 23, 2024 •

edited

Loading

brunocalza May 24, 2024

brunocalza May 24, 2024

brunocalza May 24, 2024

brunocalza May 24, 2024

brunocalza May 24, 2024

brunocalza May 24, 2024

brunocalza May 24, 2024

brunocalza May 24, 2024

brunocalza May 24, 2024

brunocalza May 24, 2024

brunocalza May 24, 2024

brunocalza May 24, 2024

avichalp left a comment

brunocalza commented May 29, 2024

adds support for query parameters #625

adds support for query parameters #625

Conversation

brunocalza commented May 23, 2024 • edited Loading

Summary

Context

Implementation overview

Controller

Resolver is now passed to the GatewayService

The resolver is not a dependency of GatewayStore and is now passed in the Read method

The resolver implementation needs to implement GetBindValues() []sqlparser.Expr

Review orientation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avichalp left a comment

Choose a reason for hiding this comment

brunocalza commented May 29, 2024

brunocalza commented May 23, 2024 •

edited

Loading

Resolver is now passed to the `GatewayService`

The resolver is not a dependency of `GatewayStore` and is now passed in the `Read` method

The resolver implementation needs to implement `GetBindValues() []sqlparser.Expr`