GraphiQL server (+ minor usability features) #112

SudoBobo · 2018-04-06T12:22:07Z

No description provided.

Totktonada · 2018-04-10T13:38:19Z

graphql/server/server.lua

+                body = json.encode({errors={{message="Body should be a valid JSON"}}})
+            }
+        end
+


We can check type(parsed) == 'table' here.

Totktonada · 2018-04-10T13:47:29Z

graphql/server/server.lua

+        if not ok then
+            return {
+                status = 200,
+                body = json.encode({errors={{message=result}}})


's/=/ = /g' (in other places too)

Did not get your point. Did you mean to 'a=b' -> 'a = b' ? If so, fixed. (I made it initially with no whitespaces to keep it one-line)

Yep, that was the point.

Totktonada · 2018-04-10T13:49:46Z

graphql/tarantool_graphql.lua

@@ -190,7 +191,7 @@ local function gql_argument_type(avro_schema)

            fields[field.name] = {
                name = field.name,
-                kind = types.nonNull(gql_field_type),
+                kind = gql_field_type,


You will not remember why this is needed after two months.

I am not sure, but it seems that current version does not need a comment, because it's quite straightforward. Previous version needed a comment because there was an additional nonNull wrapping with no obvious reason.

The field cannot be non-null here (do we need to call nullable(gql_field_type))?

I would add a comment like 'accept any subset of an input object field as a filter'. Ok?

Do you mean, that the field can't be non-null here because we want arguments (corresponding GraphQL InputObjects) to have all fields nullable? (As we discussed it verbally) If yes, so we need to do something like nullable(gql_field_type) But I think this should be done in another branch, related to InputObjects arguments support

Ok. That's what we do :)

I mean that nullability of Object should no affect nullability of InputObject.

Did not get your point. Need to discus verbally.

Filed #127 and added comment tarantool/graphql#26 (comment)

Totktonada · 2018-04-10T13:58:02Z

graphql/tarantool_graphql.lua

-    assert_gql_query_ast('gql_compile', ast)
-    local operation_name = ast.definitions[1].name.value
+
+    local operation_name


How this change related to the graphiql? What a problem you fix here?

Before this story with GraphiQL Tarantool GraphQL (as a GraphQL backend) assumed that in all incoming queries there is only one definition (an operation definition describing the query itself).
It's true if the query is simple, but in case of fragments it is not. Each fragment represented as a definition in ast.
We want to support fragments in general. And they are necessary for our GraphiQL to work as it's JS code send an introspection query with fragments.

How much part of #16 and #50 closed by this? Maybe we can extract it into a separate commit (before graphiql support) to fix these two issues.

A big part of #16 is closed (but there should be additions also). #50 is not much related to the current pull request, but can be easily (at least seems so) fixed

You can mention that they are more or less related in a commit message, but leave they open.

Okay, I'll do so.

Totktonada · 2018-04-10T14:07:58Z

graphql/tarantool_graphql.lua

+        end
+    end
+
+    assert(operation_name, "there is no 'operation' in query " ..


It is better to have --- from yaml on its own line.

Got your point, fixed.

Totktonada · 2018-04-10T14:48:55Z

graphql/tarantool_graphql.lua

+    assert(type(gql) == 'table',
+        'use :start_server(...) instead of .start_server(...)')
+    gql.server = server.init(gql)
+    gql.server:start()


Maybe return true, 'The server started at http://127.0.0.1:8000' or kinda to improve usability from console?

Good idea. Added.

Totktonada · 2018-04-10T14:50:22Z

graphql/server/graphiql/index.html

+         favored resource bundler.
+       -->
+    <link rel="stylesheet" href="/static/css/graphiql.css" />
+    <script src="/static/js/graphiql.js"></script>


Are you forget to add these files into the commit?

Yes. Sorry. Added them.

Totktonada · 2018-04-10T15:01:40Z

graphql/init.lua

@@ -9,6 +9,10 @@ graphql.accessor_general = accessor_general
 graphql.accessor_space = accessor_space
 graphql.accessor_shard = accessor_shard
 graphql.new = tarantool_graphql.new
+graphql.compile = tarantool_graphql.compile
+graphql.execute = tarantool_graphql.execute
+graphql.start_server = tarantool_graphql.start_server


No space format at all, got:

{ "errors": [ { "message": "./graphql/core/validate.lua:223: assign to undeclared variable 'kind'" } ] }

And with format too.

What was the case? I added a test with user-configured tarantool graphql instance + server usage and it passed.

I think I need to try again.

Fixed. The problem was with global variables in grahql/core

Just to track such problems: found problem with empty variables (it is converted to box.NULL and box.NULL or {} is not {}).

Totktonada

Some comments I forgot to send before.

Totktonada · 2018-04-10T15:04:01Z

graphql/init.lua

@@ -9,6 +9,10 @@ graphql.accessor_general = accessor_general
 graphql.accessor_space = accessor_space
 graphql.accessor_shard = accessor_shard
 graphql.new = tarantool_graphql.new
+graphql.compile = tarantool_graphql.compile
+graphql.execute = tarantool_graphql.execute
+graphql.start_server = tarantool_graphql.start_server


And with format too.

Totktonada · 2018-04-16T12:28:19Z

graphql/server/server.lua

+        if not ok then
+            return {
+                status = 200,
+                body = json.encode({errors={{message=result}}})


Yep, that was the point.

Totktonada

Bunch of comments.

Totktonada · 2018-04-17T08:23:23Z

graphql/utils.lua

@@ -213,4 +213,18 @@ function utils.value_in(value, array)
    return false
 end

+function utils.deep_copy(orig)


Is built-in table.deepcopy fit your needs?

Yes, it does. Removed.

Totktonada · 2018-04-17T08:30:11Z

graphql/tarantool_graphql.lua

-    assert_gql_query_ast('gql_compile', ast)
-    local operation_name = ast.definitions[1].name.value
+
+    local operation_name


How much part of #16 and #50 closed by this? Maybe we can extract it into a separate commit (before graphiql support) to fix these two issues.

Totktonada · 2018-04-17T08:31:36Z

graphql/tarantool_graphql.lua

+    local operation_name
+    for _, definition in pairs(ast.definitions) do
+        if definition.kind == 'operation' then
+            operation_name = definition.name.value


Multiple operation case is okay? Maybe we need to (add and) consider 'operation_name' argument mandatory in the case?

Multiple operation case is okay. You define as many as you want, but can execute only one at a time, specifying it with 'operation_name' (Details)
Answering to the second question: according to the spec, we should not make it mandatory. Anyway I should check (and probably re-write) this place to satisfy GraphQL spec.

As we discussed it verbally, it will be done in another branch.

Totktonada · 2018-04-17T08:47:04Z

graphql/tarantool_graphql.lua

@@ -1223,6 +1210,15 @@ local function gql_execute(qstate, variables)
        operation_name)
 end

+local function gql_execute(state, query, variables)


Proposed to use separate work for compile+execute. Maybe 'run'?

I mean 'separate name'. Don’t have idea why I wrote 'separate work'.

Seems good enough, can't think of better naming. Fxd.

Do you want to change external API as well? gql_wrapper:execute() -> gql_wrapper:run()

The idea was to change, but now I think that we can proceed with API like in lrexlib-pcre where match can be called for the module with an expression as a string as well as for compiled expression.

Discussed verbally. Internal functions renamed, external API left as it is.

Totktonada · 2018-04-17T08:57:46Z

graphql/tarantool_graphql.lua

+            execute = gql_execute,
+            start_server = start_server,
+            stop_server = stop_server,
+            cfg = cfg,


Remove it, it is in 'internal'.

Totktonada · 2018-04-17T09:49:58Z

test/local/server.test.lua

+
+    gql_wrapper:stop_server()
+
+    -- add space formats and try default instance


Are you test it in 'no_instance' test already?

In 'no_instance' I test the whole mechanism of require(graphql):do_something_cool() Here I test the http server itself.

Totktonada · 2018-04-17T09:57:20Z

README.md

+local graphql = require('graphql').new({
+    schemas = schemas,
+    collections = collections,
+    accessor = accessor


Are you sure it will work w/o indexes and service_fields?

No it will not. Fxd readme.

Totktonada · 2018-04-17T10:00:17Z

README.md

+graphql:stop_server()
+
+-- as well you may do
+require('graphql').start_server()


Why period here, but colon for an instance?

Did not get your point. In case of require(...) we call just a function. In case of instance we want to pass this instance as a first argument.

Checked shard and http.client modules, they share your approach. So, I discard my proposal.

Discussed verbally, left as it is.

Totktonada · 2018-04-17T10:18:36Z

graphql/simple_config.lua

@@ -169,6 +170,17 @@ function simple_config.get_spaces_formats()
    return spaces_formats
 end

+local function remove_empty_formats(spaces_formats)
+    local resulting_formats = utils.deep_copy(spaces_formats)


It seems you don't need deepcopy at all, table.copy will suffer your needs.

Fxd. (English remark: maybe 'serve', not 'suffer'? :)

Maybe I did went to say 'suit'… Don’t know what was with my wording in Tue.

Totktonada · 2018-04-17T10:20:54Z

graphql/tarantool_graphql.lua

@@ -190,7 +191,7 @@ local function gql_argument_type(avro_schema)

            fields[field.name] = {
                name = field.name,
-                kind = types.nonNull(gql_field_type),
+                kind = gql_field_type,


The field cannot be non-null here (do we need to call nullable(gql_field_type))?

I would add a comment like 'accept any subset of an input object field as a filter'. Ok?

features Now it is possible to use commands like require('graphql').execute(...) or .start_server() with no manual creating of graphql instance; close #1; changes in gql_compile is related to #16 and #50.

SudoBobo requested a review from Totktonada April 6, 2018 12:22

Totktonada reviewed Apr 10, 2018

View reviewed changes

Totktonada reviewed Apr 16, 2018

View reviewed changes

SudoBobo force-pushed the sb/graphiql branch 6 times, most recently from f57a9f5 to 3e4b0d2 Compare April 17, 2018 08:21

Totktonada reviewed Apr 17, 2018

View reviewed changes

SudoBobo force-pushed the sb/graphiql branch 2 times, most recently from d2dc8ed to ef2b1ac Compare April 20, 2018 07:52

SudoBobo changed the title ~~GraphiQL server (+ minor usability features) WIP PR~~ GraphiQL server (+ minor usability features) Apr 20, 2018

SudoBobo force-pushed the sb/graphiql branch 6 times, most recently from f435327 to e33a8ef Compare April 20, 2018 15:37

Totktonada approved these changes Apr 20, 2018

View reviewed changes

Add HTTP server, which provide GraphiQL interface. Add usability

b91890a

features Now it is possible to use commands like require('graphql').execute(...) or .start_server() with no manual creating of graphql instance; close #1; changes in gql_compile is related to #16 and #50.

SudoBobo force-pushed the sb/graphiql branch from e33a8ef to b91890a Compare April 20, 2018 15:43

SudoBobo merged commit ce19d43 into master Apr 20, 2018

SudoBobo mentioned this pull request Apr 20, 2018

Make 'http' module optionally required #129

Closed

SudoBobo deleted the sb/graphiql branch May 26, 2018 18:07


		gql_wrapper:stop_server()

		-- add space formats and try default instance

GraphiQL server (+ minor usability features) #112

GraphiQL server (+ minor usability features) #112

Uh oh!

Conversation

SudoBobo commented Apr 6, 2018

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!

SudoBobo Apr 16, 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!

SudoBobo Apr 20, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SudoBobo Apr 16, 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!

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!

SudoBobo Apr 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SudoBobo Apr 16, 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!

Choose a reason for hiding this comment

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

SudoBobo Apr 16, 2018 •

edited

Loading

SudoBobo Apr 20, 2018 •

edited

Loading

SudoBobo Apr 16, 2018 •

edited

Loading

SudoBobo Apr 16, 2018 •

edited

Loading

SudoBobo Apr 16, 2018 •

edited

Loading