-
-
Notifications
You must be signed in to change notification settings - Fork 977
fix: validate empty resources for non-suffix responders #2564
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
base: master
Are you sure you want to change the base?
Changes from all commits
418e5bb
a7261ae
9cb2225
39034a4
4cc4dc7
4c34c0b
58eca02
b92b496
ef16e94
746dabc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Resources without responder methods now emit a ``UserWarning`` for non-suffix responders. | ||
| Resources must define at least one responder method (e.g., ``on_get()``, ``on_post()``). | ||
| This will raise an error in Falcon 5.0. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING | ||
| import warnings | ||
|
|
||
| from falcon import constants | ||
| from falcon import responders | ||
|
|
@@ -69,11 +70,19 @@ def map_http_methods(resource: object, suffix: str | None = None) -> MethodDict: | |
| if callable(responder): | ||
| method_map[method] = responder | ||
|
|
||
| # If suffix is specified and doesn't map to any methods, raise an error | ||
| if suffix and not method_map: | ||
| raise SuffixedMethodNotFoundError( | ||
| 'No responders found for the specified suffix' | ||
| ) | ||
| # If doesn't map to any methods, raise an error | ||
| if not method_map: | ||
| resource_name = resource.__class__.__name__ | ||
| if suffix: | ||
| raise SuffixedMethodNotFoundError( | ||
| f'No responders found for the specified resource: ' | ||
| f'{resource_name} and suffix: {suffix}' | ||
| ) | ||
| else: | ||
| warnings.warn( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it emitting our flavour of |
||
| 'No responders (on_get, on_post, etc.) ' | ||
| + f'found for the specified resource: {resource_name}' | ||
| ) | ||
|
|
||
| return method_map | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1087,7 +1087,8 @@ async def on_websocket(self, req, ws): | |
|
|
||
| async def test_ws_simulator_collect_edge_cases(conductor): | ||
| class Resource: | ||
| pass | ||
| async def on_websocket(self, req, ws): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe adding an unrelated method (such as |
||
| pass | ||
|
|
||
| conductor.app.add_route('/', Resource()) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,11 +40,6 @@ | |
| ] | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def stonewall(): | ||
| return Stonewall() | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def resource_things(): | ||
| return ThingsResource() | ||
|
|
@@ -64,8 +59,6 @@ def resource_get_with_faulty_put(): | |
| def client(asgi, util): | ||
| app = util.create_app(asgi) | ||
|
|
||
| app.add_route('/stonewall', Stonewall()) | ||
|
|
||
| resource_things = ThingsResource() | ||
| app.add_route('/things', resource_things) | ||
| app.add_route('/things/{id}/stuff/{sid}', resource_things) | ||
|
|
@@ -115,10 +108,6 @@ def on_websocket(self, req, resp, id, sid): | |
| self.called = True | ||
|
|
||
|
|
||
| class Stonewall: | ||
| pass | ||
|
|
||
|
|
||
| def capture(func): | ||
| @wraps(func) | ||
| def with_capture(*args, **kwargs): | ||
|
|
@@ -238,12 +227,6 @@ def test_misc(self, client, resource_misc, catch_wsgiref_query_warning): | |
| assert resource_misc.called | ||
| assert resource_misc.req.method == method | ||
|
|
||
| def test_methods_not_allowed_simple(self, client, stonewall): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's leave this stonewall test intact (until Falcon 5.0, that is), just assert with |
||
| client.app.add_route('/stonewall', stonewall) | ||
| for method in ['GET', 'HEAD', 'PUT', 'PATCH']: | ||
| response = client.simulate_request(path='/stonewall', method=method) | ||
| assert response.status == falcon.HTTP_405 | ||
|
|
||
| def test_methods_not_allowed_complex( | ||
| self, client, resource_things, catch_wsgiref_query_warning | ||
| ): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -602,3 +602,11 @@ def test_custom_error_on_suffix_route_not_found(client): | |
| resource_with_suffix_routes, | ||
| suffix='bad-alt', | ||
| ) | ||
|
|
||
|
|
||
| def test_custom_error_route_not_found(client): | ||
| class EmptyResource: | ||
| pass | ||
|
|
||
| with pytest.warns(UserWarning): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we leave the stonewall part, maybe this test becomes somewhat redundant. But, OTOH, it doesn't hurt. |
||
| client.app.add_route('/empty', EmptyResource()) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could expand the newsfragment a bit, telling a story what the inconsistency was, and how it was addressed.