kodemore / kink

Dependency injection container made for Python
MIT License
397 stars 25 forks source link

Using Optional type is not working #21

Closed akadlec closed 2 years ago

akadlec commented 2 years ago

Hi in this example, autowiring is not working:

class IPublisher(abcABC):
    @abc.abstractmethod
    def publish(
        self,
    ) -> None:
        ...

@inject(alias=IPublisher)
class Publisher(IPublisher):
    def publish(
        self,
    ) -> None:
        print("publish")

@inject
class EntitiesSubscriber:
    def __init__(
        self,
        publisher: Optional[IPublisher] = None,
    ) -> None:
        ...

When in subscriber is services typed as optional: publisher: Optional[IPublisher] = None autowiring is not working When it is changed to List[IPublisher] autowiring will inject all services and also when Optional type is removed: publisher: IPublisher = None service is injected also but this is leading to invalid typing

akadlec commented 2 years ago

Here is working patch:

def _resolve_function_kwargs(
    alias_map: Dict[str, str], parameters_name: Tuple[str, ...], parameters: Dict[str, Parameter], container: Container,
) -> Dict[str, Any]:
    resolved_kwargs = {}
    for name in parameters_name:
        if name in alias_map and alias_map[name] in container:
            resolved_kwargs[name] = container[alias_map[name]]
            continue

        if name in container:
            resolved_kwargs[name] = container[name]
            continue

        if parameters[name].type in container:
            resolved_kwargs[name] = container[parameters[name].type]
            continue

        if get_origin(parameters[name].type) is Union:
            for unio_part in get_args(parameters[name].type):
                if unio_part is not None and unio_part in container:
                    resolved_kwargs[name] = container[unio_part]
                    continue

        if name not in resolved_kwargs and parameters[name].default is not Undefined:
            resolved_kwargs[name] = parameters[name].default

    return resolved_kwargs
dkraczkowski commented 2 years ago

Hi @akadlec. Thanks for your interest.

This is an intended behaviour. If a type is an optional type it is hard to understand from autowiring perpective in which scenario dependency should be injected, and in which not. It is safer to assume if something is optional it is not required for the code to work thus we should leave it as it is.

If you would like to keep your optional annotation and still make sure autowiring works you can bind the dependency manually, like in the example below:

    class IPublisher(abc.ABC):
        @abc.abstractmethod
        def publish(
                self,
        ) -> None:
            ...

    @inject(alias=IPublisher)
    class Publisher(IPublisher):
        def publish(
                self,
        ) -> None:
            print("publish")

    @inject(bind={"publisher": IPublisher})
    class EntitiesSubscriber:
        def __init__(
                self,
                publisher: Optional[IPublisher] = None,
        ) -> None:
            ...
dkraczkowski commented 2 years ago

@akadlec As a sidenote: Keep in mind in a good design all the classes should be always instantiated in a correct state, so if your EntitiesSubscriber aggregates or creates composition with IPublisher, IPublisher this should be a required dependency.

If your problem is that in your code you are just instantiating class without arguments because autowiring is hitting in, I would highly advice to pull dependencies from the container instead, like below:

from kink import di
from typing import Protocol

class IPublisher(Protocol):
        @abc.abstractmethod
        def publish(
                self,
        ) -> None:
            ...

@inject(alias=IPublisher)
class Publisher(IPublisher):
    def publish(
            self,
    ) -> None:
        print("publish")

@inject()
class EntitiesSubscriber:
    def __init__(
            self,
            publisher: IPublisher,
    ) -> None:
        ...

subscriber: EntitiesSubscriber  = di[EntitiesSubscriber]
akadlec commented 2 years ago

publisher: IPublisher = None is not correct declaration

Imagine EntitiesSubscriber has optional dependency on IPublisher which is implemented in another optional module. And in case this another module is installed and used, autowiring should find service and "autowire" it.

Sure I could do it manually, but in this case usage of your library is not necessary.

dkraczkowski commented 2 years ago

I fixed the example as I forgot to remove the = None in the class' initialiser.

If you want to keep you parameter Optional do it, but then you need to use bind like in the example above.

You are welcome.

akadlec commented 2 years ago

if @inject(bind={"publisher": IPublisher}) will fix this problem, then ok