Skip to content
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

encodeDefaults = false doesn't work for fields of interface type in some cases #1564

Closed
naftalmm opened this issue Jun 22, 2021 · 3 comments
Closed
Labels

Comments

@naftalmm
Copy link

naftalmm commented Jun 22, 2021

Describe the bug
encodeDefaults = false doesn't have an effect for fields of interface type if:

  1. Field's default value has a unique runtime type for each object instance (new anonymous type implementing the same interface), albeit their serialization result is the same and field itself is immutable
  2. Field is mutable and its default value was changed to a value of another runtime type (implementing the same interface), albeit their serialization result is the same.

To Reproduce
Consider the following code snippet. It defines and serializes the IntBox class, all fields of which have default values:

@file:UseSerializers(IntInterfaceWrapperSerializer::class)

import kotlinx.serialization.KSerializer
import kotlinx.serialization.Serializable
import kotlinx.serialization.UseSerializers
import kotlinx.serialization.builtins.serializer
import kotlinx.serialization.descriptors.SerialDescriptor
import kotlinx.serialization.encodeToString
import kotlinx.serialization.encoding.Decoder
import kotlinx.serialization.encoding.Encoder
import kotlinx.serialization.json.Json

interface IntInterfaceWrapper {
    val x: Int

    companion object {
        private val zero = object : IntInterfaceWrapper {
            override val x = 0
        }

        fun of(x: Int): IntInterfaceWrapper = IntDataClassWrapper(x)
        fun zeroAsNewDataClass(): IntInterfaceWrapper = IntDataClassWrapper(0)
        fun zeroAsStaticAnonymousObject(): IntInterfaceWrapper = zero
        fun zeroAsNewAnonymousObject(): IntInterfaceWrapper = object : IntInterfaceWrapper {
            override val x = 0
        }
    }
}

data class IntDataClassWrapper(override val x: Int) : IntInterfaceWrapper

@Serializable
class IntBox {
    val wrapperImpl1: IntInterfaceWrapper = IntInterfaceWrapper.zeroAsNewDataClass() // OK
    val wrapperImpl2: IntInterfaceWrapper = IntInterfaceWrapper.zeroAsStaticAnonymousObject() // OK
    val wrapperImpl3: IntInterfaceWrapper = IntInterfaceWrapper.zeroAsNewAnonymousObject()
    var wrapperImpl1ResetToImpl2: IntInterfaceWrapper = wrapperImpl1
    var wrapperImpl1ResetToImpl1: IntInterfaceWrapper = wrapperImpl1 // OK
    var wrapperImpl2ResetToImpl1: IntInterfaceWrapper = wrapperImpl2
}

object IntInterfaceWrapperSerializer : KSerializer<IntInterfaceWrapper> {
    override val descriptor: SerialDescriptor = Int.serializer().descriptor
    override fun serialize(encoder: Encoder, value: IntInterfaceWrapper) =
        Int.serializer().serialize(encoder, value.x)

    override fun deserialize(decoder: Decoder) = IntInterfaceWrapper.of(Int.serializer().deserialize(decoder))
}

fun main() {
    val result = Json { encodeDefaults = false }.encodeToString(
        IntBox().also {
            it.wrapperImpl1ResetToImpl2 = it.wrapperImpl2
            it.wrapperImpl1ResetToImpl1 = IntInterfaceWrapper.zeroAsNewDataClass() // OK
            it.wrapperImpl2ResetToImpl1 = it.wrapperImpl1
        })
    println(result)
}

Expected behavior
Console output should be: {}, but it's {"wrapperImpl3":0,"wrapperImpl1ResetToImpl2":0,"wrapperImpl2ResetToImpl1":0}

Environment

  • Kotlin version: 1.5.10
  • Library version: 1.2.1
  • Kotlin platforms: JVM
  • Gradle version: 6.6.1
@sandwwraith
Copy link
Member

encodeDefaults compares not a serialization form, but kotlin objects. Thus, if standard equals(other: Any?) method tells that objects are not equal (which I believe is the case for anonymous objects and different interfaces implementations), then it is encoded even if serialized forms are the same.

@naftalmm
Copy link
Author

naftalmm commented Jun 27, 2021

Thanks for the clarification of internal implementation, but this behavior still doesn't make sense to me. If serialized forms are the same, then the absent value would be reconstructed during decoding anyway. So the goal of "encodeDefaults = false" flag ("reduces visual clutter, and saves the amount of data being serialized") is not reached. Users have to either override equals method for property class (not an option for interfaces anyway) or define a custom serializer for enclosing class (IntBox in this case) to manually post-process resulting JSON.

Probably fair serialization of actual and default property (for subsequent comparing) is too expensive for default behavior. So I'd suggest either add some new configuration option on format level (so that 3 strategies regarding encoding defaults could be set: desired and two current), or (better) add a new method for SerializationStrategy interface with default implementation repeating current behavior (for backward compatibility):

public interface SerializationStrategy<in T> {
    //...

    //Will be called to compare default value with actual; takes effect only for properties serialization;
    // default value will not be encoded if encodeDefaults=false and this method returned true
    fun checkEqualityToDefault(default: T, actual: T): Boolean {
        return default == actual
    }
}

So, it could be overridden in custom serializer for property (IntInterfaceWrapperSerializer in this case):

object IntInterfaceWrapperSerializer : KSerializer<IntInterfaceWrapper> {
    //...

    override fun checkEqualityToDefault(default: IntInterfaceWrapper, actual: IntInterfaceWrapper): Boolean {
        return default.x == actual.x
    }
}

Also this partly restores feature parity for custom serializer with plugin-generated (regarding new annotations suggested in #1091):

@AlwaysEncode will be equivalent to override fun checkEqualityToDefault(default: IntInterfaceWrapper, actual: IntInterfaceWrapper) = false

But if both features would be implemented, they may clash - so there need to be either some priority rules between @Serializable(with=...) and @AlwaysEncode/@NeverEncode, or it should be forbidden to mix them.

@sandwwraith
Copy link
Member

I'm not sure if all this complex mechanism is better than simple equals and @EncodeDefault. In the end, the whole point of encodeDefaults = false is to avoid calling serialization mechanisms for particular objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants