Issue by 2m
_Monday Jun 15, 2015 at 07:17 GMT_
_Originally opened as https://github.com/akka/akka/issues/17716_
As reported in the mailing list akka-http is not able to handle a request with an empty body when route expects an optional entity.
Even an empty string is not a valid json it should still be possible to handle such a request when expected entity is optional.
Comment by jrudolph
_Monday Jun 15, 2015 at 09:36 GMT_
The issue on the mailing list was that
entity(as[Option[DeleteUserBody]])
returns a 400 response with this message:
The request content was malformed:
JSON terminates unexpectedly.
The underlying issue is what to expect for the above entity line. This is statically decided by the choice of the implicit unmarshaller.
In this case a JSON unmarshaller was chosen which depends on the body being well-formed JSON (which an empty entity isn't). A JSON unmarshaller for T is always available if a JsonFormat for T is available which is because we define a generic JsonFormat for Option[T] in spray-json. However, this works on a JsValue and not on the plain input string so it cannot be fixed on that level.
Instead, you could also lift a JSON unmarshaller for DeleteUserBody using GenericUnmarshallers.targetOptionUnmarshaller which would already do the right thing. So, the question here is how to disambiguate the two cases and make sure the right unmarshaller is chosen.
/cc @sirthias
Comment by sirthias
_Monday Jun 15, 2015 at 14:51 GMT_
That is indeed a good question. Basically we'd want to deprioritize DefaultJsonProtocol.optionFormat over GenericUnmarshallers.targetOptionUnmarshaller in this case.
However, because of the old-school way that spray-json defines its infrastructure implicits the DefaultJsonProtocol.optionFormat is usually explicitly imported, which puts it into a higher implicit priority bracket than the targetOptionUnmarshaller.
So, unfortunately, there doesn't appear to be a clean and good solution.
The best solution would probably be to remove the DefaultJsonProtocol.optionFormat in spray-json altogether (as it is quite limited anyway) and replace it with something better where is really needed (which is defining JsonFormats for case class hierarchies). However, we can't do that from akka-http.
The way I see it is that we need to address this via documentation, where we ask people to either
import spray.json.DefaultJsonProtocol.{optionFormat => _, _} in these cases or shadow optionFormat if they mix in the DefaultJsonProtocol trait.
Comment by jrudolph
_Tuesday Jun 16, 2015 at 08:18 GMT_
Would adding a local alias for the targetOptionUnmarshaller help? Maybe if you write
implicit val optionsAsEmptyUnmarshaller = Unmarshallers.targetOptionUnmarshaller[HttpRequest, DeleteUserBody]
this unmarshaller would have higher priority than any otherwise imported ones?
Comment by sirthias
_Tuesday Jun 16, 2015 at 10:08 GMT_
I haven't checked but that may provoke an implicit ambiguity if I import DefaultJsonProtocol._ instead of mixing it in as a trait.
Comment by divijan
_Monday Jun 22, 2015 at 15:48 GMT_
I was going to raise an issue that seems a sub-issue of this one. I define my own JsonSupport in the same vein as SprayJsonSupport is defined. I would like Entity Expected error message instead of
The request content was malformed:
...
when no body is provided. This also holds for non-optional entity routes. Can this be made a default behavior on the lowest level Unmarshallers, like byteStringUnmarshaller? Judging from MarshallingDirectivesTest in akka-http-tests, the user is expected to handle empty entities by hand, which, to my mind, should be handled automatically
Comment by sirthias
_Monday Jun 22, 2015 at 18:24 GMT_
@divijan If you write your JSON support yourself then you should simply reject with a RequestEntityExpectedRejection if no request entity is present.
Comment by ymeymann
_Wednesday Mar 16, 2016 at 15:48 GMT_
I ended up doing
extractRequest { req =>
val action = (opt: Option[MyEntityType]) => ???
if (req.entity.contentLengthOption.contains(0L)) {
action(None)
} else {
entity(as[MyEntityType]) { e =>
action(Some(e))
}
}
}
Is it a reasonable workaround?
Comment by bandrzejczak
_Thursday Aug 11, 2016 at 14:42 GMT_
@ymeymann You can enclose it into your own directive:
def optionalEntity[T](unmarshaller: FromRequestUnmarshaller[T]): Directive1[Option[T]] =
extractRequest.flatMap { req =>
if (req.entity.contentLengthOption.contains(0L)) {
provide(Option.empty[T])
} else {
entity(unmarshaller).flatMap(e => provide(Some(e)))
}
}
Comment by bandrzejczak
_Thursday Aug 11, 2016 at 16:02 GMT_
Actually I've just checked and it doesn't work for empty body. What I've actually used and tested is:
def optionalEntity[T](unmarshaller: FromRequestUnmarshaller[T]): Directive1[Option[T]] =
entity(as[String]).flatMap { stringEntity =>
if(stringEntity == null || stringEntity.isEmpty) {
provide(Option.empty[T])
} else {
entity(unmarshaller).flatMap(e => provide(Some(e)))
}
}
}
Seems hackish, but I haven't found better way to check whether the body is empty. entity.isKnownEmpty() doesn't work well for this case.
Comment by genarorg
_Monday Aug 29, 2016 at 21:14 GMT_
how about just providing an alternate route if you get a rejection from the entity directive:
entity(as[SomeModel]) { model =>
complete(OK, "a request body was received")
}
~ complete(OK, "no request body received")
I tried the solution from @bandrzejczak and it works perfectly for me, I think this should be part of akka http
I also used the solution from @bandrzejczak after much searching around.
I do not recommend the solution from @bandrzejczak. When receiving traffic from slower connections it can read an incomplete post body and then attempt to parse it. This can result in intermittent JSON parsing errors.
@morsecodist Are you sure? Why would it not wait for the entire string to be ready?
Most helpful comment
_Thursday Aug 11, 2016 at 16:02 GMT_
Actually I've just checked and it doesn't work for empty body. What I've actually used and tested is:
Seems hackish, but I haven't found better way to check whether the body is empty.
entity.isKnownEmpty()doesn't work well for this case.