Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

⬆️ Upgrade Starlette to 0.14.2, including internal UJSONResponse migrated from Starlette #2335

Merged
merged 1 commit into from May 10, 2021

Conversation

@hanneskuettner
Copy link
Contributor

@hanneskuettner hanneskuettner commented Nov 10, 2020

starlette 0.14.1 includes a fix for users that want to use the @requires decorator from the AuthenticationMiddleware on FastAPI routes.

Breaking in starlette changes as far as I can see are:

  • UJSONResponse was removed from the core as per this issue, they added documentation on how to implement custom JSON serialization. UJSONResponse is explicitly exported in FastAPI and should either be removed to follow starlette or added back in as done with ORJSONResponse. For this PR I chose to add a custom UJSONResponse and add test cases.

Is there any reason it was hard locked to 13.6 I am not aware of or is it as straightforward as updating the requirement?

@codecov
Copy link

@codecov codecov bot commented Nov 10, 2020

Codecov Report

Merging #2335 (049a91a) into master (4d208b2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master     #2335    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          243       244     +1     
  Lines         7419      7276   -143     
==========================================
- Hits          7419      7276   -143     
Impacted Files Coverage Δ
fastapi/responses.py 100.00% <100.00%> (ø)
..._tutorial/test_custom_response/test_tutorial001.py 100.00% <100.00%> (ø)
tests/test_path.py 100.00% <0.00%> (ø)
tests/test_query.py 100.00% <0.00%> (ø)
tests/test_fakeasync.py 100.00% <0.00%> (ø)
tests/test_application.py 100.00% <0.00%> (ø)
tests/test_params_repr.py 100.00% <0.00%> (ø)
tests/test_empty_router.py 100.00% <0.00%> (ø)
tests/test_router_events.py 100.00% <0.00%> (ø)
tests/test_openapi_servers.py 100.00% <0.00%> (ø)
... and 62 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c09e950...049a91a. Read the comment docs.

Loading

@miguescri
Copy link

@miguescri miguescri commented Nov 12, 2020

Will this be merged soon? It is problematic that server breaks when installing both FastAPI and Starlette latest versions.

Loading

@Kludex
Copy link
Contributor

@Kludex Kludex commented Nov 20, 2020

This needs to be refactored when the PR is accepted: https://github.com/TechEmpower/FrameworkBenchmarks/tree/master/frameworks/Python/fastapi

Just posting here as a note for people in the future 😮

Loading

@miguescri
Copy link

@miguescri miguescri commented Nov 24, 2020

@tiangolo

Will this be merged soon? It is problematic that server breaks when installing both FastAPI and Starlette latest versions.

Loading

@hanneskuettner
Copy link
Contributor Author

@hanneskuettner hanneskuettner commented Nov 30, 2020

Hey @tiangolo, anything that needs to be resolved before we can merge this?
As far as I can see only the one docs failed to build due to an unrelated issue.

Loading

@miguescri
Copy link

@miguescri miguescri commented Dec 2, 2020

@hanneskuettner try force the CI to re-run so that the tests pass. It is weird, but maybe it is not being reviewed because of it.

Loading

@Kludex
Copy link
Contributor

@Kludex Kludex commented Dec 2, 2020

The issue on the build docs stage was fixed some commits ago, so a rebase will probably fix the issue. 👍

Loading

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Dec 7, 2020

📝 Docs preview for commit 3b10ea3c052e18ea464e88d50df29565c7f756c1 at: https://5fce368cf47f96bc6bba1226--fastapi.netlify.app

Loading

@Kludex Kludex mentioned this pull request Dec 8, 2020
@yareyaredesuyo
Copy link

@yareyaredesuyo yareyaredesuyo commented Dec 9, 2020

Still need for supporting ujson?

The reason why starletter removed ujson from core looks like reducing 3rd party dependency.

Supporting a lot of 3rd libraries maybe lead to messsy situation.

How about writing custom response in docs (maybe not documented)? like this encode/starlette#1047

Loading

@hanneskuettner
Copy link
Contributor Author

@hanneskuettner hanneskuettner commented Dec 9, 2020

@yareyaredesuyo Sure thing. I personally agree with removing UJSONResponse for good and pointing at to the custom response documentation. But then I saw that @tiangolo added a ORJSONResponse to FastAPI so I thought might as well since it seems to be in the spirit of this library to offer these two out of the box and I didn't want to break backwards compatibility with this PR.

We could open up an issue and discuss removing both of them maybe.

Loading

@mweinelt
Copy link

@mweinelt mweinelt commented Feb 11, 2021

This change, reimplementing UJSONResponse, is what is preventing us in nixpkgs, and probably other distros, from upgrading starlette, as it breaks the whole fastapi testsuite on 0.63.0. Please consider fast-tracking this or something like it.

Loading

@vinayinvicible
Copy link

@vinayinvicible vinayinvicible commented Feb 12, 2021

Please consider upgrading starlette to 0.14.2.
it fixed an issue when run in debug mode.

Loading

@lwinkler-cloudflight
Copy link

@lwinkler-cloudflight lwinkler-cloudflight commented Feb 25, 2021

Seems fairly straight forward, any reason this is not merged and released yet?

Loading

Copy link

@soundstripe soundstripe left a comment

I recommend going ahead with this after pinning starlette to 0.14.2 instead of 0.14.1 -- there is a fix for Python 3.8+ when debug is set (starlette does not show/log tracebacks properly without the fix).

Loading

@juntatalor
Copy link

@juntatalor juntatalor commented Mar 5, 2021

@tiangolo hi! for version 0.63 debug response is broken in python 3.8 due to starlette bug. Could you please schedule this PR for release?

Loading

@hanneskuettner hanneskuettner changed the title ⬆️ Upgrade starlette to 0.14.1 ⬆️ Upgrade starlette to 0.14.2 Mar 5, 2021
@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Mar 5, 2021

Loading

@hanneskuettner
Copy link
Contributor Author

@hanneskuettner hanneskuettner commented Mar 5, 2021

Rebased onto current master and bumped starlette to 0.14.2.
Let's go! 🚀 ❤️

Loading

Mause
Mause approved these changes Mar 5, 2021
@jcwilson
Copy link

@jcwilson jcwilson commented Apr 7, 2021

This is awesome. Thanks @hanneskuettner!

Any chance we could change the version pin to not be so strict? For instance, use starlette>=0.14.2,<0.15.0 so we don't have to wait for an update in FastAPI to pick up bugfixes in starlette? It would still let users pin to 0.14.2 in their own projects if necessary.

edit: maybe don't even specify a max version? I get that it's recommended that one pin versions explicitly when deploying an application, but a library's requirements spec isn't really the best place to do that (pipenv, poetry and conda are tools that manage lock files for python projects). IMO, the library should specify a minimum supported version for any dependencies and let clients decide how far ahead works for them. If some dependency ships a breaking change, then we can come back and put an upper limit in this project's requirements (or better yet, adapt to the breaking change).

Loading

@miguescri
Copy link

@miguescri miguescri commented Apr 7, 2021

Still not merging?

Loading

@hanneskuettner
Copy link
Contributor Author

@hanneskuettner hanneskuettner commented Apr 9, 2021

@jcwilson I also thought about relaxing the pinned version, I found however this previous attempt and @tiangolo advocated against relaxing due to tight coupling to starlette internals.

@tiangolo any change in opinion on that front?

Loading

@jcwilson
Copy link

@jcwilson jcwilson commented Apr 9, 2021

@jcwilson I also thought about relaxing the pinned version, I found however this previous attempt and @tiangolo advocated against relaxing due to tight coupling to starlette internals.

@tiangolo any change in opinion on that front?

Thanks for pointing that out. I'd still like to advocate for not being so strict, but if doing would delay this PR at all, I'd be glad to argue for it in a follow-up PR. However, I can see how the strict version pinning can make it easier to ensure that the docs remain consistent with expected behavior since much of the FastAPI documentation is actually starlette documentation.

Loading

@drainer-cloudflight
Copy link

@drainer-cloudflight drainer-cloudflight commented Apr 15, 2021

Any news on this? Would be really nice to have this merged. @hanneskuettner @jcwilson

Loading

@hanneskuettner
Copy link
Contributor Author

@hanneskuettner hanneskuettner commented Apr 15, 2021

TBH doesn't look like there's been a lot of activity in the repo at all in the last few months.
Anything we can do to help along @tiangolo?

Loading

@Kludex
Copy link
Contributor

@Kludex Kludex commented Apr 15, 2021

There's no need for spamming.

@tiangolo will eventually see this PR.

Also, for those who don't know, he announced publically that he'll dedicate more time to FastAPI (and other OS projects) starting this month. So, you can expect this to be reviewed by him in the next weeks.

Loading

@L1ghtn1ng
Copy link

@L1ghtn1ng L1ghtn1ng commented May 3, 2021

@tiangolo any update on this? As this is causing issues for so many people and holding things up for people and distros. If this could get merged ASAP even without a release least people could then point to the master git branch

Loading

@tiangolo tiangolo changed the title ⬆️ Upgrade starlette to 0.14.2 ⬆️ Upgrade Starlette to 0.14.2, including internal UJSONResponse migrated from Starlette May 10, 2021
@tiangolo tiangolo merged commit 4aed041 into tiangolo:master May 10, 2021
6 checks passed
Loading
@hanneskuettner hanneskuettner deleted the bump-starlette branch May 10, 2021
@tiangolo
Copy link
Owner

@tiangolo tiangolo commented May 10, 2021

Awesome, thank you @hanneskuettner ! 🎉 Thanks for all the care adding all the details, explanations, and info.

Thanks for the discussion everyone!

And thanks for the help @Kludex and @Mause 🙇 🍰

Indeed, I had very little time for open source during the last months. 😔

But I recently resigned from my job to be able to dedicate a high percentage of my working time to FastAPI and the other open source projects, while doing some consultancy to make it all sustainable, so I'll be able to work a lot more no all this. 🤓 🚀

This will be available in FastAPI 0.65.0, released in a couple of hours. 🎉

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet