Collection deserializer should create empty collection instead of null (when input is null)

[pjfanning]

• fix added using 31efc10
• this is a temp workaround - Scala 3 has the same fix but implemented another way - Collection deserializer should create empty collection instead of null (when input is null)
• Jackson 2 workaround needed due to JsonDeserializer getNullValue does not match NullValueProvider jackson-databind#5056

[psliwa]

In my opinion this change is unsafe. With this change when DeserializationFeature.FAIL_ON_NULL_CREATOR_PROPERTIES is used and collection value is null then exception is not thrown (https://github.com/FasterXML/jackson-module-scala?tab=readme-ov-file#deserializationfeaturefail_on_null_creator_properties - this documentation is outdated). I would expect exception in such case would be thrown. It is impossible right now to configure jackson to old behaviour - to throw exception when collection value in json is missing or is null. It can be useful in few use cases - eg in my use case I would like to detect null values for collection for unexpected schema changes - it is better to detect such schema changes during deserialization (in api or database) and throw exception rather that use empty collection (which may cause invalid data passed to application logic).

Could this PR be re-considered and above concerns could be taken into account? In my opinion without any feature flag this change is a bug. For java collection behaviour is assigning null values for null json properties and FAIL_ON_NULL_CREATOR_PROPERTIES feature flag works.

[pjfanning]

I have no current plans to revert this. The change is more consistent with the Java collection deserialization.
Jackson is very configurable. It is very pluggable. It is one option to subclass the deserializer in this module and to modify the behaviour in the subclass.

[pjfanning]

@psliwa if you are interested in submitting a PR, I will review it - but the PR needs proof that the behaviour in this module for a Scala List differs from how jackson-databind deals with a Java List. And also, Scala developers tend to hate nulls.

Have you considered having an Option[List[T]] in your class so that you can check for null input (which should deserialize as None) and empty input (which should deserialize as Some(List.empty))

[pjfanning]

I've update the README.

[psliwa]

Thanks for your comments. It is easy to check how Java collections handing differs from Scala collections after this merge, I can provide later some snippet to show difference but preparing PR knowing it will be rejected doesn't make sense to me. Java collections handling honor FAIL_ON_NULL_CREATOR_PROPERTIES property, null is deserialized to null. Deserializing null to empty collection by default and without any configuration possibilities in my opinion is strange behaviour even in scala ecosystem. Adding Option[List[???]] also doesn't make sense in such cases because such field is not optional from application level - the best way would be to use FAIL_ON_NULL_CREATOR_PROPERTIES in order to force null check in convenient way. I am not sure how it works in Scala 3 (you mentioned this in PR description) so maybe I don't have full context and this is why for me this behaviour is strange and unsafe.

[pjfanning]

@psliwa I merged #733 and there is a 2.19.1-SNAPSHOT with this change

[psliwa]

Many thanks 🙏 Solution in this new PR looks like good compromise, thanks

[manolama]

Hi @pjfanning. Unfortunately this patch broke an app and use case where I use 3VL behavior to merge configurations. The system has a default config that users can override. If the user configuration omits a collection or has it set as null, then we use the default. If the user explicitly wants an empty collection to override the default, then we'll keep the empty list. The DeserializationFeature.FAIL_ON_NULL_CREATOR_PROPERTIES in 2.19.1-SNAPSHOT doesn't help my use case since it throws Null value for creator property 'collection' (index 1); DeserializationFeature.FAIL_ON_NULL_CREATOR_PROPERTIES enabled.... Switching to an Option[List[?]] is possible but requires extensive refactoring.

The change is more consistent with the Java collection deserialization

I'm a bit confused by this since in Java, 2.19.0 still resolves to a null collection:

record Foo(String name, List<String> collection) { }

@Test
public void testFoo() throws Exception {
  ObjectMapper mapper = new ObjectMapper();
  String json = "{\"name\":\"foo\",\"collection\":null}";
  Foo foo = mapper.readValue(json, Foo.class);
  System.out.println(foo); // Foo[name=foo, collection=null]
}

But in Scala:

case class Foo(name: String, collection: List[String]) {}

test("foo") {
    val mapper = JsonMapper.builder()
      .addModule(DefaultScalaModule)
      //.configure(DeserializationFeature.FAIL_ON_NULL_CREATOR_PROPERTIES, true)
      .build()
    val json = "{\"name\":\"foo\",\"collection\":null}"
    val foo = mapper.readValue(json, classOf[Foo])
    System.out.println(foo) // Foo(foo,List())
  }

Would it be possible to revert this and have a new option to opt-in to this behavior for Scala users?

[pjfanning]

@manolama can't you use Java if you want nulls. Very few Scala developers want them.
I won't revert my changes. One possibility is to introduce a config setting. I might look at this. Another possibility would be for you to create your own CollectionDeserializer and use that instead of this modules one.

For me, the correct Scala solution is for you to use this case class.

case class Foo(name: String, collection: Option[List[String]])

You can then differentiate between collection=None or collection=Some(List()).

[manolama]

Very few Scala developers want them.
I understand the null result is not idiomatic for Scala while Option[List[String]] is, and that's fair. But this is is now a breaking change that can impact a number of jackson-module-scala users. We're lucky we caught with unit tests before deploying. For any other projects that use the default module and depend on null behavior, if they don't have good UTs, they'll likely run into issues in production.

The issue with just switching to case class Foo(name: String, collection: Option[List[String]]) is that our config project has a number of consuming projects expecting case class Foo(name: String, collection: List[String]). Changing the API on our side is non-trivial. We'd have to either update all of those consuming projects or write a case class FooSerdes(name: String, collection: Option[List[String]]) to case class Foo(name: String, collection: List[String]) layer. Not fun.

And yes, I, and other users, can write custom deserializers, but it still a breaking semantic change where a simple opt-in on this minor version update would make idiomatic users happy without impacting existing users. It'd be fine to require an opt-out in Jackson 3.0 since users would already expect breaking changes, particularly with all of the deprecated APIs.

[pjfanning]

Very few Scala developers want them.
I understand the null result is not idiomatic for Scala while Option[List[String]] is, and that's fair. But this is is now a breaking change that can impact a number of jackson-module-scala users. We're lucky we caught with unit tests before deploying. For any other projects that use the default module and depend on null behavior, if they don't have good UTs, they'll likely run into issues in production.

The issue with just switching to case class Foo(name: String, collection: Option[List[String]]) is that our config project has a number of consuming projects expecting case class Foo(name: String, collection: List[String]). Changing the API on our side is non-trivial. We'd have to either update all of those consuming projects or write a case class FooSerdes(name: String, collection: Option[List[String]]) to case class Foo(name: String, collection: List[String]) layer. Not fun.

And yes, I, and other users, can write custom deserializers, but it still a breaking semantic change where a simple opt-in on this minor version update would make idiomatic users happy without impacting existing users. It'd be fine to require an opt-out in Jackson 3.0 since users would already expect breaking changes, particularly with all of the deprecated APIs.

Respectfully, I decline to change the code here.