Fix compatibility with starlette 0.18.0 and above #1594

TimPansino · 2022-01-25T23:14:48Z

Description

Types of Changes

Issues Fixed or Closed by This PR

Closes Incompatible with latest version of Starlette (v0.18.0) #1593

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

codecov · 2022-01-25T23:17:56Z

Codecov Report

Merging #1594 (10b411f) into main (35a627d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1594   +/-   ##
=======================================
  Coverage   98.04%   98.04%           
=======================================
  Files         143      143           
  Lines        5562     5564    +2     
  Branches     1023     1023           
=======================================
+ Hits         5453     5455    +2     
  Misses         56       56           
  Partials       53       53

strawberry/asgi/handlers/http_handler.py

TimPansino · 2022-01-25T23:24:10Z

All tests passed locally, looks like some tests failed on the runner though.

FAILED 😰  tests/asgi/test_http.py::test_returns_error_when_missing_query - a...
FAILED 😰  tests/asgi/test_http.py::test_returns_error_when_not_sending_wrong_content_type
FAILED 😰  tests/asgi/test_http.py::test_returns_error_when_method_is_not_allowed[PUT]
FAILED 😰  tests/asgi/test_http.py::test_returns_error_when_method_is_not_allowed[DELETE]
FAILED 😰  tests/asgi/test_query.py::test_fails_when_request_body_has_invalid_json
FAILED 😰  tests/asgi/test_view.py::test_renders_graphiql_disabled[/] - asser...
FAILED 😰  tests/asgi/test_view.py::test_renders_graphiql_disabled[/graphql]

patrick91 · 2022-01-26T00:04:52Z

All tests passed locally, looks like some tests failed on the runner though.

FAILED 😰  tests/asgi/test_http.py::test_returns_error_when_missing_query - a...
FAILED 😰  tests/asgi/test_http.py::test_returns_error_when_not_sending_wrong_content_type
FAILED 😰  tests/asgi/test_http.py::test_returns_error_when_method_is_not_allowed[PUT]
FAILED 😰  tests/asgi/test_http.py::test_returns_error_when_method_is_not_allowed[DELETE]
FAILED 😰  tests/asgi/test_query.py::test_fails_when_request_body_has_invalid_json
FAILED 😰  tests/asgi/test_view.py::test_renders_graphiql_disabled[/] - asser...
FAILED 😰  tests/asgi/test_view.py::test_renders_graphiql_disabled[/graphql]

Could be due to the different version of starlette on CI?

TimPansino · 2022-01-26T19:20:14Z

@patrick91

I've fixed those failing tests on starlette versions 0.16, 0.17, and 0.18. The only tests that fail when I run locally are FastAPI for starlette 0.18, which those two are incompatible so that's not really a strawberry problem.
I updated pyproject.toml to reflect the new versions of starlette that should be supported.
I added the warning message for duplicate headers, needed an actual loop which is almost certainly slower but may be a better user experience.

Let me know if there are any other changes you want made to this PR.

If I may suggest something, your testing could be made much more thorough if you used a tool like tox or nox to test each of your compatibility layers with a matrix of package and python versions. Obviously that bloats testing times on actions but is much more thorough. If you have any interest feel free to check out https://github.com/newrelic/newrelic-python-agent for an example of how we use this for testing.

patrick91 · 2022-01-26T22:01:39Z

@patrick91

* I've fixed those failing tests on starlette versions 0.16, 0.17, and 0.18. The only tests that fail when I run locally are FastAPI for starlette 0.18, which those two are incompatible so that's not really a strawberry problem.

* I updated pyproject.toml to reflect the new versions of starlette that should be supported.

Awesome!

* I added the warning message for duplicate headers, needed an actual loop which is almost certainly slower but may be a better user experience.

Yes! Hopefully people won't have too many headers :D

Let me know if there are any other changes you want made to this PR.

I'll check it soon!

If I may suggest something, your testing could be made much more thorough if you used a tool like tox or nox to test each of your compatibility layers with a matrix of package and python versions. Obviously that bloats testing times on actions but is much more thorough. If you have any interest feel free to check out https://github.com/newrelic/newrelic-python-agent for an example of how we use this for testing.

We do something similar to this for django, but maybe tox is easier to use. Maybe we could mark the tests with pytest.mark.starlette and so on so we only test relevant tests when using the matrix?

patrick91 · 2022-01-26T22:25:42Z

I wonder how we can circumvent this issue with FastAPI not supporting the latest version of starlette 🤔

patrick91 · 2022-01-26T22:28:44Z

I guess we can wait for this PR to be merged and released 😊 tiangolo/fastapi#4483

strawberry/asgi/handlers/http_handler.py

patrick91 · 2022-04-17T16:31:46Z

FastAPI is taking a long time to add support for latest starlette, so I think we should try to support both versions for a while :)

Tushant · 2022-05-06T16:28:17Z

latest starlette is now supported in fastapi.

Kludex · 2022-05-06T16:30:02Z

latest starlette is now supported in fastapi.

it's not, latest starlette is 0.20.0... but starlette 0.18.0 is.

krauss · 2022-05-10T12:27:13Z

Hey folks, do we have any news on this?

AntonOfTheWoods · 2022-05-14T15:14:08Z

FastAPI 0.77.1 is out and supports starlette 0.19.1. Does that mean this PR needs an update?

EDIT: scratch that, now 0.78.0 is out. While there is still no support for starlette 0.20.0, are we certain that we need for the latest fastapi to support the latest starlette in order to move forward, particularly given that 0.20.0 appears to mainly drop python 3.6 support, and fastapi might not be happy to do that for a while longer?

@patrick91 it would be a shame for strawberry to start perpetual waiting like graphene did...

patrick91 · 2022-05-15T04:11:26Z

@AntonOfTheWoods I'll update this PR soon, I'm a bit busy these days 😊

pyproject.toml

patrick91 · 2022-05-15T20:03:52Z

@Kludex does this PR look good to you? 😊

botberry · 2022-05-15T20:05:59Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

This release adds support for Starlette 0.18 to 0.20

It also removes upper bound dependencies limit for starlette,
allowing you to install the latest version without having to
wait for a new release of Strawberry

Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to Timothy Pansino for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

Kludex · 2022-05-15T20:45:19Z

Yep. If you're sure it works with Starlette <0.19.0, then it's fine 👍

for more information, see https://pre-commit.ci

This reverts commit 8d553f7.

AntonOfTheWoods · 2022-05-19T04:18:25Z

Yay! :-)

strawberry/types/info.py

Kludex · 2022-05-19T16:12:12Z

Is it an option to have one entry with latest Starlette without FastAPI installed, and skip the tests on FastAPI if not installed?

patrick91 · 2022-05-19T16:16:47Z

Is it an option to have one entry with latest Starlette without FastAPI installed, and skip the tests on FastAPI if not installed?

one entry of tests?

I tried that in this commit, but it didn't work: 8d553f7

I'm not sure if there's a way to force poetry to install a package, maybe I can try with pip directly, let me see.

patrick91 · 2022-05-19T16:24:17Z

Is it an option to have one entry with latest Starlette without FastAPI installed, and skip the tests on FastAPI if not installed?

I'm not sure if there's a way to force poetry to install a package, maybe I can try with pip directly, let me see.

well, that sees to have worked! thanks @Kludex

RELEASE.md

botberry · 2022-05-19T16:38:58Z

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67

And don't forget to join our discord server: https://strawberry.rocks/discord 🔥

patrick91 reviewed Jan 25, 2022

View reviewed changes

strawberry/asgi/handlers/http_handler.py Outdated Show resolved Hide resolved

TimPansino force-pushed the main branch from 8623cb4 to 696cdc4 Compare January 26, 2022 19:13

Kludex reviewed Jan 27, 2022

View reviewed changes

strawberry/asgi/handlers/http_handler.py Outdated Show resolved Hide resolved

TimPansino force-pushed the main branch from 696cdc4 to 6c71bee Compare February 2, 2022 21:19

Kludex reviewed Feb 2, 2022

View reviewed changes

strawberry/asgi/handlers/http_handler.py Outdated Show resolved Hide resolved

DoctorJohn reviewed Mar 4, 2022

View reviewed changes

strawberry/asgi/handlers/http_handler.py Outdated Show resolved Hide resolved

Kludex mentioned this pull request May 10, 2022

TypeError: '<' not supported between instances of 'NoneType' and 'int' on trying to acess http://localhost:8000/graphql encode/starlette#1633

Closed

patrick91 force-pushed the main branch from d800e87 to cc6fe74 Compare May 15, 2022 20:00

patrick91 reviewed May 15, 2022

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

patrick91 marked this pull request as ready for review May 15, 2022 20:03

ZephyrCodesStuff mentioned this pull request May 18, 2022

Fixed TypeError on any request encode/starlette#1641

Closed

TimPansino and others added 3 commits May 18, 2022 22:08

Fix compatibility with starlette 0.18.0

24156ba

[pre-commit.ci] auto fixes from pre-commit.com hooks

b415ee1

for more information, see https://pre-commit.ci

Add warnings for duplicate headers.

84fdf99

[pre-commit.ci] auto fixes from pre-commit.com hooks

58d1bd5

for more information, see https://pre-commit.ci

patrick91 force-pushed the main branch from 3c0f869 to dfe16a4 Compare May 19, 2022 03:09

TimPansino and others added 3 commits May 18, 2022 22:12

Fix missing headers extend

4f83dae

Remove upper dependency limit for starlette

b97a33a

enable tests for more versions of starlette

593d454

patrick91 force-pushed the main branch from 32ad257 to 593d454 Compare May 19, 2022 03:12

patrick91 added 4 commits May 18, 2022 22:14

Remove stale type ignore

084bb44

Fix typo

95095ee

Disable test on starlette 0.20.0

2485700

Add release file

9d1d56f

botberry added bot:has-release-file labels May 19, 2022

Remove fastapi in starlette tests

8d553f7

patrick91 force-pushed the main branch from 75e54d1 to 8d553f7 Compare May 19, 2022 03:37

Revert "Remove fastapi in starlette tests"

0bb5290

This reverts commit 8d553f7.

patrick91 reviewed May 19, 2022

View reviewed changes

strawberry/types/info.py Show resolved Hide resolved

Try installing starlette via pip

a5c9de5

patrick91 force-pushed the main branch from 69b2455 to a5c9de5 Compare May 19, 2022 16:17

patrick91 reviewed May 19, 2022

View reviewed changes

RELEASE.md Outdated Show resolved Hide resolved

Update RELEASE.md

10b411f

patrick91 changed the title ~~Fix compatibility with starlette 0.18.0~~ Fix compatibility with starlette 0.18.0 and above May 19, 2022

patrick91 approved these changes May 19, 2022

View reviewed changes

patrick91 merged commit 96f1cfc into strawberry-graphql:main May 19, 2022

TimPansino mentioned this pull request May 19, 2022

Isolate Integration Testing Environments #1917

Open

TimPansino mentioned this pull request Oct 23, 2022

Use Tox for Matrix Testing #2247

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compatibility with starlette 0.18.0 and above #1594

Fix compatibility with starlette 0.18.0 and above #1594

TimPansino commented Jan 25, 2022 •

edited

codecov bot commented Jan 25, 2022 •

edited

TimPansino commented Jan 25, 2022

patrick91 commented Jan 26, 2022

TimPansino commented Jan 26, 2022

patrick91 commented Jan 26, 2022

patrick91 commented Jan 26, 2022

patrick91 commented Jan 26, 2022

patrick91 commented Apr 17, 2022

Tushant commented May 6, 2022

Kludex commented May 6, 2022

krauss commented May 10, 2022

AntonOfTheWoods commented May 14, 2022 •

edited

patrick91 commented May 15, 2022

patrick91 commented May 15, 2022

botberry commented May 15, 2022 •

edited

Kludex commented May 15, 2022 •

edited

AntonOfTheWoods commented May 19, 2022

Kludex commented May 19, 2022

patrick91 commented May 19, 2022

patrick91 commented May 19, 2022

botberry commented May 19, 2022

Fix compatibility with starlette 0.18.0 and above #1594

Fix compatibility with starlette 0.18.0 and above #1594

Conversation

TimPansino commented Jan 25, 2022 • edited

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

codecov bot commented Jan 25, 2022 • edited

Codecov Report

TimPansino commented Jan 25, 2022

patrick91 commented Jan 26, 2022

TimPansino commented Jan 26, 2022

patrick91 commented Jan 26, 2022

patrick91 commented Jan 26, 2022

patrick91 commented Jan 26, 2022

patrick91 commented Apr 17, 2022

Tushant commented May 6, 2022

Kludex commented May 6, 2022

krauss commented May 10, 2022

AntonOfTheWoods commented May 14, 2022 • edited

patrick91 commented May 15, 2022

patrick91 commented May 15, 2022

botberry commented May 15, 2022 • edited

Kludex commented May 15, 2022 • edited

AntonOfTheWoods commented May 19, 2022

Kludex commented May 19, 2022

patrick91 commented May 19, 2022

patrick91 commented May 19, 2022

botberry commented May 19, 2022

TimPansino commented Jan 25, 2022 •

edited

codecov bot commented Jan 25, 2022 •

edited

AntonOfTheWoods commented May 14, 2022 •

edited

botberry commented May 15, 2022 •

edited

Kludex commented May 15, 2022 •

edited