Undocumented behavior on serialization of non-constructor property with defaults
marcosalis opened this issue · 12 comments
Using moshi-kotlin:1.15.1 with KotlinJsonAdapterFactory:
Moshi.Builder().add(KotlinJsonAdapterFactory()).build()
I want to serialize the following model:
private data class WithDefaultValue(val property: String = "default") {
val nonConstructor: String = "default"
}
I would expect the class JSON serialization to yield the following output:
{"property":"default","nonConstructor":"default"}
(and this, in fact, was what happened until Moshi <= 1.8)
But I get the following on the latest Moshi:
{"property":"default"}
The property outside the primary constructor is ignored. I would consider this an undocumented behavior and I honestly struggle to understand why this is happening. I would expect the property to be serialized since it's not marked as @Transient or @Json(ignored=true). Note that this also happens with normal (non data) classes.
The kotlin-serialization library allows to configure this aspect of encoding with the option encodeDefaults=true. I couldn't find anything equivalent in Moshi.
I have a legacy codebase with 300+ request/response models, and making one-by-one changes to the classes would be both unfeasible and extremely error-prone. Is there a way around this?
Thank you in advance.
Here's a short test case to display the issue (it uses assertK for the assertion):
@Test
@OptIn(ExperimentalStdlibApi::class)
fun `GIVEN a model with non-constructor property with default value WHEN encoding THEN property is serialized`() {
val moshi = Moshi.Builder().addLast(KotlinJsonAdapterFactory()).build()
val modelToSerialize = WithDefaultValue()
val adapter: JsonAdapter<WithDefaultValue> = moshi.adapter<WithDefaultValue>()
val serializedJson = adapter.toJson(modelToSerialize)
assertThat(serializedJson).isEqualTo("{\"property\":\"default\",\"nonConstructor\":\"default\"}")
}
private data class WithDefaultValue(val property: String = "default") {
@Suppress("unused") val nonConstructor: String = "default"
}
Moshi <= 1.8
Note that these versions did not support Kotlin and would serialize backing fields as well as not honor default argument values on deserialization. Behavior on Kotlin types was completely unspecified.
The kotlin-serialization library allows to configure this aspect of encoding with the option encodeDefaults=true. I couldn't find anything equivalent in Moshi.
Encode defaults does not control whether constant instance properties are serialized or not. It controls the behavior when a property value matches that of its default.
Hi Jake, first of all thank you for the super quick answer.
Encode defaults does not control whether constant instance properties are serialized or not. It controls the behavior when a property value matches that of its default.
It actually does the former when encoding a model to a string. This test passes using the same model:
...
import kotlinx.serialization.Serializable
import kotlinx.serialization.encodeToString
import kotlinx.serialization.json.Json
@Test
fun `GIVEN a model with non-constructor property with default value WHEN encoding with kotlin-serialization THEN property is serialized`() {
val json = Json { encodeDefaults = true } // setting this to `false` causes the test to fail
val modelToSerialize = WithDefaultValue()
val serializedJson = json.encodeToString(modelToSerialize)
assertThat(serializedJson).isEqualTo("{\"property\":\"default\",\"nonConstructor\":\"default\"}")
}
@Serializable
private data class WithDefaultValue(val property: String = "default") {
val nonConstructor: String = "default"
}
Aside from the behaviour of another library, which can choose to take another approach on subtleties like this one, I'm curious to know if there was a rationale behind Moshi's choice. To me, aside from the "working by accident" in older versions, it seems like a counterintuitive behaviour that can easily lead to bugs that are harder to spot.
Something that's even more obscure to me is the fact (which I just noticed) that if you turn the val property into a var, then it is correctly serialized:
private data class WithDefaultValue(val property: String = "default") {
var nonConstructor: String = "default"
}
This outputs the following with Moshi:
{"property":"default","nonConstructor":"default"}
Whether this is or not an intuitive behaviour, can you or anybody else think of a "global" workaround via adapters?
Thank you again 🙇
It actually does the former when encoding a model to a string.
That is not what is happening. The non-primary constructor property is always considered as a serializable member by kotlinx.serialization in your example. What the encodeDefaults setting controls is whether to include any candidate property in the serialized form when that property's value matches that of its default. In the case of the non-primary constructor property in your example, that is always true. So its presence in the serialized form is correlated to the setting, but that's only because its value is always that of its own default.
Moshi does not consider a val as a serializable member because it cannot be symmetrically deserialized. This is why changing it to a var suddenly made things work–it has become able to hold a deserialized value, and thus also becomes included in serialization.
I understand what you meant now, thanks for the explanation.
So the goal of Moshi in terms of serialization/deserialization is being symmetric at all costs?
Whilst I see how consistent behaviour (in terms of what can/can't be done with reflection in Kotlin) could simplify implementation, in this case I'd give the chance to override the behaviour (just as, perhaps indirectly, kotlin-serialization does).
My use case (which I assume is a very common one) is using Moshi for serialization of request models and deserialization of response models through OkHttp/Retrofit. In this context, the symmetric behaviour isn't important, at least compared to predictability and ease of use.
The specific given example was based on a common scenario, in our codebase, where there's a constant to be sent to an endpoint (e.g. an application code). I don't want to give the ability to client code to modify this constant, but I also don't want to have the need to specify that constant every time I instantiate the class. And I do need to use a data class for logging purposes.
In order to retain these constraints and get this to work with Moshi, I'd have to write this:
data class WithDefaultValue private constructor(
val property: String,
val nonConstructor: String
) {
constructor(property: String = "default") : this(property, nonConstructor = "default")
}
Which is a lot more verbose and hard to read than the previous approach, let alone the fact the private primary constructor is useless on a data class where I can use copy to create an object that violates my constant:
WithDefaultValue().copy(nonConstructor = "OH NO")
Suggestion: since this is a limitation due to a technical constraint in kotlin-reflect (and the behaviour is the same using codegen as well), I'd add this to the list of limitations in the README file for more transparency.
There is no technical limitation. It is a design choice that was made when Kotlin support was added.
I might have misunderstood then. From what you wrote in your previous comment, not serialising vals outside of the primary constructor was due to the fact it's not possible to do that using Kotlin reflection (that's the technical limitation I was referring to), so to avoid breaking symmetric serialisation/deserialisation the choice has been made to not serialise those properties as well.
I understand that, but if it weren't for that limitation, why else would the library not serialise a property that is neither transient nor marked as ignored? Without any warning as well?
It has nothing to do with Kotlin reflection. You cannot write to a non-primary constructor val using any mechanism. The vals which are present in the primary constructor have an associated parameter that can be used to supply values providing the library with a means of reading and writing. A read-only val does not participate in the same way a write-only parameter does not participate.
Okay, fair enough, a val is a val and, unlike Java reflection, there's no way to write to that (and it's probably a good idea).
However, copy can still be used for data classes, for instance.
I still don't see how it's consistent behaviour though, aside from the symmetricity rationale, from the point of view of an all-purpose serialisation library.
IMHO in this instance "failing silently" (= not serialising the property) isn't a very predictable behaviour for the developer declaring it. From a serialisation standpoint, it shouldn't make a difference whether it's a val or a var, if the developer doesn't declare they want to ignore it, ignoring it silently is dangerous and error-prone.
I think it would be extremely useful to have a way to override this behaviour in the frequent scenarios (request/response mapping) where the symmetric approach isn't needed (as opposed to caching or storing data, where I acknowledge is fundamental).
Please let me know if it's worth creating a separate issue as a feature request.
If not for the ability to override the behaviour, at least to be able to print a warning at build time so that developers can fix/migrate legacy classes.
Thank you!
copy does not work on non-primary constructor properties. The properties which are available for changing values in copy are the same that are available as parameters in the primary constructor which are the same that Moshi considers for inclusion in the serialized form.
all-purpose
Moshi is definitely not all-purpose. It has been designed very intentionally to not support any and everything that one would want to do. However, we try to cover the most common ~90% and offer escape hatches when you need to do something more advanced.
kotlinx.serialization is the same way, but it may choose different values to include and exclude in the features it supports.
I think it would be extremely useful to have a way to override this behaviour in the frequent scenarios
I think calling this "frequent" is a bit of an exaggeration as no one else has been clamoring for this in the six years of Kotlin support.
Today, you can achieve this with a custom JsonAdapter that is either hand-written, uses kotlin-reflect, or is created via code generation. Basically doing the same thing that the library does internally, except choosing to enact whatever policy of selecting properties that you want. This is generally the mechanism that lets you override any in-built behavior.
For the long term, we can have a discussion about whether this is something we want to allow. But I would keep in mind that no one is really working on Moshi actively so I would not rely on waiting for this feature if you need it today.