A request with any enum having `rawValue = null` hangs forever

[japhib]

Version

4.0.0-beta.6

Summary

Enums defined in the provided graphQL schema are defined as pseudo-enums, actually classes wrapping a String member called rawValue. While it's possible to use the provided enum constants from the class, you can also create your own, possibly passing in null.

Example enum in the graphql schema:

enum StatusEnum {
    GOOD
    BAD
}

Generated StatusEnum class:

public class StatusEnum {
  public static EnumType type = new EnumType("StatusEnum", Arrays.asList("GOOD", "BAD"));
  public static StatusEnum GOOD = new StatusEnum("GOOD");
  public static StatusEnum BAD = new StatusEnum("BAD");

  public String rawValue;

  public StatusEnum(String rawValue) {
    this.rawValue = rawValue;
  }

  public static StatusEnum safeValueOf(String rawValue) {
    switch(rawValue) {
      case "GOOD": return StatusEnum.GOOD;
      case "BAD": return StatusEnum.BAD;
      default: return new StatusEnum.UNKNOWN__(rawValue);
    }
  }
// ... lots of other stuff
}

A good usage of StatusEnum would be to use the constants such as StatusEnum.GOOD, or StatusEnum.BAD. Another valid use of StatusEnum would be to parse the string "GOOD" into StatusEnum.GOOD using StatusEnum.safeValueOf("GOOD") or even new StatusEnum("GOOD").

An invalid, yet allowed, usage of StatusEnum is to pass in null to the constructor: new StatusEnum(null). This isn't checked for invalidity at any point, even up through making the GraphQL call:

var query = GetThingQuery.builder()
            // !!! Set the input enum to null !!!
            .something(new StatusEnum(null))
            .build()

// hangs forever!!
var result = Rx3Apollo.single(apolloClient.query(query), BackpressureStrategy.BUFFER).blockingGet();

When used in conjunction with Rx3Apollo.single(...).blockingGet(), a NullPointerException is thrown in another thread when attempting to serialize this value:

Exception in thread "Apollo Dispatcher" java.lang.NullPointerException: Parameter specified as non-null is null: method com.apollographql.apollo3.api.json.internal.FileUploadAwareJsonWriter.value, parameter value
	at com.apollographql.apollo3.api.json.internal.FileUploadAwareJsonWriter.value(FileUploadAwareJsonWriter.kt)
	at com.apollographql.apollo3.api.json.internal.FileUploadAwareJsonWriter.value(FileUploadAwareJsonWriter.kt:11)
	at org.example.type.adapter.StatusEnum_ResponseAdapter.toJson(StatusEnum_ResponseAdapter.java:29)
	at org.example.type.adapter.StatusEnum_ResponseAdapter.toJson(StatusEnum_ResponseAdapter.java:16)
	at com.apollographql.apollo3.api.NullableAdapter.toJson(Adapters.kt:66)
	at com.apollographql.apollo3.api.ApolloOptionalAdapter.toJson(Adapters.kt:114)
	at org.example.adapter.GetThingQuery_VariablesAdapter.serializeVariables(GetThingQuery_VariablesAdapter.java:27)
	at org.example.GetThingQuery.serializeVariables(GetThingQuery.java:110)
	at com.apollographql.apollo3.api.http.DefaultHttpRequestComposer$Companion.composePostParams(DefaultHttpRequestComposer.kt:126)
	at com.apollographql.apollo3.api.http.DefaultHttpRequestComposer$Companion.access$composePostParams(DefaultHttpRequestComposer.kt:77)
	at com.apollographql.apollo3.api.http.DefaultHttpRequestComposer$Companion.buildPostBody(DefaultHttpRequestComposer.kt:225)
	at com.apollographql.apollo3.api.http.DefaultHttpRequestComposer.compose(DefaultHttpRequestComposer.kt:71)
	at com.apollographql.apollo3.runtime.java.network.http.HttpNetworkTransport.execute(HttpNetworkTransport.java:49)
	at com.apollographql.apollo3.runtime.java.ApolloClient$NetworkInterceptor.intercept(ApolloClient.java:147)
	at com.apollographql.apollo3.runtime.java.interceptor.internal.DefaultInterceptorChain.proceed(DefaultInterceptorChain.java:35)
	at com.apollographql.apollo3.runtime.java.ApolloClient.lambda$execute$0(ApolloClient.java:131)

Because the serialization exception happens in a different thread, but we're using Rx3Apollo.single(...).blockingGet() to block until the request finishes, the result is that the current thread hangs forever.

Obviously, this is an invalid use of the generated enum -- or maybe it's valid and null should be the resulting serialized value. In any case, it would be much more useful to have the library do something (either use null or surface the exception to the calling thread) rather than just hang forever.

Possible Solutions

1. Don't allow construction of generated enums with null values - this would be nice since it would catch this particular issue at the earliest possible spot. Just a simple if (rawValue == null) throw new NullPointerException() in the constructor would suffice.
2. Check during serialization whether rawValue is null, and if so, serialize to JSON as null -- this is in case rawValue=null is a valid state and should be allowed.
3. Surface the NullPointerException up to the calling thread like any other exception might be discovered -- this is slightly less preferred, but maybe this should be happening anyway for serialization failures?

Just some ideas. I have never contributed to this library and TBH I'm still figuring out how best to use it. Please let me know the desired path forward.

Steps to reproduce the behavior

I've created a repo to reproduce this behavior in the simplest possible way: https://github.com/japhib/apollo-null-enum-test

To reproduce the bug, simply run ./gradlew test and observe that the test TestMain.testNullEnumHangs() times out after the configured 30 second timeout.

Key parts:

• A GraphQL schema, with both an enum and a query that takes that enum as an input variable:

# schema.graphqls
type Query {
    everything(something: StatusEnum): TypeA
}

type TypeA {
    status: String
}

enum StatusEnum {
    GOOD
    BAD
}

# GetThing.graphql
query GetThing($something: StatusEnum) {
    everything(something: $something) {
        status
        __typename
    }
}

• The plugin to generate Java code based on that schema

# build.gradle
plugins {
    id("com.apollographql.apollo3") version "4.0.0-beta.6"
}

• Create the query with an instance of the enum, but passing in null as the rawValue for the enum constructor:

var query = GetThingQuery.builder()
            // !!! Set the rawValue of the enum to null !!!
            .something(new StatusEnum(null))
            .build()

• Use Rx3Apollo to make the call using the specified query:

// hangs forever!!
var result = Rx3Apollo.single(apolloClient.query(query), BackpressureStrategy.BUFFER).blockingGet();

Logs

See above.

thanks!

Just wanted to say, thanks for this library! For the most part it has been great to use and has solved the problem of using GraphQL from Java in a rather elegant way. The code generation is nice to work with and the IntelliJ plugin (maybe maintained by this org?) is also very convenient to work with.

[martinbonnin]

Hi 👋 Many thanks for the detailed issue and the kind words ❤️ .

The code generation is nice to work with and the IntelliJ plugin (maybe maintained by this org?) is also very convenient to work with.

Apollo's own @BoD built most of the Apollo IJ plugin, which depends on the IntelliJ GraphQL plugin for shared functionality.

While it's possible to use the provided enum constants from the class, you can also create your own, possibly passing in null.

Looks like you're using Java codegen? (generateKotlinModels.set(false)?). If yes, can you try with the Kotlin codegen?

apollo {
  service("service") {
    generateKotlinModels.set(true)
    sealedClassesForEnumsMatching.set(listOf(".*"))
  }
}

They should make it impossible to pass null to the enum constructors.
Also the latest SNAPSHOTs use an @ApolloEnumMarker OptIn annotation on the constructor to make it clear that the constructor is not supposed to be called directly (even if it's still possible): See #5813.

[japhib]

Using the Kotlin generated code does indeed make it impossible to pass null to the enum constructors. But then I lose access to other things that are nice for Java, such as the builder interface for queries.

I'm using the latest version but the generated enum class doesn't appear to have the @ApolloEnumMarker annotation ... does that only happen for Kotlin?

Is there a problem preventing the generated enum class from making the constructor private?

[japhib]

I'm also curious how @ApolloEnumMarker is supposed to work. Seems like it creates a compiler warning unless you "opt-in" by adding a different annotation. Is that correct?

[martinbonnin]

Using the Kotlin generated code does indeed make it impossible to pass null to the enum constructors. But then I lose access to other things that are nice for Java, such as the builder interface for queries.

Rigth 👍

I'm using the latest version but the generated enum class doesn't appear to have the @ApolloEnumMarker annotation ... does that only happen for Kotlin?

Indeed, this is only for Kotlin, and only for the SNAPSHOTs versions

Is there a problem preventing the generated enum class from making the constructor private?

Excellent question. IIRC there was some concern that some advanced use cases could still want to send unknown (to the client) enums (that would be known by the server). This could happen for an an example if the enum is passed "out of band" to an older app. You can read https://slack-chats.kotlinlang.org/t/16930109/apollo-kotlin-generates-an-unknown-entry-in-all-enums-to-be- for some background although I'm not sure what the ultimate decision was (if there was one). Maybe @BoD will remember.

I'm also curious how @ApolloEnumMarker is supposed to work. Seems like it creates a compiler warning unless you "opt-in" by adding a different annotation. Is that correct?

Correct 👍

All in all, we should add a non-null assertion for Java codegen. Whether we can do better while still keeping the constructor accessible is not clear to me. Java doesn't have @OptIn annotations but maybe there are other ways....

[japhib]

Thanks for the answers.

All in all, we should add a non-null assertion for Java codegen.

I support this. Seems like the simplest solution going forward.

[BoD]

What do you think of:

1. add a non-null assertion in safeValueOf
2. making the class constructor private
3. also making the UNKNOWN__ constructor private

2 and 3 are breaking changes but it's probably for the better

[martinbonnin]

Wouldn't that prevent users to use a custom rawValue if they really want to? cf "there was some concern that some advanced use cases could still want to send unknown (to the client) enums (that would be known by the server)"

[BoD]

Wouldn't that prevent users to use a custom rawValue if they really want to?

safeValueOf("foo") does it

[martinbonnin]

Gotcha 👍 Perfect plan ✅

[BoD]

Made the change in #5904 which will be included in the next beta release of v4. The related change has also be made in the generated Kotlin enums when using sealed classes: now safeValueOf must be used.