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

Discriminate generics inside lists #3463

Merged
merged 24 commits into from May 12, 2024

Conversation

patrick91
Copy link
Member

Still WIP, should fix #3432, but right now it only fixes
the union part 😊

Copy link

codecov bot commented Apr 20, 2024

Codecov Report

Merging #3463 (8ad5789) into main (3edf95d) will increase coverage by 0.01%.
The diff coverage is 98.70%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3463      +/-   ##
==========================================
+ Coverage   96.50%   96.51%   +0.01%     
==========================================
  Files         510      511       +1     
  Lines       32761    32859      +98     
  Branches     5430     5451      +21     
==========================================
+ Hits        31615    31713      +98     
  Misses        914      914              
  Partials      232      232              

@botberry
Copy link
Member

botberry commented Apr 20, 2024

Apollo Federation Subgraph Compatibility Results

Federation 1 Support Federation 2 Support
_service🟢
@key (single)🟢
@key (multi)🟢
@key (composite)🟢
repeatable @key🟢
@requires🟢
@provides🟢
federated tracing🔲
@link🟢
@shareable🟢
@tag🟢
@override🟢
@inaccessible🟢
@composeDirective🟢
@interfaceObject🟢

Learn more:

@patrick91
Copy link
Member Author

patrick91 commented Apr 20, 2024

Random thought we might want to add support for something like this:

return BlockRowType[str](total=3, items=[]),

because when we have 0 items when don't know which type we should use, so for now we fallback to the first

This is interesting, maybe it's useful: https://github.com/graphql-python/graphql-core/blob/3cf0d267b096ba92e7289d009800083166a5b7d9/src/graphql/execution/execute.py#L1947C1-L1957C16

Copy link

codspeed-hq bot commented Apr 20, 2024

CodSpeed Performance Report

Merging #3463 will not alter performance

Comparing feature/fix-unions-with-similar-fields (8ad5789) with main (3edf95d)

Summary

✅ 12 untouched benchmarks

strawberry/types/types.py Outdated Show resolved Hide resolved
@patrick91 patrick91 marked this pull request as ready for review April 20, 2024 20:01
@patrick91
Copy link
Member Author

/pre-release

@botberry
Copy link
Member

Pre-release

👋

Releasing commit [cddafae] to PyPi as pre-release! 📦

@botberry
Copy link
Member

Pre-release

👋

Pre-release 0.228.0.dev.1713643365 [cddafae] has been released on PyPi! 🚀
You can try it by doing:

poetry add strawberry-graphql==0.228.0.dev.1713643365

@botberry
Copy link
Member

botberry commented Apr 20, 2024

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release improves our support for generic types, now using the same the same
generic multiple times with a list inside an interface or union is supported,
for example the following will work:

import strawberry


@strawberry.type
class BlockRow[T]:
    items: list[T]


@strawberry.type
class Query:
    @strawberry.field
    def blocks(self) -> list[BlockRow[str] | BlockRow[int]]:
        return [
            BlockRow(items=["a", "b", "c"]),
            BlockRow(items=[1, 2, 3, 4]),
        ]


schema = strawberry.Schema(query=Query)

Here's the tweet text:

🆕 Release (next) is out! Thanks to @patrick91 for the PR 👏

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

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @patrick91 - 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: 2 issues found
  • 🟢 Complexity: all looks good
  • 🟢 Docstrings: 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/types/types.py Show resolved Hide resolved
strawberry/types/types.py Show resolved Hide resolved
strawberry/schema/schema_converter.py Show resolved Hide resolved
tests/schema/test_generics_nested.py Show resolved Hide resolved
tests/schema/test_generics_nested.py Show resolved Hide resolved
@patrick91
Copy link
Member Author

@Speedy1991 let me know if you have time to try this!

@Speedy1991
Copy link
Contributor

Speedy1991 commented Apr 23, 2024

I'm not 100% sure that my code is correct, but this example fails: https://play.strawberry.rocks/?gist=d7d4da4f8b8fe245e645736ba5708b83

€dit: Nvm, I mixed up Interface with Union query. Looks like it works with unions:
https://play.strawberry.rocks/?gist=b4f3ffc37b21f9bd0a4d56525d4dad78

There is the obvious type collision on the client site (items of type str/int) so you are enforced to use an alias. This is the correct behaviour IMO

@Speedy1991
Copy link
Contributor

Random thought we might want to add support for something like this:

return BlockRowType[str](total=3, items=[]),

Could this lead to problems on the client site? E.g. Apollo uses cache normalization (not sure about relay - didn't used it for years). I guess this should be ok as long as the return type matches the possible types for this field - just to make sure :)

@patrick91 patrick91 merged commit be42d02 into main May 12, 2024
64 checks passed
@patrick91 patrick91 deleted the feature/fix-unions-with-similar-fields branch May 12, 2024 11:27
@jibujacobamboss
Copy link

Adding the bug introduced as part of the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

buggy generic interfaces
4 participants