Accumulate error strategy

[stojan26]

The KotlinJsonAdapter fails fast when it encounters an error. This mean if there is a problem with the JSON we only get the first error. Of course the benefit of this is we don't parse additional data when we know the whole process is gonna fail.

In some cases it might be useful to accumulate the errors and fail at the end of the whole process. This means more information about everything that is wrong with the data.

Since this is a tradeoff I suggest a new adapter that accumulates errors or a setting in the existing one.

Would you interested in a PR that adds that functionality?

[swankjesse]

I don‘t think we can do this well without adding significant complexity or performance costs. We might be able to tell you locally about all of the fields in a single class, but even that is awkward.

[stojan26]

Good point. When I wrote this I was thinking about locally - in a single class. The way auto-value does it.

      String missing = "";
      if (this.name == null) {
        missing += " name";
      }
      if (this.numberOfLegs == null) {
        missing += " numberOfLegs";
      }
      if (!missing.isEmpty()) {
        throw new IllegalStateException("Missing required properties:" + missing);
      }

[swankjesse]

Hmm... that isn't so bad actually. Maybe for single classes the cost is low enough?

[stojan26]

It should not be that hard/costly for a single class. And it could be quite useful to have all errors in one go.

I'll try and do a prototype next week and we can go from there.

[swankjesse]

Cool! Kotlin reflect, then codegen?

[swankjesse]

Lets do it like this:

  fun fromJson(reader: JsonReader): GetterAndSetter {
    var a: Int? = null
    var b: Int? = null
    var total: Int? = null
    accumulateErrors {
      reader.beginObject()
      while (reader.hasNext()) {
        when (reader.selectName(options)) {
          0 -> a = intAdapter.fromJson(reader) ?: wasNull("a", reader)
          1 -> b = intAdapter.fromJson(reader) ?: wasNull("b", reader)
          2 -> total = intAdapter.fromJson(reader) ?: wasNull("total", reader)
          -1 -> reader.skipNameAndValue()
        }
      }
      reader.endObject()
      require("a", a)
      require("b", b)
    }
    var result = GetterAndSetter(
        a!!,
        b!!
    )
    result.total = total ?: result.total
    return result
  }

with this support:

inline fun accumulateErrors(block: ErrorsReporter.() -> Unit)

class ErrorsReporter {
  fun <T> wasNull(name: String, jsonReader: JsonReader): T?
  fun <T> require(name: String, value: T?)
}

We’ll need some Kotlin-specific runtime stuff for this.

[ZacSweers]

Iterated on this a bit - once we get stable inline classes and contracts, we can do a really clever thing with zero allocations (unless we do find errors, in which case we're bombing out anyway) and tricking the compiler to avoid unnecessary nullchecks

inline fun <T> accumulateErrors(block: ErrorsReporter.() -> T): T {
  return ErrorsReporter().block()
}

@JvmInline
value class ErrorsReporter(var errors: List<String> = emptyList()) {
  fun <T> wasNull(name: String, jsonReader: JsonReader): T? {
    errors += "$name was null at ${jsonReader.path}"
    return null
  }

  fun checkErrors() {
    check(errors.isEmpty()) { errors.joinToString("\n") }
  }
}

// Lie to the compiler here, knowing that we'll manually bomb out if any errors were recorded
public inline fun <T : Any> ErrorsReporter.require(name: String, value: T?) {
  contract {
    returns() implies (value != null)
  }
  if (value == null) {
    errors += "$name was absent"
  }
}

Then we can do this in code gen

override fun fromJson(reader: JsonReader): KCGResponse = accumulateErrors {
    var users: List<KCGUser>? = null
    var status: String? = null
    var isRealJson: Boolean? = null
    reader.beginObject()
    while (reader.hasNext()) {
      when (reader.selectName(options)) {
        0 -> users = listOfKCGUserAdapter.fromJson(reader) ?: wasNull("users", reader)
        1 -> status = stringAdapter.fromJson(reader) ?: wasNull("status", reader)
        2 -> isRealJson = booleanAdapter.fromJson(reader) ?: wasNull("isRealJson", reader)
        -1 -> {
          // Unknown name, skip it.
          reader.skipName()
          reader.skipValue()
        }
      }
    }
    reader.endObject()
    require("users", users)
    require("status", status)
    require("isRealJson", isRealJson)
    checkErrors()
    var result = KCGResponse(
        users = users,
        status = status,
        isRealJson = isRealJson)
    return@accumulateErrors result
  }
}

[JulioCottaN26]

Any news for this? It would avoid a lot of code being written by hand in my company just to check nullability of fields.

[ZacSweers]

No, the solution above can't be used until inline classes and contracts are stable

[ZacSweers]

An updated pass at how this would work. Some changes since the last iteration:

• value classes instead of inline classes
• value classes cannot have var properties. This means we can't escape an allocation entirely anymore with the previous emptyList() trick. We can use ArrayList(0) though
• Previous require() conflated null and absence. We already check null, now we need to check for absence. This is done by leveraging a second element in the contract to imply the type and use of an UNSET variable

object UNSET

inline fun <T> accumulateErrors(block: ErrorsReporter.() -> T): T {
  return ErrorsReporter().block()
}

@JvmInline
value class ErrorsReporter(val errors: MutableList<String> = ArrayList(0)) {
  fun <T> wasNull(name: String, jsonReader: JsonReader): T? {
    errors += "$name was null at ${jsonReader.path}"
    return null
  }

  fun checkErrors() {
    check(errors.isEmpty()) { errors.joinToString("\n") }
  }
}

// Lie to the compiler here, knowing that we'll manually bomb out if any errors were recorded
inline fun <reified T : Any> ErrorsReporter.require(name: String, value: Any?) {
  contract {
    returns() implies (value != null)
    returns() implies (value is T)
  }
  if (value === UNSET) {
    errors += "$name was absent"
  }
}

So then use is like so

override fun fromJson(reader: JsonReader): KCGResponse = accumulateErrors {
  var users: Any? = UNSET
  var status: Any? = UNSET
  var isRealJson: Any? = UNSET
  reader.beginObject()
  while (reader.hasNext()) {
    when (reader.selectName(options)) {
      0 -> users = listOfKCGUserAdapter.fromJson(reader) ?: wasNull("users", reader)
      1 -> status = stringAdapter.fromJson(reader) ?: wasNull("status", reader)
      2 -> isRealJson = booleanAdapter.fromJson(reader) ?: wasNull("isRealJson", reader)
      -1 -> {
        // Unknown name, skip it.
        reader.skipName()
        reader.skipValue()
      }
    }
  }
  reader.endObject()
  require<List<KCGUser>>("users", users)
  require<String>("status", status)
  require<Boolean>("isRealJson", isRealJson)
  checkErrors()
  return@accumulateErrors KCGResponse(
    users = users,
    status = status,
    isRealJson = isRealJson
  )
}