Skip to content

Conversation

svlandeg
Copy link
Member

@svlandeg svlandeg commented Sep 22, 2025

Fixing the regression that happened after #1018, recently released as v0.19.1.

In v0.19.0, this was the original code for the internal_converor:

    def internal_convertor(value: Sequence[Any]) -> Optional[List[Any]]:
        if default_value is None and len(value) == 0:
            return None
        return [convertor(v) if convertor else v for v in value]

The author in #1018 changed that to:

    def internal_convertor(value: Sequence[Any]) -> Optional[List[Any]]:
        if default_value is None and (value is None or len(value) == 0):
            return None
        return [convertor(v) if convertor else v for v in value]

which made the (new) tests succeed, but had some typing issues. value should optionally be None (the fact that we didn't account for that was the reason for the bug that #1018 was solving).

Then mypy (understandably) needed a guarantee that value wasn't None by the final statement. Because of this, I rewrote the fix in #1018 to

    def internal_convertor(value: Optional[Sequence[Any]]) -> Optional[List[Any]]:
        if value is None or len(value) == 0:
            return default_value
        return [convertor(v) if convertor else v for v in value]

but this was a mistake and has caused a new bug as reported in #1349. I've written a new unit test to cover that bug in this PR, and now propose to revert this code to a solution in the middle:

    def internal_convertor(value: Optional[Sequence[Any]]) -> Optional[List[Any]]:
        if (value is None) or (default_value is None and len(value) == 0):
            return None
        return [convertor(v) if convertor else v for v in value]

comparing this to the original code from v0.19.0, the only thing that changes is the (value is None) or part. This seems a much cleaner and much less error-prone suggestion than what we eventually had in #1018.

@svlandeg svlandeg added the bug Something isn't working label Sep 22, 2025
@svlandeg svlandeg self-assigned this Sep 22, 2025
@svlandeg svlandeg marked this pull request as ready for review September 22, 2025 16:41
Copy link

@YuriiMotov YuriiMotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you! I'll release this in 0.19.2 in a bit 🚀

@tiangolo tiangolo merged commit f34aa4a into fastapi:master Sep 23, 2025
24 checks passed
@svlandeg svlandeg deleted the fix/empty_list branch September 23, 2025 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants