Moshi: Support nullability in type parameters like List<String?>

[konmik]

@JsonClass(generateAdapter = true)
class Child()

class Parent(val child: Child?)

moshi.adapter(Parent::class.java).toJson(Parent(null))

Caused by: java.lang.NullPointerException: value was null! Wrap in .nullSafe() to write nullable values.
at ChildJsonAdapter.toJson(ChildJsonAdapter.kt:42)
at ChildJsonAdapter.toJson(ChildJsonAdapter.kt:13)
at com.squareup.moshi.ClassJsonAdapter$FieldBinding.write(ClassJsonAdapter.java:203)
at com.squareup.moshi.ClassJsonAdapter.toJson(ClassJsonAdapter.java:172)
at com.squareup.moshi.JsonAdapter$2.toJson(JsonAdapter.java:144)
at com.squareup.moshi.JsonAdapter.toJson(JsonAdapter.java:52)
at com.squareup.moshi.JsonAdapter.toJson(JsonAdapter.java:58)

[NightlyNexus]

The adapter for Parent don't know about its property's nullability unless you install the reflective Kotlin adapter or generate a Kotlin adapter for Parent.

[konmik]

I understand this.

The issue is that parent's runtime adapter makes assumption on child's adapter nullability. We should probably add an annotation to solve the issue, add default behavior or something like that.

[NightlyNexus]

The default runtime adapter is made for Java and doesn't know about the nullability of its fields. seems like @JsonClass(generateAdapter = true) class Parent(val child: Child?) is the right solution here for Kotlin.

What are you proposing?

[konmik]

Solution 1: @JsonClass(generateAdapter = true, nullable = true)

Solution 2: @field:Json(name = "child", nullable = true)

Solution 3: when generic serializer finds a generated adapter for a field it marks field as nullable by default.

[remcomokveld]

I am also running into this when trying to parse a List<Parent?>

I think creating all generatedAdapters with nullSafe would be okay for this right? if the child may not be null kotin will throw the NPE when the parent class' constructor is being called.

There is one problem with this approach, and that is that List<Parent> is no longer guaranteed to only contain non nulls, but I would think that that needs to be handled in special CollectionJsonAdapter for kotlin collections.

Edit: The problem with a nonnull list being able to contain nulls also exists when not generating a jsonadapter so I would assume feature/bug -parity there

[ZacSweers]

@remcomokveld have an example snippet? In general I'm kind of inclined to agree with @NightlyNexus' answer - @JsonClass annotated classes work best if everything they touch or touches them has the same level of kotlin nullability/mutability/etc understanding that they do

[remcomokveld]

The problem I am running into now is that I have a list which can contain null items.

With Kotlin's nullability features this should be possible to express by having

@JsonClass(generateAdapter = true)
data class Item(val itemId: String)

Now let's say you have a graphql API, which gives this response

{
  "data": [
    { "itemId": "SomeID" },
    null,
    { "itemId": "OtherId" }
  ]
}

This should be possible to represent in kotlin like this

data class GraphQlResponse(
  val data: List<Item?>
)

Right now this is not possible. I think it should be the responsibility of the List JsonAdapter to determine if the typeparameter is nullable, thus if the adapter should be nullSafe.

But then having to wrap the adapter with nullSafe seems like unneeded runtime overhead, since that check can just as well be done in generated code.

[swankjesse]

Actions to take here:

• [ ] Codegen: If we have a Kotlin type like List<String?> we should permit null elements.
• [ ] Codegen: If we have a Kotlin type like List<String> we should forbid null elements.
• [ ] Reflection: If we have a Kotlin type like List<String?> we should permit null elements.
• [ ] Reflection: If we have a Kotlin type like List<String> we should forbid null elements.

[zakayothuku]

Any progress on this?

[ZacSweers]

No one is currently working on this

[dkonurov]

@ZacSweers @swankjesse may be it's bad idea but I add annotations for nullable and nonnull values in collectionAdapter and support it and add support nonnull or nullable values for kotlin json adapter and codegen in kotlin. #1045 May be this is bad idea pls tell me :-) May be need support annotation for array and map?

[jkaan]

Hey guys :). I could try to make a PR for this. Basically it should just be supported, right? No additional annotations or anything, I assume?

[swankjesse]

You're welcome to try but I believe it's difficult to do without major changes to Moshi’s internal type model.

[jkaan]

Okay, that's good to know. I guess this is not a good issue then to start working on to get acquainted with Moshi :).

[dkonurov]

@swankjesse what do you think about add Nullable Type and NonNull Type in Moshi, because Java doesn't know any more about Null safety. as @ZacSweers said we can wrap Kotlin Type but Java doesn't know nothing about KType and cannot resolve this. If we wanna resolve KType we have to copy code from Java to Kotlin. I think we can resolve in Java and add Wrap Type like OptionalType or NullableType in Kotlin. In PR above I add this Type, may be need rename that from OptionalType to NullableType or NonNullType?

[ZacSweers]

said we can wrap Kotlin Type but Java doesn't know nothing about KType and cannot resolve this

I meant if a kotlin rewrite happens, KType would become the internal type mechanism. Java behavior would remain unchanged because Java has no notion of nullability in its types. We could possible parse Nullable type parameter annotations but they would need to be runtime retained for reflection.

I think it could work to expose new collection/map adapter APIs in Types that custom JsonAdapter.Factory or generated adapter implementations could use if they can understand nullability about a target type. That approach works probably today without any overly invasive changes to moshi’s core type machinery.

[dkonurov]

Hm... If I correct understood idea we can save Nullable from Kotlin but need support Nullable or NonNull annotation in Java and add possibility for marked in ParametrizedType actualTypeArguemnts and for GenericArrayType componentType nullable or non null. If In Java need marked non null list. (It's difficult because in Java 7 avoid marked annotation generic type, May be add custom annotation? but i think very useful add Type like Nullable or NonNull like wrapper type :-) )

[dkonurov]

I think many times about this issue, but KType must be used inside method Util.reslove, because some internal type can be annotated nullable like field inside class, and generic type can be annotated nullable like generic type by class. Than we must create new Type, but It will be similar to this pull request: https://github.com/square/moshi/pull/1139 :-(

[ZacSweers]

We're coverting to kotlin and that may lend us more information

[dkonurov]

What do you think about add KotlinType like wrap usual type and save nullable infromation? (https://github.com/square/moshi/pull/1322/files like this) but if it add this type should we detect difference between adapters:

class NoNullAdapter {
  fun fromJson(json: String): Int {
    //....
  } 
}

and

class NullableAdapter {

  fun fromJson(json: String): Int? {
    //....
  }
}

I think it's break changes :-(