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

add support of union connections, closes #8 #82

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2bd33d7
add support of union connections, closes #8
SudoBobo Mar 14, 2018
7c9d229
style fixes and clean-ups after code review
SudoBobo Mar 15, 2018
46f02b1
style fixes during review
SudoBobo Mar 16, 2018
3c9d651
style fixes during review 2
SudoBobo Mar 16, 2018
a95dacd
style fixes during review 2
SudoBobo Mar 16, 2018
c29493d
style fixes during review 3
SudoBobo Mar 16, 2018
9d471d0
Clarify our approach to union type resolving
Totktonada Mar 16, 2018
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
163 changes: 103 additions & 60 deletions graphql/accessor_general.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,62 @@ 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 +637,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 •
edited
Loading

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.

@Totktonada done #88

set_connection_index(v, 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
end
end
end
return connection_indexes
Expand Down Expand Up @@ -675,29 +699,48 @@ local function validate_collections(collections, schemas)
local schema_name = collection.schema_name
assert(type(schema_name) == 'string',
'collection.schema_name must be a string, got ' ..
type(schema_name))
type(schema_name))
assert(schemas[schema_name] ~= nil,
('cannot find schema "%s" for collection "%s"'):format(
schema_name, collection_name))
local connections = collection.connections
assert(connections == nil or type(connections) == 'table',
'collection.connections must be nil or table, got ' ..
type(connections))
'collection.connections must be nil or table, got ' ..
type(connections))
for _, connection in ipairs(connections) do
assert(type(connection) == 'table',
'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',
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',
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',
type(connection.parts))
assert(type(connection.index_name) == 'string',
'connection.index_name must be a string, got ' ..
type(connection.index_name))
type(connection.index_name))
elseif connection.variants then
for _, v in pairs(connection.variants) do
assert(type(v.determinant) == 'table', 'variant\'s ' ..
Copy link
Member

Choose a reason for hiding this comment

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

Personally I prefer to use "variant's ..." to better grepability of an error message. Don’t force you to follow, just shared it with you.

Side note: negative indent? :)

'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, ('collection doesn\'t have neither destination' ..
Copy link
Member

Choose a reason for hiding this comment

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

The resulting string will have the 'destinationcollection' word :)

The parenthesis around the string literal are redundant in the case, but maybe you had desire to add co{nn,ll}ection names and use ('connection "%s" of collection "%s"...'):format(...), then forgot?

Copy link
Contributor Author

@SudoBobo SudoBobo Mar 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You are right, I intended to make error message with collection and connection names. Fixed.

'collection nor variants field'))
end
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions graphql/core/query_util.lua
View file
Open in desktop
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)
Copy link
Member

Choose a reason for hiding this comment

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

It is not good to use 4 spaces indent for the only two statements in the code part where 2 spaces indent is used.

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
4 changes: 3 additions & 1 deletion graphql/core/types.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,13 @@ 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')
assert(type(config.resolveType) == 'function', 'must provide resolveType as a function')

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

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