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

Add optional uniqueness checks for Binder #235

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions injector/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import types
from abc import ABCMeta, abstractmethod
from collections import namedtuple
from collections import UserDict
from typing import (
Any,
Callable,
Expand Down Expand Up @@ -240,6 +241,18 @@ class UnknownProvider(Error):
"""Tried to bind to a type whose provider couldn't be determined."""


class NonUniqueBinding(Error):
"""Tried to bind to a type that already has a binding when disallowed."""

def __init__(self, interface: type) -> None:
super().__init__(interface)
self.interface = interface

def __str__(self) -> str:
exists = "Binding for '%s' already exists" % _describe(self.interface)
return "%s for Binder/Injector with unique=True" % exists


class UnknownArgument(Error):
"""Tried to mark an unknown argument as noninjectable."""

Expand Down Expand Up @@ -389,6 +402,15 @@ class ImplicitBinding(Binding):
_InstallableModuleType = Union[Callable[['Binder'], None], 'Module', Type['Module']]


class UniqueBindings(UserDict):
"""A dictionary that raises an exception when trying to add duplicate bindings."""

def __setitem__(self, key: type, value: Binding) -> None:
if key in self.data:
raise NonUniqueBinding(key)
super().__setitem__(key, value)


class Binder:
"""Bind interfaces to implementations.

Expand All @@ -400,18 +422,31 @@ class Binder:

@private
def __init__(
self, injector: 'Injector', auto_bind: bool = True, parent: Optional['Binder'] = None
self,
injector: 'Injector',
auto_bind: bool = True,
parent: Optional['Binder'] = None,
unique: bool = False,
) -> None:
"""Create a new Binder.

:param injector: Injector we are binding for.
:param auto_bind: Whether to automatically bind missing types.
:param parent: Parent binder.
:parm unique: Whether to allow multiple bindings for the same type.
"""
self.injector = injector
self._auto_bind = auto_bind
self._bindings = {}
self.parent = parent
self._unique = unique
if self._unique:
self._bindings = cast(Dict[type, Binding], UniqueBindings())
else:
self._bindings = {}

@property
def unique(self) -> bool:
return self._unique

def bind(
self,
Expand Down Expand Up @@ -881,6 +916,7 @@ class Injector:

:param auto_bind: Whether to automatically bind missing types.
:param parent: Parent injector.
:unique: Whether to allow multiple bindings for the same type.

.. versionadded:: 0.7.5
``use_annotations`` parameter
Expand All @@ -897,6 +933,7 @@ def __init__(
modules: Union[_InstallableModuleType, Iterable[_InstallableModuleType], None] = None,
auto_bind: bool = True,
parent: Optional['Injector'] = None,
unique: bool = False,
) -> None:
# Stack of keys currently being injected. Used to detect circular
# dependencies.
Expand All @@ -905,7 +942,12 @@ def __init__(
self.parent = parent

# Binder
self.binder = Binder(self, auto_bind=auto_bind, parent=parent.binder if parent is not None else None)
self.binder = Binder(
self,
auto_bind=auto_bind,
parent=parent.binder if parent is not None else None,
unique=unique,
)

if not modules:
modules = []
Expand Down Expand Up @@ -978,6 +1020,8 @@ def run(self):

def create_child_injector(self, *args: Any, **kwargs: Any) -> 'Injector':
kwargs['parent'] = self
if 'unique' not in kwargs:
kwargs['unique'] = self.binder.unique
return Injector(*args, **kwargs)

def create_object(self, cls: Type[T], additional_kwargs: Any = None) -> T:
Expand Down
44 changes: 41 additions & 3 deletions injector_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
Inject,
Injector,
NoInject,
NonUniqueBinding,
Scope,
InstanceProvider,
ClassProvider,
Expand Down Expand Up @@ -62,12 +63,12 @@ def __init__(self, b: EmptyClass):
self.b = b


def prepare_nested_injectors():
def prepare_nested_injectors(unique=False):
def configure(binder):
binder.bind(str, to='asd')

parent = Injector(configure)
child = parent.create_child_injector()
parent = Injector(configure, unique=unique)
child = parent.create_child_injector(unique=unique)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be even more interesting to add the unique flag to the parent injector as well, to see that they interact as expected, since it's an even stricter case? I think that it should be allowed and work with the current implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I agree. Also: I'm wondering if child injectors shouldn't inherit the unique value of the parent injector by default.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That crossed my mind as well, and yes, I think that would be reasonable.

return parent, child


Expand Down Expand Up @@ -1583,6 +1584,43 @@ def test_binder_has_implicit_binding_for_implicitly_bound_type():
assert not injector.binder.has_explicit_binding_for(int)


def test_injector_with_uniqueness_checking_raises_error():
def configure(binder):
binder.bind(int, to=123)
binder.bind(int, to=456)

with pytest.raises(NonUniqueBinding, match="Binding for 'int' already exists"):
_ = Injector([configure], unique=True)
Comment on lines +1587 to +1593
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want a test for the opposite case, where overriding works without unique=True?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cswartzvi bump?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@craig8 apologies, I completely forgot about this PR. I will try and push the requested changes this weekend.



def test_child_injector_with_uniqueness_checking_overrides_parent_bindings():
parent, child = prepare_nested_injectors(unique=True)
child.binder.bind(str, to='qwe')

assert (parent.get(str), child.get(str)) == ('asd', 'qwe')


def test_child_injector_with_uniqueness_checking_raises_error():
_, child = prepare_nested_injectors(unique=True)
child.binder.bind(str, to='qwe')

with pytest.raises(NonUniqueBinding, match="Binding for 'str' already exists"):
child.binder.bind(str, to='zxc')


def test_child_injector_inherits_parent_uniqueness_checking():
def configure(binder):
binder.bind(str, to='asd')

parent = Injector(configure, unique=True)
child = parent.create_child_injector() # no unique=True here

child.binder.bind(str, to='qwe')

with pytest.raises(NonUniqueBinding, match="Binding for 'str' already exists"):
child.binder.bind(str, to='qwe')


def test_get_bindings():
def function1(a: int) -> None:
pass
Expand Down
Loading