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

Alternative product formats #93

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

rditerwich
Copy link

The alternative product formats support the following:

  • Use default values defined on case-classes when fields are missing from json
  • Omit empty arrays and objects when serializing fields with default values
  • Allow property renaming
  • Override formats per field
  • Allow advanced customizations

The formats can be obtained by using formatN instead of jsonFormatN

It supersedes my previous pull request #56

@jdevelop
Copy link

Any of this to be merged into master soon?

@sirthias
Copy link
Member

Ruud, thanks for this, from the looks of it it looks like a useful addition.
However, we currently don't have any capacity for spray-json related work, so this PR might have to sit some time longer.
However, we have scheduled a discussion with the other Typesafe teams (Akka / Play) regarding a general joint strategy for JSON support and expect this to get things moving again...

Sorry for not reacting earlier!

@sirthias
Copy link
Member

@rditerwich I'd like to get that merged and shipped with 1.3.0.
However, you have to accept the Typesafe CLA first (just a few clicks). Would that be possible for you?

@sirthias
Copy link
Member

Also, the patch doesn't merge cleanly anymore.
Could you rebase on top of the current master?

@rditerwich
Copy link
Author

This is great news. I will accept the Typesafe CLA and rebase in the next couple of days.

@fzakaria
Copy link

I'd love to see this in!

@jamesmorgan
Copy link

+1 for getting this in if possible :)

@bvolders
Copy link

plz pull in master

@diversit
Copy link

diversit commented Feb 5, 2015

What is the status? Can this be merged?

@sirthias
Copy link
Member

sirthias commented Feb 6, 2015

I guess it can.
We'll merge it when we cut the next minor release, which will hopefully be soon.

@mustajarvi
Copy link

Any time plan for this one? Would be great to have.

@acruise
Copy link

acruise commented Apr 27, 2015

I would really, really like this functionality too. I have a lot of custom JsonFormat definitions that are 95% boilerplate, but with a small amount of special handling for a minority of properties.

@acruise
Copy link

acruise commented May 11, 2015

Could this be broken out into a tiny separate library, or does it need to live in the spray.json namespace? :)

@fbjon
Copy link

fbjon commented May 22, 2015

There is a problem with the way this is implemented:

If the case class contains "additional fields" (i.e. some inner case class, a lazy val etc.), then the automatic field names don't work.
Normally that would mean using the "jsonFormat" method and specifying fields names manually but I can't find such a method for this alternative product format.

I've fixed it in our project by getting a TypeTag for P in ProductFormatImpl, getting the names of the constructor parameters from that tag and then filtering the declaredFields by name. This should eliminate one reason for the formatN-methods not working:

val tpe = ru.typeOf[P]
def termsOnly(s:ru.Symbol) = if(s.isTerm) Some(s.asTerm) else None

...

// list all case class constructor parameters
val constructor = tpe.members
   .filter(_.isMethod).map(_.asMethod).find(_.isConstructor).toSeq
val constructorParams = constructor
   .flatMap(_.paramss.head).flatMap(termsOnly)

// Filter declared fields based on constructor param names
val fields = runtimeClass.getDeclaredFields
   .filter(field => constructorParams.exists(_.name.toString == field.getName))

There's probably a nice way of doing even more of the work with just the TypeTag, but this works for me for now.

@ghost
Copy link

ghost commented Jun 15, 2015

Nice improvement, +1 for getting this merged.

@ksilin
Copy link

ksilin commented Jul 21, 2015

+1 for sane optional values

@merlijn
Copy link

merlijn commented Feb 10, 2016

+1, would be really helpfull

@aditanase
Copy link

Any idea if this is still on the table? It's open for more than 2 years now.

@mboggan
Copy link

mboggan commented Jun 22, 2016

+1, this would be helpful

@shlushchanka
Copy link

+1, when this will be merged?

@ktoso
Copy link
Member

ktoso commented Oct 7, 2016

Hi there, I recently gained rights to maintain this repository and release etc from @sirthias.
Will try to get on top of issues and PRs soon. This one was waiting a long time, but hope we can get it in perhaps now :)

@ktoso ktoso added the Feature label Oct 7, 2016
@ktoso ktoso self-assigned this Oct 7, 2016
@zhangcbrian
Copy link

+1 for merging this in...

@mtomas
Copy link

mtomas commented Dec 20, 2016

+1 for merging

@greenhost87
Copy link

please release this 👍

@poohsen
Copy link

poohsen commented Feb 1, 2017

@ktoso I see you started merging in some of the newer PRs. That's great news! Do you have any idea when you will be able to look at this one?

@poohsen
Copy link

poohsen commented Mar 6, 2017

No updates on this?

@zzzhy
Copy link

zzzhy commented Mar 21, 2017

any progress on this ?

@firehooper
Copy link

Can this get merged?

@fmeeuw
Copy link

fmeeuw commented Jul 7, 2017

+1

@saraiva132
Copy link

Please do merge!

@craffit
Copy link

craffit commented Aug 8, 2017

+1

@jyotman
Copy link

jyotman commented Sep 1, 2017

Please merge!

@aednichols
Copy link

+1

1 similar comment
@kittsville
Copy link

+1

@@ -19,28 +20,22 @@ as depicted in this diagram:
### Installation

_spray-json_ is available from the [repo.spray.io] repository.
The latest release is `1.2.6` and is built against Scala 2.9.3, Scala 2.10.4 and Scala 2.11.0-RC4.
The latest release is `1.3.0` and is built against Scala 2.10.4 and Scala 2.11.2.
Copy link
Member

@jrudolph jrudolph Oct 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove patch versions as they shouldn't matter (i.e. "2.10.x" and "2.11.x" and "2.12.x" and "2.13.0-M2"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed that in #239

Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @rditerwich and sorry for keeping this around for years without doing anything about it. I like the idea a lot but we need to be careful about compatibility.

To make it happen we need:

  • a subset of this PR which only implements the new ProductFormats without any other changes, especially, binary incompatible ones
  • ProductFormats2 should be sbt-boilerplate generated
  • All the extra features need to be documented in the README and with ScalaDoc
  • We need to decide if jsonFormatN vs formatN is the right way of naming things
  • Would be nice to see some benchmarks comparing previous and new ProductFormats performance

I think it would be a pretty awesome feature to have so let us know @rditerwich if you would still like to make it happen or if something else should try to get it to the finish line.

def write(list: List[T]) = JsArray(list.map(_.toJson))
def read(value: JsValue) = value match {
case JsArray(elements) => elements.map(_.convertTo[T])
def write(list: List[T]) = JsArray(list.map(_.toJson).toVector)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these changes in here need to be performance verified. It would be easier if they'd be broken out into an extra PR.

implicit def immLinearSeqFormat[T :JsonFormat] = viaList[imm.LinearSeq[T], T](list => imm.LinearSeq(list :_*))
implicit def immSetFormat[T :JsonFormat] = viaList[imm.Set[T], T](list => imm.Set(list :_*))
implicit def vectorFormat[T :JsonFormat] = viaList[Vector[T], T](list => Vector(list :_*))
implicit def immIterableFormat[T :JsonFormat] = viaSeq[imm.Iterable[T], T](seq => imm.Iterable(seq :_*))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add explicit return types to each of these (check with mima that they don't change).


/**
* A JsonFormat construction helper that creates a JsonFormat for an Iterable type I from a builder function
* List => I.
*/
def viaList[I <: Iterable[T], T :JsonFormat](f: List[T] => I): RootJsonFormat[I] = new RootJsonFormat[I] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a public method so it needs a proper deprecation cycle.

@@ -49,20 +49,18 @@ sealed abstract class JsValue {
*/
case class JsObject(fields: Map[String, JsValue]) extends JsValue {
override def asJsObject(errorMsg: String) = this
def getFields(fieldNames: String*): Seq[JsValue] = fieldNames.flatMap(fields.get)
def getFields(fieldNames: String*): immutable.Seq[JsValue] = fieldNames.flatMap(fields.get)(collection.breakOut)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a binary compatibility problem? If yes, needs deprecated forwarder.

@@ -72,7 +71,7 @@ class JsonParser(input: ParserInput) {
// http://tools.ietf.org/html/rfc4627#section-2.2
private def `object`(): Unit = {
ws()
var map = ListMap.empty[String, JsValue]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will probably lead to incompatible semantic change.

case c => sb.append(c); rec(ix + 1, lineStartIx, lineNr)
}
val savedCursor = _cursor
_cursor = 0
_cursor = -1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a bug fix?

sb.append(callback).append('(')
print(x, sb)
sb.append(')');
}
sb.append(')')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -23,9 +23,18 @@ import scala.util.control.NonFatal
* Provides the helpers for constructing custom JsonFormat implementations for types implementing the Product trait
* (especially case classes)
*/
trait ProductFormats extends ProductFormatsInstances {
trait ProductFormats extends ProductFormatsInstances with ProductFormats2 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's a good idea to extend here? I guess it makes it easier to deprecate the old formats at some point.


import scala.reflect.ClassTag

trait ProductFormats2 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep using sbt-boilerplate for code generation.

@ktoso ktoso removed their assignment Oct 24, 2017
@giorgiovinci
Copy link

I'm in need of the feature and I just went through all the history (from 2013) and now I'm arrive at the end of this one and I feel like hanging at the end of one of game of throne book!
So @jrudolph you asked @rditerwich and I can guess he said there are other priorities but maybe you can just let us know if there's any plan about this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.