-
Notifications
You must be signed in to change notification settings - Fork 761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better egonomics when replacing nulls with default values #1809
Comments
Thanks for the write up. I think we can do this with a generic The mechanism by which it would accomplish this is by detecting the annotation on the property of a class and wrapping its normal 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. |
That would be great. If no one else volunteers I can take a crack at the codegen approach based on your sample. |
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
|
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).
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:
What are my options?
Options
Rename the properties and provide accessors
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.,
That's better, but it's still too far from the properties it's actually going to affect.
null
.Ideal
Ideally I'd like to be able to write this:
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:
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 beAny
, and annotations can't be generic over some typeT
).Looking at the generated code I don't think this is possible at the moment. The relevant generated
FocusJsonAdapter
code if I write:is
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
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 adefaultIfNull
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:Thanks for reading.
The text was updated successfully, but these errors were encountered: