Enhancement: Validate dependency types when registering dependencies and handlers

[ccrvlh]

Description

When injecting a dependency, if the type annotation on the handler is different than the one on the callable, the handler fails silently and just returns a 500. The reason I'm posting this as a bug, is because this is silent, so it can be very hard to trace the issue.

URL to code causing the issue

No response

MCVE

This works fine, note that get_host returns a str and host_context is also annotated with a string.

from litestar import get
from litestar import Controller
from litestar.di import Provide
from litestar import Response
from litestar import Request

def get_host(request: Request) -> str:
    return request.headers.dict().get("host", ["empty list"])[0]

class MyController(Controller):
    path = "/controller"

    dependencies = {"host_context": Provide(get_host)}

    @get("/{param:int}")
    def handler_get_id(self, param: int, host_context: str) -> Response:
        print(host_context)
        return Response(dict(param=param))

If the get_host is annotated with a str and the handler is taking a dict, the route fails silently, and the handler just returns a 500.

from litestar import get
from litestar import Controller
from litestar.di import Provide
from litestar import Response
from litestar import Request

def get_host(request: Request) -> str:
    return request.headers.dict().get("host", ["empty list"])[0]

class MyController(Controller):
    path = "/controller"

    dependencies = {"host_context": Provide(get_host)}

    @get("/{param:int}")
    def handler_get_id(self, param: int, host_context: dict) -> Response:
        print(host_context)
        return Response(dict(param=param))

The handler failing is not an issue per se, consider this is built with type safety in mind, but the lack of warning, error message or something that says what's happening, seems like a bug.

Litestar Version

2.0.1

Platform

• [ ] Linux
• [X] Mac
• [ ] Windows
• [ ] Other (Please specify in the description above)

---

Funding

• If you would like to see an issue prioritized, make a pledge towards it!
• We receive the pledge once the issue is completed & verified

[guacs]

If you run it in debug mode, you'll be able to see the stack trace which has this as the initial error: msgspec.ValidationError: Expectedobject, gotstr- at$.host_context``.

Would this not allow you to get an idea on where to look to fix the issue?

[ccrvlh]

It would, but it would require to run debug mode to have the warning, in a route that will necessarily fail at runtime, for a valid Python construct.

A similar case is where you annotate a route that doesn't return anything with Response, it fails instantly (even without debug mode), or annotate it with Response[dict] and then go on to return a str. This is specific framework behavior (strict about type annotations), so it seems fair that it fails instantly, since MyPy may not warn about it.

[provinzkraut]

I don't think this is a bug, it's more of a feature request, and not a small one if I understand correctly.

Consider this

from litestar import get
from typing import Any

def get_thing():
    return "hello"

@get("/", dependencies={"thing": get_thing})
def handler_get_id(thing: int) -> None:
    pass

this would result in a runtime error, and there's no way for us to check this.

Essentially what you're asking for is strict type checking of dependencies. It might be possible to create a mypy plugin that does this, but I'm not sure if that's viable.

@litestar-org/maintainers?

[guacs]

Imo, the mypy plugin is something that can be considered, but I don't think this type checking of the dependencies should be part of the runtime. Those discrepancies should be caught during testing/static type checking (though only testing is the only thing that would catch it for now).

[provinzkraut]

One thing I agree though is that this should not fail with a 500 but a 400 status code, as a regular validation error. Maybe we can spin off that part into its own issue?

[guacs]

One thing I agree though is that this should not fail with a 500 but a 400 status code, as a regular validation error. Maybe we can spin off that part into its own issue?

Should it though? It's not a bad request from the client but rather it actually is an internal server error.

[provinzkraut]

It's not a bad request from the client but rather it actually is an internal server error.

I was suggesting it should probably be a 400, because it's a validation error, and we usually handle those with a 4xx. This particular validation error resulting in a 500 is - if I understand correctly - also what was unexpected for @ccrvlh.

You are right though that this is an internal server error and not caused by a bad request, so it should be a 500.

[ccrvlh]

IMHO 500 is the ideal status code, since it is indeed an internal server error.

I'm thinking that the easiest/shortest way of handling this would be to just make it loud (error log), and the ideal solution would be to validate the types (if they're available) when the app is loading, and the handlers/dependencies are being registered (just as the handlers/response type annotations are validated).

tbh I think the mypy plugin is a bit of a stretch - another dependency for something very specific.

[ccrvlh]

Also, not directly related to the dependencies, but I found that if you don't defined the status code on a Redirect route, it will also behave the same way: returns 500 without any logs (unless you're running debug mode).

[peterschutt]

it's more of a feature request

Yes, I think so.

We do have all the information we need in order to be able to check the type of the parameter annotation against the return type of the provider during startup. However, I wouldn't call it a bug that we aren't doing this yet.

A similar case is where you annotate a route that doesn't return anything with Response, it fails instantly (even without debug mode),

Do you mean something like this?

"""Minimal Litestar application."""
from __future__ import annotations

from asyncio import sleep

from litestar import Litestar, Response, get

__all__ = ("async_hello_world",)

@get("/async")
async def async_hello_world() -> Response:
    """Route Handler that outputs hello world."""
    await sleep(0.1)
    return None

app = Litestar(route_handlers=[async_hello_world])

That fails at runtime, but the app does start up.

or annotate it with Response[dict] and then go on to return a str. This is specific framework behavior (strict about type annotations), so it seems fair that it fails instantly, since MyPy may not warn about it.

Like, this?

"""Minimal Litestar application."""
from __future__ import annotations

from asyncio import sleep

from litestar import Litestar, Response, get

__all__ = ("async_hello_world",)

@get("/async")
async def async_hello_world() -> Response[dict]:
    """Route Handler that outputs hello world."""
    await sleep(0.1)
    return Response(content="Hello world")

app = Litestar(route_handlers=[async_hello_world])

Mypy does warn about these:

app.py:15: error: Argument "content" to "Response" has incompatible type "str"; expected "dict[Any, Any]" [arg-type] Found 1 error in 1 file (checked 3 source files)

[ccrvlh]

However, I wouldn't call it a bug that we aren't doing this yet.

Agreed, the reason I thought a bug was more appropriate was only because this was completely silent, so hard to track in production. I would expect an ERROR level logging.

About the first example: yes, exactly. I'm not in front on the computer rn, but IIRC It fails with an error you can track, even when not on DEBUG mode.

About the second example: yes mypy warns about this case, but not about the dependencies. The logic is basically the same: type annotations being enforced, and not being "just" annotations/hints, so even valid and working code will fail because of annotation errors.

I honestly really like this behavior, really wish this was native to Python. I just think internal, specific behavior (specially when it differs from what would otherwise be valid and working language constructs) should fail with ERROR logs.

[peterschutt]

but IIRC It fails with an error you can track, even when not on DEBUG mode.

I just see: INFO: 127.0.0.1:58318 - "GET /async HTTP/1.1" 500 Internal Server Error - but maybe we have some differences in our config (i'm using litestar-hello-world repo).

About the second example: yes mypy warns about this case, but not about the dependencies.

Yes, but the dependency behavior is the original theme of this issue, and this was something else that you raised in addition (I'm just trying to make sure I'm not missing a nuance you are trying to get across).

should fail with ERROR logs

This is a central theme here (correct me if I'm wrong), that if there was a traceback logged when a 500 is thrown outside of debug mode, all of the issues raised here are less severe. Correct?

[guacs]

Also, not directly related to the dependencies, but I found that if you don't defined the status code on a Redirect route, it will also behave the same way: returns 500 without any logs (unless you're running debug mode).

Could you create another issue with an MCVE for this?

[ccrvlh]

@peterschutt you're right, they do fail silently. The app doesn't run if you don't annotate the response.

Yes, but the dependency behavior is the original theme of this issue, and this was something else that you raised in addition (I'm just trying to make sure I'm not missing a nuance you are trying to get across).

I mentioned the other aspects of it, since it feels this is all part of the same issue. After making some tests and reading this thread, I feel there are two different issues that can be :

1. Add WARN and ERROR level logs to the application even when not in debug mode
2. Perform type annotation validation for dependencies

If you guys don't mind, I would suggest to keep this issue as a feature request for (2): type validation for dependencies, and open a new issue for (1) the logging behavior. What do you think @litestar-org/maintainers?

Could you create another issue with an MCVE for this?

@guacs, will do, I wanted to suggest a sane default (eg. 307) for this use case, but we can talk about this is a new issue.

[peterschutt]

If you guys don't mind, I would suggest to keep this issue as a feature request for (2)

Sounds good to me - is this something you were hoping to work on @ccrvlh?

[ccrvlh]

@peterschutt I'd be happy to. I'm very new to the codebase, so I'll spend some time trying to figure out how things work and see what I can come up with.

[peterschutt]

@peterschutt I'd be happy to. I'm very new to the codebase, so I'll spend some time trying to figure out how things work and see what I can come up with.

Great! My first thought would be to look around at _kwargs.kwargs_model.KwargsModel.create_for_signature_model() - that method receives a parsed signature, which has the type annotations of the handler, and the dependency names mapped to their Provide instance.

[peterschutt]

See discussion in #2349