injector icon indicating copy to clipboard operation
injector copied to clipboard

Only inject objects with the @inject decorator and objects with a @provider

Open davidparsson opened this issue 5 years ago • 13 comments

Edit: This post originally had a question on how to solve a specific problem, but is updated with the solution and requested change described in https://github.com/alecthomas/injector/issues/116#issuecomment-647505337:

Injector should require a @provider or a constructor with the @inject decorator for an object to be created.

The original post is kept below.


As mentioned before in #78, I feel that the way Injector has default values for "value types" such as such as str, int and bool is pretty counter-intuitive, and therefore risky. Removing that would obviously break backwards compatibility, but I'd like you to consider that.

To me, the use case for Injector is primarily to inject dependent classes. It is in some cases also useful to configure these classes with parameters that may be value types, but commonly the values shouldn't be the same for every every class they configure.

Is there a design decision behind the current behavior, or is it rather a consequence of the implementation?

davidparsson avatar Jun 14 '19 11:06 davidparsson

I think it's a consequence of the implementation not caring if its dealing with user-defined classes or builtin types, but I'll let @alecthomas clarify if I'm wrong.

I wouldn't be too opposed to removing default values for those types to be honest, doesn't seem like something people should depend on too much anyway. I think NewType-based aliases of those types should work the same.

jstasiak avatar Jun 14 '19 12:06 jstasiak

Thanks! So if this were to be changed, it might be hard to decide which types that should not be created by Injector?

davidparsson avatar Jun 14 '19 12:06 davidparsson

Not necessarily, I think a blacklist condition like TYPE.__module__ == 'builtins' is a good start, it'll work for all the types you mentioned in the original post. :)

jstasiak avatar Jun 14 '19 12:06 jstasiak

True. Any third-party implementations of value types (such as numpy.float) would still slip through with that implementaion though, but I don't see how all those could be covered.

davidparsson avatar Jun 14 '19 12:06 davidparsson

What do you think of a "strict mode" where Injector require a @provider or a constructor with the @inject decorator for the object to be created?

I'm not a big fan of boolean flags that would have big impact on behaviour, and trying to create whatever may be created has its benefits, but explicit is better than implicit.

davidparsson avatar Jun 22 '20 13:06 davidparsson

That's worth considering. It'd essentially boil down to disabling instantiating types with parameterless constructors – only those can be constructed without an explicit binding, provider or @inject decorator now.

jstasiak avatar Jun 25 '20 13:06 jstasiak

That sounds very reasonable to me.

davidparsson avatar Jun 25 '20 14:06 davidparsson

I'm actually considering enabling this without a way to turn it off and dropping the auto_bind option as it'd be kind of useless after that. Do you see any major problems with that? I'll have to see how much of the test suite will break after that but I don't image it'll be a lot, it should be a manageable migration.

jstasiak avatar Jun 25 '20 18:06 jstasiak

I haven't used auto_bind=False, and I can't really tell the intricate details from the documentation, but if an @inject decorated class would be injected, along with those explicitly bound and those that have a provider, I see no functional issue. The only problem might be breaking backwards compatibility, but for my projects it would only be for the better.

davidparsson avatar Jun 25 '20 19:06 davidparsson

Right now with auto_bind=False it's not enough to have @inject – one needs an explicit Binder.bind() call or a @provider-decorated method (I think). But since one already have @inject I think it's fair to say they know what they're doing and it's ok to include @inject-decorated types in auto_bind=False mode. And then the mode is kind of redundant and auto_bind=True generates the very issue we discussed here. Anyway, I'll think about this and get back to you.

jstasiak avatar Jun 25 '20 19:06 jstasiak

I came to think of another case, but it might be slightly academic.

With the suggested change, is there a way to have a class that has dependencies (with an @inject decorator) that is included in one injector instance but forbidden in another? Perhaps one could create a provider that raises?

davidparsson avatar Jun 25 '20 20:06 davidparsson

I can only think of a provider that raises an exception right now, yes.

jstasiak avatar Jun 25 '20 23:06 jstasiak

A use-case where I think auto_bind=False would still make sense would be in tests, where it can make sense to limit which classes that may be created. I haven't used that myself though.

davidparsson avatar Nov 28 '22 10:11 davidparsson