Make `content` argument required to `JSONResponse` #1431

aminalaee · 2022-01-25T10:40:13Z

Making content argument required in JSONResponse:

Response will work as before defaulting to b"" when content is None
JSONResponse() will fail as content is required
JSONResponse({}) will work as expected
JSONResponse(None) will be rendered as null

Remove count of available convertors (#1409) Co-authored-by: Micheal Gendy <[email protected]> Fix md5_hexdigest wrapper on FIPS enabled systems (#1410) * Fix md5_hexdigest wrapper on FIPS enabled systems * Update _compat.py * lint Use typing `NoReturn` (#1412) change github issues template Sort third-party packages and add `starlette-wtf` (#1415) Improvements on authentication documentation (#1420) * Use `conn` in `AuthenticationBackend` documentation * Remove unused import in `AuthenticationBackend` documentation * Add missing imports in authentication documentation Co-authored-by: Marcelo Trylesinski <[email protected]> Add third-party CSRF middlewares (#1414) * change github issues template * Add third-party CSRF middlewares Co-authored-by: Tom Christie <[email protected]> Allow Environment options in `Jinja2Templates` (#1401) Adjust type of `exception_handlers` to allow async callable (#1423) Default WebSocket accept message headers to an empty list (#1422) * If no extra headers are passed, set it to an empty list * Test websocket.accept() with no additional headers * Update starlette/websockets.py Co-authored-by: Marcelo Trylesinski <[email protected]> * Update tests/test_websockets.py Co-authored-by: Amin Alaee <[email protected]> * Update tests/test_websockets.py Co-authored-by: Marcelo Trylesinski <[email protected]> Co-authored-by: Marcelo Trylesinski <[email protected]> Co-authored-by: Amin Alaee <[email protected]> Add reason to WebSocket closure (#1417) Co-authored-by: Marcelo Trylesinski <[email protected]> Version 0.18.0 (#1380) * Version 0.18.0 * Add changes until 14 jan * Update release-notes.md * Update release-notes.md * Update release-notes.md * Update release-notes.md Co-authored-by: Adrian Garcia Badaracco <[email protected]> Co-authored-by: Amin Alaee <[email protected]> draft Update publish.yml (#1430)

tomchristie · 2022-01-25T10:51:50Z

Okay - neat starting point.

We'll need to expand on the actual motivation here, and the desired behavioural changes, but lemme first jump in with some review points.

starlette/responses.py

tomchristie

Okay, so my expectations around the changes we'd want to see are:

The status code becomes "defaults to either 200 or 204" rather than "defaults to 200".
Omitting the content or passing content=None defaults to "don't render anything, body is b''" rather than "render None and return that as the body".

We've probably also got a follow up tweak that we'd want to not set any content-type in that case, but let's take this step by step.

starlette/responses.py

tests/test_responses.py

tomchristie · 2022-01-25T11:28:58Z

Okay, see whatcya think about that.

Kludex · 2022-01-25T11:36:27Z

The status code becomes "defaults to either 200 or 204" rather than "defaults to 200".

What about 307?

tomchristie · 2022-01-25T11:39:50Z

What about 307?

RedirectResponse continues to use 307 as the default in it's __init__ signature, and life is happy and good.

aminalaee · 2022-01-25T11:40:01Z

@Kludex I think the RedirectResponse passes the status_code=307 to Response so it's overriden.

Co-authored-by: Tom Christie <[email protected]>

aminalaee · 2022-01-25T11:52:42Z

This makes sense now. Thank you for the feedback. I'll take a look at the tests later today.

tests/test_responses.py

t1waz · 2022-01-25T12:21:33Z

hi guys, maybe You should consider my point of view:
I think 200 status code is better as default than 204. 204 is something more just than No Content:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/204

from my expierence 204 should also be used in cases when request was process successfully on some entity. It's inform that it was process and do not need to reload or something (look link above)

200 is more general, more intuitive, for empty responses it's like standard. It is also good when default status code will remain same, not to create confusion for someone new :).

I think it's good idea to stay with is there is not content body should be b''.

Let me now, what You guys think

Kludex · 2022-01-25T12:47:13Z

@aminalaee a humbly remind to add the motivation of this PR on the description 🙏

Kludex · 2022-01-25T22:15:33Z

Flask's Response class defaults to 200, jfyk:

ref.: https://github.com/pallets/werkzeug/blob/b611af2f5bf5f447d1d535b45f7c19309b0dc48d/src/werkzeug/sansio/response.py#L92

tomchristie · 2022-01-26T10:30:44Z

Hrm. I'm just not sure here. We're actually bundling up two different but related concerns...

If content=None do we still want to call render() or do we just b"" it?
For empty content do we want to default to 204.

It's possible we should consider those separately.

I'm maybe not that fussed about (2) - it probably ends up being slightly unexpected for some users.

However (1) probably? sounds like a good idea. Practically the behavioural change it makes within Starlette itself is only for the JSONResponse() and JSONResponse(None) cases, where we'll render b"" instead of null. Which... I think is probably better. Returning null is just. Well.. let's not help folks to do that.

Having said that, we'd also want to adapt init_headers to not include a content type header on those cases. Because we're not including any content. So "JSONResponse()" is a bit of a big ol' fib, if it doesn't actually have any content.

Forgetting about (2) for now, I think options are...

Leave things as they are. JSONResponse() can render null. S'all good. Move along folks.
Don't render anything when content=None. return JSONResponse() is actually the same as return Response().
Make the content argument mandatory on JSONResponse(...). (Optional: disallow None)

After reading that through, I think the last one is a pretty neat plan.

Eg...

Response() - An empty response - no content type.
JSONResponse() - Raises an error - missing mandatory argument.
JSONResponse(...) - Exactly what you'd expect.
JSONResponse(None) - Little bit of a silly thing to do, but renders null.

I quite like that approach overall. Doesn't allow me to do things that's ambiguous. Has the behaviour I'd expect in all other cases.

It's a breaking change, but it's only a breaking change for users doing something a bit wrong and marginally broken.

We'd describe it to users in this way...

"If you're currently returning JSONResponse() you now need to either return Response() (which is an empty response), or JSONResponse(None) (which is a JSON response, rendering null as the content)"

…inalaee/starlette into modify-response-class-and-status-code

tomchristie · 2022-01-26T15:59:55Z

This seems better to me, right?

Also - I don't think we really need to deal with auto-defaulting 204 for Response() - let's just leave it up to the user. It might very often not be what they want.

This change on it's own is actually a neat enough bit of guard-railing.

tomchristie

Makes a lot of sense to me, yup.

sondrelg and others added 2 commits January 25, 2022 11:43

Merge branch 'master' into modify-response-class-and-status-code

e9aea82

tomchristie reviewed Jan 25, 2022

View reviewed changes

starlette/responses.py Outdated Show resolved Hide resolved

tomchristie reviewed Jan 25, 2022

View reviewed changes

aminalaee and others added 6 commits January 25, 2022 12:46

Update starlette/responses.py

730f100

Co-authored-by: Tom Christie <[email protected]>

Update starlette/responses.py

065514e

Co-authored-by: Tom Christie <[email protected]>

Update tests/test_responses.py

9fef183

Co-authored-by: Tom Christie <[email protected]>

Update tests/test_responses.py

e02a7fa

Co-authored-by: Tom Christie <[email protected]>

Update tests/test_responses.py

ad565b6

Co-authored-by: Tom Christie <[email protected]>

update

f193ae7

update test

425427f

aminalaee commented Jan 25, 2022

View reviewed changes

tests/test_responses.py Outdated Show resolved Hide resolved

aminalaee changed the title ~~Modify response class and status code~~ Changing Response default status_code to 200 or 204 depending on content Jan 25, 2022

aminalaee changed the title ~~Changing Response default status_code to 200 or 204 depending on content~~ Change Response default status_code to 200 or 204 depending on content Jan 25, 2022

Kludex requested a review from tomchristie January 25, 2022 15:42

Merge branch 'master' into modify-response-class-and-status-code

0cd290c

Merge branch 'master' into modify-response-class-and-status-code

2e1ad11

aminalaee added 3 commits January 26, 2022 11:34

Merge branch 'master' into modify-response-class-and-status-code

b84fcfe

revert status_code change

af3bc9a

Merge branch 'modify-response-class-and-status-code' of github.com:am…

fb6790c

…inalaee/starlette into modify-response-class-and-status-code

aminalaee changed the title ~~Change Response default status_code to 200 or 204 depending on content~~ Make content argument required to JSONResponse Jan 26, 2022

aminalaee added 2 commits January 26, 2022 13:18

remove whitespace

6985784

Merge branch 'master' into modify-response-class-and-status-code

bbd7580

tomchristie approved these changes Jan 26, 2022

View reviewed changes

Merge branch 'master' into modify-response-class-and-status-code

57afede

aminalaee merged commit 5a5a5c3 into encode:master Jan 26, 2022

aminalaee deleted the modify-response-class-and-status-code branch January 26, 2022 16:11

Kludex mentioned this pull request Jan 27, 2022

⬆ Upgrade Starlette from 0.18.0 to 0.19.0 fastapi/fastapi#4488

Merged

paulo-raca mentioned this pull request Mar 2, 2022

Fix test_extra_routes on latest Starlette fastapi/fastapi#4637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `content` argument required to `JSONResponse` #1431

Make `content` argument required to `JSONResponse` #1431

aminalaee commented Jan 25, 2022 •

edited

Loading

tomchristie commented Jan 25, 2022

tomchristie left a comment

tomchristie commented Jan 25, 2022

Kludex commented Jan 25, 2022

tomchristie commented Jan 25, 2022

aminalaee commented Jan 25, 2022 •

edited

Loading

aminalaee commented Jan 25, 2022

t1waz commented Jan 25, 2022

Kludex commented Jan 25, 2022

Kludex commented Jan 25, 2022

tomchristie commented Jan 26, 2022

tomchristie commented Jan 26, 2022

tomchristie left a comment

Make content argument required to JSONResponse #1431

Make content argument required to JSONResponse #1431

Conversation

aminalaee commented Jan 25, 2022 • edited Loading

tomchristie commented Jan 25, 2022

tomchristie left a comment

Choose a reason for hiding this comment

tomchristie commented Jan 25, 2022

Kludex commented Jan 25, 2022

tomchristie commented Jan 25, 2022

aminalaee commented Jan 25, 2022 • edited Loading

aminalaee commented Jan 25, 2022

t1waz commented Jan 25, 2022

Kludex commented Jan 25, 2022

Kludex commented Jan 25, 2022

tomchristie commented Jan 26, 2022

tomchristie commented Jan 26, 2022

tomchristie left a comment

Choose a reason for hiding this comment

Make `content` argument required to `JSONResponse` #1431

Make `content` argument required to `JSONResponse` #1431

aminalaee commented Jan 25, 2022 •

edited

Loading

aminalaee commented Jan 25, 2022 •

edited

Loading