ivankorobkov / python-inject

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

Optionally allow overriding of dependencies #90

Closed NikiTsv closed 8 months ago

NikiTsv commented 8 months ago

Relates to (https://github.com/ivankorobkov/python-inject/issues/65 discussion).

Preface When configuring injector for different environments often times you need to use a the same core configuration and swap only a few dependencies. This also happens for specific tests when you need to mock something or tweak some service's configuration. The current implementation only allows for installing an exising config with binder.install and then build up upon it without overriding dependencies. This unflexibility forces developers to always create configurations from scratch for every environment or for specific tests which leads to code duplications and when introducing new dependencies in your core configuration, you have to specify them in every other config as well.

Solution With the following changes configurations are composable with the ability to allow overriding of dependencies for example:

import inject
# base config in core module
def core_config(binder):
  binder.bind(BaseUserService, UserService)
  binder.bind(BaseCase, Cache)
  binder.bind(BaseJobRunner, JobRunner)
  binder.bind(BaseLogger, ConsoleLogger)

# base test config
def test_config(binder):
   # reuse core config
   binder.install(core_config)

   # override some dependencies
   binder.bind(BaseCase, MockCache)
   binder.bind(BaseJobRunner, MockJobRunner)

# configuration where you need to mock user service 
def specific_test_config(binder):
    # reuse base test config
    binder.install(test_config)
     # override some dependencies
    binder.bind(BaseUserService, MockUserService)

# another process that runs the same code base with another logger
def job_runner_config(binder):
   # reuse core config
    binder.install(core_config)

    # override some dependencies
    binder.bind(BaseLogger, JobRunLogger)

inject.configure(core_config)
inject.clear_and_configure(test_config, allow_override=True)
inject.clear_and_configure(specific_test_config, allow_override=True)
inject.clear_and_configure(job_runner_config, allow_override=True)
ivankorobkov commented 8 months ago

Hi!

Could you explain, why you chose to use allow_override at the configuration level, and not override as a flag in binder.bind(BaseLogger, JobRunLogger, override=True).

Do you think allow_override better suits your current needs?

NikiTsv commented 8 months ago

Hi!

Could you explain, why you chose to use allow_override at the configuration level, and not override as a flag in binder.bind(BaseLogger, JobRunLogger, override=True).

Do you think allow_override better suits your current needs?

Hey, Yes there are a few reasons I had in mind:

  1. If we went with override in Binder methods, we have to pollute all methods and pass the override arg to _check_class to avoid theDuplicate binding check.
  2. That also means that in a lot of configurations we will almost always have duplicate code like this:
    binder.bind(..., ..., override=True)
    binder.bind_to_constructor(..., ..., override=True)
    binder.bind_to_provider(..., ..., override=True)
  3. And probably the most important reason is that if you install a configuration like this in another module/environment it "doesn't know" if the base configuration has this binding OR if the base configuration later on adds this binding without you knowing and crashes your configuration. So to avoid that you will always have to fallback to 2. - always setting override=True and duplicating code so you are safe.
    def workflow_module_config(binder):
    binder.install(core_module_config)
    binder.bind(Logger, SpecialWorkflowLogger) # crashes if core_config decides to add default logger logger

And generally I think it's more clear that when configuring you just set the global behavior during configuration so you are not bothered to think about every line if you need override or not, you just register your service and it works, even if you duplicate it or it gets removed from parent config etc.

What are your thoughts do you think this approach has some drawbacks?

jell-o-fishi commented 8 months ago

recommendation to points 1 and 2 - when overriding, wrap the Binder with a proxy with the same interface which handles this case ? that way you don't have to spread override=True all over the place, and still allow the override to be as an argument to the binding methods

NikiTsv commented 8 months ago

recommendation to points 1 and 2 - when overriding, wrap the Binder with a proxy with the same interface which handles this case ? that way you don't have to spread override=True all over the place, and still allow the override to be as an argument to the binding methods

Can you elaborate? Do you mean applying changes with the current approach - "allow_override" or the approach for "override" in Binder methods? Can you simple code example?

ivankorobkov commented 8 months ago

@NikiTsv

And probably the most important reason is that if you install a configuration like this in another module/environment it "doesn't know" if the base configuration has this binding OR if the base configuration later on adds this binding without you knowing and crashes your configuration.

Ok, so the main reason is you have different composable configurations which can override some bindings in some cases.

I see, I get it.

I'll merge it today.

jell-o-fishi commented 8 months ago

Might be an over-complication, but allows for both suggested methods in the discussion.

Instead of self.allow_override (which given self._bindings should probably be self._allow_override to denote it is internal property) change bind method on Binder to be

def bind(self, cls: Binding, instance: T, override:bool=False) -> 'Binder':

and add the class

class OverrideBinder(object):
    def __init__(self, binder:Binder):
        self._binder = binder
    def bind(self, cls: Binding, instance: T, override:bool=False) -> 'Binder':
        return self._binder.bind(cls, instance ,override=True)
    ... # the other binding methods

And when wanting to override:

override_binder = OverrideBinder(binder)
override_binder.bind(Klass, instance)