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

@JsonInclude content inclusion does not work #346

Closed
jefftt opened this issue Sep 29, 2017 · 17 comments · Fixed by #362
Closed

@JsonInclude content inclusion does not work #346

jefftt opened this issue Sep 29, 2017 · 17 comments · Fixed by #362
Milestone

Comments

@jefftt
Copy link

jefftt commented Sep 29, 2017

Jackson version 2.9.1

I would like to serialized Some(List.empty) to []

import java.io.StringWriter

import com.fasterxml.jackson.annotation.JsonInclude
import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.module.scala.DefaultScalaModule
import com.fasterxml.jackson.module.scala.experimental.ScalaObjectMapper

case class Test(opt: Option[List[String]] = Some(List.empty))

val mapper = new ObjectMapper with ScalaObjectMapper
mapper.registerModule(DefaultScalaModule)
mapper.setDefaultPropertyInclusion(JsonInclude.Value.construct(JsonInclude.Include.NON_ABSENT, JsonInclude.Include.ALWAYS))


val nonEmptyList = new StringWriter
mapper.writeValue(nonEmptyList, Test(Some(List("hello", "world"))))
nonEmptyList.toString // {"opt":["hello","world"]}

val emptyList = new StringWriter
mapper.writeValue(emptyList, Test(Some(List())))
emptyList.toString // {} should be {"opt": []} !?

val emptyOption = new StringWriter
mapper.writeValue(emptyOption, Test(None))
emptyOption.toString // {}
@jefftt
Copy link
Author

jefftt commented Sep 29, 2017

Willing to work on this if someone could point me in the right direction. Thanks!

@jefftt
Copy link
Author

jefftt commented Sep 29, 2017

Looks like this broke in 2.9

Works fine with 2.8.10 using
mapper.setSerializationInclusion(JsonInclude.Include.NON_ABSENT)

@cowtowncoder
Copy link
Member

@jefftt Ok, so Some should be ReferenceType, and NON_ABSENT should be fine: it'd only exclude "none" case.

What are unexpected outputs for these tests?

@jefftt
Copy link
Author

jefftt commented Sep 29, 2017

Sorry for not being clear:

An example case class:
case class Test(opt: Option[List[String]])

In 2.9.x:
Test(Some(List.empty)) is being serialized to {} however i expect the output to be { "opt": [] }

@cowtowncoder
Copy link
Member

Ok. So for some reason filtering criteria matches in this case.

Standard reference type deserializer:

src/main/java/com/fasterxml/jackson/databind/deser/std/ReferenceTypeDeserializer.java

has implementations for Java 8 and Guava Optionals, as well as AtomicReference.
Scala version probably hasn't been retrofitted since this module hasn't had active developer for quite a while now.

@brharrington
Copy link
Contributor

I hit this issue recently and was looking into a fix. It was relatively straightforward to get it working by changing the OptionSerializer to use ReferenceTypeSerializer (brharrington@f6448e8), basically copying the implementation of OptionalSerializer from the java 8 module and adjusting it a bit for the scala Option class.

However, after making this change three of the other test cases started failing. Specifically the three tests for using JsonTypeInfo with a generic collection that is in the option (L219). It isn't honoring the JsonTypeInfo annotation on the interface:

org.scalatest.exceptions.TestFailedException: "{"fruits":[{"[]color":"green"}]}" was not equal to "{"fruits":[{"[type":"Apple","]color":"green"}]}"

As a sanity check I also added a test to see what the java8 module would do with an Optional for the same scenario (brharrington/jackson-modules-java8@9178ca9) and it also fails honor the type info annotation:

org.junit.ComparisonFailure:
Expected :{"strategy":[{"type":"foo","foo":42}]}
Actual   :{"strategy":[{"foo":42}]}

Any ideas as to how to go about fixing this?

@cowtowncoder
Copy link
Member

Hmmh. I am not 100% sure if collection of polymorphic values via Option(al) is tested properly, so it is possible this could be a subtle bug in ReferenceTypeSerializer. But then again it could also be something within scala module's interaction, possibly within refinement of Optional into JavaType.

One thing I could do regarding first possibility would be if this could be reproduce with jackson-databind -- type that is equivalent in many ways is AtomicReference: it is the only JDK type (before Java 8...) that is considered ReferenceType. So if test could be modified to use AtomicReference, but use polymorphic type, this would either reproduce issue (and I could maybe fix it), or confirm it is something on Scala side.

@brharrington
Copy link
Contributor

I'll try to reproduce with AtomicReference, but the sanity check test case I mentioned for java8 Optional ((brharrington/jackson-modules-java8@9178ca9)) does not use Scala at all. So I don't think this issue is specific to Scala in any way.

@cowtowncoder
Copy link
Member

@brharrington Ok that makes sense. I'd still like test that can be included directly in jackson-databind, for convenience (and quicker catching of regression). In a way it's good problem is there since Scala handling might then work ok once this issue is resolved.

brharrington added a commit to brharrington/jackson-databind that referenced this issue Oct 14, 2017
Based on request on jackson-module-scala issue, here is a
test case showing the problem directly on jackson-databind.

For context: FasterXML/jackson-module-scala#346 (comment)
@brharrington
Copy link
Contributor

@cowtowncoder Here is a test case on jackson-databind showing the same issue with AtomicReference: brharrington/jackson-databind@1549473

@david-convey
Copy link

@brharrington This issue affects us as well; thank you for getting to the bottom of it. It looks like your fix using ReferenceTypeSerializer is the right solution; is it just blocked for now by FasterXML/jackson-databind#1673?

@brharrington
Copy link
Contributor

As far as I know that is the only blocker.

@cowtowncoder
Copy link
Member

@david-convey @brharrington Fwtw I was able to fix the issue on jackson-databind side, for 2.9.4.

There is still #357 that blocks Scala module release, but it would be good if this could be verified from Scala module side.

@brharrington
Copy link
Contributor

@cowtowncoder I confirmed that using a jackson-databind jar built with the fix for #1673 fixes the test cases that were failing.

@cowtowncoder
Copy link
Member

@brharrington Thank you. I will then close this issue, assuming it would work for 2.9.4 of Scala module, once it gets released.

@cowtowncoder cowtowncoder added this to the 2.9.4 milestone Jan 15, 2018
@brharrington
Copy link
Contributor

@cowtowncoder This issue is different and would still need a change (brharrington/jackson-module-scala@f6448e8). But the fix to jackson-databind means that the proposed fix to this issue will not break other use-cases. I can submit a PR once 2.9.4 is used by jackson-module-scala.

@cowtowncoder
Copy link
Member

Ah. So a patch is needed to change Scala Option handler to be based on (now) shared reference-type (de)serializer. Gotcha.

@cowtowncoder cowtowncoder reopened this Jan 16, 2018
brharrington added a commit to brharrington/jackson-module-scala that referenced this issue Jan 28, 2018
Fixes FasterXML#346. Note this also update the jackson version
to 2.9.4 as it has a fix that is needed to avoid breaking
other tests. See FasterXML/jackson-databind#1673 for more
details.
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 a pull request may close this issue.

4 participants