add support of union connections, closes #8 #82
Conversation
test/testdata/union_testdata.lua
Outdated
| box.once('test_space_init_spaces', function() | ||
| box.schema.create_space('hero_collection') | ||
| box.space.hero_collection:create_index('hero_id_index', | ||
| { type = 'tree', unique = true, parts = { U_USER_ID_FN, 'string' }} |
There was a problem hiding this comment.
Strictly speaking the field which number you marked via U_USER_ID_FN is not user_id. And the comment above user_collection fields is not relevant for hero/human/starship/hero_info. It is just small naming issue.
Side note: indent.
| box.space._schema:delete('oncetest_space_init_spaces') | ||
| box.space.human_collection:drop() | ||
| box.space.starship_collection:drop() | ||
| box.space.hero_collection:drop() |
There was a problem hiding this comment.
You are deleted it, okay. The comment is not more actual.
test/testdata/union_testdata.lua
Outdated
| { "name": "episode", "type": "string"} | ||
| ] | ||
| }, | ||
| "hero_info": { |
There was a problem hiding this comment.
It seems you don’t use hero_info collection / connection. Is it needed, what do you think?
test/testdata/union_testdata.lua
Outdated
|
|
||
| utils.show_trace(function() | ||
| local variables_1 = {hero_id = 'hero_id_1'} | ||
| local gql_query_1 = gql_wrapper:compile(query) |
There was a problem hiding this comment.
Just note: you can compile it once and use several times.
test/testdata/union_testdata.lua
Outdated
| local gql_query_1 = gql_wrapper:compile(query) | ||
| local result = gql_query_1:execute(variables_1) | ||
| results = results .. print_and_return( | ||
| ('RESULT\n%s'):format(yaml.encode(result))) |
There was a problem hiding this comment.
Consider using the following if you are interested (grep for the function and its use over the other tests):
results = results .. print_and_return(format_result(
'1', query_1, variables_1, result))
Side note: indent.
There was a problem hiding this comment.
Should we create test/utils.lua to store functions like this?
graphql/tarantool_graphql.lua
Outdated
| local resulting_variant | ||
| for determinant, variant in pairs(determinant_to_variant) do | ||
| local is_match = true | ||
| for determinant_key, determinant_value in pairs(determinant) do |
There was a problem hiding this comment.
Can we reuse utils.is_subtable here?
graphql/tarantool_graphql.lua
Outdated
| end | ||
| local objs = state.accessor:select(parent, | ||
| v.destination_collection, from, | ||
| object_args_instance, list_args_instance, extra) |
| '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, |
graphql/accessor_general.lua
Outdated
| connection_indexes[v.destination_collection] = {} | ||
| end | ||
| connection_indexes[v.destination_collection][c.name] = | ||
| set_connection_index(v, c.name, c.type, collection_name, |
| if connection_indexes[v.destination_collection] == nil then | ||
| connection_indexes[v.destination_collection] = {} | ||
| end | ||
| connection_indexes[v.destination_collection][c.name] = |
There was a problem hiding this comment.
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:
- Pass fake 'single connections' instead of a union connection to accessor_general.
- Expand connection_indexes to have one more level of nesting (variant_num). Expand
fromparameter to havevariant_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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@Totktonada done #88
Totktonada
force-pushed
the
sb/union_clean_fix
branch
from
0b5a258 to
9d471d0
Compare
Add new type of connections - union connections, as they described in #8. With this connection type it is possible to represent a situation when entities in child table reference different parent tables. For example, let us have child table User and parent tables Basic and Premium. One user in User table may reference to a basic user info in Basic table, while another may reference to a premium info in Premium table. In GraphQL it is represented as Union type like this: