Repository: mmerickel/wired

Issue:
Caching issue
Seems like a bug to me.

Set up code
```python
# registering two factories, one without context and another with a context
registry.register_factory(factory_a, DummyFactory)
registry.register_factory(factory_b, DummyFactory, context=ContextB)
```

Scenario 1, works fine: 
```python
# getting factories from a container, all as expected 
assert container.get(DummyFactory, context=context_b) is factory_b.result
assert container.get(DummyFactory) is factory_a.result
```

Scenario 2, returns `factory_a` instead of  `factory_b`: 
```python
# seccond assertion fails because of `factory_a` is returned
assert container.get(DummyFactory) is factory_a.result
assert container.get(DummyFactory, context=context_b) is factory_b.result
```

Tests for this issue can be found in PR https://github.com/mmerickel/wired/pull/11.

Discussion:

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index d82b91f..9dcd454 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -1,6 +1,21 @@
 Unreleased
 ==========
 
+Backward Incompatibilities
+--------------------------
+
+- ``wired.ServiceContainer.set`` has been redefined to set a service instance
+  for a specific context object instead of for a type-of-context. The new
+  method ``wired.ServiceContainer.register_singleton`` is a direct replacement
+  for the old behavior.
+
+Features
+--------
+
+- Add ``wired.ServiceContainer.register_factory`` and
+  ``wired.ServiceContainer.register_singleton`` which are per-container
+  analogues to their per-registry variants on ``wired.ServiceRegistry``.
+
 - Edit docs to (a) improve sales pitch, (b) split into a couple of sub-pages,
   and (c) provide a tutorial. Update README and ``setup.py`` description a
   bit as well.
diff --git a/src/wired/container.py b/src/wired/container.py
index 06893be..02f0230 100644
--- a/src/wired/container.py
+++ b/src/wired/container.py
@@ -59,31 +59,41 @@ class ServiceCache:
 
     _AdapterRegistry = AdapterRegistry  # for testing
 
-    def __init__(self):
-        self.contexts = {}
-        self.ref = weakref.ref(self)
+    def __init__(self, default=None):
+        self._default = None
+        self._contexts = {}
+        self._ref = weakref.ref(self)
 
     def __del__(self):
         # try to remove the finalizers from the contexts incase the context
         # is still alive, there's no sense in having a weakref attached to it
         # now that the cache is dead
-        for ctx_id, ctx_cache in self.contexts.items():
+        for ctx_id, ctx_cache in self._contexts.items():
             finalizer = ctx_cache.lookup(
                 (), IContextFinalizer, name='', default=_marker
             )
             if finalizer is not _marker:  # pragma: no cover
                 finalizer.detach()
 
-    def get(self, context):
+    def find(self, context=_marker):
+        if context is _marker:
+            context = self._default
         ctx_id = id(context)
-        ctx_cache = self.contexts.get(ctx_id, None)
+        return self._contexts.get(ctx_id, None)
+
+    def get(self, context=_marker):
+        if context is _marker:
+            context = self._default
+        contexts = self._contexts
+        ctx_id = id(context)
+        ctx_cache = contexts.get(ctx_id, None)
         if ctx_cache is None:
             ctx_cache = self._AdapterRegistry()
             try:
                 finalizer = weakref.finalize(
                     context,
                     context_finalizer,
-                    cache_ref=self.ref,
+                    cache_ref=self._ref,
                     ctx_id=ctx_id,
                 )
             except TypeError:
@@ -93,7 +103,7 @@ class ServiceCache:
             else:
                 finalizer.atexit = False
                 ctx_cache.register((), IContextFinalizer, '', finalizer)
-            self.contexts[ctx_id] = ctx_cache
+            contexts[ctx_id] = ctx_cache
         return ctx_cache
 
 
@@ -101,8 +111,8 @@ def context_finalizer(cache_ref, ctx_id):  # pragma: no cover
     # if the context lives longer than self then remove it
     # to avoid keeping any refs to the registry
     cache = cache_ref()
-    if cache is not None and ctx_id in cache.contexts:
-        del cache.contexts[ctx_id]
+    if cache is not None and ctx_id in cache._contexts:
+        del cache._contexts[ctx_id]
 
 
 class ServiceContainer:
@@ -121,9 +131,9 @@ class ServiceContainer:
 
     def __init__(self, factories, cache=None, context=None):
         if cache is None:
-            cache = self._ServiceCache()
-        self.factories = factories
-        self.cache = cache
+            cache = self._ServiceCache(context)
+        self._factories = factories
+        self._cache = cache
         self.context = context
 
     def bind(self, *, context):
@@ -134,48 +144,8 @@ class ServiceContainer:
         if context is self.context:
             return self
         return self.__class__(
-            factories=self.factories, cache=self.cache, context=context
-        )
-
-    def set(
-        self, service, iface_or_type=Interface, *, context=Interface, name=''
-    ):
-        """
-        Add a service instance to the container.
-
-        Upon success, ``service`` will be returned for any uncached lookups.
-
-        If this service registration would affect a previously-cached lookup
-        then it will raise a ``ValueError``.
-
-        :param service: A service instance to cache.
-        :param iface_or_type: A class or ``zope.interface.Interface`` object
-            defining the interface of the service. Defaults to
-            ``zope.interface.Interface`` to match any requested interface.
-        :param context: A class or ``zope.interface.Interface`` object
-            defining the type of :attr:`.context` required in order to use
-            the factory. Defaults to ``zope.interface.Interface`` to match
-            any context.
-        :param str name: An identifier for the service.
-
-        """
-        iface = _iface_for_type(iface_or_type)
-        context_iface = _iface_for_context(context)
-        cache = self.cache.get(None)
-
-        inst = cache.lookup(
-            (IServiceInstance, context_iface),
-            iface,
-            name=name,
-            default=_marker,
+            factories=self._factories, cache=self._cache, context=context
         )
-        if inst is not _marker:
-            raise ValueError(
-                'a service instance is already cached that would conflict '
-                'with this registration'
-            )
-
-        cache.register((IServiceInstance, context_iface), iface, name, service)
 
     def get(
         self,
@@ -190,14 +160,15 @@ class ServiceContainer:
 
         The instance is found using the following algorithm:
 
-            1. Find an instance matching the criteria in the container. If one
-               is found, return it directly.
+        1. Find an instance matching the criteria in the container. If one
+           is found, return it directly.
 
-            2. If no instance is found, search for the factory on the registry.
-               If one is not found, raise a ``LookupError`` or, if provided,
-               return ``default``.
+        2. Search for a factory, first in the container and second on the
+           service registry. If one is not found, raise a ``LookupError`` or,
+           if specified, return ``default``.
 
-            3. Instiantiate the factory, caching the result in the container.
+        3. Invoking the factory, cache the result in the container for later
+           lookups, and return the result.
 
         :param iface_or_type: The registered service interface.
         :param context: A context object. This object will be available as
@@ -215,7 +186,7 @@ class ServiceContainer:
         context = self.context
         iface = _iface_for_type(iface_or_type)
         context_iface = providedBy(context)
-        cache = self.cache.get(context)
+        cache = self._cache.get(context)
 
         inst = cache.lookup(
             (IServiceInstance, context_iface),
@@ -226,23 +197,20 @@ class ServiceContainer:
         if inst is not _marker:
             return inst
 
-        # there is no instance registered for this context, fallback to
-        # see if there is one registered for context=None before falling
-        # back to factories, this would normally be from a call to .set()
-        if context is not None:
-            inst = self.cache.get(None).lookup(
-                (IServiceInstance, context_iface),
-                iface,
-                name=name,
-                default=_marker,
+        svc_info = None
+
+        # lookup in the local registry if it exists
+        factories = self._cache.find()
+        if factories is not None:
+            svc_info = _find_factory(factories, iface, context_iface, name)
+
+        # lookup in the global registry
+        if svc_info is None:
+            svc_info = _find_factory(
+                self._factories, iface, context_iface, name
             )
-            if inst is not _marker:
-                return inst
 
-        svc_info = self.factories.lookup(
-            (IServiceFactory, context_iface), iface, name=name, default=_marker
-        )
-        if svc_info is _marker:
+        if svc_info is None:
             if default is not _marker:
                 return default
             raise LookupError('could not find registered service factory')
@@ -266,6 +234,83 @@ class ServiceContainer:
         )
         return inst
 
+    def set(
+        self, service, iface_or_type=Interface, *, context=_marker, name=''
+    ):
+        """
+        Add a service instance to the container.
+
+        Upon success, ``service`` will be returned for matching lookups on
+        the same context.
+
+        If this service registration would affect a previously-cached lookup
+        then it will raise a ``ValueError``.
+
+        :param service: A service instance to cache.
+        :param iface_or_type: A class or ``zope.interface.Interface`` object
+            defining the interface of the service. Defaults to
+            ``zope.interface.Interface`` to match any requested interface.
+        :param context: A context object. The ``service`` instance will be
+            cached for any later lookups using this context. Defaults to the
+            bound :attr:`.context` on the container.
+        :param str name: An identifier for the service.
+
+        """
+        if context is _marker:
+            context = self.context
+        iface = _iface_for_type(iface_or_type)
+        context_iface = providedBy(context)
+        cache = self._cache.get(context)
+        inst = cache.lookup(
+            (IServiceInstance, context_iface),
+            iface,
+            name=name,
+            default=_marker,
+        )
+        if inst is not _marker:
+            raise ValueError(
+                'a service instance is already cached that would conflict '
+                'with this registration'
+            )
+
+        cache.register((IServiceInstance, context_iface), iface, name, service)
+
+    def register_factory(
+        self, factory, iface_or_type=Interface, *, context=None, name=''
+    ):
+        """
+        Register a service factory.
+
+        This factory will override any lookups defined in the service registry.
+        Otherwise the semantics are identical to
+        :meth:`.ServiceRegistry.register_factory`.
+
+        """
+        iface = _iface_for_type(iface_or_type)
+        context_iface = _iface_for_context(context)
+        wants_context = context is not None
+
+        info = ServiceFactoryInfo(factory, iface, context_iface, wants_context)
+        factories = self._cache.get()
+        _register_factory(info, factories, iface, context_iface, name)
+
+    def register_singleton(
+        self, service, iface_or_type=Interface, *, context=None, name=''
+    ):
+        """
+        Register a singleton instance.
+
+        Functionally, the singleton is wrapped in a factory that always
+        returns the same instance when invoked. See
+        :meth:`.ServiceRegistry.register_factory` for information on the
+        parameters.
+
+        """
+        service_factory = SingletonServiceWrapper(service)
+        return self.register_factory(
+            service_factory, iface_or_type, context=context, name=name
+        )
+
 
 class ServiceRegistry:
     """
@@ -290,11 +335,11 @@ class ServiceRegistry:
     def __init__(self, factory_registry=None):
         if factory_registry is None:
             factory_registry = self._AdapterRegistry()
-        self.factories = factory_registry
+        self._factories = factory_registry
 
     def create_container(self, *, context=None):
         """
-        Create a new :class:`wired.ServiceContainer` linked to the registry.
+        Create a new :class:`.ServiceContainer` linked to the registry.
 
         A container will use all the registered service factories,
         independently of any other containers, in order to find and
@@ -309,7 +354,7 @@ class ServiceRegistry:
             the container is bound to the ``None`` context.
 
         """
-        return self._ServiceContainer(self.factories, context=context)
+        return self._ServiceContainer(self._factories, context=context)
 
     def register_factory(
         self, factory, iface_or_type=Interface, *, context=None, name=''
@@ -340,7 +385,7 @@ class ServiceRegistry:
                 return LoginService(dbsession)
 
         Notice in the above example that the ``login_factory`` requires
-        another service named ``db`` to be registered which triggers a
+        another service named ``dbsession`` to be registered which triggers a
         recursive lookup for that service in order to create the
         ``LoginService`` instance.
 
@@ -366,9 +411,7 @@ class ServiceRegistry:
         wants_context = context is not None
 
         info = ServiceFactoryInfo(factory, iface, context_iface, wants_context)
-        self.factories.register(
-            (IServiceFactory, context_iface), iface, name, info
-        )
+        _register_factory(info, self._factories, iface, context_iface, name)
 
     def register_singleton(
         self, service, iface_or_type=Interface, *, context=None, name=''
@@ -402,13 +445,21 @@ class ServiceRegistry:
         iface = _iface_for_type(iface_or_type)
         context_iface = _iface_for_context(context)
 
-        svc_info = self.factories.lookup(
-            (IServiceFactory, context_iface), iface, name=name, default=_marker
-        )
-        if svc_info is not _marker:
+        svc_info = _find_factory(self._factories, iface, context_iface, name)
+        if svc_info is not None:
             return svc_info.factory
 
 
+def _register_factory(info, factories, iface, context_iface, name):
+    factories.register((IServiceFactory, context_iface), iface, name, info)
+
+
+def _find_factory(factories, iface, context_iface, name):
+    return factories.lookup(
+        (IServiceFactory, context_iface), iface, name=name, default=None
+    )
+
+
 def _iface_for_type(obj):
     # if the object is an interface then we can quit early
     if IInterface.providedBy(obj):


Test patch:
diff --git a/tests/test_container.py b/tests/test_container.py
index 144ce00..73cb7f1 100644
--- a/tests/test_container.py
+++ b/tests/test_container.py
@@ -19,6 +19,10 @@ class ContextA:
     pass
 
 
+class ContextB:
+    pass
+
+
 class IContextInterface(Interface):
     pass
 
@@ -105,8 +109,8 @@ def test_bind_context(registry):
     ctx = object()
     c1 = registry.create_container()
     c2 = c1.bind(context=ctx)
-    assert c1.cache is c2.cache
-    assert c1.factories is c2.factories
+    assert c1._cache is c2._cache
+    assert c1._factories is c2._factories
     c3 = c2.bind(context=ctx)
     assert c2 is c3
 
@@ -167,9 +171,11 @@ def test_override_cache_via_set(registry):
     db_factory = DummyFactory()
     registry.register_factory(db_factory, name='db')
     c = registry.create_container()
-    c.set(db, context=ContextA, name='db')
+    ctx_a = ContextA()
+    c.set(db, name='db', context=ctx_a)
     assert c.get(name='db') is db_factory.result
-    assert c.get(name='db', context=ContextA()) is db
+    assert c.get(name='db', context=ctx_a) is db
+    assert c.get(name='db', context=ContextA()) is db_factory.result
 
 
 def test_override_cache_via_set_fails(registry):
@@ -179,7 +185,18 @@ def test_override_cache_via_set_fails(registry):
     c = registry.create_container()
     assert c.get(name='db') is db_factory.result
     with pytest.raises(ValueError):
-        c.set(db, context=ContextA, name='db')
+        c.set(db, name='db')
+
+
+def test_container_override_factory_per_container(registry):
+    db_factory1 = DummyFactory()
+    registry.register_factory(db_factory1, name='db')
+    c = registry.create_container()
+    db_override = DummyService()
+    c.register_singleton(db_override, context=ContextA, name='db')
+    assert c.get(name='db') is db_factory1.result
+    assert c.get(name='db', context=ContextA()) is db_override
+    assert c.get(name='db', context=ContextB()) is db_factory1.result
 
 
 def test_find_factory(registry):
@@ -217,3 +234,26 @@ def test_unique_class_objects_with_same_name_dont_conflict(registry):
     ClassB = make_class()
     registry.register_singleton(ClassA(), ClassA)
     assert registry.find_factory(ClassB) is None
+
+
+# https://github.com/mmerickel/wired/issues/12
+def test_cache_context(registry):
+    class DummyFactoryA(DummyFactory):
+        pass
+
+    class DummyFactoryB(DummyFactory):
+        pass
+
+    context_a = ContextA()
+    context_b = ContextB()
+    factory_a = DummyFactoryA()
+    factory_b = DummyFactoryB()
+
+    registry.register_factory(factory_a, DummyFactory)
+    registry.register_factory(factory_b, DummyFactory, context=ContextB)
+
+    container = registry.create_container()
+    # order matters here
+    assert container.get(DummyFactory) is factory_a.result
+    assert container.get(DummyFactory, context=context_a) is factory_a.result
+    assert container.get(DummyFactory, context=context_b) is factory_b.result