litestar-org / litestar

Production-ready, Light, Flexible and Extensible ASGI API framework | Effortlessly Build Performant APIs
https://litestar.dev/
MIT License
5.62k stars 382 forks source link

Enhancement: Remove exceptions for 404 #2738

Closed ccrvlh closed 12 months ago

ccrvlh commented 1 year ago

Summary

Currently, when a route is not found it raises a NotFoundException which is not handled when on debug mode. Arguably, this is a very common scenario, during development, and the exception doesn't bring useful information, other than just saying that the route was not found. Only having the 404 annotation on the request level should be enough. IMHO this has a great impact on DX, since it makes for very loud logs during development, when the great majority is not actionable.

My suggestion here is to not do anything with a 404, and just have the 404 at the request level, which is also similar to what Flask and FastAPI do, where the 404 is just an INFO level log at the request level - knowing the API received the request should be enough in this case.

Basic Example

from litestar import Litestar, get

@get("/")
async def index() -> str:
    return "Hello, world!"

@get("/books/{book_id:int}")
async def get_book(book_id: int) -> dict[str, int]:
    return {"book_id": book_id}

app = Litestar([index, get_book])
LITESTAR_DEBUG=1 uvicorn app:app --reload
curl localhost:8000/wrongroute

Will show a traceback.

Drawbacks and Impact

No response

Unresolved questions

No response


[!NOTE]
While we are open for sponsoring on GitHub Sponsors and OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh Litestar dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.

Fund with Polar

provinzkraut commented 1 year ago

While I understand where your coming from, I can think of a few scenarios where this would make things really hard to debug, and arguably, a debug mode should make things easier to debug.

The HTTPException is an exception, and can be raised anywhere in the application's stack. This doesn't necessarily have to happen within framework code, i.e. if a route handler is not found. It's a very common practice to return a 404 for a restful resource route. Silencing the error here would then leave you none the wiser if that route does not exist, or if the resource does not exist. Having the traceback on the other hand lets you immediately jump to the point where the exception is raised.

Now you could make the argument that, since you're also able to return a Response with a 404 status code, those two cases shouldn't be treated differently, but I disagree. Having the HTTPException is a deliberate design choice, to allow users to interrupt the regular control flow with a meaningful error response, and - at least in my opinion - should be treated differently than simply returning a response with a 404 status code.

My suggestion here is to not do anything with a 404, and just have the 404 at the request level, which is also similar to what Flask and FastAPI do, where the 404 is just an INFO level log at the request level

That's not entirely true. FastAPI (Starlette really) doesn't give you any traceback by default for http exceptions; It does not special case the 404. The log you're seeing is from (presumably) uvicorn. Try running it with --no-access-log and you'll see that you also don't get a log for a 404.

That being said, if you really want to change this, you could for example create your own logging handler for exceptions and pass that to the BaseLoggingConfig.exception_logging_handler.

ccrvlh commented 1 year ago

Understood. I can see how this could be useful for specific scenarios, but even considering that logic, is the traceback actually more helpful than just a log entry? I can see the value on having a WARN | Route "/notfound" not found vs this is returning a 404, but I'm not sure I see the value on traceback.

Thanks for the handler, I'll make some tests, but it can be a good option to silence that, at least for now.

ccrvlh commented 1 year ago

Update on this: had a very good experience with the exception_handlers. I can see the value of having the flexibility and control over the response. As this is somewhat unexpected/uncommon for people coming from other frameworks, I'd still suggest a quieter approach as default, in which there's a default set of handlers that log the exception, but don't show the traceback, and if the user wants to, he can always log with exc_info or reraise, although I don't see it as common scenario. This is also related to #2319.

provinzkraut commented 12 months ago

I'd still suggest a quieter approach as default, in which there's a default set of handlers that log the exception, but don't show the traceback, and if the user wants to, he can always log with exc_info or reraise, although I don't see it as common scenario. This is also related to #2319.

I don't think there's a lot of value in this. Making everything configurable does not result in a great experience; Sensible defaults while giving the tools necessary to circumvent them are usually a better way to handle this.

Adding a configuration specifically for how 404s are handled is unnecessarily verbose, especially since we already provide options that allow you to change this behaviour.

As for the common scenario, I already mentioned a very common scenario in which you'd want to have a traceback for your 404 in debug mode.


@litestar-org/maintainers any objections to closing this?

peterschutt commented 12 months ago

None from me.