Add support for Iterator in jsonable_encoder

[klaa97]

This PR adds support for Iterator in the jsonable_encoder.

Currently, FastAPI allows for response_model such as Iterable[BaseModel] . In those cases:

1. The Pydantic Model is used to retrieve the validated data from the response object https://github.com/tiangolo/fastapi/blob/20d48345465e5b09756cc9ef62fb940c63255a13/fastapi/routing.py#L129
2. The output of the validation is a List Iterator
3. The jsonable_encoder doesn't encode the list, but just calls dict() and returns a list of the fields of the BaseModel

By adding the support for Iterator in the jsonable_encoder, we avoid that the response object is passed to dict(), losing the values of the fields of the BaseModel.

An example of this issue is described in #3911

Fixes #3911

[interifter]

What do we need to do in order to get this approved and merged?

[klaa97]

What do we need to do in order to get this approved and merged?

Tagging randomly and asking a review to @Kludex @dmontagu? I have 2 PRs opened for some months here, and it's not clear to me who to ask for a review for bug fixes.

Thank you 🙏

[Kludex]

@tiangolo is the only one who can have this merged. Nothing to do besides wait. :kissing:

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (cf73051) 100.00% compared to head (ea49870) 100.00%. Report is 1036 commits behind head on master.

:exclamation: Current head ea49870 differs from pull request most recent head 7dd6de8. Consider uploading reports for the commit 7dd6de8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3913   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          540       540           
  Lines        13969     13954   -15     
=========================================
- Hits         13969     13954   -15     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[github-actions[bot]]

📝 Docs preview for commit ea498706949df06df76d6e711d13723599b28bf8 at: https://631e12b4780f1a4aeccb3340--fastapi.netlify.app

[github-actions[bot]]

📝 Docs preview for commit 7dd6de87a3e0d2592de202c74e77950293e99e81 at: https://639cd119ed5882175dfeaa2a--fastapi.netlify.app

[interifter]

Looks good, and I working fine.

Just a curious question:

Why don't we consider all iterable, sequence instead of explicitly specifying each types?

See https://github.com/tiangolo/fastapi/pull/3913#discussion_r712343889

Why don't we just make this isinstance(obj, Iterable)? Is that going to be too generic and pick up bad inputs?

from typing import Iterable

def gen(arr):
    for x in arr:
        yield x

l = [1, 2, 3, 4]
g = gen(l)
s = set(l)
t = tuple(l)
f = frozenset(l)

print(isinstance(g, Iterable))
print(isinstance(l, Iterable))
print(isinstance(s, Iterable))
print(isinstance(t, Iterable))
print(isinstance(f, Iterable))

# Output is all True

and the response:

The problem is exactly the one you guessed - Iterable makes some tests fail (https://github.com/tiangolo/fastapi/blob/22528373bba6a654323de416ad5c867cbadb81bb/tests/test_jsonable_encoder.py#L102).Iterator.%60Iterator%60) maintains the current interface and fix the issue you opened, and in general the behaviour with Iterable[] types.

[tiangolo]

Thanks @klaa97!

I'm struggling to find what use case would this solve that would not be solved by setting the response model to list[X], as described in the same discussion.

As there are many other things that could be Iterable and Iterator and it has nuanced behaviors, I would be inclined to leave just list and the others. Also, it's normally better to be more explicit in the output/return about the exact type.

It would be better to start with a use case that shows why this is needed and why using a list[X] is not enough, filling the GitHub Discussion question form: https://github.com/tiangolo/fastapi/discussions/new?category=questions 🙏

[tiangolo]

Assuming the original need was handled, this will be automatically closed now. But feel free to add more comments or create new issues or PRs.

[interifter]

Assuming the original need was handled, this will be automatically closed now. But feel free to add more comments or create new issues or PRs. I think so.

I ended up using a list, but the ask to support Iterator seems pretty no-nonsense, given it allows for more flexibility. After two years, I had figured this would never get merged and a list seems to work well enough.