Apollo-kotlin generates an unknown entry in all en...

# apollo-kotlin

[Stylianos Gakis]

Apollo-kotlin generates an unknown entry in all enums to be safe for when backend may add a new entry so old clients don't explode.
I realize now I was passing this unknown type to the backend in a mutation as input, as a default for when I didn't find the right thing to pass, for whatever reason. This unknown of course isn't part of the schema itself, but I am allowed to do it in my Kotlin code since it's just another entry in the enum that the mutation is taking in.

I am now I think realizing that this mutation fails 100% of the time, and I don't think it happens in the backend code where they receive this enum (as a String in their case, using dgs) but I think this fails at a validation step before our code even runs in the first place.

Does this problem ring a bell as a common scenario, and if yes is there a way to safeguard myself not to make this mistake again in the future? 👀

[bod]

Interesting!

I think this fails at a validation step before our code even runs in the first place

hm I don't think it should happen? Do you have a stacktrace?

[Stylianos Gakis]

I am still early in figuring out what’s wrong, the one error message I found in our logs is “`Variable 'context' has an invalid value: Invalid input for enum 'ChatMessageContext'. No value found for name 'UNKNOWN__'`”

[mbonnin]

variables are my bet?

[mbonnin]

yea variables are not known at build time

[mbonnin]

What we need is a different enum type for input vs output positions 😅

[kevin.cianfarini]

sealed interface SomeSchemaEnum {
  enum class Input : SomeSchemaEnum {
    ValueOne, ValueTwo, ValueThree
  }

  data object Unknown : SomeSchemaEnum
}

Maybe something like this would help?

[Stylianos Gakis]

So the enum is

enum ChatMessageContext {
    FOO,
    BAR,
}

And it is then in an input as

input SomeInput {
    otherField: ID!
    context: ChatMessageContext
}

And here I passed the UNKNOWN, but that’s not meant for the input, only for forwards compat for things coming towards our app, not from us to backend I suppose 😄

[Stylianos Gakis]

Shit 😅

[Stylianos Gakis]

So it is expected in this scenario since I send them this weird input (as far as they are concerned) that the backend will just fail early to process this input in the first place right?

[mbonnin]

Yep, it's a validation error from the server

[Stylianos Gakis]

And very optimally, if apollo-kotlin made it simply impossible for me to even pass this extra type to the input then it would solve this problem all together. Be it with a solution like the one that Kevin suggests, or something else 👀

[mbonnin]

💯

[kevin.cianfarini]

It would be nice if it were clear what comes from the schema versus what's generated for safety

[mbonnin]

Now the issue is how deep this goes. First thing that comes to mind is JSON adapters use the same type for input vs output. We have Adapter<MyEnum>

[mbonnin]

@Stylianos Gakis mind opening an issue?

[Stylianos Gakis]

https://github.com/apollographql/apollo-kotlin/issues/5796 Please re-word the title if what I wrote does not make total sense, I had a hard time finding the right words for everything I am trying to describe 😄

[wasyl]

Oh I think this is a good thread to paste one of the bits that we added in 3.x migration I think:

// TODO: Ask for an common supertype for enums on Apollo side? Or figure something out to get rid of `Any.` extension
//  Maybe there's an API in Apollo where one can hook into fallback enum conversion?
internal fun <T> Any.mapUnknownEnumValue(
    owner: String,
    target: T,
) = target.also { log.w { "Unknown $owner, value=$this" } }

that we use when mapping any Apollo enum, for example:

rating = when (val rating = rating) {
    Rating.COMPETENT -> ApiTalentSkill.Rating.Competent
    Rating.EXPERT -> ApiTalentSkill.Rating.Expert
    Rating.STRONG -> ApiTalentSkill.Rating.Strong
    is Rating.UNKNOWN__ -> rating.mapUnknownEnumValue("Rating", ApiTalentSkill.Rating.Competent)
}

The idea is to log an error for UNKNOWN__ for devs, but fall back to a safe value for the users.

An alternative to enum class Input : SomeSchemaEnum (which btw I'd call KnownValue or something, as the issue isn't exactly with input vs output, right?) maybe Apollo could define a wrapper for enums, pretty much like Optional? The Unknown doesn't have anything enum-specific inside anyway

[Nicolas Chaduc]

The solution is not just to transform UNKNOWN to null and use Input.optional() ?

[wasyl]

You also have the same enums in outputs/queries, and you can have a null already there. And if generated code emits Optional<SomeEnum?> for nullable enum fields, then we're missing the raw value returned by the API (which IMO is a must for logging/debugging)

[mbonnin]

To add on top of everything that has been said, there's actually a use case where people actually want to be able send UNKNOWN__(rawValue)
This is when the value is transparent to the client code. You get it from the server and pass it back in without needing to know what the value means

[mbonnin]

Maybe it's a bad value but maybe it's actually supported by the server

[mbonnin]

@Stylianos Gakis are you using sealedClassesForEnumsMatching?

[wasyl]

I mean, it should be, since the server clearly knows about it 🤔 This makes me wonder why it doesn't for OP

[Stylianos Gakis]

Backend just knows,

enum ChatMessageContext {
    FOO,
    BAR,
}

it does not know about that unknown value at all

And this mutation fails before it even reaches their code as it seems, DGS is just erroring out on some sort of validation step.
The tracing goes something like:
servlet.request -> spring.handler -> graphql.request
|-> graphql.parsing
|
|-> graphql.validation

Where it just throws an exception

[Stylianos Gakis]

sealedClassesForEnumsMatching

No, not using that https://github.com/HedvigInsurance/android/blob/38557b0fada325958b99c1af3ca07999a3bbd8d6/app/apollo/apollo-octopus-public/build.gradle.kts#L21-L58

[mbonnin]

What about we make sealedClassesForEnumsMatching the default and mark the UNKNOWN__ constructor with an Opt-In marker or so?

[mbonnin]

This way, only the parsers can build an UNKNOWN__ enum. Meaning it came from the backend and it's there's a very good chance it's valid there

[Stylianos Gakis]

I would be happy with that tbh yeah, changing the default would make sense perhaps for the 4.x release too anyway?

[mbonnin]

Yea, we probably need to do that quickly if we want to do that

[bod]

@mbonnin nice one! At least we can start by adding the opt-in, independently of making sealed classes the default

[mbonnin]

Indeed. Changing the default is going to be breaking a bunch of codebases

[bod]

IIRC we almost did this for 3.x but reverted to keeping enums by default - but maybe it's time! 😄

[mbonnin]

@ApolloDelicateEnum ?

[Stylianos Gakis]

If it’s part of the migration guide I don’t see it being too big of a problem for a major bump like 4.x.
But maybe I shouldn’t speak for everybody, I am not sure if everyone checks out the guide ☠️

[bod]

there's also the IDE plugin's migration helper - not sure how feasible it would be to refactor the code automatically. But if it is, that's one less friction to make the change.

[Stylianos Gakis]

The plugin as a first step can just add the sealedClassesForEnumsMatching.set(false) if it turns out to be hard to migrate 😂

[Stylianos Gakis]

Or wait, this is a list, nvm

[kevin.cianfarini]

To add on top of everything that has been said, there's actually a use case where people actually want to be able send UNKNOWN__(rawValue)

This is when the value is transparent to the client code. You get it from the server and pass it back in without needing to know what the value means

In this scenario I would think that the backing string value for the enum of an unknown value should be marked as internal somehow so that only Apollo's runtime could access it.

Perhaps a value class wrapped around a string with an internal value. Like:

// generated 
data class UNKNOWN_(val value: OpaqueEnumValue)

// Apollo runtime 
data class OpaqueEnumValue(internal val jsonValue: String)

This would allow Apollo users to pass around the unknown value without ever being able to infer what it's carrying. I think that's a nice safety guarantee given the current semantics of unknown values dont expose what they were over the wire.

[agrosner]

Would love the default true for sealed classes for enums to match the swift implementation. We at Wayfair explicitly don’t allow enums on queries to be used as input arguments at all, they must exist as separate types. So it goes deep at our schema layer to prevent this issue. Also can lead to multiple subgraph enum issues similar to how it’s described here between app and graph

[Yang]

I know we are doing it wrong 🙃 but we have a custom tool that generates mappers from Apollo code to our own models, so switching the default to sealed class would require us to rework the tool or / retrain our devs on how to use the tool (not that they should)

[mbonnin]

@Yang I wonder how big of a change it would be. While binary breaking, the change should be mostly compatible: symbols have the same names, etc... I'm curious what breaks if you change to use sealed classes

[mbonnin]

In all cases, not using sealed classes would still be an option for some time. In that future, you'll be able to set sealedClassesForEnumsMatching.set(emptyList()) and revert back to plain enums

[Yang]

yeah it’s probably not a huge effort as we basically need to teach the processor how to map from a sealed class to enum

[mbonnin]

Oh so you're using KSP to parse the generated code or so?

[Yang]

yeah, we also have a kapt version but hopefully we can remove it by the time 4.0 is out