Use the correct iscoroutinefunction function for python 3.12+ #3599

shmoon-kr · 2024-08-17T13:58:26Z

Description

Fix a bug "a custom resolver with permission_classes doesn't accept @sync_to_async decorator"
The root cause was inspect.iscoroutinefunction() function returns True only for functions defined with "async def".
Now is_async function returns True for a resolver function decorated with @sync_to_async

Types of Changes

Issues Fixed or Closed by This PR

a custom resolver with permission_classes doesn't accept @sync_to_async decorator #3385

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

Summary by Sourcery

Fix the issue where custom resolvers with permission_classes did not work with the @sync_to_async decorator by updating the is_async function to recognize SyncToAsync instances.

Bug Fixes:

Fix a bug where a custom resolver with permission_classes did not accept the @sync_to_async decorator by updating the is_async function to correctly identify such functions.

Documentation:

Add a RELEASE.md file to document the patch release fixing the bug with custom resolvers and @sync_to_async.

Tests:

Add a test to verify that a resolver function decorated with @sync_to_async works correctly with permission_classes.

sourcery-ai · 2024-08-17T13:58:33Z

Reviewer's Guide by Sourcery

This pull request addresses a bug where custom resolvers with permission_classes were not accepting the @sync_to_async decorator. The fix involves modifying the is_async property in the resolver class to correctly identify functions decorated with @sync_to_async as asynchronous. Additionally, a new test case has been added to verify the fix, and a release note has been created.

File-Level Changes

Files	Changes
`strawberry/types/fields/resolver.py`	Modified the is_async property in the resolver class to check if the function is an instance of SyncToAsync
`tests/fields/test_permissions.py`	Added a new asynchronous test case to verify the fix for permission classes with @sync_to_async decorator
`RELEASE.md`	Created a release note describing the bug fix and its root cause

Tips

Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
Continue your discussion with Sourcery by replying directly to review comments.
You can change your review settings at any time by accessing your dashboard:
- Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
- Change the review language;
You can always contact us if you have any questions or feedback.

sourcery-ai

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

Here's what I looked at during the review

🟡 General issues: 1 issue found
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟡 Documentation: 1 issue found

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/fields/resolver.py

sourcery-ai · 2024-08-17T13:59:37Z

RELEASE.md

@@ -0,0 +1,4 @@
+Release type: patch
+
+Fix a bug "a custom resolver with permission_classes doesn't accept @sync_to_async decorator"


suggestion (documentation): Consider using consistent quotation marks.

Single quotes are often preferred in markdown files for consistency.

botberry · 2024-08-17T14:00:36Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

Fix an issue where StrawberryResolver.is_async was returning False for a
function decorated with asgiref's @sync_to_async.

The root cause is that in python >= 3.12 coroutine functions are market using
inspect.markcoroutinefunction, which should be checked with
inspect.iscoroutinefunction instead of asyncio.iscoroutinefunction

Here's the tweet text:

🆕 Release (next) is out! Thanks to Hyun S. Moon for the PR 👏

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

botberry · 2024-08-17T14:00:47Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

Fix a bug "a custom resolver with permission_classes doesn't accept @sync_to_async decorator"
The root cause was inspect.iscoroutinefunction() function returns True only for functions defined with "async def".

Here's the tweet text:

🆕 Release (next) is out! Thanks to Hyun S. Moon for the PR 👏

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

codecov · 2024-08-17T14:03:17Z

Codecov Report

Attention: Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.82%. Comparing base (e9a0f13) to head (e3140e7).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3599      +/-   ##
==========================================
- Coverage   96.82%   96.82%   -0.01%     
==========================================
  Files         514      514              
  Lines       33293    33322      +29     
  Branches     5522     5528       +6     
==========================================
+ Hits        32236    32264      +28     
- Misses        833      834       +1     
  Partials      224      224

codspeed-hq · 2024-08-17T14:08:33Z

CodSpeed Performance Report

Merging #3599 will not alter performance

_{Comparing shmoon-kr:bugfix/No_3385 (e3140e7) with main (e9a0f13)}

Summary

✅ 15 untouched benchmarks

erikwrede

Thanks for your PR and investigation. I have some comments, please let me know what you think 😊

erikwrede · 2024-08-17T18:59:24Z

strawberry/types/fields/resolver.py

+        return (
+            isinstance(self._unbound_wrapped_func, SyncToAsync)
+            or iscoroutinefunction(self._unbound_wrapped_func)
+            or isasyncgenfunction(self._unbound_wrapped_func)


I'd like to avoid importing somewhat django-specific projects like asgiref into strawberry core components.
Do we have a different way to identify SyncToAsync as a coroutinefunction?

@bellini666 I thought you recently PR'd the asyncio.coroutime marker to SyncToAsync.

How about using asyncio.iscoroutinefunction() instead of inspect.iscoroutinefunction()? It was said that asyncio.iscoroutinefunction() function does two tests; it uses inspect.iscoroutinefunction() first, and if that test fails tests if the function is a function with the @acyncio.coroutine decorator applied. (https://stackoverflow.com/questions/36076619/test-if-function-or-method-is-normal-or-asynchronous)

@erikwrede it was there already, what I PR'd there was a performance improvement: django/asgiref#390 =P

@shmoon-kr sounds good!

@bellini666 oops the diff fooled me haha

tests/fields/test_permissions.py

bellini666

Request for changes is mostly for the issue I commented with python 3.12+

Other than that, looking good! :)

bellini666 · 2024-08-18T12:05:47Z

strawberry/types/fields/resolver.py

@@ -3,8 +3,9 @@
 import inspect
 import sys
 import warnings
+from asyncio import iscoroutinefunction


This will not work for python 3.12

Check here how asgiref does to support this for both 3.12 and older versions.

You can probably copy the:

if hasattr(inspect, "markcoroutinefunction"): iscoroutinefunction = inspect.iscoroutinefunction else: iscoroutinefunction = asyncio.iscoroutinefunction

In here and use it

bellini666 · 2024-08-18T12:06:59Z

tests/types/test_resolver_types.py

@@ -94,3 +96,12 @@ def get_overlap() -> Union[Venn, Diagram]: ...

    resolver = StrawberryResolver(get_overlap)
    assert resolver.type == Union[Venn, Diagram]
+
+
+def test_async_resolver():


maybe naming it more close to what it is testing?

Suggested change

def test_async_resolver():

def test_sync_to_async_resolver():

shmoon-kr · 2024-08-18T12:35:23Z

Request for changes is mostly for the issue I commented with python 3.12+

Other than that, looking good! :)

@bellini666 Thanks for a comment. I updated the PR based on your suggestion.

bellini666

Thanks for the adjustments :)

Left some extra nits and this should be good to go.

Now, not sure why some unrelated tests are broken. @patrick91 , any idea?

bellini666 · 2024-08-18T12:31:14Z

strawberry/types/fields/resolver.py

+        if hasattr(inspect, "markcoroutinefunction"):
+            iscoroutinefunction = inspect.iscoroutinefunction
+        else:
+            iscoroutinefunction = asyncio.iscoroutinefunction


nit: I would define this at module level

Also, maybe add a comment to why this is being done. I know why, but someone checking this hasattr(inspect, "markcoroutinefunction") to define iscoroutinefunction could get confused

bellini666 · 2024-08-18T12:52:04Z

RELEASE.md

+Release type: patch
+
+Fix a bug "a custom resolver with permission_classes doesn't accept @sync_to_async decorator"
+The root cause was inspect.iscoroutinefunction() function returns True only for functions defined with "async def".


Probably this needs to be updated after the change I requested? As this is still going to be used for python 3.12+ (but it actually works in there)

bellini666

Some more nits

(sorry for the trouble 😅)

bellini666 · 2024-08-18T14:23:28Z

RELEASE.md

@@ -0,0 +1,4 @@
+Release type: patch
+
+Fix a bug ''a custom resolver with permission_classes doesn't accept @sync_to_async decorator"


Suggested change

Fix a bug ''a custom resolver with permission_classes doesn't accept @sync_to_async decorator"

Fix a bug "a custom resolver with permission_classes doesn't accept @sync_to_async decorator"

also this seems weirdly written... maybe "fix a bug where doing X would raise an issue Y" or something? not really sure as I'm usually very bad at this =P

@bellini666 I modified the release note again. "Fix a bug "StrawberryResolver.is_async returns False for a function decorated by @sync_to_async" I believe this phrase, although not the best, clearly expresses the revisions.

bellini666 · 2024-08-18T14:27:36Z

strawberry/types/fields/resolver.py

@@ -171,6 +172,12 @@ def is_reserved_type(self, other: builtins.type) -> bool:

 T = TypeVar("T")

+#  asyncio.iscoroutinefunction was deprecated in python >= 3.12


it is not that it was deprecated, but actually that python 3.12 changed the way to mark something as a coroutine by requiring it to be marked using inspect.markcoroutinefunction, and that needs to be checked with inspect.iscoroutinefunction instead of the one from asyncio

@bellini666 I find it difficult to explain this situation easily and clearly. The official document states, "inspect.iscoroutinefunction(object) Changed in version 3.12: Sync functions marked with markcoroutinefunction() now return True." I am considering noting this as a comment verbatim. How about this idea?

… return True for @sync_to_async functions

for more information, see https://pre-commit.ci

…on attribute

botberry · 2024-08-30T22:31:49Z

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67

And don't forget to join our discord server: https://strawberry.rocks/discord 🔥

sourcery-ai bot reviewed Aug 17, 2024

View reviewed changes

botberry added bot:has-release-file bot:release-type-patch labels Aug 17, 2024

erikwrede reviewed Aug 17, 2024

View reviewed changes

bellini666 requested changes Aug 18, 2024

View reviewed changes

bellini666 reviewed Aug 18, 2024

View reviewed changes

shmoon-kr and others added 11 commits August 20, 2024 10:04

Fix strawberry.types.resolver.StrawberryResolver.is_async function to…

e1506ac

… return True for @sync_to_async functions

Add RELEASE.md

5cf20e3

Add test case

19482d1

Fix test cases

eefefdc

[pre-commit.ci] auto fixes from pre-commit.com hooks

3efc16a

for more information, see https://pre-commit.ci

Breakdown assert statement

2427faa

Add StrawberryResolver test

2f98cc6

Replace inspect.iscoroutinefunction with asyncio.iscoroutinefunction

f92ff83

Check is_async conditionally according to inspect.markcoroutinefuncti…

7fd9509

…on attribute

Add comments

18e8603

Fix comments and release notes

c8962df

patrick91 force-pushed the bugfix/No_3385 branch from fa7e98a to c8962df Compare August 20, 2024 08:04

bellini666 added 5 commits August 24, 2024 09:10

refactor: reword comment and release notes

6261a38

chore: change lint cache key to fix a cache issue with github actions

03fcfdd

chore: ignore assignment type error

c85dfbd

Merge remote-tracking branch 'origin/main' into bugfix/No_3385

f56b189

One more adjustment to the release notes

e3140e7

bellini666 changed the title ~~Bugfix/no 3385~~ Use the correct iscoroutinefunction function Aug 30, 2024

bellini666 approved these changes Aug 30, 2024

View reviewed changes

bellini666 changed the title ~~Use the correct iscoroutinefunction function~~ Use the correct iscoroutinefunction function for python 3.12+ Aug 30, 2024

bellini666 merged commit b1e4371 into strawberry-graphql:main Aug 30, 2024
105 of 107 checks passed

shmoon-kr deleted the bugfix/No_3385 branch August 31, 2024 03:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the correct iscoroutinefunction function for python 3.12+ #3599

Use the correct iscoroutinefunction function for python 3.12+ #3599

shmoon-kr commented Aug 17, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Aug 17, 2024 •

edited

Loading

sourcery-ai bot left a comment

sourcery-ai bot Aug 17, 2024

botberry commented Aug 17, 2024 •

edited

Loading

botberry commented Aug 17, 2024

codecov bot commented Aug 17, 2024 •

edited

Loading

codspeed-hq bot commented Aug 17, 2024 •

edited

Loading

erikwrede left a comment

erikwrede Aug 17, 2024

shmoon-kr Aug 17, 2024

bellini666 Aug 18, 2024

erikwrede Aug 18, 2024

erikwrede Aug 18, 2024

bellini666 left a comment

bellini666 Aug 18, 2024

bellini666 Aug 18, 2024

shmoon-kr commented Aug 18, 2024

bellini666 left a comment

bellini666 Aug 18, 2024

bellini666 Aug 18, 2024

bellini666 left a comment

bellini666 Aug 18, 2024

shmoon-kr Aug 20, 2024

bellini666 Aug 18, 2024

shmoon-kr Aug 20, 2024

botberry commented Aug 30, 2024

		@@ -0,0 +1,4 @@
		Release type: patch

		Fix a bug "a custom resolver with permission_classes doesn't accept @sync_to_async decorator"

	def test_async_resolver():
	def test_sync_to_async_resolver():

		@@ -0,0 +1,4 @@
		Release type: patch

		Fix a bug ''a custom resolver with permission_classes doesn't accept @sync_to_async decorator"

		@@ -171,6 +172,12 @@ def is_reserved_type(self, other: builtins.type) -> bool:

		T = TypeVar("T")

		# asyncio.iscoroutinefunction was deprecated in python >= 3.12

Use the correct iscoroutinefunction function for python 3.12+ #3599

Use the correct iscoroutinefunction function for python 3.12+ #3599

Conversation

shmoon-kr commented Aug 17, 2024 • edited by sourcery-ai bot Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

Summary by Sourcery

sourcery-ai bot commented Aug 17, 2024 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Aug 17, 2024

Choose a reason for hiding this comment

botberry commented Aug 17, 2024 • edited Loading

botberry commented Aug 17, 2024

codecov bot commented Aug 17, 2024 • edited Loading

Codecov Report

codspeed-hq bot commented Aug 17, 2024 • edited Loading

Merging #3599 will not alter performance

Summary

erikwrede left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellini666 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmoon-kr commented Aug 18, 2024

bellini666 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellini666 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

botberry commented Aug 30, 2024

shmoon-kr commented Aug 17, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Aug 17, 2024 •

edited

Loading

botberry commented Aug 17, 2024 •

edited

Loading

codecov bot commented Aug 17, 2024 •

edited

Loading

codspeed-hq bot commented Aug 17, 2024 •

edited

Loading