ivankorobkov / python-inject

Python dependency injection
Apache License 2.0
672 stars 77 forks source link

__future__ breaks DI. #52

Closed ancostas closed 4 years ago

ancostas commented 4 years ago

Hi!

I've come across an issue when injecting classes that have signatures in which they return themselves. AFAIK you need to import annotations from __future__ in order to do this kind of thing, and this seems to break DI for everything in the file where this is done, which is a bit unexpected IMO.

ex:

from __future__ import annotations

import inject
from test import BaseTestInject

class Something:
    def return_self(self) -> Something:
        return self

    @inject.autoparams()
    def test_func(self, val: int):
        return val

class AnotherThing:
    @inject.autoparams()
    def test_func(self, val: int):
        return val

class TestFutureSupport(BaseTestInject):
    def test_future_support(self):
        inject.configure(lambda binder: binder.bind(int, 123), bind_in_runtime=False)

        self.assertRaises(inject.InjectorException, Something().test_func)
        self.assertRaises(inject.InjectorException, AnotherThing().test_func)

Would it be feasible to register string versions of the annotations automatically, if they are not provided by user? It seems like this would resolve the issue, at least for my superficial test, and is something the user would not expect to need to do.

from __future__ import annotations

import inject
from test import BaseTestInject

class Something:
    def return_self(self) -> Something:
        return self

    @inject.autoparams()
    def test_func(self, val: int):
        return val

class AnotherThing:
    @inject.autoparams()
    def test_func(self, val: int):
        return val

class TestFutureSupport(BaseTestInject):
    def test_future_support(self):
        def deps(binder):
            binder.bind(int, 123)
            # This fixes things --> should it be done automatically?
            binder.bind('int', 123)
        inject.configure(deps, bind_in_runtime=False)

        assert Something().test_func() == 123
        assert AnotherThing().test_func() == 123
ivankorobkov commented 4 years ago

Hi!

this seems to break DI for everything in the file where this is done, which is a bit unexpected IMO.

It changes the way annotations work. During evaluation only strings are present and not types. This is not the standard way yet.

Would it be feasible to register string versions of the annotations automatically, if they are not provided by user?

I do not think so. Internally, there is one global shared injector instance. Imagine two different classes in two different modules with the same name 'DB' or 'Cache', etc.

Well, the correct way to support from __future__ import annotations is to migrate from inspect.getfullargspec(func) to typing.get_type_hints(func).

I can do it later this week, or you can make a PR.

ancostas commented 4 years ago

Got it! I have some time later this week dedicated to investigations of this kind, so if its all the same to you I'll handle it on my end and cut a PR.

ancostas commented 4 years ago

Looks like you've merged my PR; closing this. Thanks! :grin:

ivankorobkov commented 4 years ago

I'll post a comment here after releasing a new version later.

ivankorobkov commented 4 years ago

Please, check the master version. I restored Python3.6 compatibility. Also I had to remove support for string bindings in configuration. Unfortunately, there is no way to detect when from __future__ import annotations is used.

ivankorobkov commented 4 years ago

If everything is OK, I will make the next release.

ancostas commented 4 years ago

I will take a look tonight; I was hoping to keep support for string bindings in a "clever" way, but what I was doing may have been more of a hack than anything. Will get back to you.

ancostas commented 4 years ago

I'll need to take a second look this weekend, but a couple things worry me a bit:

The main issue, as you say, is it is basically impossible to detect whether that feature has been imported into the file in which your function resides. Surely there must be some way to achieve identical behavior regardless of whether it has been, though -- otherwise we'd be violating the principle of least surprise.

ivankorobkov commented 4 years ago

This is a breaking change for anyone who relies on string bindings

It was possible to use strings in annotations and configs, but it was not an official way. I hate to break backwards compatibility, but it seems OK here.

I've released 4.2.0 https://pypi.org/project/Inject/4.2.0/