Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.29%. Comparing base (6d49fc2) to head (58da671).
Report is 32 commits behind head on main.

@@           Coverage Diff           @@
##             main    #3632   +/-   ##
=======================================
  Coverage   98.29%   98.29%           
=======================================
  Files         330      330           
  Lines       15122    15135   +13     
  Branches     2401     2404    +3     
=======================================
+ Hits        14864    14877   +13     
  Misses        117      117           
  Partials      141      141           

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

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

While we're at it, is_non_string_sequence seems buggy - it claims to return True for types that can be cast to Sequence, but it allows Sets.

It's also smelly - it tests against a whole bunch of types, while issubclass(..., Sequence) should suffice, unless I'm missing something.

Also, Collections.abc.Sequence type differs from its supertypes by adding reverse and __gettem__. Reverse is never used and index access is only used once in _extract_multipart. I'm not sure about the intention behind using is_non_string_sequence.

I suspect the problem is with python type system lacking a type suitable for sequential processing
Sequence doesn't include sets, and Iterable includes Mapping and str.

Perhaps we could introduce a new type, lets call it ArrayLike for now, as it can be represented as JSON arrays and list parameters in HTTP.

Can someone either accept it or drop it, before the changes go completely outdated? Thanks!

Alright, I've taken another look at this and I think the change as such is good. The implementation can be simplified though to the following:

* Add an `is_iterable` property to `FieldDefinition`

* Set `is_sequence=field_definition.is_sequence and not field_definition.is_mapping`

The rest can be removed.

Makes sense

Also, this should include a test case that reproduces the actual bug it's fixing.

I'd rather copy and adjust an existing test, not to break existing patterns, but I couldn't find anything similar.

I'd rather copy and adjust an existing test, not to break existing patterns, but I couldn't find anything similar.

You could basically just copy the example you gave in #3631, and create a parametrized test case that shows it works as expected with a bunch of iterable types that should be supported.

I wrote a nice matrix parametrized tests with Iterable, Sequece, List x str, int and it fails for Iterable
The patch as it is makes litestar generate correct openapi, but msgspec officially doesn't support Iterable.

I think the list of supported types could be clearer and link to https://jcristharif.com/msgspec/supported-types.html

I wrote a nice matrix parametrized tests with Iterable, Sequece, List x str, int and it fails for Iterable The patch as it is makes litestar generate correct openapi, but msgspec officially doesn't support Iterable.

I think the list of supported types could be clearer and link to https://jcristharif.com/msgspec/supported-types.html

Well that was kind of my point :)
It doesn't really make sense to decode into an Iterable, as you'll always have to decode into a concrete type, in this case a list. If we were to support encoding into Iterable, we'd have to override its actual type for the conversion to a list at some point.

IMO it's easier (and more correct) to just use a list to begin with.

Maybe your original problem can be addressed in a different way? Iiuc it's about setting defaults for mutable types. Maybe supporting a default_factory would help?