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

fix(openapi): set default on schema from FieldDefinition #3280

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

guacs
Copy link
Member

@guacs guacs commented Mar 29, 2024

Description

Consider the following:

    def get_foo(foo_id: int = 10) -> None:
        ...

In such cases, no KwargDefinition is created since there is no metadata provided via Annotated. The default is still parsed, and set on the generated FieldDefinition, however the SchemaCreator currently only considers defaults that are set on KwargDefinition.

So in such cases, we should fallback to the default set on the FieldDefinition if there is a valid default value.

Closes

Fixes #3278.

@guacs guacs requested review from a team as code owners March 29, 2024 08:32
@guacs
Copy link
Member Author

guacs commented Mar 29, 2024

This does raise an interesting condition that we don't currently handle.

def get_foo(foo: Annotated[int, Parameter(default=20)] = 12) -> None:
    ....

In this case, the default in the generated schema would be 20. If there's such a mismatch between the values for default in FieldDefinition and it's associated KwargDefinition, then an error should be raised during startup itself. I didn't include it as part of this PR because I think that's best left as another PR, but also because I wasn't sure if that would be considered a breaking change.

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.23%. Comparing base (043a044) to head (35c4825).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3280   +/-   ##
=======================================
  Coverage   98.23%   98.23%           
=======================================
  Files         320      320           
  Lines       14448    14450    +2     
  Branches     2296     2297    +1     
=======================================
+ Hits        14193    14195    +2     
  Misses        114      114           
  Partials      141      141           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@provinzkraut
Copy link
Member

This does raise an interesting condition that we don't currently handle.

def get_foo(foo: Annotated[int, Parameter(default=20)] = 12) -> None:
    ....

In this case, the default in the generated schema would be 20. If there's such a mismatch between the values for default in FieldDefinition and it's associated KwargDefinition, then an error should be raised during startup itself. I didn't include it as part of this PR because I think that's best left as another PR, but also because I wasn't sure if that would be considered a breaking change.

Good catch! Yeah, that should be an error on startup. I'd go even further as to now allow specifying defaults in two places to begin with. A default in Parameter should only be allowed if it's being used with the old param: int = Parameter(default=1) style, but not param: Annotated[int, Parameter(default=1)].

Consider the following:

```python
    def get_foo(foo_id: int = 10) -> None:
        ...
```
In such cases, no `KwargDefinition` is created since there is no
metadata provided via `Annotated`. The default is still parsed, and
set on the generated `FieldDefinition`, however the `SchemaCreator`
currently only considers defaults that are set on `KwargDefinition`.

So in such cases, we should fallback to the default set on the
`FieldDefinition` if there is a valid default value.
Copy link

sonarcloud bot commented Mar 29, 2024

Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3280

@guacs guacs merged commit cf93c86 into main Mar 29, 2024
22 checks passed
@guacs guacs deleted the set-default-openapi branch March 29, 2024 09:49
cofin pushed a commit that referenced this pull request Apr 1, 2024
Consider the following:

```python
    def get_foo(foo_id: int = 10) -> None:
        ...
```
In such cases, no `KwargDefinition` is created since there is no
metadata provided via `Annotated`. The default is still parsed, and
set on the generated `FieldDefinition`, however the `SchemaCreator`
currently only considers defaults that are set on `KwargDefinition`.

So in such cases, we should fallback to the default set on the
`FieldDefinition` if there is a valid default value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: default not generated for query parameter in openapi spec.
3 participants