hmil / RosHTTP

Unified Scala.js + Scala HTTP client API
MIT License
126 stars 24 forks source link

Requests on node.js fail when content-type / charset header is missing. #35

Closed easel closed 8 years ago

easel commented 8 years ago

Turns out that the js-side parser blows up when no content-type header is provided, such as below. I haven't figured out how to make express produce such output in the tests, but this is the default you'll get using play with a simple BadRequest() response.

HTTP/1.1 404 Not Found
Request-Time: 82
Content-Length: 0
Date: Sun, 14 Aug 2016 20:50:46 GMT

The stack trace looks like this:

[info]   java.nio.charset.UnsupportedCharsetException: binary
[info]   at $c_Ljava_nio_charset_UnsupportedCharsetException.$c_jl_Throwable.fillInStackTrace__jl_Throwable(/Users/erik/Projects/telepath-play/src/shared/client/.js/target/scala-2.11/https:/raw.githubusercontent.com/scala-js/scala-js/v0.6.11/library/src/main/scala/scala/scalajs/runtime/StackTrace.scala:33:5)
[info]   at $c_Ljava_nio_charset_UnsupportedCharsetException.$c_jl_Throwable.init___T__jl_Throwable(/Users/erik/Projects/telepath-play/src/shared/client/.js/target/scala-2.11/https:/raw.githubusercontent.com/scala-js/scala-js/v0.6.11/javalanglib/src/main/scala/java/lang/Throwables.scala:12:19)
[info]   at java.nio.charset.UnsupportedCharsetException.<init>(/Users/erik/Projects/telepath-play/src/shared/client/.js/target/scala-2.11/https:/raw.githubusercontent.com/scala-js/scala-js/v0.6.11/javalib/src/main/scala/java/nio/charset/UnsupportedCharsetException.scala:4:34)
[info]   at java.nio.charset.Charset$.forName(/Users/erik/Projects/telepath-play/src/shared/client/.js/target/scala-2.11/https:/raw.githubusercontent.com/scala/scala/v2.11.8/src/library/scala/collection/MapLike.scala:128:10)
[info]   at scala.scalajs.runtime.RuntimeString$.newString(/Users/erik/Projects/telepath-play/src/shared/client/.js/target/scala-2.11/https:/raw.githubusercontent.com/scala-js/scala-js/v0.6.11/library/src/main/scala/scala/scalajs/runtime/RuntimeString.scala:316:14)
[info]   at fr.hmil.roshttp.HttpResponse.body$lzycompute(/Users/erik/Projects/telepath-play/src/shared/client/.js/target/scala-2.11/file:/Users/erik/Projects/telepath-play/vendor/RosHTTP/shared/src/main/scala/fr/hmil/roshttp/HttpResponse.scala:13:5)
[info]   at fr.hmil.roshttp.HttpResponse.body(/Users/erik/Projects/telepath-play/src/shared/client/.js/target/scala-2.11/file:/Users/erik/Projects/telepath-play/vendor/RosHTTP/shared/src/main/scala/fr/hmil/roshttp/HttpResponse.scala:12:12)
hmil commented 8 years ago

I am afraid this is a byproduct of the heavy refactoring that was done for v1. The body used to be sent in plain text but this made charset encoding/decoding very tricky across platforms, especially for sending binary data. Hence the unused charset variables in all three drivers (you should remove those from BrowserDriver and the jvm driver as well).

In fact, CrossPlatformUtils.oneByteCharset has become irrelevant. All three platform support ISO-8859-1 for decoding so this should be defined as a constant in HttpUtils (serves a similar purpose as CrossPlatformUtils except for shared code) and used in HttpResponse. ~~Unfortunately we can not use 'utf-8' by default as it is not supported by the browser for some strange reasons. If not defaulting to UTF-8 is an issue, the user has access to the raw body and can decode with whatever charset he wants.Or the default charset could be configurable but this should go in a separate feature request if there is a need for it.~~ Actually I don't remember if the issue with UTF-8 was also related to upload. If the tests pass with UTF-8, then we should definitely use UTF-8 as default.

easel commented 8 years ago

Yeah I was going to ask about UTF-8. As far as I am concerned, we should force UTF-8 down everybody's throat and deal with the fallout via configuration. I've been able to get away with that approach fairly successfully for the last few years -- I'd be surprised if we ran into significant issues doing it here too.

hmil commented 8 years ago

we should force UTF-8 down everybody's throat

LMAO that is so true.