Support avro-unions #115

SudoBobo · 2018-04-09T12:13:27Z

Closes #109 (but there is blocking problem described here tarantool/avro-schema#64)

Totktonada · 2018-04-11T05:49:02Z

graphql/tarantool_graphql.lua

+--- The function 'boxes' given GraphQL type so it can be used in GraphQL unions.
+--- By specification GraphQL may contain only objects. As we want to use scalar
+--- types as unions' variants, we 'box' (or wrap) them into object types with
+--- one field - _boxes. GraphQL objects are boxed for unification.


The main reason is type determinant form in avro-schema.

Fixed and clarified comment.

Totktonada · 2018-04-11T06:52:03Z

graphql/tarantool_graphql.lua

+    check(gql_type, 'gql_type', 'table')
+
+    local box_name, box_fields
+    local gql_true_type = nullable(gql_type)


We need to know true type to perform proper logic later. Refactored this part a bit.

Totktonada · 2018-04-11T07:02:26Z

graphql/tarantool_graphql.lua

+    --require('pl.pretty').dump(avro_schema)
+
+    for _, type in ipairs(avro_schema) do
+        if type == 'null' then


The approach to represent unions with null need to be commented.

Totktonada

Please, consider comments below. Some of them can be outdated after our chat discussions.

Totktonada · 2018-04-13T11:46:31Z

graphql/tarantool_graphql.lua

@@ -64,6 +64,8 @@ local function avro_type(avro_schema)
            return 'record*'
        elseif utils.is_array(avro_schema) then
            return 'union'
+        elseif utils.is_array(avro_schema.type) then
+            return 'union'


The above utils.is_array(avro_schema) branch seems to be dead code? If so, it need to be removed.

But it seems to be more consistent if we’ll use this function only for avro types and never for record fields. So, it wuold be good to fix problems with using the above utils.is_array(avro_schema) branch.

Totktonada · 2018-04-13T11:48:16Z

graphql/tarantool_graphql.lua

-            name = field.name,
-            kind = gql_type(state, field.type),
-        }
+        -- union case


Please, move the comment into the corresponding if branch.

No more actual.

Totktonada · 2018-04-13T11:50:58Z

graphql/tarantool_graphql.lua

+        if utils.is_array(field.type) then
+            res[field.name] = {
+                name = field.name,
+                kind = gql_type(state, {name = field.name, type = field.type}),


No more actual.

Totktonada · 2018-04-13T11:58:15Z

graphql/utils.lua

@@ -204,4 +204,13 @@ function utils.table_size(t)
    return count
 end

+function utils.is_in_array(key, array)


key → value

I have the same function in the another project named value_in. Maybe it worth to reuse such names. Up to you.

Agree. Fxd.

Totktonada · 2018-04-13T12:20:37Z

graphql/tarantool_graphql.lua

+--- 'Boxes' is a part of this specific form.
+---
+--- @tparam table gql_type GraphQL type to be boxed
+--- @param avro_schema table or string ... necessary for right names for boxed


avro_schema → avro_name?

Sorry, forgot to finish comment. Now it is finished.

Totktonada · 2018-04-13T13:16:36Z

graphql/tarantool_graphql.lua

+                if i ~= j then
+                    assert(avro_type(type) ~= avro_type(another_type),
+                        'Unions may not contain more than one schema with ' ..
+                            'the same type except for the named types ' ..


colon need to be after 'named types'

Totktonada · 2018-04-13T13:17:54Z

graphql/tarantool_graphql.lua

+        if type.name == nil then
+            for j, another_type in ipairs(avro_schema) do
+                if i ~= j then
+                    assert(avro_type(type) ~= avro_type(another_type),


Two records with different names are allowed by an avro-schema, but will not pass this check? Ok for postpone to backlog.

Two records will pass this check as their type.name is not nil.

Even when names are the same?

Totktonada · 2018-04-13T13:19:55Z

graphql/tarantool_graphql.lua

+            end
+            error(('result objects "%s" has no determinant field matching ' ..
+                'determinants for this union "%s"'):format(yaml.encode(result),
+                yaml.encode(field_to_type)))


IMHO, it is better to use json module for inline objects or use yaml and place multilne objects on its own line(s) after an error message.

I prefer second variant as it is seems easier to read. Fxd.

Totktonada · 2018-04-13T13:56:48Z

test/local/avro_union.test.lua

+    }
+}
+
+local USER_ID = 2


USER_ID → USER_ID_FN or USER_ID_FIELD

Totktonada · 2018-04-13T14:00:37Z

test/local/avro_union.test.lua

+    print('Validating results with initial avro-schema')
+    local _, schema = avro_schema.create(schemas.user_collection)
+    for _, r in ipairs(result.user_collection) do
+        local ok, err = avro_schema.validate(schema, r)


It is okay for this test, but in the future we need to support converting unions to avro-schema. Don’t sure there are an issue (likely not).

You mean from GraphQL Union to avro union?

Not exactly. We need to convert a query (in context of a graphql schema we can generate from some avro-schema) to avro-schema back. A part of this convertion is unions. They will be pass the way avro → graphql → avro :)

Totktonada · 2018-04-13T14:09:49Z

graphql/tarantool_graphql.lua

@@ -688,7 +836,7 @@ end
 --- 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
 --- is allowed for now. Note: map is considered scalar.
-gql_type = function(state, avro_schema, collection, collection_name)
+gql_type = function(state, avro_schema, collection, collection_name, field_name)


ldoc need to be updated.

Need to state explicitly that it wil be used only for an union generation, because avro-schema union has no any name in it.

Totktonada · 2018-04-13T17:39:09Z

graphql/tarantool_graphql.lua

+--- 1) GraphQL Union types may contain only GraphQL Objects. GraphQL Scalar
+--- types may be put inside Union only if they are 'boxed' inside an Object.
+--- 2) GraphQL Union types must have specific form, described in @{create_gql_union}.
+--- 'Boxes' is a part of this specific form.


specific form
described in

Common words and the link. It is not good problem statement. Before the last change you stated explicitly that the problem is in avro-schema union, but mistakely use the term 'initial avro-schema' (you have no one, see the comment below). Now you don’t state any problem here (in point 2). Please, give back the old comment and ask me if you have problems with check correctness of the wording.

I moved problem description to comments to create_gql_union()

Totktonada · 2018-04-13T17:58:39Z

LGTM.

I wait you will clarify some points in comments, but going to leave you face-to-face with fighting with wording :)

… and records), closes #109

SudoBobo requested a review from Totktonada April 9, 2018 12:13

SudoBobo force-pushed the sb/avro-union branch from 9d6cf22 to 18ecf1f Compare April 9, 2018 12:28

Totktonada reviewed Apr 11, 2018

View reviewed changes

SudoBobo force-pushed the sb/avro-union branch 2 times, most recently from 550cd80 to aa89196 Compare April 12, 2018 19:09

SudoBobo changed the title ~~WIP PR~~ Support avro-unions Apr 12, 2018

SudoBobo force-pushed the sb/avro-union branch 3 times, most recently from f491502 to 777a607 Compare April 13, 2018 12:39

Totktonada reviewed Apr 13, 2018

View reviewed changes

SudoBobo force-pushed the sb/avro-union branch 4 times, most recently from f5bd594 to bf5f2a5 Compare April 13, 2018 17:11

Totktonada reviewed Apr 13, 2018

View reviewed changes

Totktonada approved these changes Apr 13, 2018

View reviewed changes

SudoBobo force-pushed the sb/avro-union branch 3 times, most recently from 1ba583b to 0a4a7dd Compare April 13, 2018 18:59

Add support for avro-schema unions(including unions with arrays, maps…

46116a2

… and records), closes #109

SudoBobo force-pushed the sb/avro-union branch from 0a4a7dd to 46116a2 Compare April 13, 2018 20:44

SudoBobo merged commit e08cb5c into master Apr 13, 2018

SudoBobo deleted the sb/avro-union branch May 26, 2018 18:07

Support avro-unions #115

Support avro-unions #115

Conversation

SudoBobo commented Apr 9, 2018 • edited Loading

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

Totktonada left a 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

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

Totktonada Apr 13, 2018 • edited Loading

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

SudoBobo Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Totktonada commented Apr 13, 2018

SudoBobo commented Apr 9, 2018 •

edited

Loading

Totktonada Apr 13, 2018 •

edited

Loading

SudoBobo Apr 13, 2018 •

edited

Loading