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 loader functon for async methods #935

Merged
merged 2 commits into from Apr 25, 2024
Merged

Conversation

j0k2r
Copy link
Contributor

@j0k2r j0k2r commented Apr 20, 2024

Working with Falcon ASGI Request, webargs has issue parsing "media" inputs, because of the loader_func is not waited.

The code base of iscoroutinefunction do not detect Falcon "media" loader as coroutine function.

def iscoroutinefunction(func):
    """Return True if func is a decorated coroutine function."""
    return (inspect.iscoroutinefunction(func) or
            getattr(func, '_is_coroutine', None) is _is_coroutine)

Getting loader_func output and checking if the result is a coroutine works better.

@sirosen
Copy link
Collaborator

sirosen commented Apr 22, 2024

Thanks for the contribution! I think this is correct for some cases, like req.media for Falcon, as you say, but not for others.

If you look at the FalconParser's support for other locations (form, json, etc), you'll see that it accesses properties like req.stream with no notion that they may be async awaitables. I think that any framework data which can become awaitable may need additional handling, on a case-by-case basis.

Since the change is definitely not harmful and works for some cases, I think we should merge and release it, and we can deal with any other async trickiness as the need arises. @lafrech, do you agree?

@lafrech
Copy link
Member

lafrech commented Apr 22, 2024

@sirosen I have no idea TBH. My knowledge about async code is limited. I don't think I'll be able to provide an educated answer any soon so I'll follow you in this one rather than block this indefinitely.
Thanks.

@sirosen
Copy link
Collaborator

sirosen commented Apr 25, 2024

In that case, and having given myself a few more days to think about it, I'm going to merge this and write up a changelog entry for it. If I can get some time in my schedule to look into it, I might start investigating full support for async Falcon usage. If I can't, we can release with this fix on its own.

I'll run my changelog past you largely to knowledge-share how I understand this change.

@sirosen sirosen merged commit 106d5c1 into marshmallow-code:dev Apr 25, 2024
8 checks passed
sirosen added a commit to sirosen/webargs that referenced this pull request Apr 25, 2024
sloria pushed a commit that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants