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

Support for default values. #56

Closed
wants to merge 5 commits into from
Closed

Support for default values. #56

wants to merge 5 commits into from

Conversation

rditerwich
Copy link

With this commit, arguments of case classes that have default value specified (expressions actually) can be ommitted from input json documents when deserializing. Serializing behaviour has not changed. Unit tests are succesful.

@jrudolph
Copy link
Member

Interesting. Can you provide a more detailed description about how this works? Are there any impliciations for type-safety? What's the expected runtime performance?

@rditerwich
Copy link
Author

I expect no significant runtime performance degradation. There is a little
bit extra reflection involved, but most of it only when creating jsonFormats (that are
usually cached anyway using lazy vals). This happens in the method
extractFieldNames. For each field, a function that can calculate a default
value is stored, if the field has a default value. During deserialization,
and only if a field is not found in the incoming json object, this function
is called. This call is a java-reflection call, but the Method instance is
cached. It's nice that scala uses default-value expressions (methods)
instead of static values. This allows me to do things like this
(Id.generate generates a new UUID each time):

case class Customer(id : Id[Customer] = Id.generate, name : String,
invoices : List[Invoice] = Nil)

When providing this json document:

{ "name" : "John Smith" }

  • converting it using this expression:

    s.asJson.convertTo[Customer]

will result in:

Customer(Id("123-34-45-45"), "John Smith", Nil)

Type safety: it's as type-safe as reflection can be.

@@ -25,6 +25,10 @@ import java.lang.reflect.Modifier
trait ProductFormats {
this: StandardFormats =>

type FieldAndDefault = (String, Option[() => Any])
Copy link
Member

Choose a reason for hiding this comment

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

Can you please make this a case class:

case class FieldAndDefault[T](name: String, defaultValue: Option[() => T])

This way you should be able to remove casting below.

Copy link
Author

Choose a reason for hiding this comment

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

You're right, much nicer. I've renamed it to Field.

@jrudolph
Copy link
Member

Thanks, Ruud, for answering the questions. This looks good to me. Could you add your answers to the commit message in short form?

@rditerwich
Copy link
Author

I actually think that we should make this behavior optional since it can have far-stretching consequences. Think of a PUT REST operation, supplying only some fields. This might have the undesirable effect of resetting fields in a database. I've implemented this now with a boolean parameter on jsonFormatN methods.

@iwein
Copy link

iwein commented May 21, 2013

👍

@jrudolph
Copy link
Member

One remaining thing: can you add documentation for this feature to the corresponding section of the README? I think we are then ready to go.

Your change doesn't merge cleanly any more because of another change I made. Please just pretend it still does, I will do the merge work in the merge commit when this is otherwise finished.

@nefilim
Copy link

nefilim commented Nov 25, 2013

when will this be released? could really use this functionality, thanks!

@iwein
Copy link

iwein commented Feb 18, 2014

Is spray-json being abandoned… Guys?

@sirthias
Copy link
Member

No, it's not abandoned.
It just isn't a priority at this point.

@rditerwich
Copy link
Author

I've created a new pull request (#93) that supports default values and also some other features. I propose to abandon this pull request in favor of #93.

@sirthias
Copy link
Member

Closing as requested

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

Successfully merging this pull request may close these issues.

5 participants