Add support for `type` parameter in sealed classes

Question

Add support for `type` parameter in sealed classes

Closed this issue a year ago · 14 comments

Hello! Today I've had a few hiccups while parsing sealed classes. I've divided this into three "sub-issues" that can all be addressed separataly, but are related to each other.

I've been trying to parse a sealed class similar to this:

@Serializable
sealed class SignupConfig {

    @Serializable
    @SerialName("closed")
    // No new user can sign up
    object Closed : SignupConfig()

    @Serializable
    @SerialName("invite")
    // Only people with an invite from one of the trusted users can sign up
    data class Invite(val trustedUsers: List<String>) : SignupConfig()

    @Serializable
    @SerialName("open")
    // Anyone can freely sign up
    object Open : SignupConfig()
}

class Config(
  val signup : SignupConfig = SignupConfig.Closed,
)

Since I'm using kotlinx-serialization, I was following their doc on sealed classes, which uses a type parameter as a discriminator.
When I tried to parse the Config class using a toml file, I assumed the correct way to specify the Open object would be this:

[signup]
type = "open"

But hoplite in this case has an internal crash:

Exception in thread "main" java.lang.IllegalStateException: Not an invalid instance, was Valid(value=[])
	at com.sksamuel.hoplite.fp.Validated.getInvalidUnsafe(Validated.kt:18)
	at com.sksamuel.hoplite.decoder.SealedClassDecoder.safeDecode(SealedClassDecoder.kt:80)
	at com.sksamuel.hoplite.decoder.NullHandlingDecoder$DefaultImpls.decode(Decoder.kt:96)
	at com.sksamuel.hoplite.decoder.SealedClassDecoder.decode(SealedClassDecoder.kt:18)
	at com.sksamuel.hoplite.decoder.DataClassDecoder.safeDecode(DataClassDecoder.kt:99)
	at com.sksamuel.hoplite.decoder.NullHandlingDecoder$DefaultImpls.decode(Decoder.kt:96)
	at com.sksamuel.hoplite.decoder.DataClassDecoder.decode(DataClassDecoder.kt:25)
	at com.sksamuel.hoplite.internal.Decoding.decode(Decoding.kt:23)
	at com.sksamuel.hoplite.internal.ConfigParser.decode(ConfigParser.kt:71)
	at com.sksamuel.hoplite.ConfigLoader.loadConfig(ConfigLoader.kt:159)

First issue: the library shouldn't crash with this error, but rather fail in an expected way with a ConfigException.

I've later discovered though hoplite's sealed classes documentation that the correct way is this:

signup = "open"

This works fine, but, in my opinion, it has two problems:

It's "asymmetric": sometimes you need signup = <value>, sometimes a [signup] group. This is a bit confusing from the user perspective and it needlessly exposes a technical detail of how the config classes are structured. Furthermore, there's no way to explicitly state that you want the mode to be invite. Specifying trustedUsers alone doesn't convey enough meaning at what this configuration is doing.
Without the support for an explicit type parameter, some cases are ambiguous (second issue). For instance, take this example:

sealed class DatabaseConfig {

    class MySql(val host: String, val port: Int, val user: String, val password: String) : DatabaseConfig()

    class Postgres(val host: String, val port: Int, val user: String, val password: String) : DatabaseConfig()
}

These two classes have the same exact parameters, and there's no way to disambiguate between them.

Adding the support for specifying a type parameter would solve both these points.

Lastly, from the docs:

If keys match multiple implementations then the first match is taken

I think this is a bad choice as that might cause confusion at best and a long-living unintended configuration at worst. In my opinion a more sensible solution would be to raise an exception (third issue)

Thanks in advance :)
Mauro.

sksamuel commented a year ago

See #380

🚀1

Answer 1 · 2023-02-13T07:24:59.000Z

I agree, I encounter the exact problem as your DatabaseConfig example before. As a workaround, you may consider the following

sealed class DatabaseConfig {
    class MySql(val mysqlHost: String, val port: Int, val user: String, val password: String) : DatabaseConfig()
    class Postgres(val postgresHost: String, val port: Int, val user: String, val password: String) : DatabaseConfig()
}

I don't like this workaround as I have to put "decision logic" into the key-side, which should really belong to the value-side, but I have completed this project long ago, so I will just let it be.

Answer 2 · 2023-03-22T15:29:05.000Z

I would consider not using kotlinx serialization with hoplite as hoplite is taking care of that for you, but can you give me a full example of the IllegalStateException error so I can recreate and fix ?

Answer 3 · 2023-03-23T21:50:39.000Z

I would consider not using kotlinx serialization with hoplite as hoplite is taking care of that for you

I think I've worded it badly. I'm using kotlinx serialization elsewhere on my project and I was pointing that out because I assumed that hoplite would work similarly (or even use it under the hood).

Anyway this is the full repro:

In dependencies:

val hopliteVersion = "2.7.2"
implementation("com.sksamuel.hoplite:hoplite-core:$hopliteVersion")
implementation("com.sksamuel.hoplite:hoplite-toml:$hopliteVersion")

Test.kt

import com.sksamuel.hoplite.ConfigLoaderBuilder
import com.sksamuel.hoplite.addFileSource

sealed class SignupConfig {
    object Closed : SignupConfig()
    object Open : SignupConfig()
}


data class Config(
    val signup: SignupConfig = SignupConfig.Closed,
)

fun main(args: Array<String>) {
    val config = ConfigLoaderBuilder
        .default()
        .addFileSource("test.toml")
        .build()
        .loadConfigOrThrow<Config>()

    println(config)
}

test.toml

[signup]
type = "open"

Exception

Exception in thread "main" java.lang.IllegalStateException: Not an invalid instance, was Valid(value=[])
	at com.sksamuel.hoplite.fp.Validated.getInvalidUnsafe(Validated.kt:18)
	at com.sksamuel.hoplite.decoder.SealedClassDecoder.safeDecode(SealedClassDecoder.kt:80)
	at com.sksamuel.hoplite.decoder.NullHandlingDecoder$DefaultImpls.decode(Decoder.kt:96)
	at com.sksamuel.hoplite.decoder.SealedClassDecoder.decode(SealedClassDecoder.kt:18)
	at com.sksamuel.hoplite.decoder.DataClassDecoder.safeDecode(DataClassDecoder.kt:99)
	at com.sksamuel.hoplite.decoder.NullHandlingDecoder$DefaultImpls.decode(Decoder.kt:96)
	at com.sksamuel.hoplite.decoder.DataClassDecoder.decode(DataClassDecoder.kt:25)
	at com.sksamuel.hoplite.internal.Decoding.decode(Decoding.kt:23)
	at com.sksamuel.hoplite.internal.ConfigParser.decode(ConfigParser.kt:71)
	at com.sksamuel.hoplite.ConfigLoader.loadConfig(ConfigLoader.kt:159)
	at TestKt.main(Test.kt:44)

Answer 4 · 2023-05-14T02:54:52.000Z

I will add the type field idea, as I agree with your points.
I just need to consider what will happen in the case of people who have a subclass with a single field called type :)

Answer 5 · 2023-05-14T03:05:51.000Z

Or in fact if you do this:

sealed interface DatabaseConfig {
    class MySql(val host: String, val type: String) : DatabaseConfig
    class Postgres(val host: String, val type: String) : DatabaseConfig
}

[database]
host = "localhost"
type = "aurora"

Answer 6 · 2023-05-14T15:43:52.000Z

I just need to consider what will happen in the case of people who have a subclass with a single field called type

One idea is to add an annotation to change the name type that is used as discriminator.

In the case you described it could fail requiring you to add the annotation. That of course would be a backwards incompatible change so it needs to be thought through.

Answer 7 · 2023-05-14T16:16:42.000Z

I'm not a fan of using annotations because it means you have to import hoplite dependency to all your modules that use your config data classes.

But your suggestion has given me the idea that we could use _type as the field, and then if there's a clash, we error saying "You need to pick another field as the type disambiguator" which would you do on the config loader.

ConfigLoaderBuilder.create().withSealedTypeField("something")

Answer 8 · 2023-05-14T18:24:03.000Z

Wouldn't that be backwards incompatible if people are already using the _type field? (Admittedly, it's a weird field name to pick, but still). Otherwise seems reasonable.

What if instead we disable the feature by default? This, other than being 100% backwards compatible, has the added benefit of allowing us to disable the automatic detecting when sealedTypeField is set.

Then in a future major version you can set the default value of sealedTypeField to _type/type, thus disabling the auto detecting in one go.

I'd also add an annotation to have the flexibility to override this global setting in some particular cases, but I understand your worries and this can always be done in the future if needed. The importing hoplite dependency can be solved by shipping this annotation in a separate "meta" package.

Answer 9 · 2023-05-14T18:34:58.000Z

My way to ensure backwards compatibility was to emit an error if the sealed subtype has _type as a field. Then it would error out saying "You need to pick another field as the type disambiguator".

Answer 10 · 2023-05-14T18:41:57.000Z

Maybe I'm misunderstanding, but if you error out on someone who is currently using a _type field, that would be a backwards incompatible change.

That's why I'm suggesting adding this feature off by default.

Answer 11 · 2023-05-14T19:57:14.000Z

I see what you mean.

Answer 12 · 2024-05-13T04:44:04.000Z

@sksamuel @MMauro94 I understand this has been merged for almost a year, but I'd like to clarify the use case with this.

The recent version seems to push me to use the sealedTypeDescriptior field with sealed types, however, what should I do when I have 2 sealed classes for different configuration groups, and none of them uses the "type" field as the discriminator? Previously I was using a custom Decoder, mapping the discriminating field into a subclass.

I would appreciate some guidance here.

Answer 13 · 2024-07-30T04:09:43.000Z

The idea would be you would add a type field to the config that specifies which concrete implementation you want to use. This type field wouldn't be used by the class itself, but by hoplite to choose.