Skip to content

Commit

Permalink
util-jackson: Use field value type as field type for optional fields
Browse files Browse the repository at this point in the history
Problem
When deserializing a case class, we interpret the field
type with the field constructor type. For optional field,
Jackson returns `Option[Object]` as the constructor
type. However, field validations would require a specific
field type in order to run the constraint validator. This
causes an exception to throw when an optional field is
validated via deserializing with Jackson.

Solution
Use the filed value type as the field type for optional field.

Differential Revision: https://phabricator.twitter.biz/D915377
  • Loading branch information
jyanJing authored and jenkins committed Jul 2, 2022
1 parent 9d7852d commit 3b94d2e
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 4 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ Runtime Behavior Changes

* util-jackson: Update Jackson library to version 2.13.3 ``PHAB_ID=D906005``

* util-jackson: Deserialized case classes with validation on optional fields shouldn't throw an error.
``PHAB_ID=D915377``

22.4.0
------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import com.fasterxml.jackson.databind.JsonMappingException
import com.fasterxml.jackson.databind.JsonNode
import com.fasterxml.jackson.databind.MapperFeature
import com.fasterxml.jackson.databind.PropertyNamingStrategies
import com.fasterxml.jackson.databind.PropertyNamingStrategy
import com.fasterxml.jackson.databind.{ObjectMapper => JacksonObjectMapper}
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory
import com.fasterxml.jackson.module.scala.DefaultScalaModule
Expand Down Expand Up @@ -1359,6 +1358,53 @@ class ScalaObjectMapperTest
parse[CaseClassWithOption]("""{"value":null}""") should be(CaseClassWithOption(None))
}

test(
"A case class with an Option[Int] member and validation annotation is parsable from a JSON object with value that passes the validation") {
parse[CaseClassWithOptionAndValidation]("""{ "towing_capacity": 10000 }""") should be(
CaseClassWithOptionAndValidation(Some(10000)))
}

test(
"A case class with an Option[Int] member and validation annotation is parsable from a JSON object with null value") {
parse[CaseClassWithOptionAndValidation]("""{ "towing_capacity": null }""") should be(
CaseClassWithOptionAndValidation(None))
}

test(
"A case class with an Option[Int] member and validation annotation is parsable from a JSON without that field") {
parse[CaseClassWithOptionAndValidation]("""{}""") should be(
CaseClassWithOptionAndValidation(None))
}

test(
"A case class with an Option[Int] member and validation annotation is parsable from a JSON object with value that fails the validation") {
val e = intercept[CaseClassMappingException] {
parse[CaseClassWithOptionAndValidation]("""{ "towing_capacity": 1 }""") should be(
CaseClassWithOptionAndValidation(Some(1)))
}
e.errors.head.getMessage should be("towing_capacity: must be greater than or equal to 100")
}

test(
"A case class with an Option[Boolean] member and incompatible validation annotation is not parsable from a JSON object") {
val e = intercept[jakarta.validation.UnexpectedTypeException] {
parse[CaseClassWithOptionAndIncompatibleValidation]("""{ "are_you": true }""") should be(
CaseClassWithOptionAndIncompatibleValidation(Some(true)))
}
}

test(
"A case class with an Option[Boolean] member and incompatible validation annotation is parsable from a JSON object with null value") {
parse[CaseClassWithOptionAndIncompatibleValidation]("""{ "are_you": null }""") should be(
CaseClassWithOptionAndIncompatibleValidation(None))
}

test(
"A case class with an Option[Boolean] member and incompatible validation annotation is parsable from a JSON object without that field") {
parse[CaseClassWithOptionAndIncompatibleValidation]("""{}""") should be(
CaseClassWithOptionAndIncompatibleValidation(None))
}

test("A case class with a JsonNode member generates a field of the given type") {
generate(CaseClassWithJsonNode(new IntNode(2))) should be("""{"value":2}""")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,10 @@ case class CaseClassWithOverloadedField(id: Long) {

case class CaseClassWithOption(value: Option[String] = None)

case class CaseClassWithOptionAndValidation(@Min(100) towingCapacity: Option[Int] = None)

case class CaseClassWithOptionAndIncompatibleValidation(@Min(100) areYou: Option[Boolean] = None)

case class CaseClassWithJsonNode(value: JsonNode)

case class CaseClassWithAllTypes(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.twitter.util.validation.internal

import org.json4s.reflect.{Reflector, ScalaType}
import org.json4s.reflect.Reflector
import org.json4s.reflect.ScalaType

private[validation] object Types {

Expand Down Expand Up @@ -28,8 +29,20 @@ private[validation] object Types {
else
Reflector.scalaTypeOf(value.getClass)
} else if (scalaType.isOption) {
if (scalaType.typeArgs.nonEmpty) scalaType.typeArgs.head
else Reflector.scalaTypeOf(classOf[Object])
if (scalaType.typeArgs.nonEmpty) {
// When deserializing a case class, we interpret the field
// type with the field constructor type. For optional
// field, Jackson returns Option[Object] as the constructor
// type. However, field validations would require a
// specific field type in order to run the constraint
// validator. So we use the field value type as the field
// type for optional field.
if (scalaType.typeArgs.head.erasure.isInstanceOf[Object])
Reflector.scalaTypeOf(value.getClass)
else scalaType.typeArgs.head
} else {
Reflector.scalaTypeOf(classOf[Object])
}
} else scalaType
}

Expand Down

0 comments on commit 3b94d2e

Please sign in to comment.