Enhancement: require explict default for optional parameters

[peterschutt]

This behavior is a bit undesirable IMO @litestar-org/maintainers - if a query param is <smth> | None, and it automatically defaults to None if not provided, that could cause problems. E.g.,:

@get("/top-secret-government-cloud-using-litestar-route")
def armageddon_dead_man_switch(do_not_launch_nukes: Literal["thanks"] | None) -> None:
    if not do_not_launch_nukes:
        big_red_button.press()

# intern forgets to add query parameter to the query.

Stupid example, but YKWIM..

_Originally posted by @peterschutt in https://github.com/litestar-org/litestar/pull/2515#discussion_r1370960670_

---

[!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.

[guacs]

I agree. The docs state the following:

Query parameters come in three basic types:

• Required
• Required with a default value
• Optional with a default value

Query parameters are required by default. If one such a parameter has no value, a ValidationException will be raised.

From this, I think the automatic defaulting to None can even be classified as a bug since it goes against the documented behavior.

[peterschutt]

I agree. The docs state the following:

Query parameters come in three basic types:

• Required
• Required with a default value
• Optional with a default value

Query parameters are required by default. If one such a parameter has no value, a ValidationException will be raised.

From this, I think the automatic defaulting to None can even be classified as a bug since it goes against the documented behavior.

Oh yeah, well at least then we should update the docs to reflect current behavior.

I have a feeling a change like this might need to be an opt-in feature in v2.

[gsakkis]

+1 on the suggested change and I agree that this is a bug according to the docs but practically speaking, there is probably code relying on the current behavior that will break after the fix.

I have a feeling a change like this might need to be an opt-in feature in v2.

Given that this is arguably a bug, how about adding a warning in v2.3 and dropping it in v2.4?

[gsakkis]

I agree. The docs state the following:

Query parameters come in three basic types:

• Required
• Required with a default value
• Optional with a default value

While at it, the whole query parameters docs would benefit from some rewriting:

• What does "Required with a default value" even mean? It doesn't make sense, a parameter is either (a) required without default or (b) optional with default. The only plausible third alternative would be the one addressed in this issue - optional without default - but that's only because there is an implicit default (None) so it also falls under (b).
• The next two subsections, default values and optional parameters should be merged as they both fall under (b).
• From the optional parameters subsection:

  Here, we give a default value of None , but still declare the type of the query parameter to be a string.

  1. The code disagrees with the docs, the type is optional string: param: str | None = None
  2. The type cannot be a a string if the default value is None: param: str = None raises a ValidationError if param is not passed (i.e. it is equivalent to a required param: str).

[peterschutt]

Given that this is arguably a bug, how about adding a warning in v2.3 and dropping it in v2.4?

Probably too aggressive.

What does "Required with a default value" even mean?

Ha, yeh - no idea.. :see_no_evil:

While at it, the whole query parameters docs would benefit from some rewriting:

Absolutely.

[guacs]

I agree with @gsakkis. I think this could be changed without it being considered breaking. My reasoning is that the current behaviour is directly against the documented behavior (even though it's a bit confusing what the documented behavior is). Also, the current behaviour is unintuitive imo.

[peterschutt]

I think this could be changed without it being considered breaking.

I don't agree with this at all. It is the behavior that counts, not what is written in the docs. Assuming this is a byproduct of the way that pydantic would implicitly default optional fields to None (not sure if they still do in v2), then this behavior has been a part of the framework since pre-release starlite days.

Also, the current behaviour is unintuitive imo.

Strongly agree.

[JacobCoffee]

fwiw im pretty sure we would have changes required if this were mainlined into 2.x

[peterschutt]

What if we were to introduce future_features flag for the app that would behave like experimental_features but allow us to make 3.0 functionality available, but non-default until 3.0.

Say you want this feature in 2.x, then app = Litestar(future_features=[Future.EXPLICIT_PARAMETER_DEFAULTS])

[JacobCoffee]

Could we just utilize experimental_features? It doesn't really sound like it fits the more i look at it, but i just hate the idea of adding more things

return Litestar(
        # base
        debug=settings.app.DEBUG,
        route_handlers=[*domain.routes],
        listeners=[*domain.listeners],
        middleware=[log.controller.middleware_factory],
        exception_handlers={
            exceptions.ApplicationError: exceptions.exception_to_http_response,
        },
        plugins=[db.plugin, domain.plugins.aiosql],
        signature_namespace=domain.signature_namespace,
        dependencies=dependencies,
        # lifecycle
        on_startup=[lambda: log.configure(log.default_processors)],  # type: ignore[arg-type]
        before_send=[log.controller.BeforeSendHandler()],
        # config
        cors_config=cors.config,
        logging_config=log.config,
        openapi_config=openapi.config,
        static_files_config=static_files.config,
        template_config=template.config,
        # experimental
        experimental_features=...
    )

😢

[peterschutt]

So I've found it comes from here: https://github.com/litestar-org/litestar/blob/ff975b68da0a0ea33f74f48158219379ff9d3fd0/litestar/_kwargs/parameter_definition.py#L40

Where FieldDefinition.has_default is:

https://github.com/litestar-org/litestar/blob/ff975b68da0a0ea33f74f48158219379ff9d3fd0/litestar/typing.py#L246-L253

Those param definition things drive what ends up in the data extracted from the connection by the kwargs model for the parameters. As demonstrated, any parameter that doesn't specify a default, is made default None.

Then from here:

https://github.com/litestar-org/litestar/blob/ff975b68da0a0ea33f74f48158219379ff9d3fd0/litestar/_kwargs/kwargs_model.py#L378

... if no value for the parameter is provided by the client, then output will still have {"param": None} in it, even if the param is specified with no default value.

That output is eventually passed to SignatureModel.parse_values_from_connection_kwargs() as the **kwargs arg:

https://github.com/litestar-org/litestar/blob/ff975b68da0a0ea33f74f48158219379ff9d3fd0/litestar/_signature/model.py#L173-L203

... it then comes down to the result of that data being passed to msgspec.convert() on line 190 there. If the None value that has been assigned to the default is consistent with the parameter's annotation on the signature model, then msgspec validates it and that is where our implicit default None comes from. If the None default isn't consistent with that parameter's annotation, then msgspec will raise the validation error that you'd expect which gives us the behavior of a non-nullable field without default being required.

So this seems way less deliberate than I thought it would be :/

Interestingly, if we change that first line that determines the parameter default to this (with one other minor complimentary change):

    default = field_definition.default if field_definition.has_default else msgspec.UNSET

Then there is only 9 test failures, and the lions share of those are from returning 400s where they were previously 200s..

[peterschutt]

Here's a PR that makes these non-optional.. you can have a look at the tests to get a feel for the impact of such a change: https://github.com/litestar-org/litestar/pull/2545

Also, you can also look at the definition of is_required here:

https://github.com/litestar-org/litestar/blob/ff975b68da0a0ea33f74f48158219379ff9d3fd0/litestar/typing.py#L308-L320

On the last line, if an annotation is Optional, then its not required.. so even if we haven't had this documented, we have had it coded in. And if I change that to only return is_required if the parameter has no default value, there are a lot of test failures. Maybe the tests would have to change to reflect the underlying change in logic but I haven't looked closely at them and I'm out of steam for now. There is some eye watering complexity through this part of the code base.

[provinzkraut]

What if we were to introduce future_features flag for the app that would behave like experimental_features but allow us to make 3.0 functionality available, but non-default until 3.0.

Say you want this feature in 2.x, then app = Litestar(future_features=[Future.EXPLICIT_PARAMETER_DEFAULTS])

I like the future_features. It plays nicely with how we want the 2 -> 3 transition to happen as well. We could potentially hide all non-backwards compatible features behind a flag like this wherever possible, which would greatly reduce the friction when migrating from 2 to 3.

[peterschutt]

Closed in #3361