From 6fe5e890c5d8b346a261af9c25b2c0973fbe44a7 Mon Sep 17 00:00:00 2001 From: "m.kindritskiy" Date: Wed, 5 Jun 2024 21:20:15 +0300 Subject: [PATCH] [fix] [#149] do not merge/extract fragment fields into node fields if there is no such field in node There was a part of algo in nodes merging which will extract fields from fragment if same fields exist in node - this is to prevent same field to be resolved multiple times. The simplest example is `{ user { id ... on User { id } } }` would become `{ user { id } }` so `id` field will be resolved only once. But previous algo (before this fix) would merge eagerly, so even if no field exits in not, algo will try to eliminate fragment at all, for example `{ user { ... on User { id } } }` would become `{ user { id } }` - fragment is eliminated. This behavior breaks union queries and we must not touch union fragment, since this breaks unions. This fix tells algo to merge less eagerly, thas is - if fragment field not exist in node, we leave this field in fragment since there is nothing to optimize, no fields duplication will occur. Note that although we do not merge fragment field to node fields, we still merge fields for fragment field (see _merge_link). --- hiku/query.py | 60 ++++++++------ tests/test_read_graphql.py | 156 ++++++++++++++++++++++++------------- tests/test_union.py | 26 ++++++- 3 files changed, 163 insertions(+), 79 deletions(-) diff --git a/hiku/query.py b/hiku/query.py index 23ebc69a..a4033e6c 100644 --- a/hiku/query.py +++ b/hiku/query.py @@ -290,7 +290,6 @@ def _merge( link_directives: t.DefaultDict[t.Tuple, t.List] = defaultdict(list) to_merge = OrderedDict() fields_iter = chain.from_iterable(e.fields for e in nodes) - fragments_iter = chain.from_iterable(e.fragments for e in nodes) for field in fields_iter: key = field_key(field) @@ -309,31 +308,41 @@ def _merge( yield field if not visited_fields and not to_merge: - for fr in fragments_iter: + for fr in chain.from_iterable(e.fragments for e in nodes): yield fr else: - for fr in fragments_iter: - fr_fields = [] - for field in fr.node.fields: - key = (field.name, field.options_hash, field.alias) - - if field.__class__ is Link: - field = t.cast(Link, field) - if key not in to_merge: - to_merge[key] = [field.node] - links[key] = field + for node in nodes: + for fr in node.fragments: + fr_fields: t.List[FieldOrLink] = [] + for field in fr.node.fields: + key = (field.name, field.options_hash, field.alias) + + if field.__class__ is Link: + field = t.cast(Link, field) + + # If fragment field not exists in node fields, we + # can skip merging it with node fields and just + # leave it in a fragment. + # Field's own node will be merged as usuall + if field.name not in node.fields_map: + fr_fields.append(_merge_link(field)) + continue + + if key not in to_merge: + to_merge[key] = [field.node] + links[key] = field + else: + to_merge[key].append(field.node) + link_directives[key].extend(field.directives) else: - to_merge[key].append(field.node) - link_directives[key].extend(field.directives) - else: - if key not in visited_fields: - fr_fields.append(field) - - fr_key = (fr.type_name, tuple(field_key(f) for f in fr_fields)) - if fr_key not in visited_fragments: - visited_fragments.add(fr_key) - if fr_fields: - yield Fragment(fr.type_name, fr_fields) + if key not in visited_fields: + fr_fields.append(field) + + fr_key = (fr.type_name, tuple(field_key(f) for f in fr_fields)) + if fr_key not in visited_fragments: + visited_fragments.add(fr_key) + if fr_fields: + yield Fragment(fr.type_name, fr_fields) for key, values in to_merge.items(): link = links[key] @@ -341,6 +350,11 @@ def _merge( yield link.copy(node=merge(values), directives=tuple(directives)) +def _merge_link(link: Link) -> Link: + """Recursively merge link node fields and return new link""" + return link.copy(node=merge([link.node])) + + def merge(nodes: t.Iterable[Node]) -> Node: """Merges multiple queries into one query diff --git a/tests/test_read_graphql.py b/tests/test_read_graphql.py index eb6123bf..ba0a9b32 100644 --- a/tests/test_read_graphql.py +++ b/tests/test_read_graphql.py @@ -179,7 +179,55 @@ def test_mutation_operation(): ) -def test_named_fragments(): +def test_named_fragments() -> None: + PinsLink = Link( + "pins", + Node( + [ + Field("gunya"), + Link( + "kilned", + Node([Field("rusk")]), + ), + ], + [], + ), + ) + + SneezerLink = Link( + "sneezer", + Node( + [ + Field("flowers"), + Field("apres"), + ], + [ + Fragment('Makai', [ + Field("doozie"), + PinsLink + ]), + ] + ), + options={"gire": "noatak"}, + ) + + GiltsLink = Link( + "gilts", + Node( + [ + SneezerLink + ], + [ + Fragment("Valium", [ + Link( + "movies", + Node([Field("boree")]), + ), + ]), + ] + ), + ) + check_read( """ query Juger { @@ -200,6 +248,9 @@ def test_named_fragments(): doozie pins { gunya + kilned { + rusk + } ...Meer } } @@ -209,59 +260,7 @@ def test_named_fragments(): } } """, - Node( - [ - Link( - "gilts", - Node( - [ - Link( - "sneezer", - Node( - [ - Field("flowers"), - Field("apres"), - Link( - "pins", - Node( - [ - Field("gunya"), - - Link( - "kilned", - Node( - [ - Field("rusk"), - ] - ), - ), - ], [], - ), - ), - - ], [ - Fragment('Makai', [ - Field("doozie"), - ]), - ] - ), - options={"gire": "noatak"}, - ), - - Link( - "movies", - Node( - [ - Field("boree"), - ] - ), - ), - ], - [] - ), - ), - ] - ), + Node([GiltsLink]), ) @@ -719,7 +718,54 @@ def test_parse_interface_with_one_fragment(): ) -def test_merge_node_with_fragment_on_node(): +def test_merge_node_with_fragment_on_node() -> None: + check_read( + """ + query GetContext { + context { + user { + id + name + ... on User { + id + email + } + } + ... on Context { + user { + ... on User { + id + email + } + } + } + } + } + """, + + Node( + [ + Link( + "context", + Node([ + Link("user", Node([ + Field("id"), + Field("name"), + ], [ + Fragment('User', [ + Field("email"), + ]), + ])), + ], []), + ) + ] + ), + ) + + + +def test_merge_fragment_for_union() -> None: + """We do not know if fragments are for unions or not when we parsing query""" check_read( """ query GetContext { diff --git a/tests/test_union.py b/tests/test_union.py index 24b15496..125f83a3 100644 --- a/tests/test_union.py +++ b/tests/test_union.py @@ -86,6 +86,7 @@ def maybe_get_media(): Node('Audio', [ Field('id', Integer, resolve_audio_fields), Field('duration', String, resolve_audio_fields), + Link('user', TypeRef['User'], link_user, requires=None), ]), Node('Video', [ Field('id', Integer, resolve_video_fields), @@ -330,7 +331,7 @@ def test_query_only_typename(): } -def test_validate_query_can_not_contain_shared_fields(): +def test_validate_query_can_not_contain_shared_fields_in_union(): query = """ query SearchMedia($text: String) { searchMedia(text: $text) { @@ -395,3 +396,26 @@ def test_validate_union_type_field_has_no_such_option(): assert errors == [ 'Unknown options for "Audio.duration": size', ] + + +def test_validate_query_can_contain_shared_links(): + # TODO: the problem here is probably because of fragment merging algorythm + query = """ + query SearchMedia($text: String) { + searchMedia(text: $text) { + __typename + ... on Audio { + duration + user { + id + } + } + ... on Video { + thumbnailUrl(size: 100) + } + } + } + """ + + errors = validate(GRAPH, read(query, {'text': 'foo'})) + assert not errors \ No newline at end of file