jackson-module-scala icon indicating copy to clipboard operation
jackson-module-scala copied to clipboard

deserialization to case class with required field

Open igreenfield opened this issue 9 years ago • 23 comments

I have this case class:

 case class Time(val a: String, val b: String)

and this json:

 {
    "a" : "12345"
 }

When I serialize this json to the Time class I expect to get serialization error. Something like: "missing required field"

igreenfield avatar May 05 '15 12:05 igreenfield

This is not a bug... Your json doesn't have a 'b' field.

Try:

@JsonIgnoreProperties(ignoreUnknown = true)
case class Time(val a: String, val b: String)

Then 'b' will be null.

Another alternative:

case class Time(val a: String, val b: Option[String])

Then 'b' should be None.

nbauernfeind avatar May 05 '15 15:05 nbauernfeind

I know this is not bug. but I need a way to tell jackson to fail the parse hance this json miss one required field...

igreenfield avatar May 05 '15 15:05 igreenfield

Oh you're not getting an error? I misunderstood; I see the same behavior. This happens even without the scala specifics.

See this example

class T2{
  @JsonProperty val a: String = null
  @JsonProperty val b: String = "missing"
  override def toString = s"$a $b"
}
object T extends App {
  val mapper = new ObjectMapper() with ScalaObjectMapper
  println(mapper.readValue[T2]("{\"a\":\"a\", \"b\":\"b\"}"))
  println(mapper.readValue[T2]("{\"a\":\"a\"}"))
}

Which has output "a b", "a missing". So it's using a [sane] default value (in your example it sets to null).

You used to be able to set @JsonProperty(required = true) but that doesn't seem to throw in the non-scala use case either. Maybe @cowtowncoder or @christophercurrie can chime in on what happened to required properties.

nbauernfeind avatar May 05 '15 16:05 nbauernfeind

Databind has nothing to enforce required properties, information is passed solely as metadata for other processing such as JSON Schema generation.

One way to add checks for missing on plain Java side is to use a creator method, and add check in there. This does not allow distinguishing between explicit nulls and missing values, but if nulls are not legal it can work.

Since case class support is specific to Scala module, however, perhaps it is possible to enforce required-ness for that special case. I am guessing handling is similar to use of creator methods, so information may be available.

In theory Creator method handling could also be improved to catch missing values, more easily than the generic POJO binding. There is also an RFE for general support for enforcing required-ness, but that is unfortunately much bigger technical challenge (has to do with state tracking etc) and unlikely to be implemented on short term.

cowtowncoder avatar May 05 '15 17:05 cowtowncoder

Quick follow-up question: assuming I was able to actually add support for this for Creators (perhaps case class handling just uses creator mechanism), would it:

  1. be ok to require required for all mandatory parameters (more typing, but existing annotations)
  2. be good to add a new property for @JsonCreator, like "requireAll=true", to allow quick override
  3. add a new DeserializationFeature for requiring all creator parameters for everything, by default?

or some combination thereof? I understand that for your use case you just want them all to be mandatory, but since creator functionality is widely used there are other existing preferences. So I want to try to minimize problems caused by changing behavior, by requiring little bit of explicit definitions.

Option (2) would actually also allow case class handling to use default of "yes, require all", regardless of @JsonCreator annotation, I think. So there's that.

cowtowncoder avatar May 05 '15 18:05 cowtowncoder

I don't personally have a strong preference on mechanism; I suspect most users would want a global config, at least, as well as per-creator or per-parameter control, but I have no evidence either way.

christophercurrie avatar May 05 '15 18:05 christophercurrie

I am thinking that "all three" makes sense, based on historical track record... I tend to end up having to add global/local-override layers if I do not start with them. And it is not that much more work realistically.

What I can do is file an issue for databind, for adding (1) first. If that is feasible, other options should be easy to add.

cowtowncoder avatar May 05 '15 18:05 cowtowncoder

Added:

https://github.com/FasterXML/jackson-databind/issues/781

cowtowncoder avatar May 05 '15 18:05 cowtowncoder

That sounds like a good plan. I didn't realize that required was only documentation.

nbauernfeind avatar May 05 '15 19:05 nbauernfeind

@nbauernfeind It was... sort of speculative addition. I had the best intention to try to support it, but implementation is unfortunately quite more complicated than it would appear. So at the moment it is just for documentation.

cowtowncoder avatar May 05 '15 21:05 cowtowncoder

As for Scala I would prefer that the Option type will mark the not required fields and all other field would be required.

igreenfield avatar May 06 '15 05:05 igreenfield

@cowtowncoder I am quite surprised about your comment regarding the required attribute. Can I please ask that if issue #781 mentioned above is not solved in the next version - at least update documentation to state it is not implemented?

yairogen avatar May 06 '15 06:05 yairogen

@yairogen This is documented:

http://fasterxml.github.io/jackson-annotations/javadoc/2.5/com/fasterxml/jackson/annotation/JsonProperty.html#required%28%29

although we can always modify description.

@igreenfield the problem is not the interpretation of what properties should be required, but rather that there is nothing to enforce it.

cowtowncoder avatar May 06 '15 17:05 cowtowncoder

I implemented #781 for 2.6.0, so that required check is now honored for Creator (constructor) properties. If case class handling uses creator property construction (which I think is the case, but I may be wrong), it should then use similar requirement rules.

I'll work with @christophercurrie to try to make sure this also works with case class handling.

cowtowncoder avatar May 07 '15 06:05 cowtowncoder

@cowtowncoder I test it with 2.6.0-rc2 version get the same problem.

igreenfield avatar Jul 05 '15 06:07 igreenfield

Thanks for implementing #781. I still have a problem I'm not sure how to solve. If I enable DeserializationFeature.FAIL_ON_MISSING_CREATOR_PROPERTIES with this class:

case class CustomerCreateCmd(name: String, email: Option[String] = None) 

Users are forced to set the email field. If i disable the option, I have another problem: users can skip the name field, which will become a null value in the Case class (I would expect an error). Ideally, I'd like to make non-Option fields required. I've tried this, with no success:

CustomerCreateCmd(name: String, @JsonProperty(value = "email", required = false) email: Option[String] = None)

Any hints?... Thank you,

sesponda avatar Sep 07 '15 06:09 sesponda

@sesponda Right now, there's no explicit support for what you're trying to achieve. The only way I can think of to solve the issue is to rewrite your class:

case class CustomerCreateCmd(name: String)
{
  var email: Option[String] = None
}

If you enable FAIL_ON_MISSING_CREATOR_PROPERTIES, then name will be required, but email will not.

At this time, there's no support for default values for constructor parameters, and supporting them will be tricky until 2.12 is released; this would be a prerequisite to handling this case more gracefully.

christophercurrie avatar Oct 14 '15 15:10 christophercurrie

@igreenfield I agree with your goals, but FAIL_ON_MISSING_CREATOR_PROPERTIES has specific semantics to core Jackson, so the Scala module will need to implement it's own feature to support this use case. If core Jackson ever implements similar behavior for Java 8's Optional, then there's a chance of re-use there.

christophercurrie avatar Oct 14 '15 16:10 christophercurrie

I'm trying to discuss with the jackson team how to get this feature implemented or a hook that we can hook into. If anyone would like to help me explain the situation to them please add your comments here: https://github.com/FasterXML/jackson-databind/issues/988

NTCoding avatar Oct 29 '15 07:10 NTCoding

I set mapper.configure(DeserializationFeature.FAIL_ON_MISSING_CREATOR_PROPERTIES, true), define case class with optional field and if this field is missing in json I still have an exception. If you program in functional style this scala plugin is useless until this get fixed. Am I not right?

bosyi avatar Jun 29 '18 11:06 bosyi

I solved this issue with DeserializationFeature.FAIL_ON_NULL_CREATOR_PROPERTIES

Vistritium avatar Nov 03 '22 13:11 Vistritium

jackson-module-scala supports default values in case class constructors since v2.11.0 (#87). There have been subsequent changes too, so I would encourage anyone who needs this support to upgrade to latest version.

pjfanning avatar Nov 03 '22 14:11 pjfanning

When using latest jackson-module-scala versions:

  • you can set defaults in case class constructor
  • or you can enable DeserializationFeature.FAIL_ON_NULL_CREATOR_PROPERTIES on the mapper to have an exception thrown if a required JSON field is missing (or null)
    • I have updated the README.md to recommend that users set this DeserializationFeature - https://github.com/FasterXML/jackson-module-scala#deserializationfeaturefail_on_null_creator_properties

I added these 2 tests: https://github.com/FasterXML/jackson-module-scala/commit/e701b211b5e7556116b73ee93f99c2a5f6a45e68

I'm not sure it makes sense to change the existing behaviour.

  • this default behaviour is long established and some users may depend on it
  • if you don't like the default behaviour, you have 2 and possibly other solutions you can use

pjfanning avatar Nov 03 '22 14:11 pjfanning