Add validation for missing return statements in @asgi_app() and @wsgi_app() - SDK-510 #3675
Conversation
…_app() decorators - Validate that user functions decorated with @asgi_app() return callable ASGI applications - Validate that user functions decorated with @wsgi_app() return callable WSGI applications - Provide clear error messages indicating the type returned and suggesting the user may have forgotten a return statement - Add test coverage for both ASGI and WSGI missing return scenarios Fixes SDK-510 Co-Authored-By: Eric Hansander <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
The tests were trying to import FastAPI and Flask which aren't available in the test environment. Changed to simple functions that return None to test the validation logic. Co-Authored-By: Eric Hansander <[email protected]>
modal/_runtime/user_code_imports.py
Outdated
| asgi_app = user_defined_callable() | ||
| if not callable(asgi_app): | ||
| raise InvalidError( | ||
| f"Function decorated with @asgi_app() must return a callable ASGI application, " |
There was a problem hiding this comment.
References to Modal names in user-facing messages should always be fully-qualified.
test/webhook_test.py
Outdated
| @pytest.mark.asyncio | ||
| async def test_asgi_app_missing_return(servicer, client, monkeypatch): |
There was a problem hiding this comment.
This is a huge test for a minor feature. Can it be simplified?
… tests - Updated error messages to use 'modal.asgi_app()' and 'modal.wsgi_app()' instead of '@asgi_app()' and '@wsgi_app()' - Simplified test cases by combining multiple assertions into single regex match - Removed unused monkeypatch parameter and unnecessary module_name/function_name in test setup Co-Authored-By: Eric Hansander <[email protected]>
|
Thanks for the feedback! I've addressed both comments:
The tests are now much more concise while still verifying the important aspects of the error message (decorator name, "callable", returned type "NoneType", and the helpful hint). |
modal/_runtime/user_code_imports.py
Outdated
| if not callable(asgi_app): | ||
| raise InvalidError( | ||
| f"Function decorated with @asgi_app() must return a callable ASGI application, " | ||
| f"Function decorated with modal.asgi_app() must return a callable ASGI application, " |
There was a problem hiding this comment.
You made the name fully qualified but you removed the decorator operator, which is useful here.
Updated error messages to use '@modal.asgi_app()' and '@modal.wsgi_app()' format to maintain the decorator operator while still using fully-qualified names. Co-Authored-By: Eric Hansander <[email protected]>
Describe your changes
Fixes SDK-510
This PR adds validation to provide better error feedback when users forget to return their ASGI/WSGI app instance from
@asgi_app()or@wsgi_app()decorated functions.Problem: When users forgot the return statement, they previously got a confusing runtime error
TypeError: 'NoneType' object is not callablethat appeared to be an internal Modal error.Solution: Added validation in
construct_webhook_callableimmediately after calling the user's function to check if the return value is callable, and raise a clearInvalidErrorif it's not.Changes:
modal/_runtime/user_code_imports.py: Added validation for both ASGI and WSGI apps with clear error messagestest/webhook_test.py: Two new test cases verify error handling for missing return statementsError message example:
Requested by: Eric Hansander (@ehdr)
Link to Devin run: https://app.devin.ai/sessions/68b9bc35d4b64951bdfb15cf3befb08d
Checklists
Compatibility checklist
Release checklist
Human Review Checklist
callable()check is sufficient for validating ASGI/WSGI apps (per requirements, deep ASGI compliance checking was explicitly not requested)Changelog
@asgi_app()and@wsgi_app()decorators when users forget to return their app instance. The error now clearly indicates the type returned and suggests adding a return statement.Note
Adds validation to ensure @asgi_app() and @wsgi_app() functions return a callable, raising InvalidError with a clear message when missing.
@asgi_app()and@wsgi_app()inmodal/_runtime/user_code_imports.pyby checking callability and raisingInvalidErrorwith explicit guidance when not callable (e.g., missing return).test_asgi_app_missing_returnandtest_wsgi_app_missing_returnintest/webhook_test.pyto assert clear errors for non-callable returns.Written by Cursor Bugbot for commit 7fcdffc. This will update automatically on new commits. Configure here.