Supporting to_return in web-poet rules

pyproject.toml

@@ -8,6 +8,7 @@ multi_line_output = 3
 [[tool.mypy.overrides]]
 module = [
     "tests.test_downloader.*",
+    "tests.test_web_poet_rules.*",
     "tests.test_scrapy_dependencies.*",
 ]
 # Ignore this type of error since mypy expects an Iterable return

scrapy_poet/api.py

@@ -1,5 +1,5 @@
 from inspect import iscoroutinefunction
-from typing import Callable, Optional, Type
+from typing import Any, Callable, Optional
 
 from scrapy.http import Request, Response
 from web_poet.pages import ItemPage
@@ -29,8 +29,9 @@ def __init__(self, url: str, request=Optional[Request]):
         super().__init__(url=url, request=request)
 
 
-def callback_for(page_cls: Type[ItemPage]) -> Callable:
-    """Create a callback for an :class:`web_poet.pages.ItemPage` subclass.
+def callback_for(page_or_item_cls: Any) -> Callable:
+    """Create a callback for an :class:`web_poet.pages.ItemPage` subclass or an
+    item type.
 
     The generated callback returns the output of the
     ``ItemPage.to_item()`` method, i.e. extracts a single item
@@ -104,24 +105,27 @@ def parse(self, response):
     disk queues, because in this case Scrapy is able to serialize
     your request object.
     """
-    if not issubclass(page_cls, ItemPage):
-        raise TypeError(f"{page_cls.__name__} should be a subclass of ItemPage.")
-
-    # When the callback is used as an instance method of the spider, it expects
-    # to receive 'self' as its first argument. When used as a simple inline
-    # function, it expects to receive a response as its first argument.
-    #
-    # To avoid a TypeError, we need to receive a list of unnamed arguments and
-    # a dict of named arguments after our injectable.
-    def parse(*args, page: page_cls, **kwargs):  # type: ignore
-        yield page.to_item()  # type: ignore
-
-    async def async_parse(*args, page: page_cls, **kwargs):  # type: ignore
-        yield await page.to_item()  # type: ignore
-
-    if iscoroutinefunction(page_cls.to_item):
-        setattr(async_parse, _CALLBACK_FOR_MARKER, True)
-        return async_parse
+    if issubclass(page_or_item_cls, ItemPage):
+        # When the callback is used as an instance method of the spider, it expects
+        # to receive 'self' as its first argument. When used as a simple inline
+        # function, it expects to receive a response as its first argument.
+        #
+        # To avoid a TypeError, we need to receive a list of unnamed arguments and
+        # a dict of named arguments after our injectable.
+        def parse(*args, page: page_or_item_cls, **kwargs):  # type: ignore
+            yield page.to_item()  # type: ignore
+
+        async def async_parse(*args, page: page_or_item_cls, **kwargs):  # type: ignore
+            yield await page.to_item()  # type: ignore
+
+        if iscoroutinefunction(page_or_item_cls.to_item):
+            setattr(async_parse, _CALLBACK_FOR_MARKER, True)
+            return async_parse
+
+    else:
+
+        def parse(*args, item: page_or_item_cls, **kwargs):  # type:ignore
+            yield item
 
     setattr(parse, _CALLBACK_FOR_MARKER, True)
     return parse

scrapy_poet/downloadermiddlewares.py

@@ -17,6 +17,7 @@
 from .page_input_providers import (
     HttpClientProvider,
     HttpResponseProvider,
+    ItemProvider,
     PageParamsProvider,
     RequestUrlProvider,
     ResponseUrlProvider,
@@ -31,6 +32,7 @@
     PageParamsProvider: 700,
     RequestUrlProvider: 800,
     ResponseUrlProvider: 900,
+    ItemProvider: 1000,
 }
 
 InjectionMiddlewareTV = TypeVar("InjectionMiddlewareTV", bound="InjectionMiddleware")

scrapy_poet/injection.py

@@ -57,7 +57,7 @@ def load_providers(self, default_providers: Optional[Mapping] = None):  # noqa:
         }
         provider_classes = build_component_list(providers_dict)
         logger.info(f"Loading providers:\n {pprint.pformat(provider_classes)}")
-        self.providers = [load_object(cls)(self.crawler) for cls in provider_classes]
+        self.providers = [load_object(cls)(self) for cls in provider_classes]
         check_all_providers_are_callable(self.providers)
         # Caching whether each provider requires the scrapy response
         self.is_provider_requiring_scrapy_response = {
@@ -141,13 +141,68 @@ def build_plan(self, request: Request) -> andi.Plan:
             overrides=self.overrides_registry.overrides_for(request).get,
         )
 
+    def provider_requirements(self, request: Request, plan: andi.Plan) -> Set[Any]:
+        """Return a set of classes which indicate any requirements needed by a
+        provider in order to successfully provide for the given ``request`` and
+        ``plan``.
+        """
+        provider_requirements = set()
+        for cls, _ in plan.dependencies:
+            for provider in self.providers:
+                if not provider.is_provided(cls):
+                    continue
+                classes = provider.requirements_for(cls, request)
+                if classes:
+                    provider_requirements.update(set(classes))
+        return provider_requirements
+
+    @inlineCallbacks
+    def build_provider_requirements(
+        self, request: Request, response: Response, plan: andi.Plan
+    ):
+        """This builds out any requirements that a provider might need before
+        calling them.
+
+        The instances that are built here would later be used in andi's
+        'externally_provided' parameter when calling the providers.
+        """
+
+        provider_requirements = self.provider_requirements(request, plan)
+        provider_requirements_instances = (
+            yield from self.build_instances_from_providers(
+                request, response, provider_requirements
+            )
+        )
+
+        # TODO: recursive requirements resolution on POs that need
+        # items which are fulfilled by other POs.
+
+        for prov_req in provider_requirements:
+            for cls, kwargs_spec in andi.plan(prov_req, is_injectable=is_injectable):
+                if cls not in provider_requirements_instances.keys():
+                    provider_requirements_instances[cls] = cls(
+                        **kwargs_spec.kwargs(provider_requirements_instances)
+                    )
+
+        return provider_requirements_instances
+
     @inlineCallbacks
     def build_instances(self, request: Request, response: Response, plan: andi.Plan):
         """Build the instances dict from a plan including external dependencies."""
-        # First we build the external dependencies using the providers
-        instances = yield from self.build_instances_from_providers(
+
+        provider_requirements_instances = yield self.build_provider_requirements(
             request, response, plan
         )
+
+        dependencies = {cls for cls, _ in plan.dependencies}
+
+        instances = yield from self.build_instances_from_providers(
+            request,
+            response,
+            dependencies,
+            externally_provided=provider_requirements_instances,
+        )
+
         # All the remaining dependencies are internal so they can be built just
         # following the andi plan.
         for cls, kwargs_spec in plan.dependencies:
@@ -158,17 +213,22 @@ def build_instances(self, request: Request, response: Response, plan: andi.Plan)
 
     @inlineCallbacks
     def build_instances_from_providers(
-        self, request: Request, response: Response, plan: andi.Plan
+        self,
+        request: Request,
+        response: Response,
+        dependencies: Set,
+        externally_provided=None,
     ):
         """Build dependencies handled by registered providers"""
         instances: Dict[Callable, Any] = {}
         scrapy_provided_dependencies = self.available_dependencies_for_providers(
             request, response
         )
-        dependencies_set = {cls for cls, _ in plan.dependencies}
+        externally_provided = externally_provided or {}
+        externally_provided.update(scrapy_provided_dependencies)
         for provider in self.providers:
             provided_classes = {
-                cls for cls in dependencies_set if provider.is_provided(cls)
+                cls for cls in dependencies if provider.is_provided(cls)
             }
             provided_classes -= instances.keys()  # ignore already provided types
             if not provided_classes:
@@ -197,11 +257,11 @@ def build_instances_from_providers(
 
             if not objs:
                 kwargs = andi.plan(
-                    provider,
+                    provider.dynamic_call_signature or provider,
                     is_injectable=is_injectable,
-                    externally_provided=scrapy_provided_dependencies,
+                    externally_provided=externally_provided,
                     full_final_kwargs=False,
-                ).final_kwargs(scrapy_provided_dependencies)
+                ).final_kwargs(externally_provided)
                 try:
 
                     # Invoke the provider to get the data

scrapy_poet/overrides.py

@@ -1,12 +1,24 @@
 import logging
 from abc import ABC, abstractmethod
 from collections import defaultdict
-from typing import Callable, Dict, Iterable, List, Mapping, Optional, Tuple, Type, Union
+from typing import (
+    Any,
+    Callable,
+    Dict,
+    Iterable,
+    List,
+    Mapping,
+    Optional,
+    Tuple,
+    Type,
+    Union,
+)
+from warnings import warn
 
 from scrapy import Request
 from scrapy.crawler import Crawler
 from url_matcher import Patterns, URLMatcher
-from web_poet import ItemPage
+from web_poet import ItemPage, RulesRegistry
 from web_poet.rules import ApplyRule
 
 logger = logging.getLogger(__name__)
@@ -26,7 +38,7 @@ def overrides_for(self, request: Request) -> Mapping[Callable, Callable]:
         pass
 
 
-class OverridesRegistry(OverridesRegistryBase):
+class OverridesRegistry(OverridesRegistryBase, RulesRegistry):
     """
     Overrides registry that reads the overrides from the ``SCRAPY_POET_OVERRIDES``
     in the spider settings. It is a list and each rule can be a tuple or an
@@ -89,14 +101,18 @@ class OverridesRegistry(OverridesRegistryBase):
     def from_crawler(cls, crawler: Crawler) -> Crawler:
         return cls(crawler.settings.getlist("SCRAPY_POET_OVERRIDES", []))
 
-    def __init__(self, rules: Optional[Iterable[RuleFromUser]] = None) -> None:
-        self.rules: List[ApplyRule] = []
-        self.matcher: Dict[Type[ItemPage], URLMatcher] = defaultdict(URLMatcher)
-        for rule in rules or []:
-            self.add_rule(rule)
-        logger.debug(f"List of parsed ApplyRules:\n{self.rules}")
+    def __init__(self, rules: Optional[Iterable[ApplyRule]] = None) -> None:
+        super().__init__(rules=rules)
+        self.overrides_matcher: Dict[Type[ItemPage], URLMatcher] = defaultdict(
+            URLMatcher
+        )
+        self.item_matcher: Dict[Any, URLMatcher] = defaultdict(URLMatcher)
+        for rule_id, rule in enumerate(self._rules):
+            self.add_rule(rule_id, rule)
+        logger.debug(f"List of parsed ApplyRules:\n{self._rules}")
 
-    def add_rule(self, rule: RuleFromUser) -> None:
+    def add_rule(self, rule_id: int, rule: ApplyRule) -> None:
+        # TODO: deprecate this, alongside the tests and docs; Update CHANGELOG
         if isinstance(rule, (tuple, list)):
             if len(rule) != 3:
                 raise ValueError(
@@ -108,16 +124,47 @@ def add_rule(self, rule: RuleFromUser) -> None:
             rule = ApplyRule(
                 for_patterns=Patterns([pattern]), use=use, instead_of=instead_of
             )
-        self.rules.append(rule)
-        # FIXME: This key will change with the new rule.to_return
-        self.matcher[rule.instead_of].add_or_update(  # type: ignore
-            len(self.rules) - 1, rule.for_patterns
-        )
 
-    def overrides_for(self, request: Request) -> Mapping[Callable, Callable]:
-        overrides: Dict[Callable, Callable] = {}
-        for instead_of, matcher in self.matcher.items():
+        # A common case when a PO subclasses another one with the same URL
+        # pattern. See the test_item_return_subclass() test case.
+        matched = self.item_matcher[rule.to_return]
+        if [
+            pattern
+            for pattern in matched.patterns.values()
+            if pattern == rule.for_patterns
+        ]:
+            # TODO: It would be great to also list down the rules having the
+            # same URL pattern. But this would require some refactoring.
+            warn(
+                f"A similar URL pattern {list(matched.patterns.values())} has been "
+                f"declared earlier which uses to_return={rule.to_return}. When "
+                f"matching URLs against rules, the latest declared rule is used. "
+                f"Consider explicitly updating the priority of the rules containing "
+                f"the said URL pattern to easily match the expectations when reading "
+                f"the code."
+            )
+
+        if rule.instead_of:
+            self.overrides_matcher[rule.instead_of].add_or_update(
+                rule_id, rule.for_patterns
+            )
+        if rule.to_return:
+            self.item_matcher[rule.to_return].add_or_update(rule_id, rule.for_patterns)
+
+    # TODO: These URL matching functionalities could be moved to web-poet.
+
+    def _run_matcher(
+        self, request: Request, url_matcher
+    ) -> Mapping[Callable, Callable]:
+        result: Dict[Callable, Callable] = {}
+        for target, matcher in url_matcher.items():
             rule_id = matcher.match(request.url)
             if rule_id is not None:
-                overrides[instead_of] = self.rules[rule_id].use
-        return overrides
+                result[target] = self._rules[rule_id].use
+        return result
+
+    def overrides_for(self, request: Request) -> Mapping[Callable, Callable]:
+        return self._run_matcher(request, self.overrides_matcher)
+
+    def page_object_for_item(self, request: Request) -> Mapping[Callable, Callable]:
+        return self._run_matcher(request, self.item_matcher)