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

Fix _RESOLVER_TYPE for async functions #3241

Merged

Conversation

bricker
Copy link
Contributor

@bricker bricker commented Nov 19, 2023

Description

Fixes generic type arguments for Coroutine in RESOLVER_TYPE. The given types are incorrect: It is Coroutine[YieldType, SendType, ReturnType].

This allows catching when the return type of an async resolver is incompatible with the field type. For example:

async def resolver_func() -> int:
  ...

# currently not caught by typechecker
field: str = strawberry.field(resolver=resolver_func)

With the change made in this PR, the above code will be caught by typecheckers.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

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).

@botberry
Copy link
Member

botberry commented Nov 19, 2023

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release improves type checking for async resolver functions when used as
strawberry.field(resolver=resolver_func).

Now doing this will raise a type error:

import strawberry


def some_resolver() -> int:
    return 0


@strawberry.type
class User:
    # Note the field being typed as str instead of int
    name: str = strawberry.field(resolver=some_resolver)

Here's the tweet text:

🆕 Release (next) is out! Thanks to Bryan Ricker for the PR 👏

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

Copy link

codecov bot commented Nov 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.53%. Comparing base (014c4ec) to head (ffdc22d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3241   +/-   ##
=======================================
  Coverage   96.52%   96.53%           
=======================================
  Files         520      520           
  Lines       33243    33243           
  Branches     5527     5527           
=======================================
+ Hits        32089    32090    +1     
+ Misses        921      920    -1     
  Partials      233      233           

Copy link

codspeed-hq bot commented Nov 19, 2023

CodSpeed Performance Report

Merging #3241 will not alter performance

Comparing bricker:bcr/2311/fix-coroutine-fields (ffdc22d) with main (014c4ec)

Summary

✅ 12 untouched benchmarks

@bricker
Copy link
Contributor Author

bricker commented Nov 19, 2023

I am converting this to Draft while I investigate the test failures. I need help from the maintainers to understand these failures.

For the record: This change does fix the errors from Pyright (via Pylance in VSCode), but the test failures are from mypy.

@DoctorJohn
Copy link
Member

DoctorJohn commented Feb 28, 2024

Unfortunately, according to Github, "the logs for this run have expired and are no longer available". This also hides the "re-run jobs" button for me. Could you try to mark this PR as ready for review? Hopefully this will trigger CI again and I can take a look. Edit: looks like marking the PR as ready for review didn't help. Need to figure out how to trigger CI again

@DoctorJohn DoctorJohn marked this pull request as ready for review February 29, 2024 01:03
Copy link
Contributor

@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.

Failed to review pull request #3241 due to the following error:

<class 'KeyError'>: 0

Try re-running the review or contact [email protected] for help.

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

Sorry for the late review and merge! Looks great 😊

I've added a test for pyright, just to check the behaviour there 😊 (The error is weird, but at least it errors correctly :D)

@patrick91 patrick91 merged commit 9099e43 into strawberry-graphql:main May 25, 2024
64 checks passed
@Ansud
Copy link

Ansud commented May 27, 2024

Dynamic field creation failed (or may be i doing something wrong).

Suppose you have code

def create_get_data_format_query(obj: SomeClass) -> tuple[str, Any, Field[DataSchema]]:
    async def resolver_func() -> DataSchema:
        db = session_context.get()

        s = await SomeClass.load_from_database(db, obj.id)

        if s is None:
            raise ValueError

        return await serialize_schema(s) # This function return DataSchema and typing is ok

    resolver: StrawberryField = strawberry.field(
        resolver=resolver_func,
        name="get_schema", description="Get schema",
    )

    return "get_schema", DataSchema, field(default=resolver)

And this produce following error

error: Argument "resolver" to "field" has incompatible type "Callable[[], Coroutine[Any, Any, DataSchema]]"; expected
"StrawberryResolver[Never] | Callable[..., Never] | Callable[..., Coroutine[Any, Any, Never]] | Callable[..., Awaitable[Never]] | staticmethod[Any, Never] | classmethod[Any, Any, Never]"  [arg-type]
            resolver=resolver_func,

Why DataSchema become Never? What do i do wrong or what do typing do wrong?

I tried to change StrawberryField to DataSchema - it not hepled.

@patrick91
Copy link
Member

@Ansud it's not your fault, apparently this PR triggered a Mypy error, there's an initial fix here: #3516

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.

strawberry.field does not type check properly for async resolvers, but strawberry.field() does
5 participants