Implement runtime dangling 1:1 connection check

This check was implemented improperly: it can be triggered by an user
using filters that do not match anything. The check was removed in the
previous commit. Now it is implemented correctly.

The option 'disable_dangling_check' to disable this check was
introduced.

Fixes #135.

Author: Totktonada

graphql/accessor_general.lua

@@ -990,6 +990,7 @@ local function select_internal(self, collection_name, from, filter, args, extra)
     -- XXX: save type of args.offset at parsing and check here
     -- check(args.offset, 'args.offset', ...)
     check(args.pcre, 'args.pcre', 'table', 'nil')
+    check(extra.exp_tuple_count, 'extra.exp_tuple_count', 'number', 'nil')
 
     local collection = self.collections[collection_name]
     assert(collection ~= nil,
@@ -1043,6 +1044,18 @@ local function select_internal(self, collection_name, from, filter, args, extra)
         resolveField = extra.resolveField,
     }
 
+    -- assert that connection constraint applied only to objects got from the
+    -- index that underlies the connection
+    if extra.exp_tuple_count ~= nil then
+        local err = 'internal error: connection constraint (expected tuple ' ..
+            'count) cannot be applied to an index that is not under a ' ..
+            'connection'
+        assert(from.collection_name ~= nil, err)
+        assert(index ~= nil, err)
+        assert(pivot == nil or (pivot.value_list == nil and
+            pivot.filter ~= nil), err)
+    end
+
     if index == nil then
         -- fullscan
         local primary_index = self.funcs.get_primary_index(self,
@@ -1087,21 +1100,38 @@ local function select_internal(self, collection_name, from, filter, args, extra)
             iterator_opts.limit = args.limit
         end
 
+        local tuple_count = 0
+
         for _, tuple in index:pairs(index_value, iterator_opts) do
+            tuple_count = tuple_count + 1
+            -- check full match constraint
+            if extra.exp_tuple_count ~= nil and
+                    tuple_count > extra.exp_tuple_count then
+                error(('FULL MATCH constraint was failed: we got more then ' ..
+                    '%d tuples'):format(extra.exp_tuple_count))
+            end
             local continue = process_tuple(self, select_state, tuple,
                 select_opts)
             if not continue then break end
         end
+
+        -- check full match constraint
+        if extra.exp_tuple_count ~= nil and
+                tuple_count ~= extra.exp_tuple_count then
+            error(('FULL MATCH constraint was failed: we expect %d tuples, ' ..
+                'got %d'):format(extra.exp_tuple_count, tuple_count))
+        end
     end
 
     local count = select_state.count
     local objs = select_state.objs
 
     assert(args.limit == nil or count <= args.limit,
-        ('count[%d] exceeds limit[%s] (before return)'):format(
-        count, args.limit))
+        ('internal error: selected objects count (%d) exceeds limit (%s)')
+            :format(count, args.limit))
     assert(#objs == count,
-        ('count[%d] is not equal to objs count[%d]'):format(count, #objs))
+        ('internal error: selected objects count (%d) is not equal size of ' ..
+            'selected object list (%d)'):format(count, #objs))
 
     return objs
 end
@@ -1382,6 +1412,25 @@ end
 --- @treturn table data accessor instance, a table with the two methods
 --- (`select` and `arguments`) as described in the @{impl.new} function
 --- description.
+---
+--- Brief explanation of some select function parameters:
+---
+--- * `from` (table or nil) is nil for a top-level collection or a table with
+---   the following fields:
+---
+---   - collection_name
+---   - connection_name
+---   - destination_args_names
+---   - destination_args_values
+---
+--- * `extra` (table) is a table which contains additional data for the query:
+---
+---   - `qcontext` (table) can be used by an accessor to store any
+---     query-related data;
+---   - `resolveField(field_name, object, filter, opts)` (function) for
+---     performing a subrequest on a fields connected using a 1:1 connection.
+---   - extra_args
+---   - exp_tuple_count
 function accessor_general.new(opts, funcs)
     assert(type(opts) == 'table',
         'opts must be a table, got ' .. type(opts))

graphql/convert_schema/resolve.lua

@@ -29,10 +29,10 @@ local function gen_from_parameter(collection_name, parent, connection)
     }
 end
 
--- Check FULL match constraint before request of
--- destination object(s). Note that connection key parts
--- can be prefix of index key parts. Zero parts count
--- considered as ok by this check.
+--- Check FULL match constraint before request of destination object(s).
+---
+--- Note that connection key parts can be prefix of index key parts. Zero parts
+--- count considered as ok by this check.
 local function are_all_parts_null(parent, connection_parts)
     local are_all_parts_null = true
     local are_all_parts_non_null = true
@@ -84,8 +84,10 @@ local function separate_args_instance(args_instance, arguments)
 end
 
 function resolve.gen_resolve_function(collection_name, connection,
-        destination_type, arguments, accessor)
+        destination_type, arguments, accessor, opts)
     local c = connection
+    local opts = opts or {}
+    local disable_dangling_check = opts.disable_dangling_check or false
     local bare_destination_type = core_types.bare(destination_type)
 
     -- capture `bare_destination_type`
@@ -119,11 +121,17 @@ function resolve.gen_resolve_function(collection_name, connection,
             return c.type == '1:N' and {} or nil
         end
 
+        local exp_tuple_count
+        if not disable_dangling_check and c.type == '1:1' then
+            exp_tuple_count = 1
+        end
+
         local resolveField = genResolveField(info)
         local extra = {
             qcontext = info.qcontext,
             resolveField = resolveField, -- for subrequests
             extra_args = {},
+            exp_tuple_count = exp_tuple_count,
         }
 
         -- object_args_instance will be passed to 'filter'
@@ -148,7 +156,9 @@ function resolve.gen_resolve_function(collection_name, connection,
 end
 
 function resolve.gen_resolve_function_multihead(collection_name, connection,
-        union_types, var_num_to_box_field_name, accessor)
+        union_types, var_num_to_box_field_name, accessor, opts)
+    local opts = opts or {}
+    local disable_dangling_check = opts.disable_dangling_check or false
     local c = connection
 
     local determinant_keys = utils.get_keys(c.variants[1].determinant)
@@ -189,9 +199,13 @@ function resolve.gen_resolve_function_multihead(collection_name, connection,
             name = c.name,
             destination_collection = v.destination_collection,
         }
+        local opts = {
+            disable_dangling_check = disable_dangling_check,
+        }
         -- XXX: generate a function for each variant at schema generation time
         local result = resolve.gen_resolve_function(collection_name,
-            quazi_connection, destination_type, {}, accessor)(parent, {}, info)
+            quazi_connection, destination_type, {}, accessor, opts)(
+            parent, {}, info)
 
         -- This 'wrapping' is needed because we use 'select' on 'collection'
         -- GraphQL type and the result of the resolve function must be in

graphql/convert_schema/types.lua

@@ -155,8 +155,11 @@ local function convert_simple_connection(state, connection, collection_name)
         extra = e_args,
     }
 
+    local opts = {
+        disable_dangling_check = state.disable_dangling_check,
+    }
     local resolve_function = resolve.gen_resolve_function(collection_name, c,
-        destination_type, arguments, state.accessor)
+        destination_type, arguments, state.accessor, opts)
 
     local field = {
         name = c.name,
@@ -276,9 +279,12 @@ local function convert_multihead_connection(state, connection, collection_name,
         union_types[#union_types + 1] = variant_type
     end
 
+    local opts = {
+        disable_dangling_check = state.disable_dangling_check,
+    }
     local resolve_function = resolve.gen_resolve_function_multihead(
         collection_name, c, union_types, var_num_to_box_field_name,
-        state.accessor)
+        state.accessor, opts)
 
     local field = {
         name = c.name,

graphql/impl.lua

@@ -222,34 +222,16 @@ end
 ---         },
 ---         ...
 ---     },
----     accessor = setmetatable({}, {
----         __index = {
----             select = function(self, parent, collection_name, from,
----                     object_args_instance, list_args_instance, extra)
----                 -- * from has the following structure:
----                 --
----                 -- {
----                 --     collection_name = <...>,
----                 --     connection_name = <...>,
----                 --     destination_args_names = <...>,
----                 --     destination_args_values = <...>,
----                 -- }
----                 --
----                 -- from.collection_name is nil for a top-level collection.
----                 --
----                 -- `extra` is a table which contains additional data for
----                 -- the query:
----                 --
----                 -- * `qcontext` (table) can be used by an accessor to store
----                 --   any query-related data;
----                 -- * `resolveField(field_name, object, filter, opts)`
----                 --   (function) for performing a subrequest on a fields
----                 --   connected using a 1:1 connection.
----                 --
----                 return ...
----             end,
----         }
----     }),
+---     indexes = <table>,
+---     service_fields = <table>,
+---     accessor = <table> or <string>,
+---     accessor_funcs = <table>,
+---     collection_use_tomap = <boolean>,
+---     resulting_object_cnt_max = <number>,
+---     fetched_object_cnt_max = <number>,
+---     timeout_ms = <number>,
+---     enable_mutations = <boolean>,
+---     disable_dangling_check = <boolean>,
 --- })
 function impl.new(cfg)
     local cfg = cfg or {}
@@ -279,7 +261,9 @@ function impl.new(cfg)
         cfg.indexes = cfg.accessor.indexes
     end
 
-    local state = {}
+    local state = {
+        disable_dangling_check = cfg.disable_dangling_check,
+    }
     convert_schema.convert(state, cfg)
     return setmetatable(state, {
         __index = {

test/common/nullable_1_1_conn_nocheck.test.lua

@@ -0,0 +1,20 @@
+#!/usr/bin/env tarantool
+
+local fio = require('fio')
+
+-- require in-repo version of graphql/ sources despite current working directory
+package.path = fio.abspath(debug.getinfo(1).source:match("@?(.*/)")
+    :gsub('/./', '/'):gsub('/+$', '')) .. '/../../?.lua' .. ';' .. package.path
+
+local test_utils = require('test.test_utils')
+local testdata = require('test.testdata.nullable_1_1_conn_testdata')
+
+box.cfg({})
+
+test_utils.run_testdata(testdata, {
+    graphql_opts = {
+        disable_dangling_check = true,
+    },
+})
+
+os.exit()

test/space/nested_args.test.lua

@@ -36,7 +36,7 @@ local BODY_FN = avro_version == 3 and 5 or 7
 
 for _, tuple in box.space.email:pairs() do
     local body = tuple[BODY_FN]
-    if body:match('^[xy]$') then
+    if body:match('^[xyz]$') then
         local key = {tuple[LOCALPART_FN], tuple[DOMAIN_FN]}
         box.space.email:delete(key)
     end

test/testdata/nullable_1_1_conn_testdata.lua

@@ -255,6 +255,17 @@ function nullable_1_1_conn_testdata.fill_test_data(virtbox, meta)
         in_reply_to_domain = box.NULL,
         body = 'y',
     })
+
+    -- Check dangling 1:1 connection.
+    local localpart = prng:next_string(16):hex()
+    local non_existent_localpart = prng:next_string(16):hex()
+    test_utils.replace_object(virtbox, meta, 'email', {
+        localpart = localpart,
+        domain = domain,
+        in_reply_to_localpart = non_existent_localpart,
+        in_reply_to_domain = DOMAIN,
+        body = 'z',
+    })
 end
 
 function nullable_1_1_conn_testdata.drop_spaces()
@@ -264,7 +275,7 @@ end
 
 function nullable_1_1_conn_testdata.run_queries(gql_wrapper)
     local test = tap.test('nullable_1_1_conn')
-    test:plan(5)
+    test:plan(7)
 
     -- {{{ downside traversal (1:N connections)
 
@@ -339,10 +350,10 @@ function nullable_1_1_conn_testdata.run_queries(gql_wrapper)
     -- {{{ upside traversal (1:1 connections)
 
     local query_upside = [[
-        query emails_trace_upside($body: String) {
+        query emails_trace_upside($body: String, $child_domain: String) {
             email(body: $body) {
                 body
-                in_reply_to {
+                in_reply_to(domain: $child_domain) {
                     body
                     in_reply_to {
                         body
@@ -416,6 +427,52 @@ function nullable_1_1_conn_testdata.run_queries(gql_wrapper)
     exp_result.err = exp_result.err:gsub(', ', ',')
     test:is_deeply(result, exp_result, 'upside_y')
 
+    -- Check we get an error when trying to use dangling 1:1 connection. Check
+    -- we don't get this error when `disable_dangling_check` is set.
+    if gql_wrapper.disable_dangling_check then
+        local variables_upside_z = {body = 'z'}
+        local result = test_utils.show_trace(function()
+            return gql_query_upside:execute(variables_upside_z)
+        end)
+
+        local exp_result = yaml.decode(([[
+            ---
+            email:
+            - body: z
+        ]]):strip())
+
+        test:is_deeply(result, exp_result, 'upside_z disabled constraint check')
+    else
+        local variables_upside_z = {body = 'z'}
+        local ok, err = pcall(function()
+            return gql_query_upside:execute(variables_upside_z)
+        end)
+
+        local result = {ok = ok, err = test_utils.strip_error(err)}
+        local exp_result = yaml.decode(([[
+            ---
+            ok: false
+            err: "FULL MATCH constraint was failed: we expect 1 tuples, got 0"
+        ]]):strip())
+        test:is_deeply(result, exp_result, 'upside_z constraint violation')
+    end
+
+    -- We can got zero objects by 1:1 connection when use filters, it is not
+    -- violation of FULL MATCH constraint, because we found corresponding
+    -- tuple, but filter it then.
+    local variables_upside_f = {body = 'f', child_domain = 'non-existent'}
+    local result = test_utils.show_trace(function()
+        return gql_query_upside:execute(variables_upside_f)
+    end)
+
+    local exp_result = yaml.decode(([[
+        ---
+        email:
+        - body: f
+    ]]):strip())
+
+    test:is_deeply(result, exp_result, 'upside_f filter child')
+
     assert(test:check(), 'check plan')
 end