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

Difference detected between akka-http_2.11-10.0.1 and akka-http_2.12-10.0.1 #691

Closed
troypayne opened this issue Dec 27, 2016 · 8 comments · Fixed by #699
Closed

Difference detected between akka-http_2.11-10.0.1 and akka-http_2.12-10.0.1 #691

troypayne opened this issue Dec 27, 2016 · 8 comments · Fixed by #699
Assignees
Labels
3 - in progress Someone is working on this ticket bug t:marshalling
Projects
Milestone

Comments

@troypayne
Copy link

I'm seeing a bug in 10.0.1 for 2.11 that doesn't exist for 2.12. That is, unicode characters aren't properly being encoded/decoded. For example emoji.

This was a bug that existed in 10.0.0 that I reported, but was then fixed for 10.0.1... however it's still happening for scala 2.11.

Has anyone seen this or know what's going on? It's blocking a critical release for us and would like to get to the bottom of it sooner than later. Using 2.12 isn't an option for us right now as other dependencies aren't yet compatible with it.

Here's what my build.sbt looks like:
https://gist.github.com/troypayne/7da72ae6bd43363f13ea277d2927f098

Here are the results of sbt > show dependencyClasspath
https://gist.github.com/troypayne/13a3e1a6e8d6450f10355333ecc0d672

@jonas
Copy link
Member

jonas commented Dec 27, 2016

I got the echo example you referred to in #564 to work with the following patch:

diff --git a/akka-http-marshallers-scala/akka-http-spray-json/src/main/scala/akka/http/scaladsl/marshallers/sprayjson/SprayJsonSupport.scala b/akka-http-marshallers-scala/akka-http-spray-json/src/main/scala/akka/http/scaladsl/marshallers/sprayjson/SprayJsonSupport.scala
index 0faee1f..f71c0ab 100644
--- a/akka-http-marshallers-scala/akka-http-spray-json/src/main/scala/akka/http/scaladsl/marshallers/sprayjson/SprayJsonSupport.scala
+++ b/akka-http-marshallers-scala/akka-http-spray-json/src/main/scala/akka/http/scaladsl/marshallers/sprayjson/SprayJsonSupport.scala
@@ -36,7 +36,7 @@ trait SprayJsonSupport {
   implicit def sprayJsValueUnmarshaller: FromEntityUnmarshaller[JsValue] =
     Unmarshaller.byteStringUnmarshaller.forContentTypes(`application/json`).mapWithCharset { (data, charset) ⇒
       val input =
-        if (charset == HttpCharsets.`UTF-8`) ParserInput(data.toArray)
+        if (charset == HttpCharsets.`UTF-8`) new SprayJsonByteStringParserInput(data.compact)
         else ParserInput(data.decodeString(charset.nioCharset))
       JsonParser(input)
     }

This ensures JSON entities are unmarshalled using the decoding fix added in f7a41a0. Note however that I am not familiar with this part of the code base so caveat emptor. Spray-JSON support is only two files so if this is a major blocker you could essentially include the two files in your application until this gets fixed in Akka HTTP.

A couple of observations:

  • Commit f7a41a0 fixes sprayJsonByteStringUnmarshaller but has no effect when decoding JSON entities with entity(as[EchoRequest]) which (for the echo example) goes through sprayJsValueUnmarshaller.
  • The echo example works for Scala 2.12 because Spray JSON 1.3.2 is different from the Scala 2.11 version due to the inclusion of spray/spray-json@0c9b2fe merged in June 2015 and the 2.12.0 final release of 1.3.2 made in November 2016 (spray/spray-json@537cb5b). To resolve this, a new Spray-JSON release would be needed.

@jonas jonas added 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted t:marshalling labels Dec 27, 2016
@jonas
Copy link
Member

jonas commented Dec 27, 2016

Regarding the first point, I will let someone else from the @akka/akka-http-team decide whether this should be fixed.

As for the last point, and the motivation for this ticket, I suggest you create a request for a new Spray JSON release in the spray/spray-json project.

@troypayne
Copy link
Author

@jonas can you provide instructions used to apply the patch?

As for a more permanent fix, I'm not exactly sure what to do over at Spray-JSON. Can you perhaps paraphrase my request, things are getting a little bit cloudy for me with all the moving parts. Thanks!

@troypayne
Copy link
Author

@jonas nm. I figured out how to patch it and published locally and got your patch to work. Thanks so much, this will allow me to release while they fix this problem.

@jonas
Copy link
Member

jonas commented Dec 28, 2016

@troypayne Great that you figured it out. I didn't have much time for following up yesterday, but I went ahead and created a ticket to track the Spray-JSON versioning issue.

@jrudolph
Copy link
Member

Great analysis, @jonas. Actually, I think the right solution is to release a new version of spray-json without any changes that brings artifacts for all Scala versions to the latest state.

@sirthias
Copy link
Contributor

Yes, right. I screwed up the 1.3.2 release for Scala 2.12 when I published it from master rather than the respective commit for the 2.11 artifact. Sorry about that!
I agree with @jrudolph in that a fresh and proper release for both Scala versions is probably the best solution.

@jrudolph
Copy link
Member

@sirthias it might also have been me who made the final release, btw. At least it was me who coined the tag: https://github.com/spray/spray-json/releases/tag/v1.3.2-2.12

Anyway, I'm already in the process of releasing a new version from the current state + another small refactoring that would allow us to reuse UTF-8 decoding stuff directly from spray-json without having to copy code to the spray-json marshaller here.

jrudolph added a commit to jrudolph/akka-http that referenced this issue Dec 30, 2016
jrudolph added a commit to jrudolph/akka-http that referenced this issue Dec 30, 2016
Changes:
 * upgrade to spray-json 1.3.3
 * use new IndexedBytesParserInput to avoid copied code from spray-json
 * refactor unmarshallers to base all on a single implementation
jrudolph added a commit to jrudolph/akka-http that referenced this issue Dec 30, 2016
Changes:
 * upgrade to spray-json 1.3.3 (which has working 4-byte UTF-8 character
   decoding for all Scala versions)
 * use new IndexedBytesParserInput to avoid copied code from spray-json
 * refactor unmarshallers to base all on a single implementation
 * fix tests to actually test more kinds of UTF-8 characters
@jrudolph jrudolph self-assigned this Dec 30, 2016
jrudolph added a commit to jrudolph/akka-http that referenced this issue Dec 30, 2016
Changes:
 * upgrade to spray-json 1.3.3 (which has working 4-byte UTF-8 character
   decoding for all Scala versions)
 * use new IndexedBytesParserInput to avoid copied code from spray-json
 * refactor unmarshallers to base all on a single implementation
 * fix tests to actually test more kinds of UTF-8 characters
@jonas jonas added this to the 10.0.2 milestone Dec 31, 2016
@jrudolph jrudolph added 3 - in progress Someone is working on this ticket bug and removed 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted labels Jan 2, 2017
jrudolph added a commit to jrudolph/akka-http that referenced this issue Jan 2, 2017
Changes:
 * upgrade to spray-json 1.3.3 (which has working 4-byte UTF-8 character
   decoding for all Scala versions)
 * use new IndexedBytesParserInput to avoid copied code from spray-json
 * refactor unmarshallers to base all on a single implementation
 * fix tests to actually test more kinds of UTF-8 characters
@jrudolph jrudolph added this to Done in Bug Hunting Feb 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - in progress Someone is working on this ticket bug t:marshalling
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants