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 from 0.17.1 to 0.18.0 #4483

Merged
merged 6 commits into from
May 5, 2022

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Jan 26, 2022

@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #4483 (25feb07) into master (146f57b) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #4483   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          531       531           
  Lines        13627     13630    +3     
=========================================
+ Hits         13627     13630    +3     
Impacted Files Coverage Δ
fastapi/concurrency.py 100.00% <100.00%> (ø)
fastapi/dependencies/utils.py 100.00% <100.00%> (ø)
fastapi/routing.py 100.00% <100.00%> (ø)

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 146f57b...25feb07. Read the comment docs.

@github-actions
Copy link
Contributor

📝 Docs preview for commit e5c9343 at: https://61f176cddc1930f5b39fe9cd--fastapi.netlify.app

Comment on lines 465 to 468
if response is None:
response = Response()
del response.headers["content-length"]
response.status_code = None
Copy link
Member Author

Choose a reason for hiding this comment

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

@tomchristie FastAPI uses a Response object that travels through the dependencies, but that Response is not the response that will be returned at the end. There's a "cleanup" after the dependencies are solved. You can see the "cleanup" logic below:

https://github.com/tiangolo/fastapi/blob/291180bf2d8c39e84860c2426b1d58b6c80f6fef/fastapi/routing.py#L216-L254

I've selected the whole snippet, from the point we get the Response object used in the dependencies (solve_dependencies call) until the last 5 lines (on which the values from Response are passed to the actual response object that will be used.

This comment is JFYK, I don't expect any action to be taken on Starlette. Also, considering that FastAPI tweaks Starlette internally for some features it has.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 8d10fca at: https://61f178c33b56fbf00cd761e2--fastapi.netlify.app

@@ -126,7 +126,7 @@ async def serialize_response(
if is_coroutine:
value, errors_ = field.validate(response_content, {}, loc=("response",))
else:
value, errors_ = await run_in_threadpool(
value, errors_ = await run_in_threadpool( # type: ignore[misc]
Copy link
Member Author

Choose a reason for hiding this comment

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

We added ParamSpec to run_in_threadpool on Starlette. The error here was:

fastapi/routing.py:129: error: "<nothing>" object is not iterable  [misc]

@github-actions
Copy link
Contributor

📝 Docs preview for commit fb2fdf5 at: https://61f17c4d603b5ade0e34ca0d--fastapi.netlify.app

Copy link
Contributor

@yezz123 yezz123 left a comment

Choose a reason for hiding this comment

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

@Kludex 🌟

@L1ghtn1ng
Copy link

can this get merged soon and released please?

@ShadowLNC
Copy link

Starlette 0.19.0 has now been released, not sure if it's worth performing a further upgrade here, or leaving that for another time.

@Kludex
Copy link
Member Author

Kludex commented Mar 14, 2022

As per history, Sebastián prefers to bump progressively. Also, those two releases have a lot of changes.

@Net-Mist
Copy link

Hello !
I would like to know more regarding the status of this PR. Is there a reason not to merge it ?

@Kludex
Copy link
Member Author

Kludex commented Apr 26, 2022

Hello ! I would like to know more regarding the status of this PR. Is there a reason not to merge it ?

Let's wait for @tiangolo :)

@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

📝 Docs preview for commit e7087f5 at: https://62743b753fbbb2094bed5235--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

📝 Docs preview for commit 25feb07 at: https://62744bdcedfeb0165d6951ed--fastapi.netlify.app

@tiangolo
Copy link
Member

tiangolo commented May 5, 2022

Awesome @Kludex, thank you! 🙇

I added a filterwarning for pytest complaining about Cryptography so we can run tests while reaching the version that drops support for Python 3.6.

This will be available in the next release in a couple of hours, FastAPI 0.76.0 🏷️

@tiangolo tiangolo changed the title Bump starlette from 0.17.1 to 0.18.0 ⬆ Upgrade Starlette from 0.17.1 to 0.18.0 May 5, 2022
@tiangolo tiangolo merged commit 33d6143 into fastapi:master May 5, 2022
JeanArhancet pushed a commit to JeanArhancet/fastapi that referenced this pull request Aug 20, 2022
Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants