Add support for generic unions #3515

enoua5 · 2024-05-26T19:55:21Z

Description

This PR merges union types in strawberry.schema.schema_converter.GraphQLCoreCoverter:from_union. This allows creating a more stable interface, as a union can be created between a TypeVar and an annotated union. In the examples in #3393, this changes SomeTypeByIdNotFoundError => SomeTypeByIdResult, the latter of which won't change as more error cases are added.

Types of Changes

Issues Fixed or Closed by This PR

Unable to annotate union names with generics #3393 - The related issues have been checked as well, but this doesn't seem to fix them; except for Can not use Union with custom Strawberry types inside a generic type. #2959 which seems to have already been fixed as far as I can tell.

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

sourcery-ai

Hey @enoua5 - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 3 issues found
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

strawberry/schema/schema_converter.py

botberry · 2024-05-26T19:56:31Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

Attempt to merge union types during schema conversion.

Here's the tweet text:

🆕 Release (next) is out! Thanks to Jacob Allen for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

codecov · 2024-05-26T19:59:13Z

Codecov Report

Attention: Patch coverage is 93.97590% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 96.54%. Comparing base (a7fc70a) to head (b34106e).
Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3515      +/-   ##
==========================================
+ Coverage   96.53%   96.54%   +0.01%     
==========================================
  Files         520      523       +3     
  Lines       33243    33430     +187     
  Branches     5527     5544      +17     
==========================================
+ Hits        32090    32276     +186     
+ Misses        920      914       -6     
- Partials      233      240       +7

…into fix-3393

codspeed-hq · 2024-05-26T20:11:06Z

CodSpeed Performance Report

Merging #3515 will not alter performance

_{Comparing enoua5:fix-3393 (b34106e) with main (2f02b49)}

Summary

✅ 12 untouched benchmarks

patrick91 · 2024-05-26T20:45:39Z

@enoua5 I pushed a fix 😊 but I'll try to write a test as well

patrick91 · 2024-05-26T21:22:41Z

tests/schema/test_union.py

+ }
+
+ type Query {
+ someTypeQueries(id: ID!): SomeTypeByIdResult!


The type here is still wrong, we probably need to expand the annotated

patrick91 · 2024-05-26T22:22:24Z

RELEASE.md

+Release type: patch
+
+Attempt to merge union types during schema conversion.


with the last change this is probably a minor now, since it's a brand new feature (plus a fix) 😊

patrick91 · 2024-05-26T22:22:47Z

strawberry/schema/name_converter.py

 elif isinstance(type_, StrawberryOptional):
- name = self.get_from_type(type_.of_type) + "Optional"
+ name = self.get_name_from_type(type_.of_type) + "Optional"


I fixed the name of this function 😊

patrick91 · 2024-05-26T22:23:26Z

tests/schema/test_union.py

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


just to clarify, this is quite different from the use case below, as this creates a generic union 😊

enoua5 · 2024-05-27T00:45:56Z

Thank you for the fixes, @patrick91! It's looking good, and each of the examples in #3393 are now working as expected! 🎉

It sounds like we'll need to change the release file? Does something like "Add support for generic unions" encompass the new changes? As far as examples go for the release file, would copying an example or two from the new tests suffice?

patrick91 · 2024-05-27T08:03:53Z

@enoua5 yes! I'll change the release file ☺️ it was late yesterday and I was discussing with @bellini666 if there was a way to simplify the code, I'll take another look today

bellini666

Noice

enoua5 · 2024-05-30T14:41:42Z

Just realized we should probably also have a test to make sure unions with multiple generics work. If we think that's needed, I can add one when I get home from work today

patrick91 · 2024-05-30T14:43:08Z

@enoua5 yes please 😊

enoua5 · 2024-05-31T02:47:00Z

Also of note is that this is technically a breaking change. A breaking change that makes output more predictable and stable, but a breaking change nonetheless.
Consider the following schema definition:

from typing import Annotated, Generic, TypeVar
import strawberry


@strawberry.type
class SomeType:
    a: str

@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[T | NotFoundError, strawberry.union("ByIdResult")]:
        ...

    ...


@strawberry.type
class Query:

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

schema = strawberry.Schema(Query)

Under the current version of strawberry, this SDL is generated:

type Query {
  someTypeQueries(id: ID!): SomeTypeObjectQueries!
}

type SomeTypeObjectQueries {
  byId(id: ID!): SomeTypeNotFoundError!
}

union SomeTypeNotFoundError = SomeType | NotFoundError

type SomeType {
  a: String!
}

type NotFoundError {
  id: ID!
  message: String!
}

Whereas under this PR, this is the SDL generated:

type Query {
  someTypeQueries(id: ID!): SomeTypeObjectQueries!
}

type SomeTypeObjectQueries {
  byId(id: ID!): SomeTypeByIdResult!
}

union SomeTypeByIdResult = SomeType | NotFoundError

type SomeType {
  a: String!
}

type NotFoundError {
  id: ID!
  message: String!
}

For clarity, that:

6c6
<   byId(id: ID!): SomeTypeNotFoundError!
---
>   byId(id: ID!): SomeTypeByIdResult!
9c9
< union SomeTypeNotFoundError = SomeType | NotFoundError
---
> union SomeTypeByIdResult = SomeType | NotFoundError

While I do this this naming is better and makes it easier to write a consistent API (in the former case, adding a new error type NewError to the union would result in a type name of SomeTypeNotFoundErrorNewError, also breaking code), this could potentially break any code already using this pattern.

enoua5 · 2024-05-31T03:02:49Z

tests/schema/test_union.py

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


This test captures the combined case of one generic inside and one generic outside the annotation. Both in and both out also work, but I figured the tests would be getting a bit redundant if I included every combination of in and out.

Merge unions

5cfac3c

sourcery-ai bot reviewed May 26, 2024

View reviewed changes

strawberry/schema/schema_converter.py Show resolved Hide resolved

strawberry/schema/schema_converter.py Outdated Show resolved Hide resolved

strawberry/schema/schema_converter.py Outdated Show resolved Hide resolved

enoua5 and others added 3 commits May 26, 2024 13:58

Use extend instead of +=

747b2ea

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

Sourcery suggestion

674b0bb

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

Sourcery suggestion

b2c7fc1

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

enoua5 added 2 commits May 26, 2024 14:02

Add RELEASE.md

0f31c05

Merge branch 'fix-3393' of https://github.com/enoua5/strawberry-3393 …

b83d078

…into fix-3393

botberry added bot:has-release-file bot:release-type-patch labels May 26, 2024

Fix check on older python versions

e63fed0

patrick91 added 2 commits May 26, 2024 21:57

Add test that was already passing

318b47b

Add missing test

42a5827

patrick91 reviewed May 26, 2024

View reviewed changes

Add support for generic unions

ac29523

patrick91 reviewed May 26, 2024

View reviewed changes

patrick91 added 2 commits May 26, 2024 23:35

Lint

576593b

Lint

280d841

bellini666 approved these changes May 27, 2024

View reviewed changes

enoua5 changed the title ~~Merge unions in schema converter~~ Add support for generic unions May 30, 2024

Add test for annotated union with two generics

99cc745

enoua5 commented May 31, 2024

View reviewed changes

enoua5 added 2 commits May 30, 2024 21:08

Rewrite test to support python <= 3.9

d6c58ed

Remove unused function bodies from tests

b34106e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for generic unions #3515

Add support for generic unions #3515

enoua5 commented May 26, 2024 •

edited

Loading

sourcery-ai bot left a comment

botberry commented May 26, 2024 •

edited

Loading

codecov bot commented May 26, 2024 •

edited

Loading

codspeed-hq bot commented May 26, 2024 •

edited

Loading

patrick91 commented May 26, 2024

patrick91 May 26, 2024

patrick91 May 26, 2024

patrick91 May 26, 2024

patrick91 May 26, 2024

enoua5 commented May 27, 2024

patrick91 commented May 27, 2024

bellini666 left a comment

enoua5 commented May 30, 2024 •

edited

Loading

patrick91 commented May 30, 2024

enoua5 commented May 31, 2024 •

edited

Loading

enoua5 May 31, 2024

		Release type: patch

		Attempt to merge union types during schema conversion.

Add support for generic unions #3515

Are you sure you want to change the base?

Add support for generic unions #3515

Conversation

enoua5 commented May 26, 2024 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

sourcery-ai bot left a comment

Choose a reason for hiding this comment

botberry commented May 26, 2024 • edited Loading

codecov bot commented May 26, 2024 • edited Loading

Codecov Report

codspeed-hq bot commented May 26, 2024 • edited Loading

CodSpeed Performance Report

Merging #3515 will not alter performance

Summary

patrick91 commented May 26, 2024

patrick91 May 26, 2024

Choose a reason for hiding this comment

patrick91 May 26, 2024

Choose a reason for hiding this comment

patrick91 May 26, 2024

Choose a reason for hiding this comment

patrick91 May 26, 2024

Choose a reason for hiding this comment

enoua5 commented May 27, 2024

patrick91 commented May 27, 2024

bellini666 left a comment

Choose a reason for hiding this comment

enoua5 commented May 30, 2024 • edited Loading

patrick91 commented May 30, 2024

enoua5 commented May 31, 2024 • edited Loading

enoua5 May 31, 2024

Choose a reason for hiding this comment

enoua5 commented May 26, 2024 •

edited

Loading

botberry commented May 26, 2024 •

edited

Loading

codecov bot commented May 26, 2024 •

edited

Loading

codspeed-hq bot commented May 26, 2024 •

edited

Loading

enoua5 commented May 30, 2024 •

edited

Loading

enoua5 commented May 31, 2024 •

edited

Loading