Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Injection not working for class methods #318

Closed
scott2b opened this issue Nov 3, 2020 · 19 comments
Closed

Injection not working for class methods #318

scott2b opened this issue Nov 3, 2020 · 19 comments
Assignees
Labels

Comments

@scott2b
Copy link

scott2b commented Nov 3, 2020

I am not quite sure if this is expected behavior or not. Methods annotated as @classmethod end up getting extra parameters injected. The following code demonstrates. I discovered this while using Closing, but filled out the example a bit as I discovered that it is a general issue for Provide.

import sys
from dependency_injector import containers, providers
from dependency_injector.wiring import Provide, Closing


def my_factory():
    return 'test-factory'


def my_resource():
    yield 'test-resource'
    print('Closing')


class Container(containers.DeclarativeContainer):

    factory = providers.Factory(my_factory)
    resource = providers.Resource(my_resource)


def do_function_thing(r:str=Closing[Provide[Container.resource]]) -> None:
    print('from function', r)


class MyClass():

    def do_instance_thing(self, r:str=Closing[Provide[Container.resource]]) -> None:
        print('from instance', r)

    @classmethod
    def do_class_thing(cls, r:str=Closing[Provide[Container.resource]]) -> None:
        print('from class', r)

    @classmethod
    def non_closing_class_thing(cls, r:str=Provide[Container.factory]) -> None:
        print('non-closing from class', r)


container = Container()
container.init_resources()
container.wire(modules=[sys.modules[__name__]])


do_function_thing()
c = MyClass()
c.do_instance_thing()

# both of these end up getting multiple values for r:
c.non_closing_class_thing()
c.do_class_thing()

The resulting output is:

from function test-resource
Closing
from instance test-resource
Closing
Traceback (most recent call last):
  File "clstest.py", line 49, in <module>
    c.non_closing_class_thing()
  File "/Users/scott/repos/github.com/scott2b/Starlight/.venv/lib/python3.8/site-packages/dependency_injector/wiring.py", line 296, in _patched
    result = fn(*args, **to_inject)
TypeError: non_closing_class_thing() got multiple values for argument 'r'
@rmk135 rmk135 self-assigned this Nov 3, 2020
@rmk135
Copy link
Member

rmk135 commented Nov 3, 2020

Hi @scott2b. Thanks, I'll take a look.

@rmk135
Copy link
Member

rmk135 commented Nov 3, 2020

@scott2b , reproduced the issue. Working on a fix.

PS: The funny thing. I had a test for @classmethod. And this test passes. That test called class method from a class, not from the instance of that class.

@rmk135 rmk135 added the bug label Nov 3, 2020
@rmk135
Copy link
Member

rmk135 commented Nov 3, 2020

@scott2b , fixed in 4.3.3. Thanks for reporting the issue. I close it for now. Please, comment or re-open if needed.

@rmk135 rmk135 closed this as completed Nov 3, 2020
@scott2b
Copy link
Author

scott2b commented Nov 4, 2020

Seems to be working. Thanks!!!

@scott2b
Copy link
Author

scott2b commented Nov 4, 2020

So, FYI. I am not sure if Python dictates that @classmethod should be the final decorator on the method? The injection wiring breaks if it is not the final decorator and is stacked on top of other decorators .... but that may be a moot issue if @classmethod is supposed to be last anyway.

This is not an issue for me, just thought you would want to be aware.

Thanks again.

@rmk135
Copy link
Member

rmk135 commented Nov 4, 2020

Yep, that’s kind of known thing that classmethod should be on the very top of decorators. What actually happens is that classmethod returns an object that is treated special way by the class. If it’s not on the top, the class recognizes it as usual method. What DI does is “undecorating” of the method to get the original, decorating the original with injecting decorator and then decorating it back as classmethod.

@rmk135
Copy link
Member

rmk135 commented Nov 4, 2020

Thank you @scott2b .

@LKay
Copy link

LKay commented Nov 9, 2020

@rmk135 Is there any workaround to make it work with decorated functions/methods? I'm trying to integrate it with yet another framework and wiring doesn't work if the method is decorated as:

@app.get('/')
async def get_root(some_service: SomeService = Provide[Application.some_service]):
  ...

The result is, service is not injected.

@rmk135
Copy link
Member

rmk135 commented Nov 9, 2020

Hey @LKay. What framework do you use?

@LKay
Copy link

LKay commented Nov 9, 2020

Im trying to use FastAPI. Tried to declare route using both decorator and app.add_api_route providing handler directly but seems injectors are ignored.

@rmk135
Copy link
Member

rmk135 commented Nov 9, 2020

Ok, got it. I didn't try FastAPI. I'll try to build a sample application with it.

@LKay
Copy link

LKay commented Nov 9, 2020

@rmk135 This is minmal, not working example:

import sys

from dependency_injector import containers, providers
from dependency_injector.wiring import Provide
from fastapi import FastAPI


class Service:
    def ok(self):
        return 'OK'

class Container(containers.DeclarativeContainer):
    service = providers.Factory(
        Service
    )

container = Container()
container.wire(modules=[sys.modules[__name__]])
app = FastAPI()

@app.get('/')
async def get_root(service = Provide[Container.service]):
    return service.ok()

@rmk135
Copy link
Member

rmk135 commented Nov 12, 2020

Hey @LKay ,

I have just released version 4.3.8 that adds a hotfix to wiring to support FastAPI. Here is a working sample:

import sys

from dependency_injector import containers, providers
from dependency_injector.wiring import Provide
from fastapi import FastAPI


class Service:
    def ok(self):
        return 'OK'


class Container(containers.DeclarativeContainer):
    service = providers.Factory(
        Service
    )


async def get_root(service = Provide[Container.service]):
    return service.ok()


container = Container()
container.wire(modules=[sys.modules[__name__]])

app = FastAPI()
app.add_api_route('/', get_root)

@LKay
Copy link

LKay commented Nov 17, 2020

@rmk135 Thanks a lot for addressing this promptly! Is there any way to make it work with decorated functions as well on top of explicit app.add_api_route()?

@LKay
Copy link

LKay commented Nov 17, 2020

@rmk135 There is also one bug with the above solution. If you explicitly specify the type of what was injected, the app will fail to boot app:

async def get_root(service: Service = Provide[Container.service]):
    return service.ok()

will result in:

fastapi.exceptions.FastAPIError: Invalid args for response field! Hint: check that <class 'test.Service'> is a valid pydantic field type

Also when Provider is used instead of Provideto make a workaround, such as:

async def get_root(service: Service = Depends(Provider[Container.service])):
    return service.ok()

results in error:

TypeError: <dependency_injector.wiring.Provider object at 0x11f796a90> is not a callable object

But according to docs https://python-dependency-injector.ets-labs.org/wiring.html#markers The Provider should inject callable provider.

@rmk135
Copy link
Member

rmk135 commented Nov 17, 2020

@rmk135 Thanks a lot for addressing this promptly! Is there any way to make it work with decorated functions as well on top of explicit app.add_api_route()?

Yes, but make sure you put @inject decorator before @app.api_route():

from dependency_injector.wiring import inject, Provide


@app.api_route('/')
@inject
async def get_root(service = Provide[Container.service]):
    return service.ok()

@rmk135
Copy link
Member

rmk135 commented Nov 17, 2020

@rmk135 There is also one bug with the above solution. If you explicitly specify the type of what was injected, the app will fail to boot app:

async def get_root(service: Service = Provide[Container.service]):
    return service.ok()

will result in:

fastapi.exceptions.FastAPIError: Invalid args for response field! Hint: check that <class 'test.Service'> is a valid pydantic field type

I don't know how to fix it. That's something that FastAPI type checking system does. I'll think about kind of trick to make it work.

@rmk135
Copy link
Member

rmk135 commented Nov 17, 2020

Also when Provider is used instead of Provideto make a workaround, such as:

async def get_root(service: Service = Depends(Provider[Container.service])):
    return service.ok()

results in error:

TypeError: <dependency_injector.wiring.Provider object at 0x11f796a90> is not a callable object

But according to docs https://python-dependency-injector.ets-labs.org/wiring.html#markers The Provider should inject callable provider.

I believe it's because you use Depends. Dependency Injector can not find the marker.

The idea of using Depends is good. I will think If I can adapt it to fix type checking issue.

@rmk135
Copy link
Member

rmk135 commented Nov 18, 2020

Hey @LKay ,

I've released a new version 4.4.1. It supports Depends. Now it looks like:

import sys

from dependency_injector import containers, providers
from dependency_injector.wiring import inject, Provide
from fastapi import FastAPI, Depends


class Service:
    def ok(self):
        return 'OK'

class Container(containers.DeclarativeContainer):
    service = providers.Factory(
        Service
    )

app = FastAPI()

@app.get('/')
@inject
async def get_root(service: Service = Depends(Provide[Container.service]):
    return service.ok()


container = Container()
container.wire(modules=[sys.modules[__name__]])

Using Depends also fixes OpenAPI docs endpoint. It didn't work with a previous version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants