jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.86k stars 1.91k forks source link

Use lowercase for charsets #11741 #12347

Closed gregw closed 1 month ago

gregw commented 1 month ago

Fix #11741 as per the WhatTFWG recommendations, use lower case for charset names. Took the opportunity for some minor optimizations:

gregw commented 1 month ago

Strange location for the ContentTypeField.

Not that strange I think. I'll add some javadoc to explain the class and I think that will make the location seam reasonable

Also, what happens if we use (in jetty-core Handler) the following?

response.getHeaders().put(HttpHeader.CONTENT_TYPE, "text/html; charset=UTF-8");

Does that produce a UTF-8? or a utf-8 on the response headers that the client sees?

We will send headers as the app sets them. If they set them with a specific case, then we will send that case. But if they use APIs to setCharset etc. then we should generate as lowercase. This PR is more about improving the parsing side, so that when we find a cached content-type, we return it as lowercase rather than the previous uppercase.

joakime commented 1 month ago

We will send headers as the app sets them. If they set them with a specific case, then we will send that case. But if they use APIs to setCharset etc. then we should generate as lowercase. This PR is more about improving the parsing side, so that when we find a cached content-type, we return it as lowercase rather than the previous uppercase.

Wont this cause a change in how the HttpClient parses responses too?

gregw commented 1 month ago

@sbordet I've made the class package protected. Can we turn this review around quickly, as I was waiting 4 days for a clean CI and now have to go back to square 1.