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

Support multiple class descriminators for polymorphism. #546

Closed
Dominaezzz opened this issue Aug 29, 2019 · 19 comments
Closed

Support multiple class descriminators for polymorphism. #546

Dominaezzz opened this issue Aug 29, 2019 · 19 comments
Assignees
Labels

Comments

@Dominaezzz
Copy link
Contributor

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
Copy link
Contributor Author

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

@sandwwraith
Copy link
Member

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
Copy link
Contributor Author

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

@giangpham96
Copy link

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

@Dominaezzz
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link

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

@Dominaezzz
Copy link
Contributor Author

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

@Dominaezzz
Copy link
Contributor Author

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
Copy link

Hi, any progres one this one?

@sandwwraith
Copy link
Member

@KamilJanda It is planned and in progress now

@sandwwraith sandwwraith assigned sandwwraith and unassigned qwwdfsad May 25, 2021
sandwwraith added a commit that referenced this issue May 25, 2021
@kroegerama
Copy link

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

sandwwraith added a commit that referenced this issue Jul 20, 2021
sandwwraith added a commit that referenced this issue Aug 12, 2021
…gure discriminator per polymorphic base class

Fixes #546
sandwwraith added a commit that referenced this issue Aug 24, 2021
…gure discriminator per polymorphic base class

Fixes #546
@snowe2010
Copy link

@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
Copy link
Contributor Author

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

@snowe2010
Copy link

@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
Copy link
Member

@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
Copy link

@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
Copy link

Mikkelet commented Dec 1, 2022

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
Copy link

cdekok commented Dec 13, 2022

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")

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

10 participants