Query endpoint #8

carlosms · 2018-04-25T15:39:13Z

Part of #2. Adds a new endpoint, POST /query. It forwards SQL queries to gitbase.

A few notes:

I've used the master branch as the constraint of github.com/go-sql-driver/mysql because the latest release, 1.3, does not support ColumnType.DatabaseTypeName. See go-sql-driver/mysql#667

In the design we talked about adding extra information, such as the elapsed time, to the meta field. In this PR meta only contains the table headers. I think it's ok to add extra pieces of information later as we find the need for them.

~~I've left a couple of TODO comments with minor things, I think we can take care of that later on.~~ -> Solved with the review suggestions

.

The requests and results look like this:

{
  "query": "SELECT name, hash, is_remote(name) AS is_remote FROM refs",
  "limit": 20
}

{
    "status": 200,
    "data": [
        {
            "hash": "66fd81178abfa342f873df5ab66639cca43f5104",
            "is_remote": false,
            "name": "HEAD"
        },
        {
            "hash": "66fd81178abfa342f873df5ab66639cca43f5104",
            "is_remote": false,
            "name": "refs/heads/master"
        },
        {
            "hash": "66fd81178abfa342f873df5ab66639cca43f5104",
            "is_remote": true,
            "name": "refs/remotes/origin/master"
        }
    ],
    "meta": {
        "headers": [
            "name",
            "hash",
            "is_remote"
        ]
    }
}

A failure:

{
	"query": "SELECT * FROM somewhere",
	"limit": 20
}

{
    "status": 400,
    "errors": [
        {
            "status": 400,
            "title": "Error 1105: unknown error: table not found: somewhere"
        }
    ]
}

For a query with uast, the protobuf response is unmarshalled and the json is returned:

{
	"query": "SELECT hash, content, uast(blobs.content, 'go') FROM blobs WHERE hash='fd30cea52792da5ece9156eea4022bdd87565633'",
	"limit": 20
}

{
    "status": 200,
    "data": [
        {
            "content": "package server\n\nimport (\n\t\"net/http\"\n\n\t\"github.com/src-d/gitbase-playground/server/handler\"\n\n\t\"github.com/go-chi/chi\"\n\t\"github.com/go-chi/chi/middleware\"\n\t\"github.com/pressly/lg\"\n\t\"github.com/rs/cors\"\n\t\"github.com/sirupsen/logrus\"\n)\n\n// Router returns a Handler to serve the backend\nfunc Router(\n\tlogger *logrus.Logger,\n\tstatic *handler.Static,\n\tversion string,\n) http.Handler {\n\n\t// cors options\n\tcorsOptions := cors.Options{\n\t\tAllowedOrigins:   []string{\"*\"},\n\t\tAllowedMethods:   []string{\"GET\", \"POST\", \"PUT\", \"OPTIONS\"},\n\t\tAllowedHeaders:   []string{\"Location\", \"Authorization\", \"Content-Type\"},\n\t\tAllowCredentials: true,\n\t}\n\n\tr := chi.NewRouter()\n\n\tr.Use(middleware.Recoverer)\n\tr.Use(cors.New(corsOptions).Handler)\n\tr.Use(lg.RequestLogger(logger))\n\n\tr.Get(\"/version\", handler.APIHandlerFunc(handler.Version(version)))\n\n\tr.Get(\"/static/*\", static.ServeHTTP)\n\tr.Get(\"/*\", static.ServeHTTP)\n\n\treturn r\n}\n",
            "hash": "fd30cea52792da5ece9156eea4022bdd87565633",
            "uast(blobs.content, \"go\")": [
                {
                    "InternalType": "File",
                    "Properties": {
                        "Package": "1"
                    },
                    "Children": [
                        {
                            "InternalType": "Ident",
                            "Properties": {
                                "internalRole": "Name"
                            },
                            "Token": "server",
                            "StartPosition": {
                                "Offset": 9,
                                "Line": 1,
                                "Col": 10
                            },
                            "Roles": [
                                18,
                                1
                            ]
                        },
                        
                        [...]

                }
            ]
        }
    ],
    "meta": {
        "headers": [
            "hash",
            "content",
            "uast(blobs.content, \"go\")"
        ]
    }
}

smacker · 2018-04-25T15:56:16Z

server/handler/query.go

+	return func(r *http.Request) (*serializer.Response, error) {
+		var queryRequest queryRequest
+		body, err := ioutil.ReadAll(r.Body)
+		defer r.Body.Close()


https://golang.org/pkg/net/http/#Request

The Server will close the request body. The ServeHTTP Handler does not need to.

Fixed, thanks! I actually took it from https://github.com/src-d/code-annotation/blob/master/server/handler/assignments.go#L82

smacker · 2018-04-25T16:02:05Z

server/handler/query.go

+		query := fmt.Sprintf("%s LIMIT %d", queryRequest.Query, queryRequest.Limit)
+		rows, err := db.Query(query)
+		if err != nil {
+			return nil, serializer.NewHTTPError(http.StatusBadRequest, err.Error())


as TODO. Mysql returns MySQLError with 2 fields number&message. Would be nice to map them into our json.
https://github.com/go-sql-driver/mysql/blob/master/errors.go#L58

Good idea, done in e6deab2

smacker · 2018-04-25T16:06:03Z

server/handler/query.go

+				return nil, err
+			}
+
+			colData := make(map[string]string)


colData := make(map[string]string, nColumns)

smacker · 2018-04-25T16:07:17Z

server/handler/query.go

+
+		columnNames, _ := rows.Columns()
+		nColumns := len(columnNames)
+		columnValsPtr := genericVals(nColumns)


how does it work for integers?

ah. I see you changed it in next commit.

smacker · 2018-04-25T16:16:56Z

server/handler/query.go

+func genericVals(colTypes []*sql.ColumnType) []interface{} {
+	columnValsPtr := make([]interface{}, len(colTypes))
+
+	for i, colType := range colTypes {


should we cover all types correctly? For example I don't see double or float.
https://github.com/go-sql-driver/mysql/blob/master/fields.go#L16

You are right, I just added the types I encountered them in gitbase replies, but it's better to make it complete since the beginning. Done in e6deab2

smacker · 2018-04-25T16:19:16Z

server/handler/query.go

+						colData[columnNames[i]] = sqlVal.String
+					}
+				case *[]byte:
+					// TODO (carlosms) this may not be an array always


what else can it be? In engine there is always array.

I was thinking that gitbase may at some point add something new that could return a JSON object. For instance, a new function 'first_uast_as_json()'

smacker · 2018-04-25T16:22:00Z

server/handler/query.go

+						return nil, err
+					}
+
+					nodes := make([]*uast.Node, len(protobufs))


dashboard replaces Role ID by Role name. Most probably playground should be compatible with it:
https://github.com/bblfsh/dashboard/blob/master/server/server.go#L217

I've opened an issue so we don't forget about it when we start working on the integration with the uast frontend component:
#9

smacker · 2018-04-25T16:26:25Z

server/handler/query.go

+
+		// TODO (carlosms) this only works if the query does not end in ;
+		// and does not have a limit. It will also fail for queries like
+		// DESCRIBE TABLE


gitbase doesn't support anything besides SELECT, SHOW, DESCRIBE.
Add limit makes sense only to SELECT queries. Ending ; also very easy to fix:

query = strings.TrimRight(strings.TrimSpace(query), ";")

not perfect but better than nothing

👍 Implemented the todo with your suggestion in e6deab2

dpordomingo

Great job!
LGTM

I'd modify some things, but nothing that could not be done later after merging.
(the issue that concerns me more, is the huge handler.Query method, too large imho; I'd split and extract DB logic to a new package as we did in CAT)

dpordomingo · 2018-04-26T09:50:31Z

server/handler/query.go

+		// TODO (carlosms) this only works if the query does not end in ;
+		// and does not have a limit. It will also fail for queries like
+		// DESCRIBE TABLE
+		query := fmt.Sprintf("%s LIMIT %d", queryRequest.Query, queryRequest.Limit)


I'd move this to a service that would take care of the query sent by the user; in the future it could do validations, return readable errors...

Because the functionality is so basic, for now I've moved it to a new function in e6deab2

dpordomingo · 2018-04-26T10:09:08Z

server/handler/query.go

+					// TODO (carlosms) this may not be an array always
+					var protobufs [][]byte
+					if err := json.Unmarshal(*val.(*[]byte), &protobufs); err != nil {
+						return nil, err


Should we return a 500 if one UAST cannot be read? I'd return the rows, and warning

I actually found a problem with this. gitbase can also return a JSON column with an array of strings, for example in commits.parents

With the changes in e6deab2 if the unmarshal fails the data will be returned as a string.

dpordomingo · 2018-04-26T10:10:46Z

server/handler/query.go

+			return nil, err
+		}
+
+		return serializer.NewQueryResponse(tableData, columnNames), nil


won't be useful to send the type too? That way it won't be needed to check it in the frontend too.

👍 good suggestion. Done in e6deab2

smacker · 2018-04-26T11:11:34Z

server/handler/query.go

+					if err == nil {
+						colData[columnNames[i]] = nodes
+					} else {
+						var strings []string


why strings? Maybe []interface{} is better?

I did it thinking in the commits.parents column, which is an array of strings. But it actually doesn't even have to be an array, using interface{} will work for any JSON array or object we receive. Changed in 28fc331.

smacker · 2018-04-26T11:16:24Z

server/handler/query.go

+func addLimit(query string, limit int) string {
+	upper := strings.ToUpper(query)
+	if strings.Contains(upper, "DESCRIBE") || strings.Contains(upper, "SHOW") {
+		return query


please consider to rewrite it like this:

func addLimit(query string, limit int) string { query = strings.TrimRight(strings.TrimSpace(query), ";") if strings.HasPrefix(strings.ToUpper(query), "SELECT") { return fmt.Sprintf("%s LIMIT %d", query, limit) } return query

it covers more cases and imo easier to read

Makes sense, changed in 28fc331

smacker

Now it looks good!

The only thing I would add tests.

carlosms · 2018-04-26T13:17:24Z

@bzz, a comment for your consideration when adding your review.
We discussed via chat and we agree that since we are still at this very early dev stage we can do tests in a future PR.

bzz · 2018-04-27T07:08:54Z

@carlosms great job!

Squash
As it seems to be almost ready, do you think at this point it would be reasonable to ask to squash commits before a final pass?
Asking, as +37,039 −11 in 126 files looks a bit harsh and all previous feedback seems to be already 👍 so may be 2 commits would do: 1 for the code added for this feature and 1 for noisy deps update?
curl
One more thing, on Query endpoint #8 (comment) - you did a great job in PR description with examples of intput/output of the query, but may I ask you to add exact curl commands to reproduce those? It becomes especially valuable, as we do not have tests yet.
I would assume something like
```
curl -X POST http://localhost:8080/query \
  -H "Content-Type: application/json" \
  -d '{"query":"...", "limit":"..."}'
```
would do the trick, but imagine how valuable it will be to copy it from somewhere, if one needs to prepare for a sudden demo?

In my experience, it's valuable to have PRs as self-contained as possible - including code, at leqast some simple docs and at least some simple tests (completeness is not the goal at this point).

Docs
It's totally ok to add tests later if you wish to do so, but what I would also suggest to include here, is at least a simplest bits of documentation in here i.e most easy way I can see now is to create a /docs/rest-api.md with a single section on Query, and a paragraph that includes those few CURL commands.

bzz · 2018-04-27T07:36:00Z

cmd/server/main.go

+// Otherwise gitbase will be asked for the max_allowed_packet column and the
+// query will fail.
+// The next release should make this parameter optional for us:
+// https://github.com/go-sql-driver/mysql/pull/680
 type appConfig struct {
 	Env       string `envconfig:"ENV" default:"production"`
 	Host      string `envconfig:"HOST" default:"0.0.0.0"`


Sorry if I missed that somehow, not very related to the change, but while we are here - did not we have issues with this before, being not a valid IP on some OSes and decided that it safer to have default as localhost or 127.0.0.1 instead?

Remember discussing it with @smacker

is not 0.0.0.0 the right one?
That's how it was configured in CAT, because that way the container is listening "everywhere"
https://github.com/src-d/code-annotation/pull/153/files

0.0.0.0 is a correct one. We use this variable for the server. And server should listen on all IPs.

bzz · 2018-04-27T07:53:56Z

server/handler/query.go

+		}
+
+		if err != nil {
+			return nil, err


Question on user input validation on API level - what is the expected behaviour of playground on /query endpoint, in case "limit":"20" is specified, in payload, instead of "limit":20 ?

As an API user, I would expect it to result in

{ "status": 4XX, "errors": [ { "status": 4XX, "title": "Error description" } ] }

but I belive current implementation might result in 500 without any payload in response.

Improved in 99af6d0. Now limit is optional, and if the json unmarshall fails the reply will contain:

{ "status": 400, "errors": [ { "status": 400, "title": "Bad Request. Expected body: { \"query\": \"SQL statement\", \"limit\": 1234 }" } ] }

carlosms · 2018-04-27T09:52:35Z

@bzz:

As it seems to be almost ready, do you think at this point it would be reasonable to ask to squash commits before a final pass?

I was planning to squash before the merge, but left the small commits so you could follow the PR discussions easier.

I've squashed the previous commits and added 2 small new ones. Please take another look and I'll squash again before merging.

create a /docs/rest-api.md with a single section on Query, and a paragraph that includes those few CURL commands.

Added in 7dc93b3.

bzz · 2018-04-27T16:33:19Z

Awesome, thank you very much for taking care @carlosms !

Sorry, I can not test it locally right now due to src-d/gitbase#255 but I wander, what what would be the result on the user input with multiple queries like

$curl -d '{"query":"SELECT name, hash, is_remote(name) AS is_remote FROM refs; SELECT hash, content FROM blobs;", "limit":20}' -H "Content-Type: application/json" -X POST http://localhost:8080/query

?

As a client of API, I would expect the limit argument to be added to each of the queries.

bzz · 2018-04-27T16:35:05Z

server/handler/query.go

+}
+
+// addLimit adds LIMIT to the query, performing basic tests to skip it
+// for DESCRIBE TABLE, SHOW TABLES, and avoid '; limit'


May be it would be enough to say that it's only applied to SELECT ?

Changed in fe5811f

carlosms · 2018-04-27T16:54:43Z

@bzz

what what would be the result on the user input with multiple queries like

$curl -d '{"query":"SELECT name, hash, is_remote(name) AS is_remote FROM refs; SELECT hash, content FROM blobs;", "limit":20}' -H "Content-Type: application/json" -X POST http://localhost:8080/query

This:

{
    "status": 400,
    "errors": [
        {
            "status": 400,
            "title": "unknown error: syntax error at position 66 near 'select'",
            "mysqlCode": 1105
        }
    ]
}

And any other thing would be strange in my opinion.

bzz · 2018-04-27T17:26:36Z

I'm a bit confused then, how do you propose handling the case of user input with multiple queries?

Having input from the user in form of like

SELECT ... ;
SELECT ... ;

is a requirement gitbase playground should handle.

Right now, we discussed and decided, that frontend side of the gitbase does not make any modifications/parsing/validation of the user input and just passes it though to the gitbase.

We also discussed that backend side does query re-writing, applying the limit clause, before passing it to gitbase server.

Please, help me understand how those requirements are going to be satisfied with current implementation.

smacker · 2018-04-27T17:33:30Z

@bzz sorry, I don't remember such requirement in the design document. I also don't understand how it would work. Should it open 2 tabs? We didn't discuss it at all.

carlosms · 2018-04-27T17:55:06Z

I also don't remember any mention to multiple queries 🤔

bzz · 2018-04-27T18:42:51Z

@carlosms @dpordomingo not sure, what do you guys suggest on how to proceed here, but glad that we stumbled up on this. From my side, I'm totally open for discussing/updating design doc next week on details of "multiple query user input" feature, potentially handing it in subsequent PRs and merging this as is now.

Please let me know!

bzz

SGTM, given some answer to the discussion of multiple queries feature above

dpordomingo · 2018-04-27T19:50:02Z

Is gitbase prepared to accept two queries, or does it return an error?
From @carlosms message it seems that it returns a syntax error, so if playground should not parse nor pre validate the user input, three current behavior seems to follow the agreed expectations, doesn't it?

bzz · 2018-05-03T13:01:29Z

Is gitbase prepared to accept two queries, or does it return an error?

sorry, was not able to test before src-d/gitbase#255 but may be @carlosms could clarify?

.. behavior seems to follow the agreed expectations, doesn't it?

I think it does that way

carlosms · 2018-05-03T14:49:28Z

Is gitbase prepared to accept two queries, or does it return an error?

sorry, was not able to test before src-d/gitbase#255 but may be @carlosms could clarify?

It will result in error. If we need to support multiple queries separated by ; we would need to separate the strings either in the backend or the frontend.

And I still think this would not be the expected behaviour for most people. One query -> one tab; one request -> one response; anything else would be confusing.

bzz · 2018-05-03T15:27:08Z

Is gitbase prepared to accept two queries, or does it return an error?

It will result in error

👍 this is a strong argument for merging this work asap and iterate lated on, based on product feedback (as noted by @dpordomingo it follows the conventions we've agreed up on).

Signed-off-by: Carlos Martín <[email protected]>

Latest release for mysql driver, 1.3, does not support ColumnType.DatabaseTypeName. See go-sql-driver/mysql#667 Signed-off-by: Carlos Martín <[email protected]>

Signed-off-by: Carlos Martín <[email protected]>

carlosms self-assigned this Apr 25, 2018

carlosms requested review from smacker, dpordomingo and bzz April 25, 2018 15:39

smacker reviewed Apr 25, 2018

View reviewed changes

dpordomingo approved these changes Apr 26, 2018

View reviewed changes

dpordomingo added the new feature label Apr 26, 2018

carlosms mentioned this pull request Apr 26, 2018

Modify uast nodes the same way as babelfish dashboard #9

Closed

smacker reviewed Apr 26, 2018

View reviewed changes

smacker approved these changes Apr 26, 2018

View reviewed changes

dpordomingo approved these changes Apr 26, 2018

View reviewed changes

bzz reviewed Apr 27, 2018

View reviewed changes

carlosms force-pushed the http-proxy branch from 28fc331 to 7dc93b3 Compare April 27, 2018 09:47

This was referenced Apr 27, 2018

Tests for query handler #10

Merged

New endpoint get /tables #11

Merged

Mock uast endpoints #12

Merged

bzz reviewed Apr 27, 2018

View reviewed changes

bzz approved these changes Apr 27, 2018

View reviewed changes

carlosms added 3 commits May 3, 2018 17:55

Add POST /query. It calls gitbase and returns json

9b1a758

Signed-off-by: Carlos Martín <[email protected]>

Add dependencies, use master for mysql driver

e33f526

Latest release for mysql driver, 1.3, does not support ColumnType.DatabaseTypeName. See go-sql-driver/mysql#667 Signed-off-by: Carlos Martín <[email protected]>

Add rest api docs

65a7c83

Signed-off-by: Carlos Martín <[email protected]>

carlosms force-pushed the http-proxy branch from fe5811f to 65a7c83 Compare May 3, 2018 15:56

carlosms merged commit b92ddd4 into src-d:master May 3, 2018

Query endpoint #8

Query endpoint #8

Uh oh!

Conversation

carlosms commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smacker left a comment

Choose a reason for hiding this comment

Uh oh!

carlosms commented Apr 26, 2018

Uh oh!

bzz commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bzz Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosms commented Apr 25, 2018 •

edited

Loading

bzz commented Apr 27, 2018 •

edited

Loading

bzz Apr 27, 2018 •

edited

Loading

bzz Apr 27, 2018 •

edited

Loading

bzz commented Apr 27, 2018 •

edited

Loading

bzz commented Apr 27, 2018 •

edited

Loading

bzz commented Apr 27, 2018 •

edited

Loading

bzz left a comment •

edited

Loading