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.18.0 to 0.19.0 #4488

Merged
merged 21 commits into from May 9, 2022

Conversation

Kludex
Copy link
Sponsor Collaborator

@Kludex Kludex commented Jan 27, 2022

Set of breaking changes:

Starlette 0.19.0 is not released yet. Feel free to ignore this for now.

@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #4488 (fd46adb) into master (9090c77) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #4488   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          531       531           
  Lines        13630     13629    -1     
=========================================
- Hits         13630     13629    -1     
Impacted Files Coverage Δ
fastapi/exceptions.py 100.00% <100.00%> (ø)
tests/test_extra_routes.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 9090c77...fd46adb. Read the comment docs.

@github-actions
Copy link
Contributor

📝 Docs preview for commit b19e848 at: https://61f2f482339a46310dd7a236--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 1b3342b at: https://61f42253a83c1522555c776e--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 09c70c4 at: https://61f6b9f3886122caa7fb096d--fastapi.netlify.app

pyproject.toml Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

📝 Docs preview for commit 880dd0d at: https://61f8720b27a15533d4fa2da5--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2022

📝 Docs preview for commit c48a399 at: https://61f94879d85c46e5edab7f8e--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2022

📝 Docs preview for commit 7b519b8 at: https://61f9bcf8e9ce9131496180ee--fastapi.netlify.app

super().__init__(status_code=status_code, detail=detail)
self.headers = headers

from starlette.exceptions import HTTPException as HTTPException # noqa

Choose a reason for hiding this comment

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

Is the renaming of import, i.e., as HTTPException needed at the last?

Copy link

Choose a reason for hiding this comment

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

Suggested change
from starlette.exceptions import HTTPException as HTTPException # noqa
from starlette.exceptions import HTTPException

I suppose there's not reason for noqa anymore.

@musicinmybrain
Copy link
Contributor

This works great for me in the Fedora Linux RPM package. However, I still do have to ask pytest to ignore DeprecationWarning, since apparently starlette.middleware.wsgi is going away:

==================================== ERRORS ====================================
______ ERROR collecting tests/test_tutorial/test_wsgi/test_tutorial001.py ______
tests/test_tutorial/test_wsgi/test_tutorial001.py:3: in <module>
    from docs_src.wsgi.tutorial001 import app
docs_src/wsgi/tutorial001.py:2: in <module>
    from fastapi.middleware.wsgi import WSGIMiddleware
../../BUILDROOT/python-fastapi-0.75.0-3.fc37.x86_64/usr/lib/python3.10/site-packages/fastapi/middleware/wsgi.py:1: in <module>
    from starlette.middleware.wsgi import WSGIMiddleware as WSGIMiddleware  # noqa
/usr/lib/python3.10/site-packages/starlette/middleware/wsgi.py:11: in <module>
    warnings.warn(
E   DeprecationWarning: starlette.middleware.wsgi is deprecated and will be removed in a future release. Please refer to https://github.com/abersheeran/a2wsgi as a replacement.
=========================== short test summary info ============================
ERROR tests/test_tutorial/test_wsgi/test_tutorial001.py - DeprecationWarning:...
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 2.38s ===============================

@Kludex
Copy link
Sponsor Collaborator Author

Kludex commented Mar 16, 2022

I need to update this PR. I'll do it today (I think).

@Kludex
Copy link
Sponsor Collaborator Author

Kludex commented Mar 16, 2022

I'm going to just ignore the warning from WSGIMiddleware.

@tiangolo This is the PR that deprecates it: encode/starlette#1504

I'm not sure if you want to import a2wsgi on fastapi.middleware.wsgi, and update the docs...

@github-actions
Copy link
Contributor

📝 Docs preview for commit d382aa5 at: https://62321787dc48cd00848f8cda--fastapi.netlify.app

@musicinmybrain
Copy link
Contributor

Still works for me, including the newly-ignored warning. Thanks!

super().__init__(status_code=status_code, detail=detail)
self.headers = headers

from starlette.exceptions import HTTPException as HTTPException # noqa
Copy link

Choose a reason for hiding this comment

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

Suggested change
from starlette.exceptions import HTTPException as HTTPException # noqa
from starlette.exceptions import HTTPException

I suppose there's not reason for noqa anymore.

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

Choose a reason for hiding this comment

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

How about adding proper type hints for these names?

def __init__(
self,
status_code: int,
detail: Any = None,
Copy link
Owner

Choose a reason for hiding this comment

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

There are several examples and places in the code base that expect detail to be any JSON-able data, not only a string (as is with Starlette).

So, only for this little type hint we still need the custom HTTPException class in FastAPI.

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2022

📝 Docs preview for commit fd46adb at: https://6279506becc1a10283b84487--fastapi.netlify.app

@tiangolo tiangolo changed the title Bump starlette from 0.18.0 to 0.19.0 ⬆ Upgrade Starlette from 0.18.0 to 0.19.0 May 9, 2022
@tiangolo tiangolo merged commit 86fa3cb into tiangolo:master May 9, 2022
@tiangolo
Copy link
Owner

Thanks for the work @Kludex! 🚀 🎉

This is now available in FastAPI version 0.77.0 🔖

johnoppenheimer added a commit to betagouv/euphrosyne-tools-api that referenced this pull request Aug 8, 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.

None yet

7 participants