search

akka / akka-http

The Streaming-first HTTP server/module of Akka
https://doc.akka.io/docs/akka-http
Other
1.34k stars 594 forks source link

Review decision to require `RootJsonFormat` to marshal from/to Json #282

Open akka-ci opened 7 years ago

akka-ci commented 7 years ago

Issue by jrudolph Thursday Jun 18, 2015 at 15:02 GMT Originally opened as https://github.com/akka/akka/issues/17775

The latest JSON RFC 7159 is less strict than other specifications before:

A JSON text is a serialized value. Note that certain previous specifications of JSON constrained a JSON text to be an object or an array. Implementations that generate only objects or arrays where a JSON text is called for will be interoperable in the sense that all implementations will accept these as conforming JSON texts.

In spray-json the former restriction was modelled by RootJsonFormat which only consumes or produces json arrays or objects. Removing the restriction would make using spray-json with akka-http simpler with the potential disadvantage that we encourage a format that some peers possibly cannot understand.

On the other hand, changing to requiring JsonFormat would mean that suddenly Marshallers to/from most primitive types would be in scope that are likely to interfere with other ones.

/cc @sirthias

akka-ci commented 7 years ago

Comment by jrudolph Thursday Jun 18, 2015 at 15:03 GMT

See also this recent akka-user discussion: https://groups.google.com/d/topic/akka-user/hicTjKXHGeg/discussion

akka-ci commented 7 years ago

Comment by rkuhn Wednesday Jun 24, 2015 at 15:54 GMT

What about adding an implicit that needs to be imported separately that allows the marshalling of JsonFormat?

akka-ci commented 7 years ago

Comment by sirthias Friday Jun 26, 2015 at 07:23 GMT

Yeah, I like the idea of making things configurable with another implict, which you'd have to bring into scope in order to enable the marshalling of non-root JsonFormats.

akka-ci commented 7 years ago

Comment by fommil Sunday Jul 12, 2015 at 10:46 GMT

Cool, just seen this. I have an alternative SprayJsonSupport object that I bring into scope as an alternative when an external API demands non-root objects.

I just wanted to bring something to your attention regarding derived formats that you might not have thought of.

The compiler ignores other container types when doing implicit resolution, so if the user provides a RootJsonFormat[T] and a JsonFormat[T] the compiler treats them as completely independent (and will not flag an ambiguous implicit). If only RootJsonFormat[T] is provided, then it will be searched only after the compiler confirms that there is no JsonFormat[T].

When using automatic derivers, e.g. spray-json-shapeless, RootJsonFormat[T] will be created automatically, but the user could provide a JsonFormat[T] which would silently win this battle only when the JsonFormat[T] is requested. This might confuse some users, because previously the automatically derived format will have won.

It gets even more complicated when users provide a JsonFormat[T] for a case class, because we demand a RootJsonFormat[T] in the trait serialisation and the user's format is ignored entirely.

We have a testcase for this exact point, but it's currently disabled because it doesn't compile under 2.10 (failing to compile is probably the right thing to do!) https://github.com/fommil/spray-json-shapeless/blob/master/src/test/scala/fommil/sjs/FamilyFormatsSpec.scala#L238

akka-ci commented 7 years ago

Comment by ktoso Tuesday Oct 20, 2015 at 12:34 GMT

Marking as triaged, we'll want to do some small improvements to the marshalling infra anyway.