400 JSON parse error when "constant.value=null"

[wajda]

When the execution plan contains a literal with value: null then POST /producer/execution-plans returns 400.

{
  "id": "7c505765-7218-479e-b0b0-c31e6d63d68a",
  "dataType": "3a278371-515e-4090-9790-6b48a757491b",
  "extra": {
    "simpleClassName": "Literal",
    "_typeHint": "expr.Literal"
  },
  "value": null
}

com.twitter.finatra.json.internal.caseclass.exceptions.CaseClassValidationException: expressions.constants.value: field is required

The issue is related to AbsaOSS/spline-spark-agent#200

[wajda]

@dk1844 @AdrianOlosutean

[wajda]

That's a Twitter Finatra Jackson deserializer issue. It seems to require non-null values for non-optional fields. I need to investigate what can we do about it.

[cerveada]

Twitter Finatra Jackson Modul

Yes, Twitter Finatra Jackson requires non-null values for non-optional case class fields. It's also not really made to be configurable. There is an option to define a custom JsonDeserializer, but the null check happens after the deserialization. I don't think there is a way how to turn this off for just one field.

As a workaround it is possible to set a default value to null. That would mean when there is null in input the default value will be used which is a null. The downside may be that this happens even when the json property for this field is missing in the input.

Finatra seems to be useful only for enforcing the mandatory values. The json conversion seems to be working without it, but this should be tested better if we decide to stop using it.

Jackson

There is very limited support for enforcing required fields in Jackson. They consider this a validation that should be handled separately.

json4s

Seems to support mandatory fields out of the box, but I don't know how hard is to turn this off for one specific field

conclusion

The Best solution for now seems to be used default null value. If we want more grained control over this we will have to switch to a different Jackson parser or implement some Jackson Module ourselves.

[wajda]

Adding a default value also makes this fiend optional in Swagger, and subsequently the field is treated as such in the client side code generated from the Swagger definitions. Being seemingly a small thing it could create unnecessary questions, confusion or a side effect in a long term perspective. I would prefer having a dirty hack on the server but keeping the API model correct.

What if we fork Finatra and patch it? Perhaps introducing a custom annotation wouldn't be much of a problem there..

[cerveada]

Ok I will fork the Finatra.

One potential issue there is that they seem to make mandatory only case class fields. But since we use case classes everywhere in the API it's not a problem for us right now.