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

Inheritance/polymorphic serialization #427

Open
zshamrock opened this issue Apr 9, 2019 · 13 comments
Open

Inheritance/polymorphic serialization #427

zshamrock opened this issue Apr 9, 2019 · 13 comments

Comments

@zshamrock
Copy link

zshamrock commented Apr 9, 2019

I have the following hierarchy:

@Serializable
sealed class Search(@SerialName("type") private var searchType: SearchType,
                    @Transient open val table: String = "",
                    @Transient open val index: String? = null,
                    @Transient open val filters: List<QueryCondition> = listOf(),
                    @Transient open val order: Order = Order.DESC) {
    fun isAscOrdered(): Boolean {
        return order == Order.ASC
    }
}

@Serializable
class QuerySearch(override val table: String,
                       override val index: String?,
                       val keys: List<QueryCondition>,
                       override val filters: List<QueryCondition>,
                       override val order: Order) : Search(SearchType.QUERY, table, index, filters, order) {
...
}

@Serializable
class ScanSearch(override val table: String,
                 override val index: String?,
                 override val filters: List<QueryCondition>) :
        Search(SearchType.SCAN, table, index, filters, Order.ASC) {
...
}

And it already looks "ugly" in order to do the proper serialization, i.e. have to mark parent properties as open, and override them (in order to make them properties, as required by the serialization library) in the children.

Also in order to not have parent properties duplicated in the output JSON, have to mark them @Transient, but it then requires to set the default values (where there are no really meaningful default values).

Only then it produces the proper JSON, like the one below:

{
  "type": "QUERY",
  "table": "Table A",
  "index": "Index A",
  "keys": [
    {
      "name": "Id",
      "type": "NUMBER",
      "operator": "EQ",
      "values": [
        "10"
      ]
    }
  ],
  "filters": [
    {
      "name": "Timestamp",
      "type": "NUMBER",
      "operator": "BETWEEN",
      "values": [
        "100",
        "200"
      ]
    },
    {
      "name": "Name",
      "type": "STRING",
      "operator": "BEGINS_WITH",
      "values": [
        "Test"
      ]
    }
  ],
  "order": "ASC"
}

Although when deserialize the JSON back to the class, the parent properties are set to the default values instead of the values from the child class.

And moreover don't know how to do the polymorphic deserialization, i.e. using the discriminator value, like "type" property.

Is then this library suitable for the above use case, or it is the ultimate end goal, but it is not ready yet to handle this?

Or this library was specifically designed in mind to work only with data classes?

Here is the unit test to produce the JSON above:

class SearchSpec : StringSpec({
    "serialize query search" {
        val search = QuerySearch(
                "Table A",
                "Index A",
                listOf(QueryCondition("Id", Type.NUMBER, Operator.EQ, listOf("10"))),
                listOf(QueryCondition("Timestamp", Type.NUMBER, Operator.BETWEEN, listOf("100", "200")),
                        QueryCondition("Name", Type.STRING, Operator.BEGINS_WITH, listOf("Test"))),
                Order.ASC)
        val data = Json.stringify(QuerySearch.serializer(), search)
        println(data)
        val parse = Json.parse(Search.serializer(), data) // Obviously this line fails, 
        // val parse = Json.parse(QuerySearch.serializer(), data) // but this works, but results in the default properties for the parent Search class
        parse shouldBe search
    }
})
@zshamrock
Copy link
Author

How I really would like it to see is to be able to write the following:

@Serializable
sealed class Search(@SerialName("type") private var searchType: SearchType,
                    val table: String = "",
                    val index: String? = null,
                    protected val filters: List<QueryCondition> = listOf(),
                    private val order: Order = Order.DESC) {
    fun isAscOrdered(): Boolean {
        return order == Order.ASC
    }
}

@Serializable
class QuerySearch(table: String,
                       index: String?,
                       keys: List<QueryCondition>,
                       filters: List<QueryCondition>,
                       order: Order) : Search(SearchType.QUERY, table, index, filters, order) {
...
}

And it works. I.e. be able to figure the properties from the parent class and apply them to the child class. Plus extra annotation to describe the discriminator property/polymorphic behavior.

@sandwwraith
Copy link
Member

We have this goal in mind. Next release of the library will relax some restrictions, e.g. properties which do not have backing fields (an abstract ones) would be implicitly transient, and there would be a setting in JSON format to include class discriminator directly into JSON object (#50).

The one big limitation left is the requirement that @serializable class constructor should have only vals or vars so the framework can create object back.

@valeriyo
Copy link

Has this ever worked?

import kotlinx.serialization.Serializable

@Serializable
sealed class Search(
  @SerialName("type") private var searchType: Int,
  @Transient open val table: String = "",
)

@Serializable
class QuerySearch(
  override val table: String,
  override val index: String?,
) : Search(123, table)

because with 0.11.0 it produces :

e: java.lang.IllegalStateException: class QuerySearch has duplicate serial name of property table, either in it or its parents.
	at org.jetbrains.kotlinx.serialization.compiler.resolve.SerializableProperties.validateUniqueSerialNames(SerializableProperties.kt:59)
	at org.jetbrains.kotlinx.serialization.compiler.resolve.SerializableProperties.<init>(SerializableProperties.kt:54)
	at org.jetbrains.kotlinx.serialization.compiler.resolve.KSerializerDescriptorResolver.createLoadConstructorDescriptor(KSerializerDescriptorResolver.kt:296)
	at org.jetbrains.kotlinx.serialization.compiler.extensions.SerializationResolveExtension.generateSyntheticSecondaryConstructors(SerializationResolveExtension.kt:79)

@valeriyo
Copy link

Inheritance doesn't seem to work at all...

import kotlinx.serialization.Serializable

@Serializable
open class Fruit(
  val name: String? = null
)

@Serializable
class Apricot(
  name: String
): Fruit(name)

==> IllegalStateException: Class Apricot have constructor parameters which are not properties and therefore it is not serializable automatically

AND

import kotlinx.serialization.Serializable

@Serializable
open class Fruit(
  open val name: String? = null
)

@Serializable
class Apricot(
  override val name: String
): Fruit(name)

==> IllegalStateException: class Apricot has duplicate serial name of property name, either in it or its parents.

@mhammouri98
Copy link

Any updates on this topic ?

@nhoughto
Copy link

Bit of a hijack, but is there a current way to support just the type string deserialisation portion of this issue? Ignoring the ergonomics of data class / property declaration etc, is there a way to deserialise to a class from a known string that isnt the class name?

The current behavior of forcing a class name means you need to expose class names to your consumers..? Something like Jacksons type-id system https://github.com/FasterXML/jackson-docs/wiki/JacksonPolymorphicDeserialization#2-on-type-ids

@Dominaezzz
Copy link
Contributor

@nhoughto Have a look at https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/polymorphism.md#a-bit-of-customizing

@nhoughto
Copy link

Oh great! That’s a well hidden nugget, and here was me digging around the code of the builder 🙄

Thanks!

@rgmz
Copy link

rgmz commented Oct 5, 2021

Inheritance doesn't seem to work at all...
...
==> IllegalStateException: class Apricot has duplicate serial name of property name, either in it or its parents.

@sandwwraith: Are there plans to support inheritance/polymorphism with open properties?

(Apologies for all the pings; you seem to be the main contributor who's dealt with some of these older issues.)


I stumbled across this issue in the midst of creating a new one. I'll copy and paste the content from that into here:

What is your use-case and why do you need this feature?

(Explanation omitted, as it's already covered by other comments on this issue.)

The following code is a basic example of my use case. I've omitted dozens of fields and Formats for brevity.

@Serializable
enum class Format { go, maven2 }

@Serializable
sealed class Component {
    abstract val format: Format
    abstract val name: String
    open val namespace: String? = null
    abstract val version: String

    open val packageUrl: String by lazy {
        val format = if (format == Format.maven2) "maven" else format.name
        val namespace = if (namespace != null) "${namespace}/" else ""
        "pkg:$format/$namespace$name@$version"
    }
}

@Serializable // compilation error
data class GoComponent(override val name: String, override val version: String) : Component() {
    override val format: Format = Format.go
}

@Serializable // compilation error
data class MavenComponent(override val namespace: String, override val name: String, override val version: String) : Component() {
    override val format: Format = Format.maven2
}

Describe the solution you'd like

Ideally, the code above would 'just work', or there'd be an annotation to say "use this overriden property instead of the supertype one".

While the above code could be fixed by making namespace an abstract val and overriding it in each sub-class, or omitting from the base class and making packageUrl an extension function, this would be tedious (and ugly) as there are several other open fields that I've omitted that would need to be overridden on dozens of Formats.

@sandwwraith
Copy link
Member

The problem with open vars is that if you override them, you get two backing fields: in the parent and in the subclass (and subclass can't access parent field!). Depending on what exact runtime type you serialize, you serialize different fields, and since we traverse all hierarchy (including private fields), we end up having two same json properties.

Btw, what exact error do you have with GoComponent ? You shouldn't get one, because you override only abstract vals.

@marcosalis
Copy link

marcosalis commented May 17, 2022

I'm running into a "usability" issue with inheritance that is similar to the one in the first message.
I have a legacy API with large objects which can be of several types, and have a couple dozen shared fields in the JSON (and some that are type-specific).

Based on the current design, I need to create a sealed class (or interface), and then a set of concrete classes, one per type. I have to declare the shared fields in the sealed class, and then override all of them in the inheriting classes (and pass the values to the base class constructor). This means:

  • Lots of repeated code, as I have to declare the overriding values in each subclass.
  • What's more, if I want to declare a default value for a shared property, I have to do it in each subclass (properties in the base class must be abstract or they won't be decoded).

Example:

@Serializable(with = ...) // uses a `JsonContentPolymorphicSerializer`
sealed class MyBaseClass {
    val property: String? = null
}

@Serializable data class A(val anotherProperty: String) : MyBaseClass()

@Serializable data class B(val aProperty: Float) : MyBaseClass()

The above fails decoding of a List<MyBaseClass> with the following error:

Encountered unknown key 'property'.
Use 'ignoreUnknownKeys = true' in 'Json {}' builder to ignore unknown keys.

To make it work, I'd have to override property in each superclass, and give it the same default value.
This doesn't seem viable for large classes with many properties, and it leads to a really poor inheritance design, and error-prone duplication of code. Is there a solution around this?
Thank you!

@raqfc
Copy link

raqfc commented Jun 16, 2022

I'm running into a "usability" issue with inheritance that is similar to the one in the first message. I have a legacy API with large objects which can be of several types, and have a couple dozen shared fields in the JSON (and some that are type-specific).

Based on the current design, I need to create a sealed class (or interface), and then a set of concrete classes, one per type. I have to declare the shared fields in the sealed class, and then override all of them in the inheriting classes (and pass the values to the base class constructor). This means:

* **Lots** of repeated code, as I have to declare the overriding values in each subclass.

* What's more, if I want to declare a default value for a shared property, I have to do it in _each_ subclass (properties in the base class must be abstract or they won't be decoded).

Example:

@Serializable(with = ...) // uses a `JsonContentPolymorphicSerializer`
sealed class MyBaseClass {
    val property: String? = null
}

@Serializable data class A(val anotherProperty: String) : MyBaseClass()

@Serializable data class B(val aProperty: Float) : MyBaseClass()

The above fails decoding of a List<MyBaseClass> with the following error:

Encountered unknown key 'property'.
Use 'ignoreUnknownKeys = true' in 'Json {}' builder to ignore unknown keys.

To make it work, I'd have to override property in each superclass, and give it the same default value. This doesn't seem viable for large classes with many properties, and it leads to a really poor inheritance design, and error-prone duplication of code. Is there a solution around this? Thank you!

i'm encountering the same issue, i'm using a class that inherits from an abstract class but with with its own variables (they aren't open nor abstract), i can't alter the super class since its from a library, but i do need the variables to be encoded aswell, any suggestions?

@pseusys
Copy link

pseusys commented Jul 21, 2022

The problem with open vars is that if you override them, you get two backing fields: in the parent and in the subclass (and subclass can't access parent field!). Depending on what exact runtime type you serialize, you serialize different fields, and since we traverse all hierarchy (including private fields), we end up having two same json properties.

Don't you think that it would be logical to prefer child property to parent? After all, it's overriden for purpose.

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

No branches or pull requests

10 participants