fix: validate empty resources for non-suffix responders #2564
Conversation
- Change from SuffixedMethodNotFoundError to MethodNotFoundError - Added Indicators for the resource and suffix
- Stonewall related test cases removed - Already covered by test_custom_error_route_not_found which tests that empty resources raise MethodNotFoundError
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2564 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 64 64
Lines 7860 7864 +4
Branches 1076 1077 +1
=========================================
+ Hits 7860 7864 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sonephyo
changed the title
vytas7
changed the title
There was a problem hiding this comment.
Thanks for this contribution @sonephyo!
I think this looks good for most of it, however, Falcon is known for going to great lengths for avoiding breaking surprises. So I think we should just emit a warning for now, and actually start raising an error only in 5.0.
Also, we need a newsfragment to describe what has been improved or fixed.
CaselIT
left a comment
CaselIT
left a comment
There was a problem hiding this comment.
great, but as mentioned let's start with warning
sonephyo
force-pushed
the
bug/validate-empty-resources
branch
from
df4073c to
6d20382
Compare
MethodNotFoundError change back to SuffixedMethodNotFoundError warnings.warn use emit UserWarning
sonephyo
force-pushed
the
bug/validate-empty-resources
branch
from
6d20382 to
b92b496
Compare
vytas7
left a comment
vytas7
left a comment
There was a problem hiding this comment.
Thanks, this looks good for most of it 💯
Just a couple of cosmetic nitpicks before merging (see inline).
| f'{resource_name} and suffix: {suffix}' | ||
| ) | ||
| else: | ||
| warnings.warn( |
There was a problem hiding this comment.
I think it emitting our flavour of DeprecationWarning would be more consistent with the behaviour and evolution of the framework so far. We could also expand the wording a bit, explaining that adding resources without any responders is deprecated, and it would result in an error in Falcon 5.0.
| assert resource_misc.called | ||
| assert resource_misc.req.method == method | ||
|
|
||
| def test_methods_not_allowed_simple(self, client, stonewall): |
There was a problem hiding this comment.
Let's leave this stonewall test intact (until Falcon 5.0, that is), just assert with pytest.warns(...) that a deprecation warning was emitted by add_route below.
| class EmptyResource: | ||
| pass | ||
|
|
||
| with pytest.warns(UserWarning): |
There was a problem hiding this comment.
If we leave the stonewall part, maybe this test becomes somewhat redundant. But, OTOH, it doesn't hurt.
| async def test_ws_simulator_collect_edge_cases(conductor): | ||
| class Resource: | ||
| pass | ||
| async def on_websocket(self, req, ws): |
There was a problem hiding this comment.
Maybe adding an unrelated method (such as on_delete or whatever) would match the original intent of this test case better? (I.e., not having any on_websocket() responder.)
| @@ -0,0 +1,3 @@ | |||
| Resources without responder methods now emit a ``UserWarning`` for non-suffix responders. | |||
There was a problem hiding this comment.
We could expand the newsfragment a bit, telling a story what the inconsistency was, and how it was addressed.
3 participants
Summary of Changes
This PR fixes suffixed and normal responders to have consistent behavior by raising MethodNotFound when method_map is empty. This is to disable allowing empty resources by nature, as mentioned in Issue #2167.
Test cases related to Stonewall are an empty Resource, which contradicts, and therefore, is removed.
Test case test_ws_simulator_collect_edge_cases is updated to include on_websocket function.
Test case (test_custom_error_route_not_found) is introduced for testing an empty resource.
Related Issues
Closes #2167
Pull Request Checklist
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. Reading our contribution guide at least once will save you a few review cycles!
If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.
tox.docs/.docs/.versionadded,versionchanged, ordeprecateddirectives.docs/_newsfragments/, with the file name format{issue_number}.{fragment_type}.rst. (Runtox -e towncrier, and inspectdocs/_build/html/changes/in the browser to ensure it renders correctly.)