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

Unable to annotate union names with generics #3393

Open
jacobmoshipco opened this issue Feb 24, 2024 · 7 comments
Open

Unable to annotate union names with generics #3393

jacobmoshipco opened this issue Feb 24, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@jacobmoshipco
Copy link
Contributor

jacobmoshipco commented Feb 24, 2024

Describe the Bug

Strawberry's automatic type-naming for generics is ignoring annotated types on unions. Here are a few examples of what I've tried:

This causes the union type to be named SomeTypeNotFoundError:

from typing import Annotated, Generic, TypeVar, Union
import strawberry

from my_types import OtherType, SomeType


@strawberry.type
class NotFoundError:
    id: strawberry.ID
    message: str

T = TypeVar('T')

@strawberry.type
class ObjectQueries(Generic[T]):
    ...

    @strawberry.field
    def by_id(self, id: strawberry.ID) -> Annotated[Union[T, NotFoundError], strawberry.union("ByIdResult")]:
        ...

    ...


@strawberry.type
class Query:

    @strawberry.field
    def some_type_queries(self, id: strawberry.ID) -> ObjectQueries[SomeType]:
        return ObjectQueries(SomeType)

    @strawberry.field
    def other_type_queries(self, id: strawberry.ID) -> ObjectQueries[OtherType]:
        return ObjectQueries(OtherType)
    

schema = strawberry.Schema(Query)

This causes TypeError: SomeTypeObjectQueries fields cannot be resolved.:

from typing import Annotated, Generic, TypeVar, Union
import strawberry

from my_types import OtherType, SomeType


@strawberry.type
class NotFoundError:
    id: strawberry.ID
    message: str

T = TypeVar('T')

@strawberry.type
class ObjectQueries(Generic[T]):
    ...

    @strawberry.field
    # NOTE annotaion moved to just the union
    def by_id(self, id: strawberry.ID) -> Union[T, Annotated[NotFoundError, strawberry.union("ByIdResult")]]:
        ...

    ...


@strawberry.type
class Query:

    @strawberry.field
    def some_type_queries(self, id: strawberry.ID) -> ObjectQueries[SomeType]:
        return ObjectQueries(SomeType)

    @strawberry.field
    def other_type_queries(self, id: strawberry.ID) -> ObjectQueries[OtherType]:
        return ObjectQueries(OtherType)
    

schema = strawberry.Schema(Query)

Pulling the definition up has the same behaviour in both cases:

from typing import Annotated, Generic, TypeVar, Union
import strawberry

from my_types import OtherType, SomeType


@strawberry.type
class NotFoundError:
    id: strawberry.ID
    message: str

T = TypeVar('T')

ByIdResult = Annotated[Union[T, NotFoundError], strawberry.union("ByIdResult", (T, NotFoundError))]
# NOTE this does the same thing, but with a linter warning:
# ByIdResult = strawberry.union("ByIdResult", (T, NotFoundError))

@strawberry.type
class ObjectQueries(Generic[T]):
    ...

    @strawberry.field
    # NOTE annotaion moved to just the union
    def by_id(self, id: strawberry.ID) -> ByIdResult:
        ...

    ...


@strawberry.type
class Query:

    @strawberry.field
    def some_type_queries(self, id: strawberry.ID) -> ObjectQueries[SomeType]:
        return ObjectQueries(SomeType)

    @strawberry.field
    def other_type_queries(self, id: strawberry.ID) -> ObjectQueries[OtherType]:
        return ObjectQueries(OtherType)
    

schema = strawberry.Schema(Query)

Specifying T in the union doesn't help

from typing import Annotated, Generic, TypeVar, Union
import strawberry

from my_types import OtherType, SomeType


@strawberry.type
class NotFoundError:
    id: strawberry.ID
    message: str

T = TypeVar('T')

ByIdResult = Union[T, Annotated[NotFoundError, strawberry.union("ByIdResult")]]

@strawberry.type
class ObjectQueries(Generic[T]):
    ...

    @strawberry.field
    # NOTE annotaion moved to just the union
    def by_id(self, id: strawberry.ID) -> ByIdResult[T]:
        ...

    ...


@strawberry.type
class Query:

    @strawberry.field
    def some_type_queries(self, id: strawberry.ID) -> ObjectQueries[SomeType]:
        return ObjectQueries(SomeType)

    @strawberry.field
    def other_type_queries(self, id: strawberry.ID) -> ObjectQueries[OtherType]:
        return ObjectQueries(OtherType)
    

schema = strawberry.Schema(Query)

Splitting out the error union doesn't help, this also can't resolve fields:

from typing import Annotated, Generic, TypeVar, Union
import strawberry

from my_types import OtherType, SomeType


@strawberry.type
class NotFoundError:
    id: strawberry.ID
    message: str

T = TypeVar('T')

ByIdError = Annotated[NotFoundError, strawberry.union("ByIdError")]

@strawberry.type
class ObjectQueries(Generic[T]):
    ...

    @strawberry.field
    # NOTE annotaion moved to just the union
    def by_id(self, id: strawberry.ID) -> T | ByIdError:
        ...

    ...


@strawberry.type
class Query:

    @strawberry.field
    def some_type_queries(self, id: strawberry.ID) -> ObjectQueries[SomeType]:
        return ObjectQueries(SomeType)

    @strawberry.field
    def other_type_queries(self, id: strawberry.ID) -> ObjectQueries[OtherType]:
        return ObjectQueries(OtherType)
    

schema = strawberry.Schema(Query)

System Information

  • Operating system: macOS Ventura 13.2.1
  • Strawberry version (if applicable): 0.218.0

Additional Context

It appears that the issue with the ones that raise an exception rather than simply having the wrong names have their problem stemming from not being able to from a union with an instance of strawberry.union

Other potentially related issues

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@jacobmoshipco jacobmoshipco added the bug Something isn't working label Feb 24, 2024
@jacobmoshipco
Copy link
Contributor Author

jacobmoshipco commented Feb 26, 2024

From what I can tell Union[T, Annotated[EntityNotFoundError, strawberry.union("ByIdResponse")]] "should" be the correct format for this. However, when compiling to GraphQL, Annotated[EntityNotFoundError, strawberry.union("ByIdResponse")] is first compiled to a GraphQLUnionType before attempting to create an invalid Type | Union union, which is invalid.

Union merging functionality such as with #711 would help. Does anyone know a workaround for this? Either via union merging or naming unions that contain a TypeVar?

@jacobmoshipco
Copy link
Contributor Author

jacobmoshipco commented Feb 26, 2024

Made a quick-and-dirty patch that merges unions (at the schema_converter level) and it worked exactly as I expected. The patch also caused 70 test failures, so I didn't exactly fix it 😅 but yes, union merging would fix this.

@patrick91
Copy link
Member

@jacobmoshipco is this still valid? would you like to share the patch and see if we can fix the tests? 😊

@jacobmoshipco
Copy link
Contributor Author

@patrick91 I'll have to check if I still have it on my home computer; I'll get back to you on that. I think it caused some other issues with unions, though.

@enoua5
Copy link
Contributor

enoua5 commented May 25, 2024

(just switching accounts, I'm the same person as @jacobmoshipco)

/strawberry/schema/schema_converter.py, GraphQLCoreConverter.from_union

859c859
<             assert isinstance(graphql_type, GraphQLObjectType)
---
>             assert isinstance(graphql_type, GraphQLObjectType | GraphQLUnionType)
861c861,864
<             graphql_types.append(graphql_type)
---
>             if isinstance(graphql_type, GraphQLUnionType):
>                 graphql_types += list(graphql_type.types)
>             else:
>                 graphql_types.append(graphql_type)

@enoua5
Copy link
Contributor

enoua5 commented May 25, 2024

Tests:
Without patch: 69 failed, 2727 passed, 160 skipped, 9 xfailed, 13 xpassed, 82 warnings in 356.93s (0:05:56)
With patch: 69 failed, 2727 passed, 160 skipped, 12 xfailed, 10 xpassed, 82 warnings in 431.60s (0:07:11)

Tbh, I may have just forgotten to do a proper control test when testing it before. Unless you (@patrick91) think this is the wrong way to patch this, or that it would cause too much of a breaking change (it does rename some unions), I can go ahead and submit it as a PR.

@patrick91
Copy link
Member

@enoua5 feel free to send the PR, I'll be happy to check the failures 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants