Import patches from tarantool/cartridge #9

olegrok · 2021-02-12T10:41:53Z

This patchset imports graphql patches from tarantool/cartridge.

It's impossible to preserve all history as it was. But I starts from one point and then apply commits with reference to original commits in cartridge.

Also there are some several noticeable changes:

Switch lulpeg graphql parser to luagraphqlparser that is libgraphqlparser
Add some tests from cartridge libgraphqlparser patch
CI based on Github Actions was added

Also here I need to cite my description from "cartridge libgraphqlparser patch":

Before this patch:

tarantool> require('cartridge.graphql.parse').parse('{ Test')
---
- error: Syntax error near line 1
...
It was completely unclear where an error occurred.

After this patch:

tarantool> require('luagraphqlparser').parse('{ Test')
---
- null
- '1.7: syntax error, unexpected EOF'
...
Error occured at the first line, seventh character and it's
"unexpected EOF".

But this patch breaks backward compatibility. Before this patch
empty bracets was allowd. After this patch behaviour is following:

tarantool> require('luagraphqlparser').parse('{ }')
---
- null
- '1.3: syntax error, unexpected }'
...
This error is returned from libgraphqlparser[1]. And seems changes
could be considered as bugfix.

Also lulpeg based graphql doesn't support inline "null" value
that's not so in libgraphqlparser.

[1] https://github.com/graphql/libgraphqlparser

See also tarantool/cartridge#1261
Closes #7
Closes #1
Closes #8

@knazarov

This patch imports graphql module in state as it was in https://github.com/tarantool/cartridge/tree/0330f4b8514277e2ffde02e4bd77d0b03137f16a/cluster/graphql. With some small changes: - Remove "errors" module usage - Remove "checks" module usage - Don't import "funcall" module - Drop some unused code (anyway it will be removed later but currently I do that to make history cleaner as possible) - Edit rockspec a bit. Sort module names in alphabetic order. Then I'll try to cherry-pick following in history commits from cartridge. Currently I don't know any motivation for this changes. There is an important difference from original graphql. One day @knazarov added possibility to resolve graphql types by name and "registered_types" - registry of types was introduced. The roots of this changes are in "TDG" project. Here some commit messages titles (this commits weren't any detailed description): * Hack graphql type system to permit string type names in schema * Fix introspection to work with Graphiql * ...

``` The certain order of execution is required for mutations. See: 8ba78a6 Signed-off-by: Yaroslav Dynnikov <[email protected]> ```

Imported from tarantool/cartridge@3570988 Seems it was needed to improve error message

Imported from tarantool/cartridge@1936080

Imported from tarantool/cartridge@0f6d63d

Imported from tarantool/cartridge@c41ac7d

Imported from tarantool/cartridge@2b27b01

Imported from tarantool/cartridge@7b52600

Some modules use `return nil, err` approach for error handling. This patch rethow an error in case if the second argument (err) is not nil. Imported from tarantool/cartridge@cde7c47

Imported from tarantool/cartridge@a8ac273

Imported from tarantool/cartridge@ee8490f ``` * Fix graphql.validate.getParentFields Now it doesn't fail and and returns nil, if there are no fields in object * Better error description for this case: `Field "x" is not defined on type "String"` instead of `attempt to index nil value` ```

(only graphql part is imported) tarantool/cartridge@b483fa2 ``` WebUI usually polls server list using the following GraphQL request: ```graphql query { ReplicasetList: replicasets {...} ServerList: servers {...} ServerStat: servers {statistics {...}} } ``` Functions `get_servers` and `get_replicaset` internally use one common `get_topology` method. But due to the nature of GraphQL resolvers, each of them calls `get_topology` independently and they can't share a result. Similar problem affects statistics requests: it requires additional netbox call to every server in list, and all that calls are executed sequentially one after another. It's harmful for the WebUI performance and responsivness. Moreover, it can also produce inconsistent results since netbox calls yield and instance status may change in the mean time. This patch adds caching mechanism for the GraphQL requests: the cache is shared across all resolvers within the request. It makes possible to avoid excess calculations and to make netbox requests concurrent. ```

This patch imports graphql variables validation that were imported from graphql.0. (See tarantool/cartridge@bbb9d83) ``` * graphql: extract typeFromAST into separate file Port of tarantool/graphql.0@d2c2dce * graphql: fix typo rename "evaluateSelection" to "evaluateSelections" before we define global variable. * graphql: port util from tarantool/graphql.0 * graphql: move global function to local scope in graphql.types * graphql: adapt "Validate a variable type" fron graphql.0 This patch is a port of tarantool/graphql.0@38fc560 * graphql: get rid of unused variables in graphql/validate.lua * graphql: Fix selection existence validation Port of tarantool/graphql.0@08704ed#diff-34e04e59ba1e150da39b9eeef9a9fe38 * graphql: enum support in validateVariables To preserve backward compatibility ```

Imported from tarantool/cartridge@9e67c8c ``` GraphQL validation significantly improved: scalar values can't have subselections; composite types must have subselections; omitting non-nullable arguments in variable list is forbidden. ```

Also make GraphQL errors neater: eliminate source path from messages. Imported from tarantool/cartridge@3ce7744

Change `"%s"` to `%q` in validate_variable.lua. Inspired by tarantool/cartridge@086d401

Fix luacheck warnings. See also tarantool/cartridge@c94918c

See also tarantool/cartridge@81dabfb ``` 1. Improve graphql tests 2. Make "isValueOfTheType" required field for scalar types In order to avoid possible misuse. It will throw an error anyway. This infact moves such check from runtime[1] to declaration time. Closes tarantool/cartridge#854 [1] https://github.com/tarantool/cartridge/blob/a9165b69af471e2c9d7ee1dce516c62c8389b9e8/cartridge/graphql/validate_variables.lua#L98 3. graphql: fix possible type mismatch between field type and variable type Closes tarantool/cartridge#853 ```

Imported from tarantool/cartridge@6eb4a43 ``` See: https://graphql.org/learn/queries/#default-variables Note: there is a difference between explicitly passed "null" value and just omitted variable. See graphql/graphql-spec#418 Closes tarantool/cartridge#866 ```

Imported from tarantool/cartridge@7ee77c9 ``` This patch fixes two problems. When we return wrong value for List of nonNull type. nil dereference happened because error handler didn't consider nonNullable wrapper. innerType.name for Type is "Type" and nil for Type! (expected "Type" or "NonNull(Type)"). (https://github.com/tarantool/cartridge/pull/919/files#diff-926c2702f04ab81baa008eec661b481eR200) Scalar could be passed instead of Object. This patch adds a check that Object is "table". (https://github.com/tarantool/cartridge/pull/919/files#diff-926c2702f04ab81baa008eec661b481eR218) ```

Imported from tarantool/cartridge@95a924e Before this patch when we tried to coerse inputObject/list to Scalar type we got an error: ``` graphql internal error: Could not coerce "nil" to "String" ``` It's completely non-informative. This patch fixes it and changes message to: ``` graphql internal error: Could not coerce "inputObject" to "String" ```

One extension of cartridge graphql is the possibility to resolve graphql types by name. However, sometimes it leads to some unpredictable results. E.g. one type could easily redefine another. This patch introduces a new option "schema" for complex types. It allows to avoid types sharing between schemas. Before patch "Encountered multiple types" error occurred. Imported from tarantool/cartridge@b8e12ad

We want to create our own scalar type with JSON parsing. Due to the fact that the `parseValue` function isn't called, it's impossible to make the correct use of the variables with JSON type. In particular this test fails: ```lua local resp = server:graphql({ query = 'query($field: Json) { test_json_type(field: $field) }', variables = {field = '{"test": 123}'}} ) t.assert_equals(resp.data.test_json_type, '{"test":123}') ``` Changes - The `parseValue` function is called during the variables parsing step. - In `parseValue` and `parseLiteral` one can use `box.NULL` as a return value, returning `nil` is considered as a parsing error. - The `serialize` function is called for `nil` scalar values too. - Parsing of primitive scalar variables is fixed, `parseValue` is eliminated there. - The `parseLiteral` function is mandatory for scalar types Links https://medium.com/@alizhdanov/lets-understand-graphql-scalars-3b2b016feb4a https://atheros.ai/blog/how-to-design-graphql-custom-scalars Imported from tarantool/cartridge@fb4b3a3

Acually it was unused

In following commits it will be replaced with luatest written tests that consider changes that have been already done.

This test are tests that were removed in previous commit but written in luatest (originally it was done here tarantool/cartridge@374dddf). Also it contains several changes after some bugs were fixed: * tarantool/cartridge@81dabfb#diff-c490ea8b58fb3785da5cf9c55acb6d9b491d2702dd06dbbbe5e5964fefe21efd * tarantool/cartridge@b8e12ad#diff-c490ea8b58fb3785da5cf9c55acb6d9b491d2702dd06dbbbe5e5964fefe21efd * tarantool/cartridge@fb4b3a3#diff-c490ea8b58fb3785da5cf9c55acb6d9b491d2702dd06dbbbe5e5964fefe21efd

Copy-pasted from cartridge as well

This patch adds ".luacheck.rc" configuration file for luacheck.

Before this patch: ```lua tarantool> require('graphql.parse').parse('{ Test') --- - error: Syntax error near line 1 ... ``` It was completely unclear where an error occurred. After this patch: ```lua tarantool> require('luagraphqlparser').parse('{ Test') --- - null - '1.7: syntax error, unexpected EOF' ... ``` Error occured at the first line, seventh character and it's "unexpected EOF". But this patch breaks backward compatibility. Before this patch empty bracets was allowd. After this patch behaviour is following: ```lua tarantool> require('luagraphqlparser').parse('{ }') --- - null - '1.3: syntax error, unexpected }' ... ``` This error is returned from libgraphqlparser[1]. And seems changes could be considered as bugfix. Also lulpeg based graphql doesn't support inline "null" value that's not so in libgraphqlparser. [1] https://github.com/graphql/libgraphqlparser

Before this patch graphql parser doesn't support "null" literal. Since parser was changed we could support "null" literal specification.

This patch adds tests from cartridge (https://github.com/tarantool/cartridge/blob/7396390a5bbdefba123e632f55fb4a54d2391295/test/integration/graphql_test.lua). It also imports tests for "null value support" (ba99503) commit that imported from tarantool/cartridge@7d2c2e1

The problem was introduced in e37651d (support "null" arguments) here we add redundant check that started to throw error "graphql/util.lua:72: attempt to index local 'node' (a nil value)" This patch removes this check and adds test (with small difference copied from TDG) that catches a problem.

* Changes installation instruction * Fix example to conform Tarantool Lua Styleguide * Fix features list (we use luagraphqlparser in contrast of original module) * Fix instruction for running tests (currently they are implemented in luatest)

olegrok added 30 commits February 11, 2021 14:23

Hold order of fields traversal

7f817bb

``` The certain order of execution is required for mutations. See: 8ba78a6 Signed-off-by: Yaroslav Dynnikov <[email protected]> ```

allow to pass fieldName in execute

9718dea

Imported from tarantool/cartridge@3570988 Seems it was needed to improve error message

add accessor to nonNull version of lists to list object

7cdfa06

Imported from tarantool/cartridge@1936080

Fix graphql coerce: include InputObject field names

6925795

Imported from tarantool/cartridge@0f6d63d

add argument positions

86721a8

Imported from tarantool/cartridge@c41ac7d

add key and value tuple for positioning graphql arguments

8c99d09

Imported from tarantool/cartridge@2b27b01

return box.NULL as value if function returns nil

61b322e

Imported from tarantool/cartridge@7b52600

throw error if second return value from resolver is not nil

59176b2

Some modules use `return nil, err` approach for error handling. This patch rethow an error in case if the second argument (err) is not nil. Imported from tarantool/cartridge@cde7c47

remove redundant spaces from types description

85fdd49

Imported from tarantool/cartridge@a8ac273

improve verbosity in graphql errors

6675061

Imported from tarantool/cartridge@9e67c8c ``` GraphQL validation significantly improved: scalar values can't have subselections; composite types must have subselections; omitting non-nullable arguments in variable list is forbidden. ```

properly handle nested input objects

4d4498a

Also make GraphQL errors neater: eliminate source path from messages. Imported from tarantool/cartridge@3ce7744

prettify error messages

0cc793a

Change `"%s"` to `%q` in validate_variable.lua. Inspired by tarantool/cartridge@086d401

enable linting

c0f3fc5

Fix luacheck warnings. See also tarantool/cartridge@c94918c

drop init file

9e8ed48

Acually it was unused

test: drop old tests

baa9b72

In following commits it will be replaced with luatest written tests that consider changes that have been already done.

add gitignore to the project

d496f10

Copy-pasted from cartridge as well

add luacheck configuration

457ba82

This patch adds ".luacheck.rc" configuration file for luacheck.

support "null" arguments

e37651d

Before this patch graphql parser doesn't support "null" literal. Since parser was changed we could support "null" literal specification.

olegrok added 2 commits February 12, 2021 13:32

setup tests and linter run in CI

7be9372

olegrok force-pushed the cartridge-import branch from e8ebabf to a995138 Compare February 12, 2021 10:42

olegrok mentioned this pull request Feb 12, 2021

add graphql submodule tarantool/cartridge#1261

Merged

olegrok added 2 commits February 15, 2021 22:06

update readme

ef7dd0f

* Changes installation instruction * Fix example to conform Tarantool Lua Styleguide * Fix features list (we use luagraphqlparser in contrast of original module) * Fix instruction for running tests (currently they are implemented in luatest)

olegrok force-pushed the cartridge-import branch from 7c865c1 to ef7dd0f Compare February 15, 2021 19:06

add workflow for pushing rockspec/rocks

d01b2d4

rosik approved these changes Feb 25, 2021

View reviewed changes

olegrok merged commit 681453d into master Feb 25, 2021

olegrok deleted the cartridge-import branch February 25, 2021 13:43

olegrok mentioned this pull request Feb 25, 2021

Added luacheck configuration to the project #5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Import patches from tarantool/cartridge #9

Import patches from tarantool/cartridge #9

Uh oh!

olegrok commented Feb 12, 2021 •

edited

Loading

Uh oh!

Uh oh!

Import patches from tarantool/cartridge #9

Import patches from tarantool/cartridge #9

Uh oh!

Conversation

olegrok commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

olegrok commented Feb 12, 2021 •

edited

Loading