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

Providing dependencies across more than one level of inheritance #350

Closed
shaunc opened this issue Jan 10, 2021 · 9 comments
Closed

Providing dependencies across more than one level of inheritance #350

shaunc opened this issue Jan 10, 2021 · 9 comments
Assignees
Labels

Comments

@shaunc
Copy link

shaunc commented Jan 10, 2021

I have a derived chain of containers (say, C1 -> C2 -> C3). I want to instantiate C3 with a dependency for C1, which is then used (directly and indirectly) in the derived containers.

In fact, I have found a way to do it, but it is ugly:

class C1(containers.DeclarativeContainer):
    a1 = providers.Dependency(instance_of=str)


class C2(C1):
    a1_ = C1.a1
    a1 = providers.Dependency(instance_of=str)
    a1_.override(a1)
    a2 = providers.Callable(C1.a1)


class C3(C2):
    a3 = providers.Callable(C2.a1)

c3 = C3(a1="foo")
print("c3", c3.a3())  # prints "foo"

I can't seem to get it to work without the intermediate redefinition of a1 ... is this a bug? E.g. if I don't have that and try to instantiate C3(a1="foo") I get an error:

AttributeError: 'DynamicContainer' object has no attribute 'a1'

(I tried fiddling with @containers.copy() as well, unsuccessfully).

@shaunc
Copy link
Author

shaunc commented Jan 10, 2021

Hmm... this work-around seems insufficient in more complex situations, especially in cases where there are "optional" dependencies, implemented as in #334 (comment).

I don't really have a simple case to post -- but if the above doesn't have a "good" resolution and you need more information, I can see what I can do to boil one down.

@rmk135
Copy link
Member

rmk135 commented Jan 11, 2021

Hey @shaunc , that's good example, I'll take a look.

@rmk135 rmk135 added the bug label Jan 11, 2021
@rmk135
Copy link
Member

rmk135 commented Jan 12, 2021

Hey @shaunc , #350, #351, #354, and #355 are on track and in the top of my backlog. I plan to process them in next 2 days. I'm sorry for the delay, a bit short on time cause some other things.

@shaunc
Copy link
Author

shaunc commented Jan 12, 2021

Great -- thanks for the update! And thanks for picking off the easy ones! :)

@rmk135
Copy link
Member

rmk135 commented Jan 12, 2021

Confirming that's a bug. C3 looses a1:

from dependency_injector import containers, providers


class C1(containers.DeclarativeContainer):
    a1 = providers.Dependency(instance_of=str)


class C2(C1):
    a2 = providers.Callable(C1.a1)


class C3(C2):
    a3 = providers.Callable(C2.a1)


if __name__ == '__main__':
    print('C1 providers', C1.providers.keys())
    print('C2 providers', C2.providers.keys())
    print('C3 providers', C3.providers.keys())

    # Prints:
    # C1 providers dict_keys(['a1'])
    # C2 providers dict_keys(['a2', 'a1'])
    # C3 providers dict_keys(['a3', 'a2'])  <---- NO 'a1'

    c3 = C3(a1='foo')  # Raises AttributeError: 'DynamicContainer' object has no attribute 'a1'

@rmk135
Copy link
Member

rmk135 commented Jan 12, 2021

Working on a fix.

@rmk135 rmk135 removed the question label Jan 12, 2021
@rmk135
Copy link
Member

rmk135 commented Jan 12, 2021

Released a fix in 4.8.1.

from dependency_injector import containers, providers


class C1(containers.DeclarativeContainer):
    a1 = providers.Dependency(instance_of=str)


class C2(C1):
    a2 = providers.Callable(C1.a1)


class C3(C2):
    a3 = providers.Callable(C2.a1)


if __name__ == '__main__':
    print('C1 providers', C1.providers.keys())
    print('C2 providers', C2.providers.keys())
    print('C3 providers', C3.providers.keys())

    # Prints:
    # C1 providers dict_keys(['a1'])
    # C2 providers dict_keys(['a2', 'a1'])
    # C3 providers dict_keys(['a3', 'a2', 'a1'])

    c3 = C3(a1='foo')  # No error

@rmk135
Copy link
Member

rmk135 commented Jan 12, 2021

@shaunc , thanks for finding this one. It was kind of tricky and there were no tests for 2+ levels of inheritance. Many thanks!

@rmk135
Copy link
Member

rmk135 commented Jan 12, 2021

Closing the issue. Re-open if needed.

@rmk135 rmk135 closed this as completed Jan 12, 2021
@rmk135 rmk135 removed the feature label Jan 12, 2021
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

2 participants