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

Case class with Seq parameter default = Nil and not supported, deserialized to null #87

Closed
kfiron opened this issue Jun 24, 2013 · 58 comments

Comments

@kfiron
Copy link

kfiron commented Jun 24, 2013

Given case class:

case class Cat(name : String, kittens: Seq[Cat] = Nil)

Read in test:

mapper.readValue("{"name":"kozmo"}", classOf[Cat])

Expected:

expected cat with Nil kittens, however it is null. it seems like SeqModule does not check for the default value if the parameter does not provieded

@christophercurrie
Copy link
Member

This is true, currently; default values behave in interesting ways in Scala. IIRC, the default value itself is encoded as a method of the class, and the compiler supplies the method call when the parameter is omitted in the source.

Getting this right means improving reflection support to find the default value methods and matching them up to the parameters. Definitely on my road map, but the road is long. I'll keep this open for updates as I have them.

@christophercurrie
Copy link
Member

As an added note, SeqModule wouldn't be responsible for this, as it's only responsible for deserializing the sequence from the JSON; this is more of an infrastructure feature for "bean" deserialization.

@mattkrae34
Copy link

What would be the best place to hack in support for default values? Assuming I don't care about the performance hit associated with the reflection referenced above.

@christophercurrie
Copy link
Member

I'd start looking at ScalaValueInstantiator, and seeing if you can plug something into there. One thing I'm not sure about is whether core Jackson will even let you get that far, if you don't have all the property values you need. If not, you'll have to go further up the chain, probably around ScalaClassIntrospector, and see how you'd need to supply information about the presence of default values to Jackson.

Now that 2.9 support will be dropped, there will likely be rework on introspection to use Scala reflection rather than the current Java reflection + paranamer setup, so be aware that there may be changes coming in this space. I can't predict if I'll get to default values quickly, though.

@rbuckland
Copy link

I have a complete hack to use in your code base - which solves two problems I was having.
Problem 1 - this - default values are not used from the case class
Problem 2 - Jackson cannot see properties from super classes.

https://groups.google.com/forum/#!topic/jackson-user/94J6ws7jGzE

So the hack work around fix for both of these, Jackson will fire off a default ctor if there is one.
So .. by giving the case class a default empty ctor, you can also supply the values there.

sealed abstract class PersonCommand(val timestamp: Long)
case class RegisterNewPerson( override val timestamp: Long = DateTime.now.getMillis,
                             firstname: String,
                             surname: String,
                             emailAddress: String,
                             campusOfRegistration: String,
                             dob: Option[LocalDate]) extends PersonCommand(timestamp)  {
  def this() = this(DateTime.now.getMillis,null,null,null,null,None)
}

@rbuckland
Copy link

brought comment over to this live issue
@mattkrae34 said:

Just started looking at the issue today as I was trying to move from 2.9 and a branched/patched > Jerkson. Even in 2.10.X it looks like a bit of pain to get from the prop to the associated default > value method:

http://stackoverflow.com/questions/14034142/how-do-i-access-default-parameter-values-via-scala-reflection

Currently looking at JsonDeserializer getNullValue method I think that's what hits if the keys are > omitted from the JSON as above.

I will take a look tomorrow ast the above Matt, thanks - I recall looking into deriving the default values once for something else.

@vaedama
Copy link

vaedama commented Nov 11, 2014

@christophercurrie I have a PR that's almost ready for this issue. Just wanted to give you a heads-up so you can prioritize your things.

@christophercurrie
Copy link
Member

@vaedama Cool; be aware that it's a change that would go into 2.5, and introspection is likely to change significantly in that version; depending on the nature of the change there may be some rework. Feel free to submit the PR, but just a heads up on what's coming.

vaedama pushed a commit to vaedama/jackson-module-scala that referenced this issue Nov 12, 2014
@vaedama
Copy link

vaedama commented Nov 12, 2014

@christophercurrie Please take a look at: #170

@christophercurrie christophercurrie modified the milestones: 2.4, 2.6 Nov 26, 2014
bkempe pushed a commit to Lulo/jackson-module-scala that referenced this issue Jan 27, 2015
…not always return the empty default constructor first, which leads to the workaround in FasterXML#87 breaking in Ubuntu
@yairogen
Copy link

I also have the same issue. When will this fix be released?

@christophercurrie
Copy link
Member

There is no fix currently ready for this issue. #170 made a very strong start at it, but had some issues that needed resolving before it could be merged. I don't have a timeline for when this issue might be resolved, as it requires either (a) abandoning Scala 2.10, which does not have thread-safe reflection, or (b) re-implementing the Scala reflection support for detecting and invoking default parameter value functions. Neither of these is a quick fix.

@yairogen
Copy link

I would branch and support in 2.11 although still open in 2.10.

It will at least enable some users to benefit from this fix.

Better than nothing, but that's just my 2 cents.

@cowtowncoder
Copy link
Member

Branching is tempting from user perspective, but amount of work needed makes it less than ideal for people who actually support the project. This is especially because we have maintenance branches for old versions; and since Scala's backwards compatibility has been quite challenging this would just add one more dimension to the puzzle.

So branching may not be worth the cost here, unfortunately.
Although maybe there's a way to do multi-module project for "legacy" version of 2.10, to avoid multi-dimensional git branches.

@yairogen
Copy link

I see you point. Is the multi-module option something you can do? I assume that as the time passes more and more 2.11 users will be out there and can benefit from this.

@christophercurrie
Copy link
Member

This might be possible, it's not an option I had considered. I'll look into it to see how difficult it might be. At earliest it will be for Jackson 2.6, as it would probably require some changes to the dependency structure.

@yairogen
Copy link

Better 2.6 than later. I appreciate your efforts. Thanks. Looking forward for an update.

@vkuptcov
Copy link

vkuptcov commented Mar 3, 2016

Recently I found a nice library, which could help https://github.com/mesosphere/jackson-case-class-module/

@normana400
Copy link

is there an update on this? seems like this issue has gone dormant for about a year, but its a pretty basic blocking feature for anyone wanting to use jackson with scala.

@mancvso
Copy link

mancvso commented Mar 31, 2016

In 2.7.1 Scala module still does not support default values.

@cowtowncoder
Copy link
Member

I'm pretty sure there would be an update if support for default values was added, either by Scala runtimes implementing this in more easily supportable form, or by workaround on jackson scala module.

@mancvso
Copy link

mancvso commented Apr 22, 2016

How can it be in a more easily supportable form?
We can hack up a SIP (Scala Improvement Process) document if it's needed.

@nicojs
Copy link

nicojs commented Jan 17, 2017

@cowtowncoder could you provide an update on this? What are the time lines on dropping scala 2.10 support and releasing jackson 2.9?

@cowtowncoder
Copy link
Member

@nicojs for update, please bring this up on dev mailing list. No new discussions regarding scala compatibility. Also, not much activity wrt module, no new development.

On Jackson 2.9: was hoping to get rc1 out by january. Not happening, so release no sooner than March 2017. Perhaps rc1 late february if things go well.

@nicojs
Copy link

nicojs commented Jan 18, 2017

Do you mean this list? https://groups.google.com/forum/#!forum/jackson-dev

@cowtowncoder
Copy link
Member

@nicojs Yes, this is related more to development of Jackson (otherwise jackson-user), so I think that is correct.

@talbenshabtay
Copy link

talbenshabtay commented Apr 26, 2017

still failing with version 2.8.7
tried @dotta solution but it seems jackson response is non-deterministic i get a roughly 33%
chance that default values will be set.

i hacked a solution using json4s .

implicit val formats = DefaultFormats
...
def read[T](str:String) = parse(str).extract[T]

def parse(str: String) = JsonMethods.parse(str)

build.sbt adding the following
"org.json4s" %% "json4s-native" % "3.5.1",
"org.json4s" %% "json4s-jackson" % "3.5.1"

@kylejmcintyre
Copy link

kylejmcintyre commented Jun 19, 2017

Big warning on the def this() workaround: You must add the @JsonCreator annotation to that constructor, otherwise you may see non-deterministic deserialization behavior across process runs on certain JVM/system configurations (I've confirmed it on Hotspot JVM/Java 8/Mac OSX but believe it's also happening on Hotspot/Java 8/Linux). See #159 for more information.

@alain-marcel
Copy link

May scala 2.13 help to solve the issue, or dotty 0.16.0-RC3 ?

@plokhotnyuk
Copy link

@alain-marcel As W/A use jsoniter-scala - it deserializes properly default values for all supported types including Scala collections. Also it is much safe and efficient in runtime.

@jroper
Copy link
Member

jroper commented Dec 5, 2019

Presumably, this should be trivial to implement, just override AnnotationIntrospector.findPropertyDefaultValue. If the class the property is on is a Scala case class, and there is no default value declared for the property using a JsonProperty annotation, and there is a synthetic method that provides the default value (this can be determined just by understanding how Scala generates the bytecode for default values), then invoke that method to get the default value.

@jroper
Copy link
Member

jroper commented Dec 5, 2019

So, AnnottaionIntrospector.findPropertyDefaultValue wouldn't work, because it just returns String, and is only for documentation purposes - Jackson databind doesn't use it at all. So, once FasterXML/jackson-modules-base#23 is implemented, this feature can be implemented.

@cowtowncoder
Copy link
Member

Kotlin module (https://github.com/FasterXML/jackson-module-kotlin) actually supports this, if anyone wants to have a look; I think it is via

src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinValueInstantiator.kt

and uses ValueInstantiator extension point (possible coupled with getNullValue(...) of JsonDeserializer to deal with things like "absent" Option instead of null)

@jroper
Copy link
Member

jroper commented Dec 5, 2019

lol, I just found the ValueInstantiator myself and implemented it in #439.

@jroper
Copy link
Member

jroper commented Dec 5, 2019

I like my impl better, it overrides getFromObjectArguments to add a NullPropertyProvider to any CreatorProperty that is backed by a constructor parameter that has a default value.

@cowtowncoder
Copy link
Member

Sounds like reasonable approach, and great improvement for Scala module in 2.11.

Just one request: if a link to this issue can be added on release notes page:

https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.11

that'd be great (and whichever file(s) repo has -- I think there is separate changes/release file)

@jroper
Copy link
Member

jroper commented Dec 10, 2019

This can be closed now that #439 is merged.

@jroper
Copy link
Member

jroper commented Dec 10, 2019

@cowtowncoder I don't have access to edit the wiki, I'm in the FasterXML org but probably not in the right team.

@cowtowncoder
Copy link
Member

@jroper Ah. I added you to Scala team, but it won't give access to main Jackson repo. I think I'll need to create a different team for that.

But I think release-notes/VERSION.md should be fine, I can merge entries from there on short term.

@pjfanning
Copy link
Member

will be in 2.11.0 release

@wix-andriusb
Copy link

When will the 2.11.0 release happen?

@pjfanning
Copy link
Member

when jackson-databind 2.11.0 is released -- can be tracked at https://github.com/FasterXML/jackson-databind/blob/master/release-notes/VERSION-2.x

@cowtowncoder
Copy link
Member

@wix-andriusb in typical OSS fashion it will be released "when it's ready", planning is not based on specific date goals. Having said that the first release candidate for 2.11 could be by early February, that is, in couple of weeks. And actual release before end of February 2020.

@mley
Copy link

mley commented Jul 7, 2020

I came across this issue while updating our stack to Spring Boot 2.3 (which comes with jackson 2.11 by default). We are making heavy use of Scala case classes. Using the default-value for deserialising breaks a lot of stuff at our end.

In this example it's very confusing, since the JSON value is explicitly set to null:

import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.module.scala.DefaultScalaModule
import org.scalatest.{FlatSpec, Matchers}

case class Test(value: String = "the value")

class JacksonSpec extends FlatSpec with Matchers {
  val objectMapper = new ObjectMapper().registerModule(DefaultScalaModule)

  "Test.value" should "be deserialized correctly" in {
    objectMapper.readValue("""{"value":null}""", classOf[Test]).value shouldBe null
  }
}

This spec is ok with jackson 2.10.4, but fails with 2.11. Is there a way to disable this feature?

@pjfanning
Copy link
Member

pjfanning commented Jul 7, 2020

@mley wouldn't it be better to use case class Test(value: Option[String]) if you need to support null?

@mley
Copy link

mley commented Jul 8, 2020

@pjfanning You are right, the given test is not very idiomatic. Here is a better example, which should prove my point:

import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.module.scala.DefaultScalaModule
import org.scalatest.{FlatSpec, Matchers}

case class Test(value: Option[String] = Some("the default"))

class JacksonSpec extends FlatSpec with Matchers {
  val objectMapper = new ObjectMapper().registerModule(DefaultScalaModule)

  "Test.value" should "be deserialized correctly" in {
    objectMapper.readValue("""{"value":null}""", classOf[Test]).value.isEmpty shouldBe true
    objectMapper.readValue("""{}""", classOf[Test]).value shouldBe Some("the default")
    objectMapper.readValue("""{"value":"the value"}""", classOf[Test]).value shouldBe Some("the value")
  }
}

With Jackson 2.10.4 deserialising {} results in value being null (as the feature is not yet implemented).
With Jackson 2.11 deserialising {"value":null} results in value having the default value.

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

No branches or pull requests