Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link does not resolve on Union types #149

Open
n4mespace opened this issue Jun 3, 2024 · 1 comment
Open

Link does not resolve on Union types #149

n4mespace opened this issue Jun 3, 2024 · 1 comment
Labels

Comments

@n4mespace
Copy link
Collaborator

When I tried to create a query with a link within a Union type, got an unexpected error

example query

    query SearchMedia($text: String) {
      searchMedia(text: $text) {
        __typename
        ... on Audio {
          duration
          user {
            id
          }
        }
        ... on Video {
          thumbnailUrl(size: 100)
        }
      }
    }

example error

"Cannot query field 'user' on type 'Media'. "
"Did you mean to use an inline fragment on 'Audio' or 'Video'?"

another libraries, like strawberry allow such a behavior, GraphQL doc also says it's valid

@n4mespace
Copy link
Collaborator Author

example test case for tests/test_union.py

import pytest
from hiku.denormalize.graphql import DenormalizeGraphQL

from hiku.endpoint.graphql import GraphQLEndpoint
from hiku.engine import Engine
from hiku.executors.sync import SyncExecutor
from hiku.graph import Field, Graph, Link, Node, Option, Root, Union
from hiku.types import (
    Any,
    Integer,
    Optional,
    Sequence,
    String,
    TypeRef,
    UnionRef,
)
from hiku.utils import listify
from hiku.readers.graphql import read
from hiku.validate.graph import GraphValidationError
from hiku.validate.query import validate


def execute(graph, query):
    engine = Engine(SyncExecutor())
    result = engine.execute(graph, query)
    return DenormalizeGraphQL(graph, result, "query").process(query)


@listify
def resolve_user_fields(fields, ids):
    def get_field(fname, id_):
        if fname == 'id':
            return id_

    for id_ in ids:
        yield [get_field(f.name, id_) for f in fields]


@listify
def resolve_audio_fields(fields, ids):
    def get_field(fname, id_):
        if fname == 'id':
            return id_
        if fname == 'duration':
            return f'{id_}s'
        if fname == '_user':
            return id_

    for id_ in ids:
        yield [get_field(f.name, id_) for f in fields]


@listify
def resolve_video_fields(fields, ids):
    def get_field(fname, id_):
        if fname == 'id':
            return id_
        if fname == 'thumbnailUrl':
            return f'/video/{id_}'

    for id_ in ids:
        yield [get_field(f.name, id_) for f in fields]


def link_user_media():
    return [
        (1, TypeRef['Audio']),
        (2, TypeRef['Video']),
    ]


def link_user():
    return 111


def direct_link(ids_):
    return ids_


def search_media(opts):
    if opts['text'] != 'foo':
        return []
    return [
        (1, TypeRef['Audio']),
        (2, TypeRef['Video']),
        (3, TypeRef['Audio']),
        (4, TypeRef['Video']),
    ]


def get_media():
    return 1, TypeRef['Audio']


def maybe_get_media():
    return 2, TypeRef['Video']


GRAPH = Graph([
    Node('Audio', [
        Field('id', Integer, resolve_audio_fields),
        Field('duration', String, resolve_audio_fields),
        Field('_user', Any, resolve_audio_fields),
        Link('user', TypeRef['User'], direct_link, requires='_user'),
    ]),
    Node('Video', [
        Field('id', Integer, resolve_video_fields),
        Field('thumbnailUrl', String, resolve_video_fields, options=[
            Option('size', Integer),
        ]),
    ]),
    Node('User', [
        Field('id', Integer, resolve_user_fields),
        Link('media', Sequence[UnionRef['Media']], link_user_media, requires=None),
    ]),
    Root([
        Link(
            'searchMedia',
            Sequence[UnionRef['Media']],
            search_media,
            options=[
                Option('text', String),
            ],
            requires=None
        ),
        Link('media', UnionRef['Media'], get_media, requires=None),
        Link('maybeMedia', Optional[UnionRef['Media']], maybe_get_media, requires=None),
        Link('user', Optional[TypeRef['User']], link_user, requires=None),
    ]),
], unions=[
    Union('Media', ['Audio', 'Video']),
])


...

def test_validate_query_can_contain_shared_links():
    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

@n4mespace n4mespace added the bug label Jun 3, 2024
kindermax pushed a commit that referenced this issue Jun 5, 2024
… 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).
kindermax pushed a commit that referenced this issue Jun 5, 2024
… 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).
kindermax added a commit that referenced this issue Jun 10, 2024
[fix] [#149] do not merge/extract fragment fields into node fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant