michaelf-crwd commented on code in PR #584:
URL: https://github.com/apache/pekko-http/pull/584#discussion_r1718262612
##########
http-tests/src/test/scala/org/apache/pekko/http/scaladsl/server/directives/MarshallingDirectivesSpec.scala:
##########
@@ -275,4 +275,23 @@ class MarshallingDirectivesSpec extends RoutingSpec with
Inside {
}
}
}
+
+ "The marshalling infrastructure for text" should {
+ val foo = "Hällö"
+ "render text with UTF-8 encoding if no `Accept-Charset` request header is
present" in {
+ Get() ~> complete(foo) ~> check {
+ responseEntity shouldEqual HttpEntity(ContentType(`text/plain`,
`UTF-8`), foo)
+ }
+ }
+ "render text with requested encoding if an `Accept-Charset` request header
requests a non-UTF-8 encoding" in {
+ Get() ~> `Accept-Charset`(`ISO-8859-1`) ~> complete(foo) ~> check {
+ responseEntity shouldEqual HttpEntity(ContentType(`text/plain`,
`ISO-8859-1`), foo)
+ }
+ }
+ "render text with UTF-8 encoding if an `Accept-Charset` request header
requests an unknown encoding" in {
+ Get() ~> `Accept-Charset`(HttpCharset("unknown")(Nil)) ~> complete(foo)
~> check {
+ responseEntity shouldEqual HttpEntity(ContentType(`text/plain`,
`UTF-8`), foo)
+ }
+ }
Review Comment:
Hi,
Thanks for having a look at this bug / feature request. I recently sent in a
duplicate issue (https://github.com/apache/pekko-http/issues/583) unaware of
this one. I worked on a similar PR the past two days, and have now found and
looked through your changes - that would seem to fix it nicely! Great.
For your consideration, I'd like to suggest adding in an extra unit-test in
"MarshallingSpec.scala" with something along the lines of:
```
val invalidAcceptCharsetHeader =
`Accept-Charset`(HttpCharsetRange(HttpCharset.custom("invalid")))
val request = HttpRequest().withHeaders(Seq(invalidAcceptCharsetHeader))
marshalToResponse("Hello", request).entity.contentType.charsetOption should
be(Some(HttpCharsets.`UTF-8`))
```
Some refactoring thoughts; Perhaps the try-catch in
"PredefinedToEntityMarshallers.scala" could be more subtly placed in
"HttpCharset.scala" as:
```
/** Returns this HttpCharset instance if the provided nio charset is
available or a desired default otherwise */
def withNioCharsetOrElse(default: HttpCharset): HttpCharset = {
if (_nioCharset.isSuccess) {
// Return this instance, as the provided nioCharset did not result in
a java.nio.charset.UnsupportedCharsetException
this
} else {
default
}
}
```
and then have the "PredefinedToEntityMarshallers.scala" do:
```
Marshaller.withOpenCharset(mediaType) { (s, cs) => {
HttpEntity(mediaType.withCharset(cs.withNioCharsetOrElse(HttpCharsets.`UTF-8`)),
s)
} }
```
Looking forward to seeing your changes merged.
Thanks again and best regards,
Michael
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
