[WIP] Sb/avro arr #48

SudoBobo · 2018-02-21T18:22:42Z

closes #4

generation behavior - now args are NOT generated from arrays; do small refactor; add test for array avro type usage

necessary for tests

move nullable function to utils

Totktonada

Style comments, opinions re implementation approaches.

Totktonada · 2018-02-23T20:01:32Z

graphql/tarantool_graphql.lua

-    assert(gql_class.ofType ~= nil, 'gql_class.ofType must not be nil')
-    return gql_class.ofType
-end
+local nullable = utils.nullable


utils.lua is for auxiliary functions that do not contain domain-specific abstractions or logic. We work with graphql-lua types in core/*.lua and tarantool_graphql.lua. It is better to keep these objects within these files (or create new modules working within such terms), but don't mix abstraction layers. In other words, I propose to give back this function to this module. You can create graphql_utils.lua alternatively, but I don't think we have enough helpers to charge it.

Totktonada · 2018-02-23T20:12:38Z

graphql/tarantool_graphql.lua

@@ -96,15 +95,25 @@ local function convert_scalar_type(avro_schema, opts)
    elseif avro_t == 'string*' then
        return types.string
    end
+
+    if opts.is_items_type then


// This is not more actual because I proposed to remove this option, but I kept this comment here to give you more on our conventions within the project.

Please, keep consistency with asserts and default values for options within the 'opts' parameter. Consider 'raise' as the example. More specific: add the local variable with a default value (or false), add assert for a type of this option. This is more about readability, then about runtime checks, because such function preamble give a developer information re function contract.

Totktonada · 2018-02-23T20:13:28Z

graphql/tarantool_graphql.lua

+
+    if opts.is_items_type then
+        error('avro array items must have know scalar type, not: ' ..
+                json.encode(avro_schema))


We use 4 spaces as indent.

Totktonada · 2018-02-23T20:16:16Z

graphql/tarantool_graphql.lua

@@ -96,15 +95,25 @@ local function convert_scalar_type(avro_schema, opts)
    elseif avro_t == 'string*' then
        return types.string
    end
+
+    if opts.is_items_type then
+        error('avro array items must have know scalar type, not: ' ..


A user must have info to distinguish not-implemented-yet errors from its mistakes in configuration options (like avro-schemas). Proposed wording: "only scalars are supported as array items for now".

Totktonada · 2018-02-23T20:18:39Z

graphql/tarantool_graphql.lua

    end
+


We don't use python-style double empty lines (moreover, here they are within a function, so don't fit PEP8 too). Consider our lua style guidelines: https://tarantool.org/en/doc/1.7/dev_guide/lua_style_guide.html

Totktonada · 2018-02-23T23:40:54Z

graphql/tarantool_graphql.lua

@@ -414,6 +497,8 @@ local function parse_cfg(cfg)
    return state
 end

+--- The function checks if given query has an appropriate type 'query'


It seems that the function does the other action: it checks that one and only one GraphQL operation (query/mutation/subscription) is defined in the AST. That just the minimum check that we got an query AST and not mistakely some other value.

Totktonada · 2018-02-23T23:42:03Z

graphql/tarantool_graphql.lua

@@ -414,6 +497,8 @@ local function parse_cfg(cfg)
    return state
 end

+--- The function checks if given query has an appropriate type 'query'
+--- (mutations are not supported yet).


That note is unrelated to the function. It will doing what it doing despite types of GraphQL operations.

Totktonada · 2018-02-23T23:50:48Z

graphql/tarantool_graphql.lua

@@ -442,6 +529,12 @@ local function gql_execute(qstate, variables)
        operation_name)
 end

+--- The function parses a raw query string, validate the resulting query


The term 'raw' is a bit confusing, I think. Just 'a query string'.

'Validate the resulting query'. Too common. Make it specific by adding 'against the GraphQL scheme.

'Make it ready for execution'. Too common. You can say 'provides an object with the function to execute the query with specific variables values).

Totktonada · 2018-02-23T23:51:20Z

graphql/tarantool_graphql.lua

+--- The function parses a raw query string, validate the resulting query
+--- and make it ready for execution.
+---
+--- @tparam table state current state of graphql-lib, including


We don't used the term 'graphql-lib' before.

Totktonada · 2018-02-23T23:51:50Z

graphql/tarantool_graphql.lua

+---
+--- @tparam table state current state of graphql-lib, including
+--- schemas, collections and accessor
+--- @tparam string query raw query string


Proposed to omit the word 'raw'.

# Conflicts: # graphql/tarantool_graphql.lua

Totktonada

Some style comments.

Totktonada · 2018-02-26T16:54:20Z

graphql/tarantool_graphql.lua

@@ -79,7 +87,7 @@ local function convert_scalar_type(avro_schema, opts)
    assert(type(opts) == 'table', 'opts must be nil or table, got ' ..
        type(opts))
    local raise = opts.raise or false
-    assert(type(opts.raise) == 'boolean', 'opts.raise must be boolean, got ' ..
+    assert(type(raise) == 'boolean', 'opts.raise must be boolean, got ' ..
        type(opts.raise))


Replace opts.raise with raise here too for consistency.

Totktonada · 2018-02-26T16:55:11Z

graphql/tarantool_graphql.lua

    if raise then
        error('unrecognized avro-schema scalar type: ' ..
-            json.encode(avro_schema))
+        json.encode(avro_schema))


Totktonada · 2018-02-26T16:59:25Z

graphql/tarantool_graphql.lua

+--- An error will be raised if avro_schema type is 'record'
+--- and its' fields are not scalar type because currently
+--- triple nesting level (record with record as a field - ok,
+--- record with record wich has inside another level - not ok)


wich → which

Totktonada · 2018-02-26T17:00:00Z

graphql/tarantool_graphql.lua

@@ -149,44 +166,69 @@ local function gql_argument_type(state, avro_schema)
    end
 end

+


Extra empty line.

Totktonada · 2018-02-26T17:04:04Z

graphql/tarantool_graphql.lua

    end
    return args
 end

--- Convert each field of an avro-schema to a graphql type and corresponding
--- argument for an upper graphql type.
+--- Recursively convert each field of an avro-schema to a graphql type and


Totktonada · 2018-02-26T18:56:27Z

test/testdata/array_and_map_testdata.lua

+function array_testdata.get_test_metadata()
+
+    local schemas = json.decode([[{
+    "user": {


Need one more indent level.

Totktonada · 2018-02-26T18:56:57Z

test/testdata/array_and_map_testdata.lua

+            { "name": "user_id", "type": "string" },
+            { "name": "favorite_food", "type": {"type": "array", "items": "string"} }
+        ]
+        }


Here indent is broken too.

Totktonada · 2018-02-26T19:02:55Z

test/testdata/array_and_map_testdata.lua

+    box.once('test_space_init_spaces', function()
+        box.schema.create_space('user_collection')
+        box.space.user_collection:create_index('user_id_index',
+            { type = 'tree', unique = true, parts = {


Minor comment. I’m a bit against such style of curly circles / indent. If the line carry, then do not start it on the same line as open curly bracket.

Totktonada · 2018-02-26T19:03:22Z

test/testdata/array_and_map_testdata.lua

+end
+
+function array_testdata.run_queries(gql_wrapper)
+


Redundant empty line after the function header.

Totktonada · 2018-02-26T19:03:48Z

test/testdata/array_and_map_testdata.lua

+        }
+    ]]
+
+    --assert(false, 'err')


Forgotten debug, I guess.

Totktonada · 2018-02-26T22:16:08Z

graphql/utils.lua 100644 → 100755

The module is not desined to be run as executable. I think it is better to don’t have executable bit set on such modules. @SudoBobo, please, revert (chmod a-x graphql/utils.lua) the change.

and mock accessor

Totktonada · 2018-02-28T11:33:19Z

test/testdata/array_and_map_testdata.lua

+                user_balances {
+                    value
+                }
+                customer_balances


Should it work when you don’t choose any fields inside?

Yes, because it is Map and our Map is a scalar-based type. In GraphQL scalar-based types don't have any inner fields

Okay, I got it. Thanks! I think we should mention record vs map differences in README.md or somewhere like.

Totktonada

README comments.

Totktonada · 2018-02-28T16:30:26Z

README.md

+While Object is a GraphQL
+built-in type, Map is a scalar-based type. In case of Object-based type 
+all key-value pairs are set during type definition and values may have different 
+arbitrary types.


different arbitrary types → different types as defined in the schema; what do you think?

Your variant is more specific.

Totktonada · 2018-02-28T16:34:02Z

README.md

+arbitrary types.
+
+In contrast, all values of Map-based type must have the same
+type and specific key-value pairs are not set during type definition.


specific key-value pairs are not set during type definition → set of valid keys is not defined in the schema, any key-value pair is valid despite name of the key while value has schema-determined type; what do you think?

set of valid keys is not defined in the schema, any key-value pair is valid despite name of the key while value has schema-determined type (which is the same among all values in the map) ?

Totktonada · 2018-02-28T16:34:43Z

README.md

+type and specific key-value pairs are not set during type definition.
+
+Map-based types should be queried as a scalar type, not as an object type 
+(because map's keys are not part of the schema)


Period at the end.

Totktonada · 2018-02-28T16:36:39Z

README.md

+In contrast, all values of Map-based type must have the same
+type and specific key-value pairs are not set during type definition.
+
+Map-based types should be queried as a scalar type, not as an object type 


as a scalar type → as a scalar type (strictly w/o set of nested fields);

as an object type → as an object type (strictly with set of nested fields).

Totktonada · 2018-02-28T16:40:25Z

graphql/core/execute.lua

+  --require('pl.pretty').dump(object)
+  --print('print from default resolver in 86')
+  --require('pl.pretty').dump(info.fieldASTs[1].name.value)
+  --require('pl.pretty').dump(object[info.fieldASTs[1].name.value])


Forgotten debug.

Totktonada · 2018-02-28T16:42:08Z

graphql/core/execute.lua

@@ -148,6 +155,25 @@ end

 local evaluateSelections

+--- check if given object is flat and have only scalars


If you want to add ldoc comment, then use it's convention: start short function description with a capital letter and end it with a period. Otherwise, use a regular comment (--).

Totktonada · 2018-02-28T16:58:32Z

graphql/tarantool_graphql.lua

+        'arbitrary but same among all values type',
+    serialize = function(value) return value end,
+    parseValue = function(value)return value end,
+    -- node == ast


I think you should comment what is purpose of this line of code or remove the line completely.

Totktonada · 2018-02-28T17:00:48Z

graphql/tarantool_graphql.lua

    return nil
 end

 --- Non-recursive version of the @{gql_type} function that returns
 --- InputObject instead of Object.
+--- An error will be raised if avro_schema type is 'record'
+--- and its' fields are not scalar type because currently
+--- triple nesting level (record with record as a field - ok,


Currently triple nesting level... what? Is not supported?

Totktonada · 2018-02-28T17:04:37Z

graphql/tarantool_graphql.lua

@@ -197,6 +251,10 @@ end
 ---    automatically generate corresponding decucible fields.
 --- 2. The collection name will be used as the resulting graphql type name
 ---    instead of the avro-schema name.
+---
+--- XXX As it is not clear now what to do with complex types inside arrays
+--- (just pass to results or allow to use filters), only scalar arrays


You can mention here that map is included into scalar types for now.

Totktonada · 2018-02-28T17:08:55Z

graphql/tarantool_graphql.lua

+            'got %s (avro_schema %s)'):format(type(avro_schema.values),
+            json.encode(avro_schema)))
+
+        gql_type(state, avro_schema.values)


Do you want to comment that it is for asserts? Just to validate the values format against avro-schema format (

SudoBobo added 8 commits February 19, 2018 19:46

add comments on gql queries assert and compile functions

589778a

do small refactor; add comments

417c83e

add support for array avro type (only with scalars); change args

24ec6c4

generation behavior - now args are NOT generated from arrays; do small refactor; add test for array avro type usage

simplify conditional statements

9a43096

add mised simple test for array support

6bd1ca2

add simple test for array avro type usage

31897d6

move nullable function to utils as it is will be

f0fb9f4

necessary for tests

readability refactor; clean up comments;

ac9f0ed

move nullable function to utils

SudoBobo force-pushed the sb/avro_arr branch from 6bd1ca2 to ac9f0ed Compare February 23, 2018 17:28

Update tarantool_graphql.lua

8110208

Totktonada reviewed Feb 23, 2018

View reviewed changes

SudoBobo added 8 commits February 24, 2018 16:20

Merge remote-tracking branch 'origin/sb/avro_arr' into sb/avro_arr

f50f9fc

# Conflicts: # graphql/tarantool_graphql.lua

small style fixes

043d6f9

readability and comments fixes

7714393

remove excess test

470e8e1

remove redundant 'map' test cases

0160c69

fix small error; now general convert_scalar_type accepts nil as opt

19b8d8b

add test for avro-array usage (with space accessor);

1377c75

move nullable back to tarantool_graphql from utils

5af08d8

Totktonada reviewed Feb 26, 2018

View reviewed changes

Totktonada assigned SudoBobo Feb 27, 2018

Totktonada added enhancement New feature or request prio1 labels Feb 27, 2018

SudoBobo added 3 commits February 27, 2018 11:29

add support of avro map and test for simple case with scalar map values

be8457c

and mock accessor

add test for map with space accessor

fec3b31

add gql support of avro map/array with records as items/values

0423ec4

Totktonada reviewed Feb 28, 2018

View reviewed changes

SudoBobo added 2 commits February 28, 2018 14:36

style/readability fixes after review

a936771

remove executable bit

c8abf95

SudoBobo added 3 commits February 28, 2018 16:41

add info about Map

951e7ea

typo fix

91bd8bf

Merge branch 'master' into sb/avro_arr

148f69f

Totktonada reviewed Feb 28, 2018

View reviewed changes

remove whitespaces at the end of lines

d9a7623

Totktonada reviewed Feb 28, 2018

View reviewed changes

SudoBobo added 3 commits February 28, 2018 19:42

Merge remote-tracking branch 'origin/sb/avro_arr' into sb/avro_arr

31abefa

remove all debug prints

1f2583a

make Map and Object explanation more specific

e406a31

Totktonada reviewed Feb 28, 2018

View reviewed changes

Totktonada closed this in b046ce8 Feb 28, 2018

Totktonada deleted the sb/avro_arr branch February 28, 2018 21:31

		@@ -149,44 +166,69 @@ local function gql_argument_type(state, avro_schema)
		end
		end

		@@ -148,6 +155,25 @@ end

		local evaluateSelections

		--- check if given object is flat and have only scalars

[WIP] Sb/avro arr #48

[WIP] Sb/avro arr #48

Uh oh!

Conversation

SudoBobo commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Totktonada 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!

Totktonada 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!

Totktonada commented Feb 26, 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!

Totktonada 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!

SudoBobo commented Feb 21, 2018 •

edited

Loading