Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: Better error message when a reserved keyword argument is incorrectly used. #2765

Closed
AgarwalPragy opened this issue Nov 26, 2023 · 4 comments
Labels
Enhancement This is a new feature or request

Comments

@AgarwalPragy
Copy link
Contributor

AgarwalPragy commented Nov 26, 2023

Summary

Litestar defines the following "reserved" keyword arguments for route handlers
https://docs.litestar.dev/2/usage/routing/handlers.html#reserved-keyword-arguments

  • cookies: injects the request cookies as a parsed dictionary.
  • headers: injects the request headers as an instance of Headers , which is a case-insensitive mapping.
  • query : injects the request query_params as a parsed dictionary.
  • request: injects the Request instance. Available only for http route handlers
  • scope : injects the ASGI scope dictionary.
  • socket: injects the WebSocket instance. Available only for websocket route handlers
  • state : injects a copy of the application State.
  • body : the raw request body. Available only for http route handlers

It is possible for someone to use these names for their route parameters, without realizing that these are reserved names.
In this case, the error messages can be cryptic.

Basic Example

from litestar import Litestar, get

@get(path='/')
async def hello(state: int = 0) -> str:
    return f'state: {state}'

app = Litestar([hello])

This gives the following error

File ".venv/lib/python3.11/site-packages/litestar/typing.py", line 599, in from_parameter
    raise ImproperlyConfiguredException(
litestar.exceptions.http_exceptions.ImproperlyConfiguredException: 500: The type annotation `<class 'int'>` is an invalid type for the 'state' reserved kwarg. It must be typed to a subclass of `litestar.datastructures.ImmutableState` or `litestar.datastructures.State`.

The current error message does mention that state is a reserved kwarg, but that gets lost in the rest of the message.

IMO, the message should read something like

The parameter name 'state' is reserved and cannot be used for custom route parameters. Please choose a different name for your parameter.
https://docs.litestar.dev/2/usage/routing/handlers.html#reserved-keyword-arguments
The type annotation `<class 'int'>` is an invalid type for the 'state' reserved kwarg. It must be typed to a subclass of `litestar.datastructures.ImmutableState` or `litestar.datastructures.State

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
@AgarwalPragy AgarwalPragy added the Enhancement This is a new feature or request label Nov 26, 2023
@provinzkraut
Copy link
Member

provinzkraut commented Nov 26, 2023

The issue with your proposal is that we don't have enough information to display such an error message. The message you proposed assumes some intent, namely that what the user wanted was to create a query parameter.
This however doesn't necessarily have to be the case. It could also be that the user wanted to inject State, but used a wrong type annotation.

Because we can't know what the intent was, the error message has to be generic. An error message saying you're doing something wrong that you're not even trying to do is even more confusing than an error message that's a bit generic.

@provinzkraut provinzkraut closed this as not planned Won't fix, can't repro, duplicate, stale Nov 26, 2023
@AgarwalPragy
Copy link
Contributor Author

@provinzkraut fair.

Can we still include a link to the relevant docs, please? This will point the user in the right direction, irrespective of their intent

@provinzkraut
Copy link
Member

Can we still include a link to the relevant docs, please? This will point the user in the right direction, irrespective of their intent

I'm not entirely against this, but since we're currently not doing this anywhere else, I think we should have a general discussion about this first.

Adding those links will also add some maintenance burden, as we'll have to ensure they're all valid (some pages might move), update with the version, and stay semantically correct as the docs change. Nothing is more frustrating than a dead of nonsensical link.

I also think that if we were to do this, we shouldn't do it just in this case, but more broadly. We could for example opt to employ a system like SQLAlchemy has, where all of these exception classes have their own error code, for which an explanation in the docs exists, linking to further reading material.

@provinzkraut
Copy link
Member

@AgarwalPragy I have created #2768 to further discuss this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This is a new feature or request
Projects
None yet
Development

No branches or pull requests

2 participants