Merge pull request #188 from Gallaecio/future-annotations

Fix DummyResponse detection when using strings as annotations

Author: kmike

scrapy_poet/injection.py

@@ -4,7 +4,18 @@
 import os
 import pprint
 import warnings
-from typing import Any, Callable, Dict, List, Mapping, Optional, Set, Type, cast
+from typing import (
+    Any,
+    Callable,
+    Dict,
+    List,
+    Mapping,
+    Optional,
+    Set,
+    Type,
+    cast,
+    get_type_hints,
+)
 from weakref import WeakKeyDictionary
 
 import andi
@@ -38,6 +49,10 @@
 logger = logging.getLogger(__name__)
 
 
+class _UNDEFINED:
+    pass
+
+
 class Injector:
     """
     Keep all the logic required to do dependency injection in Scrapy callbacks.
@@ -387,11 +402,13 @@ def is_callback_requiring_scrapy_response(
         # Let's assume response is going to be used.
         return True
 
-    if first_parameter.annotation is first_parameter.empty:
+    callback_type_hints = get_type_hints(callback)
+    first_parameter_type_hint = callback_type_hints.get(first_parameter_key, _UNDEFINED)
+    if first_parameter_type_hint is _UNDEFINED:
         # There's no type annotation, so we're probably using response here.
         return True
 
-    if issubclass_safe(first_parameter.annotation, DummyResponse):
+    if issubclass_safe(first_parameter_type_hint, DummyResponse):
         # See: https://github.com/scrapinghub/scrapy-poet/issues/48
         # See: https://github.com/scrapinghub/scrapy-poet/issues/118
         if raw_callback is None and not is_min_scrapy_version("2.8.0"):

tests/test_response_required_logic.py

@@ -157,6 +157,40 @@ def parse11(self, response: TextResponse):
     def parse12(self, response: TextResponse, book_page: DummyProductPage):
         pass
 
+    # Strings as type hints (which in addition to something users may do, is
+    # also functionally-equivalent to having from __future__ import annotations
+    # in your code, see https://peps.python.org/pep-0649/).
+
+    def parse13(self, response: "DummyResponse"):
+        pass
+
+    def parse14(self, res: "DummyResponse"):
+        pass
+
+    def parse15(self, response, book_page: "BookPage"):
+        pass
+
+    def parse16(self, response: "DummyResponse", book_page: "BookPage"):
+        pass
+
+    def parse17(self, response, book_page: "DummyProductPage"):
+        pass
+
+    def parse18(self, response: "DummyResponse", book_page: "DummyProductPage"):
+        pass
+
+    def parse19(self, response, book_page: "FakeProductPage"):
+        pass
+
+    def parse20(self, response: "DummyResponse", book_page: "FakeProductPage"):
+        pass
+
+    def parse21(self, response: "TextResponse"):
+        pass
+
+    def parse22(self, response: "TextResponse", book_page: "DummyProductPage"):
+        pass
+
 
 def test_get_callback():
     spider = MySpider()
@@ -226,6 +260,36 @@ def test_is_callback_using_response_for_scrapy28_below() -> None:
     assert (
         is_callback_requiring_scrapy_response(spider.parse12, request.callback) is True
     )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse13, request.callback) is False
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse14, request.callback) is False
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse15, request.callback) is True
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse16, request.callback) is False
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse17, request.callback) is True
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse18, request.callback) is False
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse19, request.callback) is True
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse20, request.callback) is False
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse21, request.callback) is True
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse22, request.callback) is True
+    )
     # Callbacks created with the callback_for function won't make use of
     # the response, but their providers might use them.
     assert (
@@ -263,6 +327,21 @@ def test_is_callback_using_response_for_scrapy28_below() -> None:
     assert (
         is_callback_requiring_scrapy_response(spider.parse12, request.callback) is True
     )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse15, request.callback) is True
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse17, request.callback) is True
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse19, request.callback) is True
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse21, request.callback) is True
+    )
+    assert (
+        is_callback_requiring_scrapy_response(spider.parse22, request.callback) is True
+    )
 
     for method in (
         spider.parse3,
@@ -337,6 +416,46 @@ def test_is_callback_using_response_for_scrapy28_and_above() -> None:
                 is_callback_requiring_scrapy_response(spider.parse12, request.callback)
                 is True
             )
+            assert (
+                is_callback_requiring_scrapy_response(spider.parse13, request.callback)
+                is False
+            )
+            assert (
+                is_callback_requiring_scrapy_response(spider.parse14, request.callback)
+                is False
+            )
+            assert (
+                is_callback_requiring_scrapy_response(spider.parse15, request.callback)
+                is True
+            )
+            assert (
+                is_callback_requiring_scrapy_response(spider.parse16, request.callback)
+                is False
+            )
+            assert (
+                is_callback_requiring_scrapy_response(spider.parse17, request.callback)
+                is True
+            )
+            assert (
+                is_callback_requiring_scrapy_response(spider.parse18, request.callback)
+                is False
+            )
+            assert (
+                is_callback_requiring_scrapy_response(spider.parse19, request.callback)
+                is True
+            )
+            assert (
+                is_callback_requiring_scrapy_response(spider.parse20, request.callback)
+                is False
+            )
+            assert (
+                is_callback_requiring_scrapy_response(spider.parse21, request.callback)
+                is True
+            )
+            assert (
+                is_callback_requiring_scrapy_response(spider.parse22, request.callback)
+                is True
+            )
             # Callbacks created with the callback_for function won't make use of
             # the response, but their providers might use them.
             assert (
@@ -382,3 +501,13 @@ def check_response_required(expected, callback):
     yield from check_response_required(False, spider.parse10)
     yield from check_response_required(True, spider.parse11)
     yield from check_response_required(True, spider.parse12)
+    yield from check_response_required(False, spider.parse13)
+    yield from check_response_required(False, spider.parse14)
+    yield from check_response_required(True, spider.parse15)
+    yield from check_response_required(True, spider.parse16)
+    yield from check_response_required(True, spider.parse17)
+    yield from check_response_required(False, spider.parse18)
+    yield from check_response_required(True, spider.parse19)
+    yield from check_response_required(False, spider.parse20)
+    yield from check_response_required(True, spider.parse21)
+    yield from check_response_required(True, spider.parse22)