Issue #4719 Keep previous charencoding if not set in content type #4720
Conversation
Signed-off-by: Jan Bartel <janb@webtide.com>
janbartel
added
Specification
There was a problem hiding this comment.
Questions about this one.
Does this preservation persist through dispatches?
What about error processing? does the charset persist there as well?
How does someone remove the charset on the response's Content-Type after the fact now?
What happens if the user uses HttpServletResponse.reset() after the charset has been set?
|
@joakime the change doesn't affect any of those things. All it does, is ensure that setContentType() without a char set doesn't change the existing value of the character encoding. As Request has setCharacterEncoding() method and a setContentType() method, the char set can be independent of the content-type. Previously we were treating the content-type and char-set as always inextricably linked, even if setContentType() didn't explicitly pass in a char-set (we used the defaults from MimeType). |
| @@ -624,7 +635,7 @@ public void testResetContentTypeWithCharacterEncoding() throws Exception | |||
|
|
|||
| response.setContentType("wrong/answer;charset=utf-8"); | |||
| response.setContentType("foo/bar"); | |||
| assertEquals("foo/bar", response.getContentType()); | |||
| assertEquals("foo/bar;charset=utf-8", response.getContentType()); | |||
There was a problem hiding this comment.
Does this get sent over the wire on HTTP/1.1 as ...
Content-Type: foo/bar; charset=utf-8
?
If so, then this PR is either doing too much, or we lack the extra tests to prove that we allow a user to turn off that charset= portion on the HTTP response.
There was a problem hiding this comment.
This change won't force a change to the charset: in fact, the change prevents a change to the charset that that spec does not support. Unfortunately, the javadoc for ServletResponse.setContentType() does not make this behaviour clear in the case of the content-type without char-set, so all we have to go on is the tck test: https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/src/com/sun/ts/tests/servlet/api/common/response/ResponseTests.java#L680
There was a problem hiding this comment.
I created a new branch (from this one) called jetty-10.0.x-4719-keepcharencoding-extratests with new testcases to show the concern (see commit e0b322f).
Two of the new testcases in ResponseHeadersTest are now failing ...
- testCharsetChangeToJsonMimeType()
- testCharsetChangeToJsonMimeTypeSetCharsetToNull()
Interesting aside, I didn't know you could use
HttpServletResponse.reset()after obtaining agetWriter()and still use that writer.
There was a problem hiding this comment.
@joakime I've merged your branch into mine, and added a fix for content types like json that must have an assumed char set. Thanks for the concrete tests!!
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Minor formatting and more comments. Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
…ssumed Signed-off-by: Jan Bartel <janb@webtide.com>
Closes #4719