Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

add support of union connections, closes #8 #82

Merged
merged 7 commits into from
Mar 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ lint:
test/testdata/*.lua \
test/common/*.test.lua test/common/lua/*.lua \
test/extra/*.test.lua \
test/*.lua \
--no-redefined --no-unused-args

.PHONY: test
Expand Down
165 changes: 104 additions & 61 deletions graphql/accessor_general.lua
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,61 @@ local function build_index_parts_tree(indexes)
return roots
end

local function set_connection_index(c, c_name, c_type, collection_name,
indexes, connection_indexes)
assert(type(c.index_name) == 'string',
'index_name must be a string, got ' .. type(c.index_name))

-- validate index_name against 'indexes'
local index_meta = indexes[c.destination_collection]
assert(type(index_meta) == 'table',
'index_meta must be a table, got ' .. type(index_meta))

assert(type(collection_name) == 'string', 'collection_name expected to ' ..
'be string, got ' .. type(collection_name))

-- validate connection parts are match or being prefix of index
-- fields
local i = 1
local index_fields = index_meta[c.index_name].fields
for _, part in ipairs(c.parts) do
assert(type(part.source_field) == 'string',
'part.source_field must be a string, got ' ..
type(part.source_field))
assert(type(part.destination_field) == 'string',
'part.destination_field must be a string, got ' ..
type(part.destination_field))
assert(part.destination_field == index_fields[i],
('connection "%s" of collection "%s" has destination parts that ' ..
'is not prefix of the index "%s" parts ' ..
'(destination collection - "%s")'):format(c_name, collection_name,
c.index_name, c.destination_collection))
i = i + 1
end
local parts_cnt = i - 1

-- partial index of an unique index is not guaranteed to being
-- unique
assert(c_type == '1:N' or parts_cnt == #index_fields,
('1:1 connection "%s" of collection "%s" ' ..
'has less fields than the index of "%s" collection ' ..
'(cannot prove uniqueness of the partial index)'):format(c_name,
collection_name, c.index_name, c.destination_collection))

-- validate connection type against index uniqueness (if provided)
if index_meta.unique ~= nil then
assert(c_type == '1:N' or index_meta.unique == true,
('1:1 connection ("%s") cannot be implemented ' ..
'on top of non-unique index ("%s")'):format(
c_name, c.index_name))
end

return {
index_name = c.index_name,
connection_type = c_type,
}
end

--- Build `connection_indexes` table (part of `index_cache`) to use in the
--- @{get_index_name} function.
---
Expand All @@ -581,60 +636,28 @@ local function build_connection_indexes(indexes, collections)
assert(type(collections) == 'table', 'collections must be a table, got ' ..
type(collections))
local connection_indexes = {}
for _, collection in pairs(collections) do
for collection_name, collection in pairs(collections) do
for _, c in ipairs(collection.connections) do
if connection_indexes[c.destination_collection] == nil then
connection_indexes[c.destination_collection] = {}
end
local index_name = c.index_name
assert(type(index_name) == 'string',
'index_name must be a string, got ' .. type(index_name))
if c.destination_collection ~= nil then
if connection_indexes[c.destination_collection] == nil then
connection_indexes[c.destination_collection] = {}
end

-- validate index_name against 'indexes'
local index_meta = indexes[c.destination_collection]
assert(type(index_meta) == 'table',
'index_meta must be a table, got ' .. type(index_meta))

-- validate connection parts are match or being prefix of index
-- fields
local i = 1
local index_fields = index_meta[c.index_name].fields
for _, part in ipairs(c.parts) do
assert(type(part.source_field) == 'string',
'part.source_field must be a string, got ' ..
type(part.source_field))
assert(type(part.destination_field) == 'string',
'part.destination_field must be a string, got ' ..
type(part.destination_field))
assert(part.destination_field == index_fields[i],
('connection "%s" of collection "%s" ' ..
'has destination parts that is not prefix of the index ' ..
'"%s" parts'):format(c.name, c.destination_collection,
c.index_name))
i = i + 1
end
local parts_cnt = i - 1

-- partial index of an unique index is not guaranteed to being
-- unique
assert(c.type == '1:N' or parts_cnt == #index_fields,
('1:1 connection "%s" of collection "%s" ' ..
'has less fields than the index "%s" has (cannot prove ' ..
'uniqueness of the partial index)'):format(c.name,
c.destination_collection, c.index_name))

-- validate connection type against index uniqueness (if provided)
if index_meta.unique ~= nil then
assert(c.type == '1:N' or index_meta.unique == true,
('1:1 connection ("%s") cannot be implemented ' ..
'on top of non-unique index ("%s")'):format(
c.name, index_name))
connection_indexes[c.destination_collection][c.name] =
set_connection_index(c, c.name, c.type, collection_name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent.

indexes, connection_indexes)
end

connection_indexes[c.destination_collection][c.name] = {
index_name = index_name,
connection_type = c.type,
}
if c.variants ~= nil then
for _, v in ipairs(c.variants) do
if connection_indexes[v.destination_collection] == nil then
connection_indexes[v.destination_collection] = {}
end
connection_indexes[v.destination_collection][c.name] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It always set index for the last variant (let’s expend your test on this case). There are two option I see to mitigate this problem:

  1. Pass fake 'single connections' instead of a union connection to accessor_general.
  2. Expand connection_indexes to have one more level of nesting (variant_num). Expand from parameter to have variant_num.

I think the patch should try consolidate main changes either in tarantool_graphql or accessor_general, otherwise we likely spread some abstractions across both modules instead of hide it in the one. Maybe the first option is better in the scope of this view. But 'fake single connection' can be considered as redundant new abstraction, so I don’t sure.

Are you understand the question I raise in this terms? My communication skills are not good within this domain.

Copy link
Contributor Author

@SudoBobo SudoBobo Mar 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connection_indexes[v.destination_collection][c.name] = ... sets indexe for a destination collection, doesn't it? As a result we will have right index for each destination collection and that's what we want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that I missed that that is destination collection. There is an one problem, but it is not the regression. Consider A → C and B → C connections with the same names. Only last one will be saved in the connection_indexes structure and A → C connection can fail during execution.

There is one more problem, but it seems to be quite synthetic: variants to the same destination collection have the same problem.

@SudoBobo Can you file an issue on that, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_connection_index(v, c.name, c.type, collection_name,
indexes, connection_indexes)
end
end
end
end
return connection_indexes
Expand Down Expand Up @@ -678,7 +701,7 @@ local function validate_collections(collections, schemas)
type(schema_name))
assert(schemas[schema_name] ~= nil,
('cannot find schema "%s" for collection "%s"'):format(
schema_name, collection_name))
schema_name, collection_name))
local connections = collection.connections
assert(connections == nil or type(connections) == 'table',
'collection.connections must be nil or table, got ' ..
Expand All @@ -688,16 +711,36 @@ local function validate_collections(collections, schemas)
'connection must be a table, got ' .. type(connection))
assert(type(connection.name) == 'string',
'connection.name must be a string, got ' ..
type(connection.name))
assert(type(connection.destination_collection) == 'string',
'connection.destination_collection must be a string, got ' ..
type(connection.destination_collection))
assert(type(connection.parts) == 'table',
'connection.parts must be a string, got ' ..
type(connection.parts))
assert(type(connection.index_name) == 'string',
'connection.index_name must be a string, got ' ..
type(connection.index_name))
type(connection.name))
if connection.destination_collection then
assert(type(connection.destination_collection) == 'string',
'connection.destination_collection must be a string, got ' ..
type(connection.destination_collection))
assert(type(connection.parts) == 'table',
'connection.parts must be a string, got ' ..
type(connection.parts))
assert(type(connection.index_name) == 'string',
'connection.index_name must be a string, got ' ..
type(connection.index_name))
elseif connection.variants then
for _, v in pairs(connection.variants) do
assert(type(v.determinant) == 'table', "variant's " ..
"determinant must be a table, got " ..
type(v.determinant))
assert(type(v.destination_collection) == 'string',
'variant.destination_collection must be a string, ' ..
'got ' .. type(v.destination_collection))
assert(type(v.parts) == 'table',
'variant.parts must be a table, got ' .. type(v.parts))
assert(type(v.index_name) == 'string',
'variant.index_name must be a string, got ' ..
type(v.index_name))
end
else
assert(false, ('connection "%s" of collection "%s" does not ' ..
'have neither destination collection nor variants field'):
format(connection.name, collection_name))
end
end
end
end
Expand Down
19 changes: 15 additions & 4 deletions graphql/core/execute.lua
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,13 @@ end

local evaluateSelections

local function completeValue(fieldType, result, subSelections, context)
-- @param[opt] resolvedType a type to be used instead of one returned by
-- `fieldType.resolveType(result)` in case when the `fieldType` is Interface or
-- Union; that is needed to increase flexibility of an union type resolving
-- (e.g. resolving by a parent object instead of a current object) via
-- returning it from the `fieldType.resolve` function, which called before
-- `resolvedType` and may need to determine the type itself for its needs
local function completeValue(fieldType, result, subSelections, context, resolvedType)
local fieldTypeName = fieldType.__type

if fieldTypeName == 'NonNull' then
Expand Down Expand Up @@ -111,7 +117,11 @@ local function completeValue(fieldType, result, subSelections, context)
local fields = evaluateSelections(fieldType, result, subSelections, context)
return next(fields) and fields or context.schema.__emptyObject
elseif fieldTypeName == 'Interface' or fieldTypeName == 'Union' then
local objectType = fieldType.resolveType(result)
local objectType = resolvedType or fieldType.resolveType(result)
while objectType.__type == 'NonNull' do
objectType = objectType.ofType
end

return evaluateSelections(objectType, result, subSelections, context)
end

Expand Down Expand Up @@ -151,10 +161,11 @@ local function getFieldEntry(objectType, object, fields, context)
qcontext = context.qcontext
}

local resolvedObject = (fieldType.resolve or defaultResolver)(object, arguments, info)
-- resolvedType is optional return value
local resolvedObject, resolvedType = (fieldType.resolve or defaultResolver)(object, arguments, info)
local subSelections = query_util.mergeSelectionSets(fields)

return completeValue(fieldType.kind, resolvedObject, subSelections, context)
return completeValue(fieldType.kind, resolvedObject, subSelections, context, resolvedType)
end

evaluateSelections = function(objectType, object, selections, context)
Expand Down
4 changes: 2 additions & 2 deletions graphql/core/query_util.lua
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,15 @@ function query_util.collectFields(objectType, selections, visitedFragments, resu
end
elseif selection.kind == 'inlineFragment' then
if shouldIncludeNode(selection, context) and doesFragmentApply(selection, objectType, context) then
collectFields(objectType, selection.selectionSet.selections, visitedFragments, result, context)
query_util.collectFields(objectType, selection.selectionSet.selections, visitedFragments, result, context)
end
elseif selection.kind == 'fragmentSpread' then
local fragmentName = selection.name.value
if shouldIncludeNode(selection, context) and not visitedFragments[fragmentName] then
visitedFragments[fragmentName] = true
local fragment = context.fragmentMap[fragmentName]
if fragment and shouldIncludeNode(fragment, context) and doesFragmentApply(fragment, objectType, context) then
collectFields(objectType, fragment.selectionSet.selections, visitedFragments, result, context)
query_util.collectFields(objectType, fragment.selectionSet.selections, visitedFragments, result, context)
end
end
end
Expand Down
8 changes: 8 additions & 0 deletions graphql/core/rules.lua
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,14 @@ function rules.fragmentSpreadIsPossible(node, context)
local fragmentTypes = getTypes(fragmentType)

local valid = util.find(parentTypes, function(kind)
local kind = kind
-- Here is the check that type, mentioned in '... on some_type'
-- conditional fragment expression is type of some field of parent object.
-- In case of Union parent object and NonNull wrapped inner types
-- graphql-lua missed unwrapping so we add it here
while kind.__type == 'NonNull' do
kind = kind.ofType
end
return fragmentTypes[kind]
end)

Expand Down
6 changes: 5 additions & 1 deletion graphql/core/types.lua
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,15 @@ end
function types.union(config)
assert(type(config.name) == 'string', 'type name must be provided as a string')
assert(type(config.types) == 'table', 'types table must be provided')
if config.resolveType then
assert(type(config.resolveType) == 'function', 'must provide resolveType as a function')
end

local instance = {
__type = 'Union',
name = config.name,
types = config.types
types = config.types,
resolveType = config.resolveType
}

instance.nonNull = types.nonNull(instance)
Expand Down
Loading