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

Either deserializers Option[T] with value None as null #472

Closed
ptrdom opened this issue Sep 15, 2020 · 4 comments
Closed

Either deserializers Option[T] with value None as null #472

ptrdom opened this issue Sep 15, 2020 · 4 comments
Labels

Comments

@ptrdom
Copy link
Contributor

ptrdom commented Sep 15, 2020

Jackson version 2.10.3.
Code to reproduce:

import com.fasterxml.jackson.module.scala.DefaultScalaModule

val right: Either[Option[String], Option[String]] = Right(None)
val left: Either[Option[String], Option[String]] = Left(None)

val mapper = new ObjectMapper()
mapper.registerModule(new DefaultScalaModule)

val rightString = mapper.writeValueAsString(right)
mapper.readValue(rightString, classOf[Either[Option[String], Option[String]]])

val leftString = mapper.writeValueAsString(left)
mapper.readValue(leftString, classOf[Either[Option[String], Option[String]]])
@ptrdom
Copy link
Contributor Author

ptrdom commented Sep 15, 2020

Apparently this is covered in tests as valid behavior:

it should "be able to deserialize right with null value" in {
deserialize[Either[_, String]](s"""{"r":null}""") should be (Right(null))
deserialize[Either[_, String]](s"""{"right":null}""") should be (Right(null))
}
it should "be able to deserialize left with null value" in {
deserialize[Either[String, String]](s"""{"l":null}""") should be (Left(null))
deserialize[Either[String, String]](s"""{"left":null}""") should be (Left(null))
}

That basically makes Either containing Option not possible. Is this really correct?

@pjfanning
Copy link
Member

@dope9967 those tests are not specifically about Either wrapping Options - this looks like a use case that was not considered and it should ideally be fixed

Would you be able to try producing a Pull Request to fix this? 2.10 will not have any more releases. 2.11 is probably not a good place for a big change. I would suggest targeting the 2.12 branch.

@ptrdom
Copy link
Contributor Author

ptrdom commented Sep 15, 2020

@pjfanning sure, I can give it a try.

@pjfanning
Copy link
Member

should be fixed in 2.12.0-SNAPSHOT now - I will backport to 2.11.3-SNAPSHOT in near future

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

No branches or pull requests

2 participants