Kotlin / kotlinx.serialization

Kotlin multiplatform / multi-format serialization
Apache License 2.0
5.39k stars 621 forks source link

Support multiple class descriminators for polymorphism. #546

Closed Dominaezzz closed 3 years ago

Dominaezzz commented 5 years ago

What is your use-case and why do you need this feature? As of writing this issue, only a single classDiscriminator can be specified on a Json object. I'm working with a REST api where for different endpoints or error codes, the object returned will use a different "class discriminator".

For example, if the response code is != 200, then an error in form of JSON is returned.

The discriminator here is errcode.

sealed class MatrixError : Throwable() {
    abstract val errcode: String
}

@Serializable
@SerialName("M_NOT_FOUND")
data class NotFound(override val error: String) : MatrixError() {
    override val errcode get() = "M_NOT_FOUND"
}

@Serializable
@SerialName("M_LIMIT_EXCEEDED")
data class LimitExceeded(
    override val error: String,

    @SerialName("retry_after_ms")
    val retryAfterMillis: Long
) : MatrixError() {
    override val errcode get() = "M_LIMIT_EXCEEDED"
}

If the response code is 200, then the model uses type as the discriminator.

sealed class Event

@SerialName("m.room")
data class RoomEvent : Event

@SerialName("m.join")
data class JoinEvent(val userId: String) : Event

Describe the solution you'd like I would like to be able to annotate the parent class with a discriminator.

Or supply a class discriminator in the module like so.

SerializersModule {
    polymorphic<MatrixError>(discriminator="errcode") {
        addSubclass(MatrixError.Unknown.serializer())
        addSubclass(MatrixError.NotFound.serializer())
        addSubclass(MatrixError.Forbidden.serializer())
        addSubclass(MatrixError.UnsupportedRoomVersion.serializer())
        addSubclass(MatrixError.LimitExceeded.serializer())
   }
}
Dominaezzz commented 4 years ago

Would a PR for this be accepted or is this already being worked on?

sandwwraith commented 4 years ago

This is not being worked on, but it seems to me that it requires some additional handling from plugin: @SerialInfo annotations are not stored when used on abstract classes, so it's impossible to retrieve smth like

@JsonTypeDiscriminator("errcode")
@Serializable
abstract class ErrCode

Although it is still possible to configure from polymorphic { } builder, I'm not sure this is a great idea because builders and modules should be format-agnostic.

Dominaezzz commented 4 years ago

Is it possible to enable storage of @SerialInfo annotations in the serializers of abstract classes?

giangpham96 commented 3 years ago

For the time being, how should we configure class discriminator in polymorphic { } as you mentioned? @sandwwraith

Dominaezzz commented 3 years ago

When an abstract class ExampleClass is annotated with @Serializable, what does ExampleClass.serializer() return? Can the plugin pass the class discriminator in to the ctor of the generated serializer?

sandwwraith commented 3 years ago

@giangpham96 It is not implemented yet.

@Dominaezzz It is possible, although it requires changes in the plugin — and the proper solution (store serialinfo/serialname annotations in PolymorphicSerializer) requires them too

mykola-dev commented 3 years ago

any news on this one? it's extremely desirable feature for us

Dominaezzz commented 3 years ago

Probably waiting for 1.5.0 when most of the backends will be IR based.

Dominaezzz commented 3 years ago

This is my current workaround for this if anyone is interested.

public class DiscriminatorChanger<T : Any>(
    private val tSerializer: KSerializer<T>,
    private val discriminator: String
) : KSerializer<T> {
    override val descriptor: SerialDescriptor get() = tSerializer.descriptor

    override fun serialize(encoder: Encoder, value: T) {
        require(encoder is JsonEncoder)
        val json = Json(encoder.json) { classDiscriminator = discriminator }
        val element = json.encodeToJsonElement(tSerializer, value)
        encoder.encodeJsonElement(element)
    }

    override fun deserialize(decoder: Decoder): T {
        require(decoder is JsonDecoder)
        val element = decoder.decodeJsonElement()
        val json = Json(decoder.json) { classDiscriminator = discriminator }
        return json.decodeFromJsonElement(tSerializer, element)
    }
}

Then you use it like this.

KamilJanda commented 3 years ago

Hi, any progres one this one?

sandwwraith commented 3 years ago

@KamilJanda It is planned and in progress now

kroegerama commented 3 years ago

Will this be included in the next release? I'm really looking forward to it.

snowe2010 commented 2 years ago

@sandwwraith this doesn't seem to be done to me, it looks like it only applies to array polymorphism, not regular, and even though it wasn't specified in the requirements, I feel that still only allowing one class discriminator in the hierarchy is super limiting.

Dominaezzz commented 2 years ago

What do you mean? It works for regular polymorphism as well. Do you have an example of it not working?

snowe2010 commented 2 years ago

@Dominaezzz I think I must have misread the commit when looking it over. It appeared like the tests were passing the discriminator outside of the type itself. I haven't tested this, because I need my subtypes to have different discriminators.

sandwwraith commented 2 years ago

@snowe2010 What's the use-case for providing different discriminators? Keep in mind that since the property name for a discriminator is determined statically (by looking at the class annotations), it may be impossible to deserialize certain subclasses if they have a different discriminator than the base class.

snowe2010 commented 2 years ago

@sandwwraith

I honestly don't really see the gain if you can't provide multiple discriminators, it just seems kinda pointless (besides simple error polymorphism) to only be able to discriminate with a single key throughout the entire hierarchy. I'll give you our exact use case, but I'll rip out some of the duplication so that it is easier to read:

@Serializable
data class UnifiedResponse(
    val detailLevel: String, // decides whether we want ApiPricePoint or DetailedPricePoint
    val monthly: AllPricePoints,
    val loan: AllPricePoints,
)

@Serializable
data class AllPricePoints(
    val includedPricePoints: List<PricePoint>,
    val excludedPricePoints: List<PricePoint>,
)

@Serializable
sealed class PricePoint {
    abstract val pricingModelType: PricingModelType // decides whether we need pps or not, also decides if transcript should have flex or freedom
    abstract val productType: ProductType  // decides whether we're monthly or loan
    abstract val pps: Int?
    //... lots of other common properties
}

@Serializable
sealed class ApiPricePoint : PricePoint() {
    abstract val serviceVersions: ServiceVersions // only available on ApiPricePoints
}

@Serializable
@SerialName("Monthly")
data class ApiMonthlyPricePoint(
    override val pricingModelType: PricingModelType,
    override val serviceVersions: ServiceVersions,
    override val pps: Int?,
    val escalator: BigDecimal,
) : ApiPricePoint() {
    override val productType: ProductType
        get() = ProductType.Monthly
}

@Serializable
@SerialName("Loan")
data class ApiLoanPricePoint(
    override val pricingModelType: PricingModelType,
    override val serviceVersions: ServiceVersions,
    override val pps: Int?,
    val apr: BigDecimal,
    val term: Int,
    val vendor: String,
) : ApiPricePoint() {
    override val productType: ProductType
        get() = ProductType.Loan
}

@Serializable
sealed class DetailedPricePoint : PricePoint() {
    abstract val transcript: Transcript  // only available on DetailedPricePoints
}

@Serializable
@SerialName("DetailedMonthly")
data class DetailedMonthlyPricePoint(
    override val pricingModelType: PricingModelType,
    override val transcript: Transcript,
    override val pps: Int?,
    val escalator: BigDecimal,
) : DetailedPricePoint() {
    override val productType: ProductType
        get() = ProductType.Monthly
}

@Serializable
@SerialName("DetailedLoan")
data class DetailedLoanPricePoint(
    override val pricingModelType: PricingModelType,
    override val transcript: Transcript,
    override val pps: Int?,
    val apr: BigDecimal,
    val term: Int,
    val vendor: String,
) : DetailedPricePoint() {
    override val productType: ProductType
        get() = ProductType.Loan
}

@Serializable
data class Transcript(
    val serviceVersions: ServiceVersions,
    val flex: FlexTranscript?,              // if pricingModelType is Flex then this is populated. I would love to have solved this with more polymorphism
    val freedom: FreedomTranscript?,        // if pricingModelType is Freedom then this is populated. I would love to have solved this with more polymorphism
    val battery: BatteryTranscript?,
    val lambdaVersion: String = "static",
)

ignoring the transcript stuff at the bottom, because that is just a minor thing compared to the large issue with the nested polymorphism here. We have several pricing models at Sunrun, and they all return many many common things, but with a few minor changes. It would be great to have these minor changes handled with polymorphism rather than nullability, since the minor changes are usually in groups. For example, Loans always have apr, term, and vendor. If the Loan is a Flex Loan then we need to populate details for that. If it's Freedom it's a completely different set of details. If we're serializing for an API we want to exclude certain details, to save on response size, so that is handled at the top level detailLevel discriminator, then the productType discriminator should choose the appropriate pricing type, and the pricingModelType should affect nested values. I understand some of this might not be possible at all, for example the discriminator at a higher level, like detailLevel or pricingModelType.

So I tried to work around that by instead making two top level ...UnifiedResponse classes that instead have their own strong types:

@Serializable(UnifiedResponse.TheSerializer::class)
abstract class UnifiedResponse {
    abstract val monthly: AllPricePoints
    abstract val loan: AllPricePoints
    public object TheSerializer : KSerializer<UnifiedResponse> by DiscriminatorChanger(
        PolymorphicSerializer(UnifiedResponse::class), "modelType"
    )

    @Serializable
    abstract class AllPricePoints {
        abstract val includedPricePoints: List<PricePoint>
        abstract val excludedPricePoints: List<PricePoint>
    }
}
@Serializable
@SerialName("Detailed")
data class DetailedUnifiedResponse(
    override val monthly: DetailedPricePoints,
    override val loan: DetailedPricePoints,
): UnifiedResponse()

@Serializable
data class DetailedPricePoints(
    override val includedPricePoints: List<DetailedPricePoint>,
    override val excludedPricePoints: List<DetailedPricePoint>,
) : UnifiedResponse.AllPricePoints()

@Serializable(DetailedPricePoint.TheSerializer::class)
sealed class DetailedPricePoint : PricePoint() {
    abstract val transcript: Transcript

    object TheSerializer : KSerializer<DetailedPricePoint> by DiscriminatorChanger(
        PolymorphicSerializer(DetailedPricePoint::class), "productType"
    )
}
//... api pretty much the same

which partially works, but introduces a lot of duplication and a lot of custom serialization that I feel shouldn't be necessary. And I'm not even sure it's going to work properly with Drools due to the weird typing.

Mikkelet commented 1 year ago

Would love the ability to just define custom. I'm working with a backend that has a unique descriptor for its entries, and I don't think I can ask them to change it to 'type' just for my usecase.

Also semantically 'type' only covers some, but all. Sometimes you want 'status' or 'identifier' or something third.

cdekok commented 1 year ago

It's already solved by the new annotation in the mentioned commit, I missed it to and seems the one above me to :)

@JsonClassDiscriminator("something")