Better egonomics when replacing nulls with default values

Question

Better egonomics when replacing nulls with default values

nikclayton opened this issue 10 months ago · 4 comments

Hi. I've read and understand all of #843 and the solutions presented there.

As best as I can tell the ergonomics of those solutions could be improved but require some changes in Moshi, hence this feature request.

I think JsonAdapter should be extended to allow adapters to signal that the default value for the field should be used, because the JSON is invalid in some way (unexpected null, unparseable, etc).

Motivating example

I maintain an Android client for Mastodon server software. The server can respond with the following JSON snippet (embedded in a larger JSON document).

"focus": {
  "x": 0.5,
  "y": 0.9
}

Except, due to server bugs outside my control the server might set one or other of those fields to null. Making changes to the server, or expecting those changes to be rolled out in a reasonable timeframe is unrealistic, so my client has to handle them.

Given this data class:

@JsonClass(generateAdapter = true)
data class Focus(
    val x: Float = 0f,
    val y: Float = 0f,
)

What are my options?

Options

Rename the properties and provide accessors

@JsonClass(generateAdapter = true)
data class Focus(
    @Json(name = "x") val _x: Float?,
    @Json(name = "y") val _y: Float?,
) {
    val x: Float
        get() = _x ?: 0f
    val y: Float
        get() = _y ?: 0f
}

The simplest approach, but does not scale if I have a lot of properties, or a lot of classes that need the same treatment.

It's also possible to accidentally miss a property.

And because Moshi requires the _x and _y properties to be public they show up in IDE autocompletion causing more confusion.

A `DefaultIfNull` adapter

#843 presents several variations on a DefaultIfNull adapter that can be used. I've experimented with them, and:

Some of them are installed in to Moshi when the Moshi instance is constructed. So the field's de/serialisation behaviour is configured very far in the code from where the field is defined. That's an impediment to anyone trying to reason about the behaviour of the code they're looking at.
Some of them use annotations that have to appear on the containing field, e.g.,

@JsonClass(generateAdapter = true)
data class MetaData(
    @DefaultIfNull            // <-- annotation has to appear here
    val duration: Float?,
    val original: Size?,
    val small: Size?,
    val focus: Focus?,
) : Parcelable

// ... more definitions

@JsonClass(generateAdapter = true) // <-- and not here
data class Focus(
    val x: Float = 0f,
    val y: Float = 0f,
)

That's better, but it's still too far from the properties it's actually going to affect.

The futher up the JSON object hierarchy the more fields they might affect by accident -- i.e. fields that might be either absent (where do you want the default value to be used) or null (where you want to retain the null) might be accidentally set to the default value if they were received as an explicit null.

Ideal

Ideally I'd like to be able to write this:

@JsonClass(generateAdapter = true)
data class Focus(
    @DefaultIfNull val x: Float = 0f,
    @DefaultIfNull val y: Float = 0f,
)

That keeps the adapter scoped tightly to the relevant fields and the point they're declared.

That doesn't work because the adapter does not have access to the default values for the fields, so if one of them is null it can't return the correct value.

You could write an adapter that takes a default value, and use it like so:

@JsonClass(generateAdapter = true)
data class Focus(
    @DefaultIfNull(value = 0f) val x: Float = 0f,
    @DefaultIfNull(value = 0f) val y: Float = 0f,
)

But now you have to repeat the default value twice (and I'm not even sure that'll work without creating an annotation for each type, since the type of value can't be Any, and annotations can't be generic over some type T).

Looking at the generated code I don't think this is possible at the moment. The relevant generated FocusJsonAdapter code if I write:

@JsonClass(generateAdapter = true)
data class Focus(
    @DefaultIfNull val x: Float = 0f,
    @DefaultIfNull val y: Float = 0f,
)

is

// ...

public override fun fromJson(reader: JsonReader): Attachment.Focus {
  var x: Float? = 0f
  var y: Float? = 0f
  var mask0 = -1
  reader.beginObject()
  while (reader.hasNext()) {
    when (reader.selectName(options)) {
      0 -> {
        x = floatAtDefaultIfNullAdapter.fromJson(reader) ?: throw Util.unexpectedNull("x", "x",
            reader)
        // $mask = $mask and (1 shl 0).inv()
        mask0 = mask0 and 0xfffffffe.toInt()
      }
      1 -> {
        y = floatAtDefaultIfNullAdapter.fromJson(reader) ?: throw Util.unexpectedNull("y", "y",
            reader)
        // $mask = $mask and (1 shl 1).inv()
        mask0 = mask0 and 0xfffffffd.toInt()
      }
      -1 -> {
        // Unknown name, skip it.
        reader.skipName()
        reader.skipValue()
      }
    }
  }
  reader.endObject()
// ...

Looking at that it's clear that the generated code knows what the default value is, but it's out of reach of the @DefaultIfNull adapter, which can't return anything to signal that the default should be used.

Bonus request

I said

@JsonClass(generateAdapter = true)
data class Focus(
    @DefaultIfNull val x: Float = 0f,
    @DefaultIfNull val y: Float = 0f,
)

is my ideal end goal.

But actually I think this would be more useful if it was then implemented in the existing @Json annotation as a defaultIfNull parameter, so I (and many others, judging from issues in this repo, questions on Stack Overflow, etc) wouldn't have to write my own adapter, and could instead write:

@JsonClass(generateAdapter = true)
data class Focus(
    @Json(defaultIfNull = true) val x: Float = 0f,
    @Json(defaultIfNull = true) val y: Float = 0f,
)

Thanks for reading.

Answer 1 · 2024-02-16T05:19:09.000Z

Thanks for the write up.

I think we can do this with a generic JsonAdapter.Factory that is installed once "globally" on the Moshi instance ahead of Kotlin adapter factory. This would then enable a @DefaultIfNull annotation (or, more accurately, an @IgnoreExplicitNull annotation) on arbitrary properties.

The mechanism by which it would accomplish this is by detecting the annotation on the property of a class and wrapping its normal JsonAdapter. It would parse the names of the JSON object keys which have this behavior. Then, at runtime, it would deserialize the JSON into tree form (a map, in this case), and remove entries whose values are null and whose keys were in the set of those wanting this behavior. Then, it would pass that map to the delegate JsonAdapter which would see the key as absent and cause the default to be used.

The downsides to this are the same downsides as the rest of Moshi's Kotlin support: it requires either even more code generation or the use of Kotlin reflection.

I would be happy to try and build the reflect-based solution as a sample. Although I'm off for the next four days, so it'd be late next week.

I have never done KSP and I'm not eager to break that streak. Someone else would have to do that part, but it would very trivial. You're basically generating a class which contains the JSON keys which are opting into this behavior for a class. Then that class is looked up at runtime and queried for the names.

Answer 2 · 2024-02-16T17:06:05.000Z

I would be happy to try and build the reflect-based solution as a sample. Although I'm off for the next four days, so it'd be late next week.

That would be great. If no one else volunteers I can take a crack at the codegen approach based on your sample.

Answer 3 · 2024-02-23T18:07:23.000Z

Here's a quick take:

public fun main() {
  val moshi = Moshi.Builder()
    .add(IgnoreExplicitNullFactory)
    .add(KotlinJsonAdapterFactory())
    .build()
  val adapter = moshi.adapter(IgnoreTest::class.java)
  val value = adapter.fromJson("""{"one":null,"two":null,"three":null,"four":null}""")
  println(value)
}

@Json
public data class IgnoreTest(
  val one: String? = "one",
  @IgnoreExplicitNull val two: String? = "two",
  @Json(name = "three") val tres: String? = "three",
  @Json(name = "four") @IgnoreExplicitNull val quatro: String? = "four",
)

@Retention(RUNTIME)
@Target(VALUE_PARAMETER)
public annotation class IgnoreExplicitNull

public object IgnoreExplicitNullFactory : JsonAdapter.Factory {
  override fun create(type: Type, annotations: Set<Annotation>, moshi: Moshi): JsonAdapter<*>? {
    if (annotations.isNotEmpty()) return null
    if (type !is Class<*>) return null
    val primaryConstructor = type.kotlin.primaryConstructor ?: return null
    val ignoreNullParameters = primaryConstructor.parameters
      .filter { parameter ->
        parameter.annotations.any { annotation ->
          annotation.annotationClass == IgnoreExplicitNull::class
        }
      }
    if (ignoreNullParameters.isEmpty()) return null

    val ignoreNullKeys = Array(ignoreNullParameters.size) {
      val parameter = ignoreNullParameters[it]
      val jsonAnnotation = parameter.annotations.singleOrNull { annotation ->
        annotation.annotationClass == Json::class
      } as Json?
      jsonAnnotation?.name ?: parameter.name!!
    }

    val delegate = moshi.nextAdapter<Any>(this, type, annotations)
    return IgnoreExplicitNullJsonAdapter(ignoreNullKeys, delegate)
  }
}

private class IgnoreExplicitNullJsonAdapter<T>(
  private val ignoreNullKeys: Array<String>,
  private val delegate: JsonAdapter<T>
) : JsonAdapter<T>() {
  override fun fromJson(reader: JsonReader): T? {
    if (reader.peek() != JsonReader.Token.BEGIN_OBJECT) {
      // We expect a JSON object but got something else.
      // Let the original JsonAdapter throw an appropriate error.
      return delegate.fromJson(reader)
    }
    val jsonValue = (reader.readJsonValue() as Map<*, *>).toMutableMap()
    for (ignoreNullKey in ignoreNullKeys) {
      jsonValue.remove(ignoreNullKey, null)
    }
    return delegate.fromJsonValue(jsonValue)
  }

  override fun toJson(writer: JsonWriter, value: T?) {
    delegate.toJson(writer, value)
  }
}

prints

IgnoreTest(one=null, two=two, tres=null, quatro=four)

Answer 4 · 2024-06-19T15:04:20.000Z

I sent bitcoin to my account but it doesn't go through.