Repository: hynek/svcs

Issue:
`container.get` raises TypeError when factory returns a MagicMock
Calling `Container.get` raises a TypeError when the factory is configured to return a MagicMock. This makes it very inconventient to combine svcs with MagicMock in some cases.

This is demonstrated in the following example:

```python
from unittest.mock import MagicMock
import uuid
import svcs

registry = svcs.Registry()
registry.register_factory(uuid.UUID, lambda: uuid.uuid4())
container = svcs.Container(registry=registry)

def test_example():
    registry.register_factory(uuid.UUID, lambda: MagicMock())
    container.get(uuid.UUID)
    # TypeError: Use `aget()` for async factories.
```

The registered factory for `uuid.UUID` is incorrectly interpreted as an `AbstractAsyncContextManager`, because `MagicMock` will always return `True` on `isinstance` checks.

Under normal circumstances I would be highly skeptical of explicitly checking if something is a `MagicMock` or `AsyncMock` in production code. However, given that the core premise of late-binding with `svcs` is to simplify testing, I think it may be warranted in this case.

In some cases, the error can be circumvented using `spec` on the `MagicMock`:

```
registry.register_factory(uuid.UUID, lambda: MagicMock(spec=uuid.UUID))
```

However,  this does not work when the constructor of a class defines additional properties that would be accessible during normal operations. Below is a simplified example from my own code where I wanted per-instance caching for the result of a method call:

```python
class MyClass:
    def __init__(self):
        self.expensive_operation = cachetools.func.ttl_cache(ttl=60)(self._do_something)

    def _expensive_operation(self): ...

registry.register_factory(MyClass, lambda: MagicMock(spec=MyClass))

my_class = container.get(MyClass)
my_class.expensive_operation()
# AttributeError: Mock object has no attribute 'expensive_operation'
```

The following amendment to `Container.get` in `_core.py` resolves the issue and does not break any of the existing tests:

```python
if not isinstance(svc, MagicMock) and (
    iscoroutine(svc)
    or isinstance(svc, AbstractAsyncContextManager)
):
    msg = "Use `aget()` for async factories."
    raise TypeError(msg)
```


Discussion hints:
Moortiii: Yes, this looks reasonable.

A quick, possibly related, question. Shouldn't re-registering a factory invalidate `self._instantiated` on the instance of `Container`? I was just verifying your fix again locally when I tried to do the following:

```python
from unittest.mock import MagicMock
import svcs

class Greeter:
    def greet(self, name: str):
        return f"Hello, {name}!"

def greeter_factory() -> Greeter:
    return Greeter()


registry = svcs.Registry()
container = svcs.Container(registry=registry)

registry.register_factory(Greeter, factory=greeter_factory)
greeter = container.get(Greeter)
print("Without mock:", greeter.greet("Hynek"))

mock = MagicMock()
mock.greet.return_value = "I was mocked!"
registry.register_factory(Greeter, lambda: mock, enter=False)
greeter = container.get(Greeter)
print("With mock:", greeter.greet(name="irrelevant"))
``` 

I would've expected the result to be:

```
Without mock: Hello, Hynek!
With mock: I was mocked!
```

But the result is actually:

```
Without mock: Hello, Hynek!
With mock: Hello, irrelevant!
```

This is because the call to `.get` caches the return value for `Greeter`, even though we've overwritten the factory afterwards. Is this a  bug or am I misunderstanding how `svcs` is intended to work?
Moortiii: Come to think of it, to facilitate temporary overrides in my tests, I initially wrote the following contextmanager:

```python
@contextmanager
def Override(registry: svcs.Registry, overrides: dict[type, Any]):
    old_overrides = {}

    for dependency_type, value in overrides.items():
        if dependency_type in registry._services:
            old_overrides[dependency_type] = registry.get_registered_service_for(dependency_type)

        if callable(value):
            registry.register_factory(dependency_type, value)
        else:
            registry.register_value(dependency_type, value)

    try:
        yield
    finally:
        for dependency_type, original_value in old_overrides.items():
            registry._services[dependency_type] = original_value
```

Which would be used as such:

```python
class Greeter:
    def greet(self, name):
        return f"Hello, {name}!"


def test__override():
    registry = svcs.Registry()
    container = svcs.Container(registry=registry)

    with Override(
        registry=registry,
        overrides={
            Greeter: lambda: stub(greet = lambda name: "The mock says hello!")
        }
    ):
        greeter = container.get(Greeter)
        print(greeter.greet("does-not-matter"))
```

I quickly ran into issues when I started to override the same type, i.e. `Greeter` across multiple tests with a global registry defined. In a debugger I could clearly see that I was resetting my dependencies after each test. Yet, magically, container would "revert" to the initially defined override for every subsequent test. I guess what was actually happening was that my new override was never ran, because the cached value was always returned directly.
hynek: > Ah, you're absolutely right.

Thanks Claude! ;)


Solution patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b3bba95..65a7a40 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -40,6 +40,12 @@ You can find our backwards-compatibility policy [here](https://github.com/hynek/
   [#83](https://github.com/hynek/svcs/pull/83)
 
 
+### Fixed
+
+- Factories now can return [`MagicMock`](https://docs.python.org/3/library/unittest.mock.html#unittest.mock.MagicMock)s without crashing with a TypeError.
+  [#137](https://github.com/hynek/svcs/issues/137)
+
+
 ## [25.1.0](https://github.com/hynek/svcs/compare/24.1.0...25.1.0) - 2025-01-25
 
 ### Added
diff --git a/src/svcs/_core.py b/src/svcs/_core.py
index cd83af4..955422c 100644
--- a/src/svcs/_core.py
+++ b/src/svcs/_core.py
@@ -26,6 +26,7 @@ from inspect import (
 )
 from types import TracebackType
 from typing import Any, TypeVar, overload
+from unittest.mock import MagicMock
 
 import attrs
 
@@ -1006,8 +1007,9 @@ class Container:
                 rv.append(svc)
                 continue
 
-            if iscoroutine(svc) or isinstance(
-                svc, AbstractAsyncContextManager
+            if not isinstance(svc, MagicMock) and (
+                iscoroutine(svc)
+                or isinstance(svc, AbstractAsyncContextManager)
             ):
                 msg = "Use `aget()` for async factories."
                 raise TypeError(msg)


Test patch:
diff --git a/tests/test_integration.py b/tests/test_integration.py
index cbe4a15..a9c5235 100644
--- a/tests/test_integration.py
+++ b/tests/test_integration.py
@@ -12,6 +12,7 @@ from contextlib import (
     contextmanager,
 )
 from typing import NewType
+from unittest.mock import MagicMock
 
 import pytest
 
@@ -258,6 +259,40 @@ def test_get_on_async_factory_raises_type_error(
         ) == recwarn.pop().message.args
 
 
+def test_get_with_magicmock_factory_works(registry, container):
+    """
+    A sync factory can return a MagicMock as long as they're registered with
+    enter=False.
+    """
+
+    mm = MagicMock()
+    registry.register_factory(Service, lambda: mm, enter=False)
+
+    svc = container.get(Service)
+
+    assert mm is svc
+    assert svc is container.get(Service)
+
+
+@pytest.mark.asyncio
+async def test_aget_with_asyncmock_factory_works(registry, container):
+    """
+    A async factory can return a MagicMock as long as they're registered with
+    enter=False.
+    """
+    mm = MagicMock()
+
+    async def factory():
+        return mm
+
+    registry.register_factory(Service, factory, enter=False)
+
+    svc = await container.aget(Service)
+
+    assert mm is svc
+    assert svc is container.get(Service)
+
+
 def test_local_value_overrides_global_value(registry, container):
     """
     If a container registers a local value, it takes precedence of the global