Skip to content

🐛 Fix list convertor with an empty list default factory #1350

Merged
tiangolo merged 6 commits intofastapi:masterfrom
svlandeg:fix/empty_list
Sep 23, 2025
Merged

🐛 Fix list convertor with an empty list default factory #1350
tiangolo merged 6 commits intofastapi:masterfrom
svlandeg:fix/empty_list

Conversation

@svlandeg
Copy link
Copy Markdown
Member

@svlandeg svlandeg commented Sep 22, 2025 •
edited
Loading

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
YuriiMotov
YuriiMotov approved these changes Sep 22, 2025
View reviewed changes
Copy link
Copy Markdown
Member

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

@tiangolo tiangolo mentioned this pull request Sep 23, 2025
Closed
7 tasks
tiangolo
tiangolo approved these changes Sep 23, 2025
View reviewed changes
Copy link
Copy Markdown
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
@svlandeg svlandeg removed their assignment Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tiangolo tiangolo tiangolo approved these changes

@YuriiMotov YuriiMotov YuriiMotov approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@svlandeg @tiangolo @YuriiMotov