Support optional parameters

[AvlWx2014]

Overview

Support bindings for optional parameters.

Background

In application code some pieces of configuration can be considered optional. For example, consider an API server that supports authentication/authorization in staging and production, but can have this feature "turned off" for development by simply omitting the auth section of the server configuration.

injector already supports providers for optional types e.g.:

AuthConfig: TypeAlias = OAuth2Config | OidcConfig


@dataclass
class AppConfig:
    auth: AuthConfig | None = None


class ConfigurationModule(Module):
    @singleton
    @provider
    def provide_app_config(self) -> AppConfig:
        return AppConfig.load()

    @singleton
    @provider
    def provide_auth_config_or_none(self, app_config: AppConfig) -> AuthConfig | None:
        return app_config.auth

In this example, I can easily set up an injector and get AuthConfig | None from the graph:

World: Final[Injector] = Injector(ConfigurationModule())

if __name__ == "__main__":
    appconfig = World.get(AppConfig)
    print(f"AppConfig resolved: {appconfig!s}"
    
    authconfig = World.get(AuthConfig | None)
    print(f"AuthConfig | None resolved: {authconfig!s}")

When this is run it prints:

AppConfig resolved: ...   # real values hidden for brevity and security
AuthConfig | None resolved: None

However, if I then write a provider function which requests AuthConfig | None as a dependency like:

class MiddlewareModule(Module):
    @provider
    @singleton
    def provide_authn_middleware(
        self, config: AuthConfig | None, injector: Injector
    ) -> AuthenticationMiddleware[Secret[str] | None]:
        middleware: AuthenticationMiddleware[Secret[str] | None]
        match config:
            case OAuth2Config() | OidcConfig():
                middleware = injector.get(BearerTokenMiddleware)
            case None:
                middleware = NoopAuthenticationMiddleware()
            case _:
                assert_never(config)
        return middleware

The injector raises indicating there is no provider for Union[OAuth2Config, OidcConfig] (type names shortened for brevity). Given the AuthConfig: TypeAlias = OAuth2Config | OidcConfig alias from earlier this message implies something somewhere is requesting plain AuthConfig as a dependency rather than AuthConfig | None, which has a provider in the graph.

However, after combing my whole dependency graph I could not find a place where this was actually happening in my application code so I started digging further. After a lot of debugging I found that the function _infer_injected_bindings removes NoneType from unions before resolving the rest of the injectable dependencies from a function signature:

# We don't treat Optional parameters in any special way at the moment.
union_members = v.__args__
new_members = tuple(set(union_members) - {type(None)})

After removing the set difference operation from the assignment to new_members the code examples I share above start working instead of raising an exception about missing providers.

Solution

The solution proposed here removes - {type(None)} from new_members = ... to allow optional dependencies to be fulfilled if there is a provider configured for them.

I also included a test to demonstrate the behavior in test_get_bindings. In the first commit the new test fails, but in the second commit with the proposed change applied it passes.

This patch induces a change in the test for test_get_bindings_for_pep_604 since now a: int | None resolves to int | None instead of just int.

The rest of the test suite passes with this patch in place, so from what I can tell there aren't any unintended side effects on the rest of the codebase, but let me know if I'm missing something. Looking forward to getting your thoughts on this!

[AvlWx2014]

@davidparsson @jstasiak wanted to get your thoughts on this. Feel free to tag anyone else for discussion that I may have missed!

[davidparsson]

@AvlWx2014, thanks for your contribution. Your use case is reasonable, but I will have to consider the implications of this. I am pretty sure this will break things for me, and probably for other people as well, since str | None previously was resolved to str.

I think Injector's handling of optional arguments is not ideal either, and while it's not the same issue it might be related.

The obvious workaround without breaking typing with the current version would in your example be to only provide AppConfig, but I understand that this is not what you want.

[AvlWx2014]

Hi @davidparsson sorry for the delayed reply, I was on vacation 😄

I am pretty sure this will break things for me, and probably for other people as well, since str | None previously was resolved to str.

I see what you mean here. My thinking originally was that this change would remain backwards compatible since str | None is a wider type than just str. Theoretically any existing dependency graphs relying on str | None resolving to just str would not contain any requests for optional types since this wouldn't work in the past. It's based on this I think that such graphs would still work without breakage.

I imagine you've seen plenty more use cases than I have which might prove my assumptions wrong, so let me know what you think!

The obvious workaround without breaking typing with the current version would in your example be to only provide AppConfig, but I understand that this is not what you want.

I definitely could, yes, and there's also the option to use the null object pattern here. Something like:

@dataclass
class NullConfig:
    pass

    def __bool__(self) -> bool:
        return False


AuthConfig: TypeAlias = OAuth2Config | OidcConfig | NullConfig


@dataclass
class AppConfig:
    auth: AuthConfig = NullConfig()

but I think supporting requests for optional types is still useful.

[davidparsson]

It seems I'm not as fast myself either. 🐢

My thinking originally was that this change would remain backwards compatible since str | None is a wider type than just str.

I was hoping for that as well, and since I found no real evidence that this was working, I created a test on this branch myself and found that it failed:

def test_optional_arguments_can_be_resolved_by_non_optional_bindings():
    injector = Injector()
    injector.binder.bind(str, to='provided')

    @inject
    class A:
        def __init__(self, value: str | None):
            self.value = value

    a = injector.get(A)  # injector.UnknownProvider: couldn't determine provider for typing.Optional[str] to None
    assert a.value == 'provided'

It actually seems that only the last one of these four tests I created pass, and I would ideally like all of them to pass, althogh the one with injector.binder.bind(str | None, to='provided') is a bit theoretical and might be excluded:

def test_optional_arguments_can_be_resolved_by_a_non_optional_binding():
    injector = Injector()
    injector.binder.bind(str, to='provided')

    @inject
    class A:
        def __init__(self, value: str | None):
            self.value = value

    a = injector.get(A)
    assert a.value == 'provided'


def test_optional_arguments_can_be_resolved_by_an_optional_binding():
    injector = Injector()
    injector.binder.bind(str | None, to='provided')

    @inject
    class A:
        def __init__(self, value: str | None):
            self.value = value

    a = injector.get(A)
    assert a.value == 'provided'


def test_optional_arguments_can_be_resolved_by_a_non_optional_provider():
    class MyModule(Module):
        @provider
        def provide_str(self) -> str:
            return 'provided'

    injector = Injector([MyModule])

    @inject
    class A:
        def __init__(self, value: str | None):
            self.value = value

    a = injector.get(A)
    assert a.value == 'provided'


def test_optional_arguments_can_be_resolved_by_an_optional_provider():
    class MyModule(Module):
        @provider
        def provide_str(self) -> str | None:
            return 'provided'

    injector = Injector([MyModule])

    @inject
    class A:
        def __init__(self, value: str | None):
            self.value = value

    a = injector.get(A)
    assert a.value == 'provided'

I also need to refresh my memory on how value: str | None relates to value: Optional[str] in the supported Python versions, but quick tests indicate that they are handled indentically internally.

[davidparsson]

It seems parametrizing the types works well, and would show all combinations:

@pytest.mark.parametrize(
    'provided_type',
    [str, str | None, Optional[str]],
    ids=('provided=str', 'provided=str|None', 'provided=Optional[str]'),
)
@pytest.mark.parametrize(
    'argument_type',
    [str, str | None, Optional[str]],
    ids=('argument=str', 'argument=str|None', 'argument=Optional[str]'),
)
def test_optional_arguments_can_be_resolved_by_optional_and_non_optional_bindings(
    provided_type, argument_type
):
    injector = Injector()
    injector.binder.bind(provided_type, to='provided')

    @inject
    class A:
        def __init__(self, value: argument_type):
            self.value = value

    a = injector.get(A)
    assert a.value == 'provided'


@pytest.mark.parametrize(
    'provided_type',
    [str, str | None, Optional[str]],
    ids=('provided=str', 'provided=str|None', 'provided=Optional[str]'),
)
@pytest.mark.parametrize(
    'argument_type',
    [str, str | None, Optional[str]],
    ids=('argument=str', 'argument=str|None', 'argument=Optional[str]'),
)
def test_optional_arguments_can_be_resolved_optional_and_non_optional_providers(provided_type, argument_type):
    class MyModule(Module):
        @provider
        def provide_str(self) -> provided_type:
            return 'provided'

    injector = Injector([MyModule])

    @inject
    class A:
        def __init__(self, value: argument_type):
            self.value = value

    a = injector.get(A)
    assert a.value == 'provided'

[davidparsson]

...but regarding the tests with parametrization, I'm not sure it's a great idea to let str be provided/resolved by str | None.

[AvlWx2014]

It seems parametrizing the types works well, and would show all combinations:

Thanks for sharing this test case I think it's useful to add in to this PR to demonstrate what we're discussing here so I'll do that and push a new patch set.

...but regarding the tests with parametrization, I'm not sure it's a great idea to let str be provided/resolved by str | None.

When you say it like this my immediate reaction is "I agree", which means that my original thinking that this would remain backwards compatible is flawed.

I still think support for optional parameters is useful, though, so we could label it a breaking change on the basis that Something | None resolving to Something up until now was an implementation detail. I'm open to input here!