litestar-org / litestar

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

Bug: using a multi-list union type for query params fails when sending through an array of params #2600

Closed rseeley closed 1 year ago

rseeley commented 1 year ago

Description

Using a nullable multi-list union type for query params (e.g. list[str] | list[int] | None) will throw an error of "expected array | null, got str" when a valid list of one of the types is passed (e.g. ids=1&id=2).

Using a non-nullable multi-list union type (e.g. list[str] | list[int]) will throw a 500 error. Turning on debug mode, the error is:

TypeError: Type unions may not contain more than one array-like (list, set, tuple) type - type `typing.Union[typing.Annotated[list[int], msgspec.Meta(extra={'examples': None, 'external_docs': None, 'content_encoding': None, 'title': None, 'description': None, 'const': None, 'gt': None, 'ge': None, 'lt': None, 'le': None, 'multiple_of': None, 'min_length': None, 'max_length': None, 'pattern': None, 'lower_case': None, 'upper_case': None, 'format': None, 'enum': None, 'header': None, 'cookie': None, 'query': None, 'required': False})], typing.Annotated[list[str], msgspec.Meta(extra={'examples': None, 'external_docs': None, 'content_encoding': None, 'title': None, 'description': None, 'const': None, 'gt': None, 'ge': None, 'lt': None, 'le': None, 'multiple_of': None, 'min_length': None, 'max_length': None, 'pattern': None, 'lower_case': None, 'upper_case': None, 'format': None, 'enum': None, 'header': None, 'cookie': None, 'query': None, 'required': False})], typing.Annotated[list[uuid.UUID], msgspec.Meta(extra={'examples': None, 'external_docs': None, 'content_encoding': None, 'title': None, 'description': None, 'const': None, 'gt': None, 'ge': None, 'lt': None, 'le': None, 'multiple_of': None, 'min_length': None, 'max_length': None, 'pattern': None, 'lower_case': None, 'upper_case': None, 'format': None, 'enum': None, 'header': None, 'cookie': None, 'query': None, 'required': False})]]` is not supported

Updating the allowed type to be list[Any] | None or list[Any] works, though that has some downsides (non-automatic type conversion, for example).

The initial error "expected array | null, got str" is a little confusing, though, since it happens when you're actually passing in an array of params. It seems that this union style typing should be supported, especially to support codebases where models may have different ID types (I personally have this scenario at work).

URL to code causing the issue

https://github.com/rseeley/litestar-mcves/blob/mcve/list-params/tests/test_params.py

MCVE

No response

Steps to reproduce

1. Install dependencies with `pdm install`
2. Run `pdm run test`

Screenshots

No response

Logs

No response

Litestar Version

2.2.1

Platform


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

We seem to be swallowing the msgspec error somewhere along the way since msgspec.json.decode('{"foo": [1, 2]}', type=list[str] | list[int] | None) gives the same expected error message as not using a union with None.

peterschutt commented 1 year ago

With None in the union the fields on the signature model are:

fields(cls)=(FieldInfo(name='ids', encode_name='ids', type=typing.Optional[typing.Annotated[list[int], msgspec.Meta(extra={'examples': None, 'external_docs': None, 'content_encoding': None, 'title': None, 'description': None, 'const': None, 'gt': None, 'ge': None, 'lt': None, 'le': None, 'multiple_of': None, 'min_length': None, 'max_length': None, 'pattern': None, 'lower_case': None, 'upper_case': None, 'format': None, 'enum': None, 'header': None, 'cookie': None, 'query': None, 'required': False})]], default=NODEFAULT, default_factory=NODEFAULT),)

without None:

fields(cls)=(FieldInfo(name='ids', encode_name='ids', type=typing.Union[typing.Annotated[list[int], msgspec.Meta(extra={'examples': None, 'external_docs': None, 'content_encoding': None, 'title': None, 'description': None, 'const': None, 'gt': None, 'ge': None, 'lt': None, 'le': None, 'multiple_of': None, 'min_length': None, 'max_length': None, 'pattern': None, 'lower_case': None, 'upper_case': None, 'format': None, 'enum': None, 'header': None, 'cookie': None, 'query': None, 'required': False})], typing.Annotated[list[str], msgspec.Meta(extra={'examples': None, 'external_docs': None, 'content_encoding': None, 'title': None, 'description': None, 'const': None, 'gt': None, 'ge': None, 'lt': None, 'le': None, 'multiple_of': None, 'min_length': None, 'max_length': None, 'pattern': None, 'lower_case': None, 'upper_case': None, 'format': None, 'enum': None, 'header': None, 'cookie': None, 'query': None, 'required': False})], typing.Annotated[list[uuid.UUID], msgspec.Meta(extra={'examples': None, 'external_docs': None, 'content_encoding': None, 'title': None, 'description': None, 'const': None, 'gt': None, 'ge': None, 'lt': None, 'le': None, 'multiple_of': None, 'min_length': None, 'max_length': None, 'pattern': None, 'lower_case': None, 'upper_case': None, 'format': None, 'enum': None, 'header': None, 'cookie': None, 'query': None, 'required': False})]], default=NODEFAULT, default_factory=NODEFAULT),)

Bit of a pain to read but with the | None we are putting list[int] | None on the signature model, and without it we are putting list[int] | list[str] | list[UUID].

So this explains why we are getting different errors. However, even though it isn't the correct annotation, I can't think of any reason that the [1, 2, 3] test shouldn't pass when the annotation is list[int] | None.

peterschutt commented 1 year ago

https://github.com/litestar-org/litestar/blob/dfe872c6379142d6c5a266a6e5a19fa48dacddb8/litestar/_signature/model.py#L296

Well that would explain it. It just ignores all other union members other than the first if None is in the union :shrug:

peterschutt commented 1 year ago

2776 fixes the inconsistent error types - I don't know that we can do much about allowing that type of parameter though given that the restriction is upstream of us.

provinzkraut commented 1 year ago

Closing this as it's not really a bug, but intended behaviour of msgspec. IMO the restriction is sensible, since list[str] | list[int] is ambiguous, as a str may be converted into an int, so it's not really possible to make a distinction between "this should be an int, but threw a parsing error" and "this should be a str, so I won't try to parse it as an int".