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

JsonCreator annotation should work but does not #447

Closed
devshorts opened this issue Feb 9, 2018 · 4 comments
Closed

JsonCreator annotation should work but does not #447

devshorts opened this issue Feb 9, 2018 · 4 comments
Labels

Comments

@devshorts
Copy link

devshorts commented Feb 9, 2018

The jackson statement on the finatra page states that

Support “wrapped values” using WrappedValue (needed since jackson-module-scala does not support the @JsonCreator annotation).

However, this isn't true. JsonCreator does work with scala:

case class Foo(int: Int)

object Foo {
  @JsonCreator
  def apply(s: String): Foo = Foo(s.toInt)
}

class Tests extends FlatSpec with Matchers {
  "Json Creator" should "deserialize" in {
    val mapper = new ObjectMapper with ScalaObjectMapper

    mapper.readValue[Foo]("1") shouldEqual Foo(1)
  }
}

Expected behavior

The finatra object mapper should respect json creator annotations

Actual behavior

Finatra object mapper doesn't:

case class Foo(int: Int)

object Foo {
  @JsonCreator
  def apply(s: String): Foo = Foo(s.toInt)
}

class Tests extends FlatSpec with Matchers {
  "Json Creator" should "deserialize" in {
    val mapper = FinatraObjectMapper.create()

    mapper.parse[Foo]("1") shouldEqual Foo(1)
  }
}
Can not deserialize instance of tests.Foo out of VALUE_NUMBER_INT token
 at [Source: 1; line: 1, column: 1]
 at [Source: 1; line: 1, column: 3]
com.fasterxml.jackson.core.JsonParseException: Can not deserialize instance of tests.Foo out of VALUE_NUMBER_INT token
 at [Source: 1; line: 1, column: 1]
 at [Source: 1; line: 1, column: 3]
	at com.twitter.finatra.json.internal.caseclass.jackson.FinatraCaseClassDeserializer.incrementParserToFirstField(FinatraCaseClassDeserializer.scala:102)
	at com.twitter.finatra.json.internal.caseclass.jackson.FinatraCaseClassDeserializer.deserializeNonWrapperClass(FinatraCaseClassDeserializer.scala:83)
	at com.twitter.finatra.json.internal.caseclass.jackson.FinatraCaseClassDeserializer.deserialize(FinatraCaseClassDeserializer.scala:67)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:3798)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2880)
	at com.fasterxml.jackson.module.scala.experimental.ScalaObjectMapper.readValue(ScalaObjectMapper.scala:190)
	at com.fasterxml.jackson.module.scala.experimental.ScalaObjectMapper.readValue$(ScalaObjectMapper.scala:189)
	at com.twitter.finatra.json.modules.FinatraJacksonModule$$anon$1.readValue(FinatraJacksonModule.scala:73)
	at com.twitter.finatra.json.FinatraObjectMapper.parse(FinatraObjectMapper.scala:86)

We can work around this by getting the raw request and parsing the body using a standard jackson object mapper, but its a crappy workaround as it forces a non standard access pattern. We could also work around this by adding a custom deserializer module that forces a parse by companion object, but thats also nasty since it should just work out of the box

@cacoco
Copy link
Contributor

cacoco commented Feb 24, 2018

@devshorts thanks for the issue, we'll take a look. Pretty sure this functionality has changed with more recent Jackson upgrades since this code was first written. That is to say, I am not convinced this is intended to work with Finatra as at the time this was written the jackson-scala-module did not support the @JsonCreator annotation and we have not made major updates in this area.

We're working on re-vamping our Jackson integration but in the interim we'll take a look at this. Thanks!

@cacoco
Copy link
Contributor

cacoco commented Feb 24, 2018

@devshorts also...this works -- is it not acceptable to extend WrappedValue in your case?

case class Foo(int: Int) extends WrappedValue[Int]
test("wrapped value") {
  val mapper = FinatraObjectMapper.create()
  mapper.parse[Foo]("1") should be(Foo(1))
}

@devshorts
Copy link
Author

devshorts commented Feb 24, 2018 via email

@cacoco
Copy link
Contributor

cacoco commented Feb 25, 2018

@devshorts ok, understood. I'm glad you have a workaround for now at least. I've added this to the list of issues to address in re-working our Jackson integration. Thanks for the notes.

@cacoco cacoco closed this as completed Mar 3, 2018
finaglehelper pushed a commit that referenced this issue Jun 14, 2019
Problem/Solution

A link to the `@JsonCreator` issue #447 in Github is incorrectly rendered. Let's
fix it.

Differential Revision: https://phabricator.twitter.biz/D328669
@cacoco cacoco added answered and removed in triage labels Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants