Fix spray-json unmarshalling #699
Conversation
|
Test FAILed. |
jrudolph
force-pushed
the
jr/w/691-spray-json-UTF-8-decoding
branch
from
948d7c4
to
dd3c33f
Compare
|
Needed to mima around some things... ;) |
akka-ci
added
tested
|
Test PASSed. |
| @@ -70,6 +28,7 @@ final class SprayJsonByteStringParserInput(bytes: ByteString) extends DefaultPar | |||
| StandardCharsets.UTF_8.decode(bytes.slice(start, end).asByteBuffer).array() | |||
| } | |||
|
|
|||
| @deprecated("Not needed any more. Should have been private.", "10.0.2") | |||
| object SprayJsonByteStringParserInput { | |||
| private final val EOI = '\uFFFF' | |||
There was a problem hiding this comment.
Would it make sense to remove the private members?
| implicit def sprayJsonByteStringUnmarshaller[T](implicit reader: RootJsonReader[T]): FromByteStringUnmarshaller[T] = | ||
| implicit def sprayJsValueUnmarshaller: FromEntityUnmarshaller[JsValue] = | ||
| Unmarshaller.byteStringUnmarshaller | ||
| .forContentTypes(`application/json`) |
There was a problem hiding this comment.
If I understand correctly this forces the content type to avoid errors but keeps the charset attribute, which the old code explicitly handled. With the move to use SprayJsonByteStringParserInput is this explicit decoding no longer needed?
There was a problem hiding this comment.
Content type json includes it being UTF-8 actually so this forces that.
There was a problem hiding this comment.
So do we need a test that it will re-encode the original content? I'm mainly concerned that this could introduce a regression in the sense that the original special case must have been put in place for a reason.
There was a problem hiding this comment.
According to http://www.iana.org/assignments/media-types/application/json adding charset parameter should have no effect:
Note: No "charset" parameter is defined for this registration.
Adding one really has no effect on compliant recipients.
So, yes, we might introduce a regression for people relying on non-standard behavior. In reality, it might not be a big deal because almost everyone seems to be on UTF-8 nowadays. It is still possible to create a custom unmarshaller if you need to deal with non-standard data. If it turns out that this is a pain point for lots of people we can still revert to more relaxed behavior.
| } else EOI | ||
| } | ||
| } | ||
| @deprecated("Will be made private.", "10.0.2") |
| * Deprecated in favor of [[unmarshal]]. | ||
| */ | ||
| @deprecated("Use unmarshal instead.", "10.0.2") | ||
| def unmarshall(a: A, ec: ExecutionContext, mat: Materializer): CompletionStage[B] = unmarshal(a, ec, mat) |
| @@ -57,7 +57,7 @@ object Dependencies { | |||
| val alpnApi = "org.eclipse.jetty.alpn" % "alpn-api" % "1.1.3.v20160715" // ApacheV2 | |||
|
|
|||
| object Docs { | |||
| val sprayJson = "io.spray" %% "spray-json" % "1.3.2" % "test" | |||
| val sprayJson = Compile.sprayJson % "test" | |||
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
force-pushed
the
jr/w/691-spray-json-UTF-8-decoding
branch
from
dd3c33f
to
d67300e
Compare
akka-ci
added
validating
|
Test PASSed. |
Changes:
Fixes #691.