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

fix(response): avoid duplicated charset #2443

Merged
merged 3 commits into from
Feb 3, 2024

Conversation

mikkelduif
Copy link
Contributor

Summary

When populating the media_type with a MIME-type explicitly indcluding the ; charset= for text/ MIME-types, Starlette would by default append an additional default ; charset=utf-8 to the corresponding headers response Content-Type, which would result in a duplicated charset.
This PR introduces a check if either ; charset= or ;charset= has already been included in the media_type to avoid charset being set twice.

Before:

Response(media_type="text/plain;charset=UTF-8")

Response header Content-Type:
Content-Type: text/xml;charset=UTF-8; charset=utf-8

After:

Response(media_type="text/plain;charset=UTF-8")

Response header Content-Type:
Content-Type: text/xml;charset=UTF-8

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@Kludex
Copy link
Sponsor Member

Kludex commented Jan 24, 2024

This is not an acceptable solution. The media_type isn't supposed to have a charset.

You can do the following:

response = Response()
response.headers['content-type'] = "text/plain;charset=UTF-8"

@Kludex Kludex closed this Jan 24, 2024
@mikkelduif
Copy link
Contributor Author

This is not an acceptable solution. The media_type isn't supposed to have a charset.

You can do the following:

response = Response()
response.headers['content-type'] = "text/plain;charset=UTF-8"

@Kludex no problem.
I guess it must have been due to my misunderstanding of media_type probably being a bad variable name for this if it's not really conceptually a media type.
As I understand it media types per officials specs (https://datatracker.ietf.org/doc/html/rfc6838) does have support for CHARSET. So I would probably consider renaming media_type to be a bit more explicit around this.
I still do believe it is an unexpected behaviour of the framework.

@Kludex
Copy link
Sponsor Member

Kludex commented Jan 24, 2024

I think it's my bad. Hold on. 🙏

Thanks for the reference. I'll check this later.

@Kludex Kludex reopened this Jan 24, 2024
starlette/responses.py Outdated Show resolved Hide resolved
tests/test_responses.py Outdated Show resolved Hide resolved
@mikkelduif
Copy link
Contributor Author

mikkelduif commented Jan 24, 2024

I think it's my bad. Hold on. 🙏

Thanks for the reference. I'll check this later.

@Kludex thanks for the comments, have addressed them.
I believe the only minor detail could be whether to check for charset or charset=. I really don't have a strong opinion on this, however, left it at charset= for now - so it should only be whether it is allowed to specify it as charset = utf-8 which would fail this check (i am unsure whether that would live up to the specs though).

I have additionally pushed a small change to the docs to clarify when the charset is set and when it is not set.

docs/responses.md Outdated Show resolved Hide resolved
@Kludex Kludex enabled auto-merge (squash) February 3, 2024 08:37
@Kludex Kludex merged commit b8eebef into encode:master Feb 3, 2024
5 checks passed
nixroxursox pushed a commit to nixroxursox/starlette that referenced this pull request Mar 18, 2024
* fix(response): avoid duplicated charset

* Update docs/responses.md

---------

Co-authored-by: Marcelo Trylesinski <marcelotryle@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

2 participants