Add addJvmOverloads support to Input types

[GokhanArik]

Version

v3.8.2

Summary

Problem
Adding a new nullable field to the schema is a “safe” operation from the point of GQL Schema checks, but it is not safe on the client side since the binary of the generated code changes. This may result in NoSuchMethodExceptions at runtime.

Example:

input Login {
  username: String!
  password: String!
  rememberMe: Boolean // A new Optional field is added
}

The input type above generates the following data class:

data class Login(
    val username: String,
    val password: String
)

If we modify the schema,

input Login {
  username: String!
  password: String!
  rememberMe: Boolean // A new Optional field is added
}

the generated data class constructor changes

data class Login(
    val username: String,
    val password: String,
    val rememberMe: Optional<Boolean> = Optional.Absent
)

When precompiled libraries call Login("user", "1234"), they will receive `NoSuchMethodException.

Proposal
We should extend the functionality added with #3848 and add addJvmOverloads support to Input types. Adding this support to Fragment Data Classes would be nice as well, but I don't know how this would work since they don't have default values. Possibly, we can add a flag that defaults nullable fragment arguments to null.

Considerations

• Adding @JvmOverloads will not make it fully binary compatible. This only works if you add the new parameter to the end of the constructor. Since the schema dictates the order, this will be difficult to control on the client side but can be handled when designing the schema.
• Even if you add the argument to the end, the copy() function will not be binary compatible. However, this may not be a problem for the majority of the use cases.
• In v4, generateInputBuilders can be used to mitigate this. Can generateInputBuilders be extended to Operations and Fragments as well?

Steps to reproduce the behavior

No response

Logs

(Your logs here)

[martinbonnin]

Good catch!

See also #4659 for a similar issue with argument order.

I think you're 100% right that addJvmOverloads should also apply to Input types. A PR would be very welcome!

In the longer term, I'd aim for generateInputBuilders as it seems to be a more robust solution to that problem space. Builders have been around in Java for as long as I can remember and they also have the advantage of being able to express null vs absent (see #4747).

To your question, I think generateInputBuilders also applies to operations and fragments. The "input" is more about "input value" than "input type". There is still time to bikeshed this name though as it's never been released in a stable version. Any suggestion?

Even with generateInputBuilders, there is still the question of required arguments order (see graphql/graphql-spec#1062). One way to workaround would be to enforce named parameters (see https://youtrack.jetbrains.com/issue/KT-14934)

[GokhanArik]

+1 to input builders and making constructors private.

I see, it makes sense to call it "input" but it may sound confusing. Maybe generateBuilders? If not, I think it is okay as it is. We can explicitly call this out in the documentation.

I don't see how (https://github.com/graphql/graphql-spec/issues/1062)[https://github.com/graphql/graphql-spec/issues/1062] would cause issues with builders. Maybe I'm missing something.

Another option to consider is to move away from data classes to fully fletched classes for better maintainability as outlined in this article.

[martinbonnin]

Maybe generateBuilders

Could work. 2 potential caveats:

• not all generated classes support generating builders. All the output models for an example do not have Builders. So without more context, it's hard to tell where builders are going to be generated.
• it somehow clashes with Data Builders. That's a lot of builders and I wish we could keep a more descriptive name.

Another way to look at this, especially if we were to make Builders the default would be to have the opposite: generateConstructors or so... Naming is hard 😅

I don't see how graphql/graphql-spec#1062 would cause issues with builders.

Sorry I meant to link #5375 instead. If you have 2 mandatory parameters in your Builder constructor and they are the same type, changing the order could break the app at runtime without breaking build time. The tradeoff is:

• Either we lose the ability to require arguments (if everything is a Builder funciton)
• Or we risk arguments changing order without the compiler noticing (if unamed arguments are used)

Another option to consider is to move away from data classes to fully fletched classes for better maintainability as outlined in this article.

Funny you mentioned that article, I had this in mind too :). AFAIK, data classes are relatively used for programmatically updating the cache (where you end up using a lot of .copy). I also think there's a general expectation that toString() is nice. Not sure about equals/hashCode but in the given state of things, I wouldn't change the default data class. We could make it yet another codegen option though.

[GokhanArik]

If you have 2 mandatory parameters in your Builder constructor and they are the same type, changing the order could break the app at runtime without breaking build time

I haven't used the input builders yet so I'm not familiar how it works internally, but when I get a chance and when we start the spike for migration I will take a look.

The more we look into this, the more problems are surfacing. We discovered that @JvmOverloads works for Java clients but doesn't provide backward compatibility for Kotlin clients. The issue is reported here.

For Java clients @JvmOverloads may work but for Kotlin clients I think builders will be a much better option going forward.

Example:
Let's say you have this data class in v1 and FooTester precompiled with v1

data class Foo @JvmOverloads constructor(
    val a: String = "a",
    val b: String = "b",
)

class FooTester {

    val validCase1 = Foo(a = "a", b= "b")
    val validCase2 = Foo("a", "b")
    val validCase3 = Foo(a = "a", "")

    val failureCase1 = Foo("a")
    val failureCase2 = Foo(a = "a")
    val failureCase3 = Foo()
}

You add a new parameter c in v2.

data class Foo @JvmOverloads constructor(
    val a: String = "a",
    val b: String = "b",
    val c: String = "c",
)

When the library gets updated transitively failureCase1, failureCase2, failureCase3 will throw NoSuchMethod error.

Cause

If you decompile to Java, you will see those constructors are calling the synthetic constructor:

And the signature of the synthetic constructor changes between two versions:

From:

To:

[martinbonnin]

Makes a lot of sense 👍 . Good old Builders will live forever! :).

I still think we should add the @JvmOverloads to input object types for consistency as long as we support addJvmOverloads

Re: generateInputBuilders, there are the questions of:

1. naming
2. wheter we enable them by default
3. whether we make required argument parts of the Builder constructor

If you get a chance to enable generateInputBuilders and let us know what you think, that'd be a great data point to decide whether or not we should make them the default.

[BoD]

V4 is now out, which includes generateInputBuilders. If you have a chance to enable it, please don't hesitate to give us your feedback on it.

[martinbonnin]

FWIW, I changed my mind on @JvmOverloads. The more I look into this, the more I think generateInputBuilders should be the default so I'd rather not invest too much on the other thing.

See also #6095

[GokhanArik]

Yes, I agree with the approach. JvmOverloads only helps with Java, not Kotlin. I will close this.

[github-actions]

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Kotlin usage and allow us to serve you better.