python-injector / injector

Python dependency injection framework, inspired by Guice
BSD 3-Clause "New" or "Revised" License
1.29k stars 81 forks source link

Issue with injecting Generic classes - broke in python 3.7 #175

Open davebirch opened 3 years ago

davebirch commented 3 years ago

Hi there, currently upgrading a project using injector to python 3.8. We use MyPy generics in our project and I discovered that I can no longer inject my generic class.

On further investigation, this seems to be still present in master, works in python 3.6.8, then is broken in anything 3.7 and above.

The following unit test reproduces the issue:

GenericType = TypeVar("GenericType")

def test_inject_generic_class() -> None:

    class GenericClass(Generic[GenericType]):
        pass

    class InjectsGeneric:
        @inject
        def __init__(self, injected_generic: GenericClass[str]):
            self.injected_generic = injected_generic

    def bindings(binder: Binder) -> None:
        binder.bind(GenericClass)

    injector = Injector(bindings)
    instance = injector.get(InjectsGeneric)

    assert isinstance(instance, InjectsGeneric)
    assert isinstance(instance.injected_generic, GenericClass)

Output on the failing python versions is:

______________________ test_inject_simple_generic_class _______________________
Traceback (most recent call last):
  File "D:\git\injector\injector\__init__.py", line 658, in get_binding
    return self._get_binding(interface, only_this_binder=is_scope)
  File "D:\git\injector\injector\__init__.py", line 653, in _get_binding
    raise KeyError
KeyError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "D:\git\injector\injector_test.py", line 1494, in test_inject_simple_generic_class
    instance = injector.get(InjectsGeneric)
  File "D:\git\injector\injector\__init__.py", line 963, in get
    result = scope_instance.get(interface, binding.provider).get(self)
  File "D:\git\injector\injector\__init__.py", line 291, in get
    return injector.create_object(self._cls)
  File "D:\git\injector\injector\__init__.py", line 990, in create_object
    self.call_with_injection(cls.__init__, self_=instance, kwargs=additional_kwargs)
  File "D:\git\injector\injector\__init__.py", line 1021, in call_with_injection
    dependencies = self.args_to_inject(
  File "D:\git\injector\injector\__init__.py", line 111, in wrapper
    return function(*args, **kwargs)
  File "D:\git\injector\injector\__init__.py", line 1069, in args_to_inject
    instance = self.get(interface)  # type: Any
  File "D:\git\injector\injector\__init__.py", line 952, in get
    binding, binder = self.binder.get_binding(interface)
  File "D:\git\injector\injector\__init__.py", line 667, in get_binding
    binding = self.create_binding(interface)
  File "D:\git\injector\injector\__init__.py", line 582, in create_binding
    provider = self.provider_for(interface, to)
  File "D:\git\injector\injector\__init__.py", line 644, in provider_for
    raise UnknownProvider('couldn\'t determine provider for %r to %r' % (interface, to))
injector.UnknownProvider: couldn't determine provider for injector_test.test_inject_simple_generic_class.<locals>.GenericClass[str] to None

Could do with getting this one fixed soon-ish, and will be taking a look tomorrow to see if I can create a PR that fixes it. If you have any hints or tips that might help, it would be appreciated :)

davebirch commented 3 years ago

As an interesting point of note, if I change the binding to bind to the exact generic type specified in the injectable class GenericClass[str], the same exception is thrown, but on instantiation of the injector instead:

Changed code from unit test:

def bindings(binder: Binder) -> None:
        binder.bind(GenericClass[str])

Test failure and stacktrace:

_____________________________ test_inject_generic _____________________________

    def test_inject_generic() -> None:
        class GenericClass(Generic[GenericType]):
            pass

        assert issubclass(type(GenericClass), type)

        class InjectsGeneric:
            @inject
            def __init__(self, injected_generic: GenericClass[str]):
                self.injected_generic = injected_generic

        def bindings(binder: Binder) -> None:
            binder.bind(GenericClass[str])

>       injector = Injector(bindings)

test_jenkins_injector.py:80: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
..\..\..\..\.venv.win\py3.8\lib\site-packages\injector\__init__.py:924: in __init__
    self.binder.install(module)
..\..\..\..\.venv.win\py3.8\lib\site-packages\injector\__init__.py:588: in install
    instance(self)
test_jenkins_injector.py:78: in bindings
    binder.bind(GenericClass[str])
..\..\..\..\.venv.win\py3.8\lib\site-packages\injector\__init__.py:490: in bind
    self._bindings[interface] = self.create_binding(interface, to, scope)
..\..\..\..\.venv.win\py3.8\lib\site-packages\injector\__init__.py:593: in create_binding
    provider = self.provider_for(interface, to)
jstasiak commented 3 years ago

Hey, I don't think this was ever explicitly supported – if it works on some Python versions it's purely by accident. :) I'm not opposed to supporting this in principle, could you provide an example what would you use it for though? (A real example can affect the design decisions to make here)

davebirch commented 3 years ago

Hehe, that makes some sense, yeah, gotta love a happy accident. Still not quite got to the bottom of why it works in 3.6, but need to spend some time figuring out the differences in the typing module between 3.6 and 3.8 as well as remind myself of how the Injector internals work :D

As to our use cases:

One is a simple generic cache class, with generic types for the key and value types of the cached data. (essentially a more complicated dict) - the generic types used here in practice are a mixture of custom objects (URL objects as keys, with JenkinsAPI data objects as the values. Other implementations have str keys instead, but the value types could be pretty much anything.

The other hasn't been written yet, but is likely to at some point soon, which is generic scanner, with generic types for the thing being scanned, and the result class to be returned. TypeVar for the generic result type here will likely be using bounds if that makes any difference?

Is there any more detail you would need to influence design decisions?

I could probably remove the generics usage if entirely necessary, or wrap over the generics with a concrete implementation, but would like to explore the options first that don't involve a load of boilerplate :)

jstasiak commented 3 years ago

I'd ideally like to see some real world-like pieces of example code that demonstrates the need for this feature, from the description and from the unit tests given so far I'm not entirely convinced this is something that I want to support (but I still don't understand it fully, I admit).

wooyek commented 3 years ago

We're using parameterized generics to inject dependencies that use generic parameter to configure itself. For eg. here DataclassSerializer will use its generic parametr (Order in the example) to build serialization schema.

@injector.inject
@injector.singleton
class OrderLineProvider:
    _orders_collection: OrdersCollection
    _serializer: DataclassSerializer[Order]
jstasiak commented 3 years ago

Thank you – what's the DataclassSerializer constructor signature and what's its interface the OrderLineProvider uses? I'm mainly interested in what places does the generic parameter of DataclassSerializer show up and what's being injected into the constructor.

hf-kklein commented 1 year ago

Hi, I'm late to the party but can give an example on why this could be useful. We're building software that retrieves data from somewhere and does something with it. And this somewhere, where the data comes from varies. We'd like to use generics for that.

Imagine you have class MyModel, it describes a datastructure you'd like to process. Now we use injector to decide where the MyModel comes from: Could be a database, could be a file, could be a S3 Bucket, HTTP request... you name it. The different sources are all ModelProvider[MyModel]s, that have specific methods like:

ModelType = TypeVar("ModelType")
class ModelProvider(Generic[ModelType]):
    def get_data()->list[ModelType]
        """returns a list of models from somewhere"""

There's one implementation of the generic model provider per data source. So there's a DatabaseMyModelProvider, FileMyModelProvider, S3BucketMyModelProvider ...

The signature of the class that processes MyModels looks like this:

def MyModelProcessor:
   """reads data from the model provider and does something with them"""

   @inject
   def __init__(my_model_provider: ModelProvider[MyModel]):
        self._my_model_provider = my_model_provider

   def do_something():
        models = self._my_model_provider.get_data()
        # do something

Now it'd be super nice to have something like this:

def configure_for_database(binder):
    """used in integration tests and the running application"""
    binder.bind(ModelProvider[MyModel], to=DatabaseMyModelProvider(host, usr, pwd))

def configure_for_database(binder):
    """used in simpler unittests"""
    binder.bind(ModelProvider[MyModel], to=FileMyModelProvider(path_to_a_file_with_testdata))

Being able to easily switch between those different implementations is great and it de-couples the processing logic from the choice of the datasource.

But currently, we cannot use the straight forward code above. But instead, we need a little bit of boilerplate, because we cannot bind ModelProvider[MyModel] directly:

class MyModelProvider(ModelProvider[MyModel], ABC):
    """this class is pure boilerplate"""
    # here's nothing
    pass

...

def configure_for_database(binder):
    """used in integration tests and the running application"""
    binder.bind(MyModelProvider, to=to=DatabaseMyModelProvider(host, usr, pwd))

def configure_for_database(binder):
    """used in simpler unittests"""
    binder.bind(MyModelProvider, to=FileMyModelProvider(path_to_a_file_with_testdata))

...

def MyModelProcessor:
   """reads data from the model provider and does something with them"""

   @inject
   def __init__(my_model_provider: MyModelProvider):
        self._my_model_provider = my_model_provider

This artificial class MyModelProvider does nothing. it's only a non-generic class that inherits from the generic class to be used in the binder. And it feels unnecessary. If we could bind ModelProvider[MyModel] directly, this workaround wouldn't be necessary.

jstasiak commented 1 year ago

Thank you @hf-kklein, I think this is a convincing example.

I'd be open to accept a PR implementing some support for that (there was https://github.com/python-injector/injector/pull/188 once upon a time).

dkmiller commented 11 months ago

Thanks @jstasiak! @wooyek — do you need any help reifying your old PR?