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

Added default value support #439

Merged
merged 1 commit into from
Dec 7, 2019

Conversation

jroper
Copy link
Member

@jroper jroper commented Dec 5, 2019

Fixes #87

@jroper
Copy link
Member Author

jroper commented Dec 5, 2019

The build failure occurs on the 2.11 branch without this change. Should this change be made to the 2.11 branch? I didn't make it on the master branch because that doesn't even compile. Would the 2.10 branch be better?

@jroper jroper force-pushed the default-value-support branch from f83bb10 to 2fb5d63 Compare December 5, 2019 03:59
@jroper
Copy link
Member Author

jroper commented Dec 5, 2019

One thing about this change, if you have:

case class Foo(bar: Option[String] = Some("bar"))

And you don't pass bar, you'll get Some("bar"), not None. I could make it so you get None, but not sure if that should be the desired behavior or not.

@pjfanning
Copy link
Member

master branch is for the 3.0 release and it needs plenty of work to get all the tests to pass.

2.11 branch is the best place to start. The build was passing last month but it does seem to be broken now. It might be a few days before I can investigate the issue.

@pjfanning
Copy link
Member

@cowtowncoder do you know of any changes in jackson-databind 2.11.0-SNAPSHOT that could be causing this? The tests were passing a month ago and no jackson-module-scala have been done.

[info] - should be able to deserialize left with null value *** FAILED ***
[info]   com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot find a Value deserializer for abstract type [reference type, class scala.util.Either<scala.util.Either<java.lang.String,java.lang.String>><[simple type, class scala.util.Either<java.lang.String,java.lang.String>]>]
[info]  at [Source: (String)"["left", null]"; line: 1, column: 1]
[info]   at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:67)
[info]   at com.fasterxml.jackson.databind.DeserializationContext.reportBadDefinition(DeserializationContext.java:1596)
[info]   at com.fasterxml.jackson.databind.deser.DeserializerCache._handleUnknownValueDeserializer(DeserializerCache.java:591)
[info]   at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:148)
[info]   at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:476)
[info]   at com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:4389)
[info]   at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4198)
[info]   at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3205)
[info]   at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3188)
[info]   at com.fasterxml.jackson.module.scala.deser.DeserializerTest.deserialize(DeserializerTest.scala:13)

case std: StdValueInstantiator =>
new ScalaValueInstantiator(std, config, descriptor)
case other =>
throw new IllegalArgumentException("Cannot customise a non StdValueInstantiatiator: " + other.getClass)
Copy link
Member Author

Choose a reason for hiding this comment

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

The Kotlin module does exactly the same thing here, and it's been doing that for 4 years presumably with no problems, so I think we're fine to fail too:

https://github.com/FasterXML/jackson-module-kotlin/blob/3eb66acf2638dfd99a53a3d6e1389844b5caed2e/src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinValueInstantiator.kt#L174

@cowtowncoder
Copy link
Member

@pjfanning Can not think of anything specific that would trigger such a failure, offhand. :-(

@pjfanning
Copy link
Member

pjfanning commented Dec 6, 2019

the existing jackson-module-scala 2.11 branch tests pass when run against jackson-databind 2.10.1

@cowtowncoder I rebuilt various versions of jackson-databind 2.11.0-SNAPSHOT with the code from various days and it appears it is the changes on 2.11 on Nov 9th that have caused the jackson-module-scala tests to fail for the scala Either[L, R] type.

@pjfanning pjfanning merged commit 9d880df into FasterXML:2.11 Dec 7, 2019
@jroper jroper deleted the default-value-support branch December 10, 2019 02:13
@jroper
Copy link
Member Author

jroper commented Dec 10, 2019

Thanks! Should I submit another PR to forward port to master? And is there any chance this could be backported to 2.10.x or will we wait for 2.11?

@pjfanning
Copy link
Member

@jroper this does affect existing users, who might have case classes with default values but rely on the fact that jackson does not support this - could we make the behaviour configurable? If it was configurable, then it could disabled by default in the 2.10 backport and enabled by default in 2.11.

@pjfanning
Copy link
Member

@jroper when ported to master branch, the unit test has never seemed to work - the master branch is for the yet-to-be-released jackson v3. Your code works well in jackson v2. If you have time would you be able to look at the issue in master? The test is currently commented out. The issue could be a mistake I made when porting the code but I can't see anything obvious.

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

Successfully merging this pull request may close these issues.

3 participants