Add creation of avro-schema from query with multi-head connections, Unions, Maps and directives #201

SudoBobo · 2018-08-02T19:37:28Z

Also changed GraphQL Map type: now it has subType = Map and name = values.type .. '_Map'
(Example: 'String_Map') This is to avoid name clash.
Сloses #200 #198 #197 #85 #84

Totktonada

Comments for maps generation.

Totktonada · 2018-08-02T20:15:06Z

graphql/query_to_avro.lua

@@ -29,7 +33,9 @@ local gql_scalar_to_avro_index = {

 local function gql_scalar_to_avro(fieldType)
    assert(fieldType.__type == "Scalar", "GraphQL scalar field expected")
-    assert(fieldType.name ~= "Map", "Map type is not supported")
+    if (fieldType.subtype == "Map") then


Redundant parenthesis.

Sorry, fixed.

Totktonada · 2018-08-07T14:50:14Z

graphql/convert_schema/types.lua


-        local res = core_types.map
+        local res = core_types.map({values = converted_values, name = map_name })


Code style: no whitespace after '{' and whitespace before '}' (I prefer to avoid these whitespaces.)

Fixed to 'no whitespaces'

Totktonada · 2018-08-07T14:50:43Z

graphql/convert_schema/types.lua

+        local converted_values = types.convert(state, avro_schema.values,
+            {context = context})
+
+        local true_values_type = converted_values


Use core_types.nullable().

BTW, why do you need this?

BTW, suggest to use 'bare' or 'nullable' name depending of what do you need. The functions named in the same way are in 'core.types'.

Fixed with core_types.nullable() and 'nullable_values_type'.
We need this to name Map type properly. Previously we have single Map type which has no info about values type. Now we have ValuesType_Map GraphQL type for each avro-schema map type. Like this:

{"type": "map", "values": "string"} -> String_Map

After verbal discussion we decided not to name Map types like ValuesType_Map and use context.path .. Map names instead.

Totktonada · 2018-08-07T14:53:02Z

graphql/convert_schema/union.lua

@@ -38,6 +38,9 @@ local function box_type(type_to_box, box_field_name, opts)
    -- for the argument type to avoid 'Encountered multiple types' error. See
    -- also the comment in @{types.box_collection_type}.
    local box_name = (gql_true_type.name or gql_true_type.__type) .. '_box'
+    if (gql_true_type.__type == "Scalar" and gql_true_type.subtype == 'Map') then


You are use two types of quotation marks in the one line.

BTW, redundant parenthesis.

Sorry, fixed.

Totktonada · 2018-08-07T14:54:48Z

graphql/core/types.lua

-  end,
-})
+function types.map(config)
+    local instance = {


2 spaces indent in this file.

Totktonada · 2018-08-07T14:56:58Z

graphql/core/types.lua

-      'we should not go here')
-  end,
-})
+function types.map(config)


Add nonNull into the instance (see types.inputMap).

I thought we use another mechanism for setting nullability.
Like this:

local res = core_types.map({values = converted_values, name = map_name}) return avro_t == 'map' and core_types.nonNull(res) or res

We do like this with records, arrays, etc. Why we may want to handle maps different way?

We don’t, but I propose to support it in the type object for unification with other type objects.

Well, okay, 'for unification' is a valid argument for me.

Totktonada · 2018-08-07T14:59:22Z

graphql/query_to_avro.lua

+--- The function converts a GraphQL Map type to avro-schema map type.
+map_to_avro = function(mapType)
+    assert(mapType.values ~= nil, "GraphQL Map type must have 'values' field")
+    return  {


Double whitespace.

Totktonada · 2018-08-07T15:11:59Z

graphql/convert_schema/union.lua

@@ -38,6 +38,9 @@ local function box_type(type_to_box, box_field_name, opts)
    -- for the argument type to avoid 'Encountered multiple types' error. See
    -- also the comment in @{types.box_collection_type}.
    local box_name = (gql_true_type.name or gql_true_type.__type) .. '_box'
+    if (gql_true_type.__type == "Scalar" and gql_true_type.subtype == 'Map') then
+        box_name = 'Map_box'


Is it for compatibility? For cases when a map is inside a union?

Exactly. Previously we created box_name = ScalarType.name .. '_box' and this covered Map type case as well.
But now Map type has some custom name, not 'Map', so we have to manually name box_type.

So, please, let short comment here.

It seems that base_name below does thw work and that if is not needed here.

You are right. Removed that if

Totktonada · 2018-08-07T15:12:37Z

graphql/query_to_avro.lua

+    assert(mapType.values ~= nil, "GraphQL Map type must have 'values' field")
+    return  {
+        type = "map",
+        values =  gql_type_to_avro(mapType.values),


Double whitespace.

Totktonada

A bit more comments re 'Maps' commit.

Totktonada · 2018-08-08T10:01:36Z

graphql/convert_schema/types.lua


-        local res = core_types.map
+        local map_name =  helpers.full_name("Map", context)


Sorry for nitpicking: single quotes is used in this file.

Fixed. Consistency is good

Totktonada · 2018-08-08T10:14:32Z

graphql/convert_schema/types.lua

@@ -448,10 +448,11 @@ function types.convert(state, avro_schema, opts)
            'got %s (avro_schema %s)'):format(type(avro_schema.values),
            json.encode(avro_schema)))

-        -- validate avro schema format inside 'values'
-        types.convert(state, avro_schema.values, {context = context})
+        local converted_values = types.convert(state, avro_schema.values,


Add 'Map' to path before converting and remove after. It does not matter in context of name clashes (I think), but would be consistent with the other converting code: we add type names to path.

Totktonada · 2018-08-08T10:14:55Z

graphql/convert_schema/union.lua

@@ -38,6 +38,9 @@ local function box_type(type_to_box, box_field_name, opts)
    -- for the argument type to avoid 'Encountered multiple types' error. See
    -- also the comment in @{types.box_collection_type}.
    local box_name = (gql_true_type.name or gql_true_type.__type) .. '_box'
+    if (gql_true_type.__type == "Scalar" and gql_true_type.subtype == 'Map') then
+        box_name = 'Map_box'


So, please, let short comment here.

Totktonada · 2018-08-08T10:18:42Z

graphql/query_to_avro.lua

@@ -29,7 +30,9 @@ local gql_scalar_to_avro_index = {

 local function gql_scalar_to_avro(fieldType)
    assert(fieldType.__type == "Scalar", "GraphQL scalar field expected")
-    assert(fieldType.name ~= "Map", "Map type is not supported")
+    if fieldType.subtype == "Map" then


Note: here double quotes is used accross the file, so it is okay. The inconsistency btw file is because of different origin authors.

Totktonada · 2018-08-08T11:30:16Z

graphql/convert_schema/union.lua

@@ -38,6 +38,9 @@ local function box_type(type_to_box, box_field_name, opts)
    -- for the argument type to avoid 'Encountered multiple types' error. See
    -- also the comment in @{types.box_collection_type}.
    local box_name = (gql_true_type.name or gql_true_type.__type) .. '_box'
+    if (gql_true_type.__type == "Scalar" and gql_true_type.subtype == 'Map') then
+        box_name = 'Map_box'


It seems that base_name below does thw work and that if is not needed here.

Totktonada

Comments for null multi-head connections.

Totktonada · 2018-08-08T12:49:26Z

graphql/convert_schema/resolve.lua

@@ -190,6 +190,25 @@ function resolve.gen_resolve_function_multihead(collection_name, connection,
    end

    return function(parent, _, info)
+        -- If parent object does not have all source_fields (for any of
+        -- variants) non-nill then we do not resolve variant and just return


Typos:

non-nill → non-null

parent object → a parent object

source_fields → source fields (because it is not the name of a field)

started with uppercase letter, but does not end with a period.

Thank you, fixed.

Totktonada · 2018-08-08T14:16:51Z

graphql/convert_schema/resolve.lua

@@ -190,6 +190,25 @@ function resolve.gen_resolve_function_multihead(collection_name, connection,
    end

    return function(parent, _, info)
+        -- If parent object does not have all source_fields (for any of


I think we should allow to omit a variant determination only when source fields for all variants are null.

It would be good to have it described explicitly in README. Like so:

Multi-head connections

A parent object is matching against a multi-head connection variants in the order of the variants. The parent object should match with a determinant of at least one variant except the following case. When source fields of all variants are null the multi-head connection obligated to give null object as the result. In this case the parent object is allowed to don’t match any variant. One can use this feature to avoid to set any specific determinant value when a multi-head connection is known to have no connected object.

Ideally, we should have description of the entire multi-head connection feature (but it of course is not part of this set of issues).

Do you propose to add this short paragraph to README now?

I think we should allow to omit a variant determination only when source fields for all variants are null.

Ok, done.

Yep, to README.

Totktonada · 2018-08-08T14:18:39Z

graphql/convert_schema/resolve.lua

+        -- variants) non-nill then we do not resolve variant and just return
+        -- box.NULL
+        for _, variant in ipairs(c.variants) do
+            for _, p in ipairs(variant.parts) do


It seems that it would be good to use are_all_parts_null function we already have.

Really not sure. This function has an assertion inside:

assert(ok, 'FULL MATCH constraint was failed: connection ' .. 'key parts must be all non-nulls or all nulls; ' .. 'object: ' .. json.encode(parent))

Consider the case: collection has three fields: 1, 2 and 3. Fields 1 and 2 are used as source fields for connection 1, while fields 2 and 3 are used as source fields for connection 2.
And some collection object has fields 1 and 2 not null and field 3 null. This is ok in general, but would met assertion condition.

After discussion in chat we decided to add opts argument to are_all_parts_null which would suppress assert and use this function.

Totktonada · 2018-08-08T14:52:56Z

test/testdata/multihead_conn_testdata_with_nulls.lua

@@ -0,0 +1,456 @@
+local tap = require('tap')


Maybe briefly describe why it is needed and where differs from multihead_conn_testdata.lua? Maybe also it worth to link the issue here.

multihead_conn_testdata_with_nulls.lua → multihead_conn_with_nulls_testdata.lua

Added brief description. Link to the issue seems inappropriate here from my point of view.

Fixed.

Totktonada · 2018-08-08T14:53:53Z

test/testdata/multihead_conn_testdata_with_nulls.lua

+end
+
+function multihead_conn_testdata.run_queries(gql_wrapper)
+    local test = tap.test('multihead_conn_null')


multihead_conn_null → multihead_conn_with_nulls

For the sake of consistency.

Totktonada · 2018-08-08T14:58:18Z

test/testdata/multihead_conn_testdata_with_nulls.lua

+    ]]):strip())
+    test:is_deeply(result_1_2.data, exp_result_1_2, '1_2')
+
+    local variables_1_3 = {hero_id = 'hero_id_3'}


Please, comment this case and link with the issue.

Totktonada

Comments for 'small refactor'. BTW, code style fixes is not a refactorng.

Totktonada · 2018-08-08T15:22:45Z

graphql/query_to_avro.lua

@@ -176,19 +176,19 @@ union_to_avro = function(fieldType, subSelections, context)
        if box_sub_selections.selectionSet.selections[1].selectionSet ~= nil then
            -- Object GraphQL type case.
            type_sub_selections =
-                box_sub_selections.selectionSet.selections[1].selectionSet.selections
+            box_sub_selections.selectionSet.selections[1].selectionSet.selections


Don’t break indent. Carry like so:

aaaaa = xxxxxxx.yyyyyyyy .zzzzzzz

Can be merged with 'Make small clean up'.

Totktonada · 2018-08-08T15:24:30Z

graphql/query_to_avro.lua

        else
            -- Scalar GraphQL type case.
            type_sub_selections = box_sub_selections.selectionSet.selections[1]
        end

        if is_multihead then
            local avro_type = gql_type_to_avro(type.kind,
-            type_sub_selections, context, {is_nullable = true})
+                type_sub_selections, context, {is_nullable = true})


Your wrote it within the PR in the commit re unions. Merge it into that.

Totktonada · 2018-08-08T15:24:35Z

graphql/query_to_avro.lua

            table.insert(result.fields, {name = type.name, type = avro_type})
        else
            table.insert(result, gql_type_to_avro(type.kind,
-            type_sub_selections, context))
+                type_sub_selections, context))


Your wrote it within the PR in the commit re unions. Merge it into that.

Totktonada · 2018-08-08T15:25:29Z

graphql/query_to_avro.lua

@@ -224,7 +224,7 @@ local function field_to_avro(object_type, fields, context)
            assert(d.kind == "directive")
            check(d.name.value, "directive.name.value", "string")
            assert(d.name.value == "include" or d.name.value == "skip",
-            "Only 'include' and 'skip' directives are supported for now")
+                "Only 'include' and 'skip' directives are supported for now")


Your wrote it within the PR in the commit re directives. Merge it into that.

Totktonada · 2018-08-08T16:07:03Z

test/test_utils.lua

+
+-- Converts map recursively into array. If an array with maps is
+-- encountered then all this maps will be converted too.
+function test_utils.convert_table_to_array(map)


function recursively_map_to_array(t) if type(t) == 'table' and not utils.is_array(t) do local res = {} for k, v in pairs(t) do assert(type(k) == 'string') table.insert(res, {k, v}) end table.sort(res, function(a, b) return a[1] < b[1] end) return res end return t end

I don’t test it. Just propose a bit simper variant.

pairs would give us numbers (1, 2, 3) as well, wouldn't it?
So assert(type(k)) == 'string' should fail.
And we may have maps inside arrays as well.

Sorry, forget about recursion. It should like so I guess:

function recursively_map_to_array(t) if type(t) == 'table' and not utils.is_array(t) then local res = {} for k, v in pairs(t) do assert(type(k) == 'string') table.insert(res, {k, recursively_map_to_array(v)}) end table.sort(res, function(a, b) return a[1] < b[1] end) return res elseif type(t) == 'table' then local res = {} for _, v in ipairs(t) do table.insert(res, recursively_map_to_array(v)) end return res end return t end

But as discussed verbally field order in arrays are changed sporadically too and it is hard to simply sort it, because elements can be tables.

Totktonada

Several months later even you himself will have difficulties with saying for what it was. Comment it in the commit message, add usage in introspection.test.lua.

Totktonada

Comments re unions.

Totktonada · 2018-08-08T16:13:53Z

graphql/query_to_avro.lua

@@ -97,6 +113,88 @@ map_to_avro = function(mapType)
    }
 end

+--- The function converts a GraphQL Union type to avro-schema type.


'The function' is redundant. Just 'Convert …'.

Totktonada · 2018-08-08T16:18:20Z

graphql/query_to_avro.lua

+--- avro-schema records. Each field represents one union variant. Variant type
+--- name is taken as a field name. Such records must have all fields nullable.
+---
+--- We convert multi-head Unions to records instead of unions because in case


multi-head Unions → multi-head connections?

I think writing 'multi-head connections' here is a little bit wrong, because we are talking in this comment not about multi-head connections, but about their implementation - Unions.
Changed like this:
multi-head Unions → Unions implementing multi-head connections

We are going to be too formal :) I’m okay.

Totktonada · 2018-08-08T16:22:22Z

graphql/query_to_avro.lua

+--- We convert multi-head Unions to records instead of unions because in case
+--- of 1:N connections we would not have valid avro-schema (if use unions).
+--- This is because according to avro-schema standard unions may not contain
+--- more than one schema with the same type (in case of 1:N multi-head


I propose to reuse standard wording here:

Unions may not contain more than one schema with the same type, except for the named types record, fixed and enum.

Note for 'except for' part.

Totktonada · 2018-08-08T16:22:45Z

graphql/query_to_avro.lua

+--- of 1:N connections we would not have valid avro-schema (if use unions).
+--- This is because according to avro-schema standard unions may not contain
+--- more than one schema with the same type (in case of 1:N multi-head
+--- connections we would have more than one 'array' in union)


No period at the end.

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

graphql/query_to_avro.lua

+        assert(utils.table_size(box_type.fields) == 1, 'Box Object must ' ..
+            'have exactly one field')
+        local type =
+            box_type.fields[utils.get_keys(box_type.fields)[1]]


local type = select(2, next(box_type.fields))

BTW, I would prefer name field_name.

Fixed with select.
field_name seems to be a wrong naming since type is actually a type. Latter we use type.name and type.kind

Totktonada · 2018-08-08T16:33:22Z

graphql/query_to_avro.lua

+
+        if is_multihead then
+            local avro_type = gql_type_to_avro(type.kind,
+            type_sub_selections, context, {is_nullable = true})


There is avro_helpers.make_avro_type_nullable, so you don’t need to introduce the new option.

Ok, option removed.

Totktonada · 2018-08-08T16:34:18Z

graphql/query_to_avro.lua

+    assert(fieldType.types ~= nil, "GraphQL Union must have 'types' field")
+    check(fieldType.types, "fieldType.types", "table")
+    local is_multihead = (fieldType.resolveType == nil)
+    local result = {}


In case of is_multihead the table ({}) is always replaced with the new one.

Yes, it was my intention. I thought this previous construction is clearer then:

if is_multihead then check(fieldType.name, "fieldType.name", "string") result = { type = 'record', name = fieldType.name, fields = {} } else result = {} end

Maybe it is clearer in some way, maybe doesn’t. I would prefer to avoid it, because don’t sure how such empty objects can affect gc.

Totktonada · 2018-08-08T16:36:58Z

graphql/query_to_avro.lua

+            type_sub_selections, context, {is_nullable = true})
+            table.insert(result.fields, {name = type.name, type = avro_type})
+        else
+            table.insert(result, gql_type_to_avro(type.kind,


We need to add 'null' depending of whether the GraphQL Union is nullable. I think it is already handled somewhere with avro_helpers.make_avro_type_nullable. Is it so?

It is. We do it in gql_type_to_avro() before returning result in case of 'natural' Unions.

Seems to be correct. Thanks.

Totktonada · 2018-08-08T16:56:03Z

test/extra/to_avro_multihead.test.lua

+local meta = testdata.get_test_metadata()
+testdata.fill_test_data(box.space, meta)
+
+local accessor = graphql.accessor_space.new({


Better to use just graphql.new, I want to deprecate explicit use of built-in accessors at some time.

Totktonada · 2018-08-08T16:56:20Z

test/extra/to_avro_multihead.test.lua

+]]
+
+local expected_avro_schema = [[
+type: record


Totktonada

Comments for directives support.

The test is really huge, thanks!

Totktonada · 2018-08-09T04:50:31Z

graphql/query_to_avro.lua

+            assert(d.name.value == "include" or d.name.value == "skip",
+            "Only 'include' and 'skip' directives are supported for now")
+        end
+        if type(fieldTypeAvro) == "string" then


Just use avro_helpers.make_avro_type_nullable.

Totktonada · 2018-08-09T04:51:23Z