Repository: falconry/falcon
Pull request #1402 (2019-01-01 17:21:54)

fix(hooks): apply resource-level hooks even to suffixed responders
Fixes https://github.com/falconry/falcon/issues/1351 (solution path **(A)** -- see https://github.com/falconry/falcon/pull/1352#issuecomment-439998925).

I will add a note in the documentation, as well as a solution-specific unit test if we choose to tread this path **(A)**.

codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=h1) Report
> Merging [#1402](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/db7ab35dd55bbab11ef2c58e407d710ae41ea970?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1402/graphs/tree.svg?width=650&token=zOtkfNQ3h6&height=150&src=pr)](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=tree)

```diff
@@          Coverage Diff           @@
##           master   #1402   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          38      38           
  Lines        2545    2538    -7     
  Branches      391     392    +1     
======================================
- Hits         2545    2538    -7
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/hooks.py](https://codecov.io/gh/falconry/falcon/pull/1402/diff?src=pr&el=tree#diff-ZmFsY29uL2hvb2tzLnB5) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=footer). Last update [db7ab35...83419c1](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=h1) Report
> Merging [#1402](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/db7ab35dd55bbab11ef2c58e407d710ae41ea970?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1402/graphs/tree.svg?width=650&token=zOtkfNQ3h6&height=150&src=pr)](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=tree)

```diff
@@          Coverage Diff           @@
##           master   #1402   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          38      38           
  Lines        2545    2538    -7     
  Branches      391     392    +1     
======================================
- Hits         2545    2538    -7
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/hooks.py](https://codecov.io/gh/falconry/falcon/pull/1402/diff?src=pr&el=tree#diff-ZmFsY29uL2hvb2tzLnB5) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=footer). Last update [db7ab35...83419c1](https://codecov.io/gh/falconry/falcon/pull/1402?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/docs/api/hooks.rst b/docs/api/hooks.rst
index e03de7d..dfafabe 100644
--- a/docs/api/hooks.rst
+++ b/docs/api/hooks.rst
@@ -39,6 +39,15 @@ decorate the resource class:
         def on_get(self, req, resp, project_id):
             pass
 
+.. note::
+    When decorating an entire resource class, all method names that resemble
+    responders, including *suffix*\ed (see also :meth:`~falcon.API.add_route`)
+    ones, are decorated. If, for instance, a method is called ``on_get_items``,
+    but it is not meant for handling ``GET`` requests under a route with the
+    *suffix* ``items``, the easiest workaround for preventing the hook function
+    from being applied to the method is renaming it not to clash with the
+    responder pattern.
+
 Note also that you can pass additional arguments to your hook function
 as needed:
 
diff --git a/falcon/api.py b/falcon/api.py
index f006028..fd53c1a 100644
--- a/falcon/api.py
+++ b/falcon/api.py
@@ -584,10 +584,10 @@ class API(object):
             req: The request object.
 
         Returns:
-            tuple: A 3-member tuple consisting of a responder callable,
+            tuple: A 4-member tuple consisting of a responder callable,
             a ``dict`` containing parsed path fields (if any were specified in
-            the matching route's URI template), and a reference to the
-            responder's resource instance.
+            the matching route's URI template), a reference to the responder's
+            resource instance, and the matching URI template.
 
         Note:
             If a responder was matched to the given URI, but the HTTP
diff --git a/falcon/hooks.py b/falcon/hooks.py
index d983759..f3583c5 100644
--- a/falcon/hooks.py
+++ b/falcon/hooks.py
@@ -15,6 +15,8 @@
 """Hook decorators."""
 
 from functools import wraps
+from inspect import getmembers
+import re
 
 import six
 
@@ -22,6 +24,10 @@ from falcon import COMBINED_METHODS
 from falcon.util.misc import get_argnames
 
 
+_DECORABLE_METHOD_NAME = re.compile(r'^on_({})(_\w+)?$'.format(
+    '|'.join(method.lower() for method in COMBINED_METHODS)))
+
+
 def before(action, *args, **kwargs):
     """Decorator to execute the given action function *before* the responder.
 
@@ -57,28 +63,18 @@ def before(action, *args, **kwargs):
         if isinstance(responder_or_resource, six.class_types):
             resource = responder_or_resource
 
-            for method in COMBINED_METHODS:
-                responder_name = 'on_' + method.lower()
-
-                try:
-                    responder = getattr(resource, responder_name)
-                except AttributeError:
-                    # resource does not implement this method
-                    pass
-                else:
-                    # Usually expect a method, but any callable will do
-                    if callable(responder):
-                        # This pattern is necessary to capture the current
-                        # value of responder in the do_before_all closure;
-                        # otherwise, they will capture the same responder
-                        # variable that is shared between iterations of the
-                        # for loop, above.
-                        def let(responder=responder):
-                            do_before_all = _wrap_with_before(responder, action, args, kwargs)
-
-                            setattr(resource, responder_name, do_before_all)
-
-                        let()
+            for responder_name, responder in getmembers(resource, callable):
+                if _DECORABLE_METHOD_NAME.match(responder_name):
+                    # This pattern is necessary to capture the current value of
+                    # responder in the do_before_all closure; otherwise, they
+                    # will capture the same responder variable that is shared
+                    # between iterations of the for loop, above.
+                    def let(responder=responder):
+                        do_before_all = _wrap_with_before(responder, action, args, kwargs)
+
+                        setattr(resource, responder_name, do_before_all)
+
+                    let()
 
             return resource
 
@@ -112,24 +108,14 @@ def after(action, *args, **kwargs):
         if isinstance(responder_or_resource, six.class_types):
             resource = responder_or_resource
 
-            for method in COMBINED_METHODS:
-                responder_name = 'on_' + method.lower()
-
-                try:
-                    responder = getattr(resource, responder_name)
-                except AttributeError:
-                    # resource does not implement this method
-                    pass
-                else:
-                    # Usually expect a method, but any callable will do
-                    if callable(responder):
-
-                        def let(responder=responder):
-                            do_after_all = _wrap_with_after(responder, action, args, kwargs)
+            for responder_name, responder in getmembers(resource, callable):
+                if _DECORABLE_METHOD_NAME.match(responder_name):
+                    def let(responder=responder):
+                        do_after_all = _wrap_with_after(responder, action, args, kwargs)
 
-                            setattr(resource, responder_name, do_after_all)
+                        setattr(resource, responder_name, do_after_all)
 
-                        let()
+                    let()
 
             return resource
 
@@ -154,7 +140,7 @@ def _wrap_with_after(responder, action, action_args, action_kwargs):
         responder: The responder method to wrap.
         action: A function with a signature similar to a resource responder
             method, taking the form ``func(req, resp, resource)``.
-        action_args: Additiona positional agruments to pass to *action*.
+        action_args: Additional positional agruments to pass to *action*.
         action_kwargs: Additional keyword arguments to pass to *action*.
     """
 
@@ -191,7 +177,7 @@ def _wrap_with_before(responder, action, action_args, action_kwargs):
         responder: The responder method to wrap.
         action: A function with a similar signature to a resource responder
             method, taking the form ``func(req, resp, resource, params)``.
-        action_args: Additiona positional agruments to pass to *action*.
+        action_args: Additional positional agruments to pass to *action*.
         action_kwargs: Additional keyword arguments to pass to *action*.
     """
 


Test patch:
diff --git a/tests/test_after_hooks.py b/tests/test_after_hooks.py
index b8cf3d2..c32f4d5 100644
--- a/tests/test_after_hooks.py
+++ b/tests/test_after_hooks.py
@@ -303,3 +303,81 @@ def test_wrapped_resource_with_hooks_aware_of_resource(client, wrapped_resource_
     result = client.simulate_options('/wrapped_aware')
     assert result.status_code == 200
     assert not result.text
+
+
+class ResourceAwareGameHook(object):
+
+    VALUES = ('rock', 'scissors', 'paper')
+
+    @classmethod
+    def __call__(cls, req, resp, resource):
+        assert resource
+        assert resource.seed in cls.VALUES
+        assert resp.text == 'Responder called.'
+
+        header = resp.get_header('X-Hook-Game')
+        values = header.split(', ') if header else []
+        if values:
+            last = cls.VALUES.index(values[-1])
+            values.append(cls.VALUES[(last + 1) % len(cls.VALUES)])
+        else:
+            values.append(resource.seed)
+        resp.set_header('X-Hook-Game', ', '.join(values))
+
+
+_game_hook = ResourceAwareGameHook()
+
+
+@falcon.after(_game_hook)
+@falcon.after(_game_hook)
+class HandGame(object):
+
+    def __init__(self):
+        self.seed = None
+
+    @falcon.after(_game_hook)
+    def on_put(self, req, resp):
+        self.seed = req.media
+        resp.text = 'Responder called.'
+
+    @falcon.after(_game_hook)
+    def on_get_once(self, req, resp):
+        resp.text = 'Responder called.'
+
+    @falcon.after(_game_hook)
+    @falcon.after(_game_hook)
+    def on_get_twice(self, req, resp):
+        resp.text = 'Responder called.'
+
+    @falcon.after(_game_hook)
+    @falcon.after(_game_hook)
+    @falcon.after(_game_hook)
+    def on_get_thrice(self, req, resp):
+        resp.text = 'Responder called.'
+
+
+@pytest.fixture
+def game_client():
+    app = falcon.API()
+    resource = HandGame()
+
+    app.add_route('/seed', resource)
+    app.add_route('/once', resource, suffix='once')
+    app.add_route('/twice', resource, suffix='twice')
+    app.add_route('/thrice', resource, suffix='thrice')
+
+    return testing.TestClient(app)
+
+
+@pytest.mark.parametrize('seed,uri,expected', [
+    ('paper', '/once', 'paper, rock, scissors'),
+    ('scissors', '/twice', 'scissors, paper, rock, scissors'),
+    ('rock', '/thrice', 'rock, scissors, paper, rock, scissors'),
+    ('paper', '/thrice', 'paper, rock, scissors, paper, rock'),
+])
+def test_after_hooks_on_suffixed_resource(game_client, seed, uri, expected):
+    game_client.simulate_put('/seed', json=seed)
+
+    resp = game_client.simulate_get(uri)
+    assert resp.status_code == 200
+    assert resp.headers['X-Hook-Game'] == expected
diff --git a/tests/test_before_hooks.py b/tests/test_before_hooks.py
index 01bebb1..74e6906 100644
--- a/tests/test_before_hooks.py
+++ b/tests/test_before_hooks.py
@@ -149,7 +149,7 @@ class WrappedClassResource(object):
         self.bunnies = bunnies
 
 
-# NOTE(swistakm): we both both type of hooks (class and method)
+# NOTE(swistakm): we use both type of hooks (class and method)
 # at once for the sake of simplicity
 @falcon.before(resource_aware_bunnies)
 class ClassResourceWithAwareHooks(object):
@@ -349,3 +349,113 @@ def test_wrapped_resource_with_hooks_aware_of_resource(client, wrapped_aware_res
     result = client.simulate_get('/wrapped_aware', query_string='limit=11')
     assert result.status_code == 400
     assert wrapped_aware_resource.bunnies == 'fuzzy'
+
+
+_another_fish = Fish()
+
+
+def header_hook(req, resp, params):
+    value = resp.get_header('X-Hook-Applied') or '0'
+    resp.set_header('X-Hook-Applied', str(int(value) + 1))
+
+
+@falcon.before(header_hook)
+class PiggybackingCollection(object):
+
+    def __init__(self):
+        self._items = {}
+        self._sequence = 0
+
+    @falcon.before(_another_fish.hook)
+    def on_delete(self, req, resp, fish, itemid):
+        del self._items[itemid]
+        resp.set_header('X-Fish-Trait', fish)
+        resp.status = falcon.HTTP_NO_CONTENT
+
+    @falcon.before(header_hook)
+    @falcon.before(_another_fish.hook)
+    @falcon.before(header_hook)
+    def on_delete_collection(self, req, resp, fish):
+        if fish != 'wet':
+            raise falcon.HTTPUnavailableForLegalReasons('fish must be wet')
+        self._items = {}
+        resp.status = falcon.HTTP_NO_CONTENT
+
+    @falcon.before(_another_fish)
+    def on_get(self, req, resp, fish, itemid):
+        resp.set_header('X-Fish-Trait', fish)
+        resp.media = self._items[itemid]
+
+    def on_get_collection(self, req, resp):
+        resp.media = sorted(self._items.values(),
+                            key=lambda item: item['itemid'])
+
+    def on_head_(self):
+        return 'I shall not be decorated.'
+
+    def on_header(self):
+        return 'I shall not be decorated.'
+
+    def on_post_collection(self, req, resp):
+        self._sequence += 1
+        itemid = self._sequence
+        self._items[itemid] = dict(req.media, itemid=itemid)
+        resp.location = '/items/{}'.format(itemid)
+        resp.status = falcon.HTTP_CREATED
+
+
+@pytest.fixture
+def app_client():
+    items = PiggybackingCollection()
+
+    app = falcon.API()
+    app.add_route('/items', items, suffix='collection')
+    app.add_route('/items/{itemid:int}', items)
+
+    return testing.TestClient(app)
+
+
+def test_piggybacking_resource_post_item(app_client):
+    resp1 = app_client.simulate_post('/items', json={'color': 'green'})
+    assert resp1.status_code == 201
+    assert 'X-Fish-Trait' not in resp1.headers
+    assert resp1.headers['Location'] == '/items/1'
+    assert resp1.headers['X-Hook-Applied'] == '1'
+
+    resp2 = app_client.simulate_get(resp1.headers['Location'])
+    assert resp2.status_code == 200
+    assert resp2.headers['X-Fish-Trait'] == 'slippery'
+    assert resp2.headers['X-Hook-Applied'] == '1'
+    assert resp2.json == {'color': 'green', 'itemid': 1}
+
+    resp3 = app_client.simulate_get('/items')
+    assert resp3.status_code == 200
+    assert 'X-Fish-Trait' not in resp3.headers
+    assert resp3.headers['X-Hook-Applied'] == '1'
+    assert resp3.json == [{'color': 'green', 'itemid': 1}]
+
+
+def test_piggybacking_resource_post_and_delete(app_client):
+    for number in range(1, 8):
+        resp = app_client.simulate_post('/items', json={'number': number})
+        assert resp.status_code == 201
+        assert resp.headers['X-Hook-Applied'] == '1'
+
+        assert len(app_client.simulate_get('/items').json) == number
+
+    resp = app_client.simulate_delete('/items/7'.format(number))
+    assert resp.status_code == 204
+    assert resp.headers['X-Fish-Trait'] == 'wet'
+    assert resp.headers['X-Hook-Applied'] == '1'
+    assert len(app_client.simulate_get('/items').json) == 6
+
+    resp = app_client.simulate_delete('/items')
+    assert resp.status_code == 204
+    assert resp.headers['X-Hook-Applied'] == '3'
+    assert app_client.simulate_get('/items').json == []
+
+
+def test_decorable_name_pattern():
+    resource = PiggybackingCollection()
+    assert resource.on_head_() == 'I shall not be decorated.'
+    assert resource.on_header() == 'I shall not be decorated.'