Default argument for object with no required fields fails to build in Xcode

[tt-pkaminski]

Summary

Example:

I have a type that has no required fields. There is a default argument for the only field

input MyType {
    a: Float = 1.0
}

In my query, I provide a default argument

query myQuery($arg: MyType! = {})

When I run code gen, there are no errors from the script and I get the following output:

struct MyType {
    public init(a: GraphQLNullable<Double>) {
        ...
    }
}

class MyQuery {
    public init(arg: MyType = MyType()) {...}
}

However, because the initializer for MyType does not include either a default argument of 1.0 or nil, the generated code fails to compile

MyQuery.graphql => Missing argument for parameter #1 in call

Version

1.5

Steps to reproduce the behavior

See the above code for an reproducible example

Logs

No response

Anything else?

No response

[AnthonyMDev]

Thanks so much for the bug report! We will definitely need to address this for the next release. We have a release slated to go out tomorrow, so this fix won't make it into that one, but we'll get to this ASAP.

There is some complex nuances about default argument handling, so we're going to need to discuss what the actual expected behavior here is. But once we have that defined, I don't anticipate it will be much work to write the actual code to make whatever behavior we want here actually happen.

[AnthonyMDev]

I’d like to discuss how to proceed with this issue. @martinbonnin @BoD @BobaFetters @calvincestari

Internally, we’ve had some past conversations about default arguments and how they are handled in apollo ios and kotlin.
The gist of those conversations was:

• Don’t generate default values for fields on input types that are provided in the schema
  • This is because the actual default value could change on the server side without notifying the client side.
  • So if we hard coded in that generated default value, it would stay consistent on the client side, but server side changes wouldn’t be respected
  • And defaulting to nil would use the server's default value, which could change out from underneath the client
  • By not providing a default here, the user can provide a value or explicitly pass in nil, which would use the server’s default value
    • In this case the default is subject to change server-side, but the user has explicitly opted in to this behavior
• Do generate default argument values that are defined in the query operation’s.
  • Because the client controls these operation definitions, they have the ability to set the default values. So these should be respected.

In this issue, these concepts are combined. The client operation is defining a default argument for the MyType input object of {}. This would seem to indicate the user's intent to use an input object with just the default values. But the default values for the field on the input object are defined in the schema (ie. they are subject to change server side). I'm not sure that the possibility of these values changing is clear to the user.

What should we do in this case? I see a few possible solutions, but I’m not sure which (if any) of these are consistent with our current philosophy and design here.

• Option A: Disallow this:
  • Fail during validate and require them to add the a client-side default value for the field to the input argument.
  • They would have to change query myQuery($arg: MyType! = {}) to query myQuery($arg: MyType! = { a: 1.0 })
  • This seems most consistent with our design philosophy.
  • But the user seems to be explicitly indicating that they want to use the schema’s default value for the a field, and we aren’t letting them.
  • We allow them to do that by providing MyType(a: nil), in the swift code, but not in the graphql operation definition.
• Option B: Interpret the default value of {} as the user indicating they want to use the schema’s default value,
  • This implicitly means the value may change on the server without the client being notified.
  • This feels unclear to the user, but they are explicitly making that choice.
• Option C: Create the generated default value, hard coding in the current default value for a at the time code generation is ran.
  • Our generated initializer becomes public init(arg: MyType = MyType(a: GraphQLNullable<Float> = 1.0)) {...}
  • This feels closer to what our current design intent is
  • But it also contradicts the actual GraphQL specification functionality here.
    • Because sending query myQuery($arg: MyType! = {}) would use the current schema's default value for a on the schema side.

Any preferences or opinions on how we should proceed here?

[martinbonnin]

My vote goes to not generating anything in codegen and just omit the variable in the request (Option B?):

class MyQuery {
    public init(arg: GraphQLNullable<MyType> = nil) {...}
}

and send this over the wire:

{
  "query": "query myQuery($arg: MyType! = {})",
  "variables": {}
}

Then the server will know how to interpret the variable default value from the document and use the current MyType.a value.

Another fun edge case is when custom scalar are involved:

input Filter {
  before: Date
}

query GetEvents($filter: {before: "2021-11-14"}) {
   events(filter: $filter) {
     id
     title
     # ...
   }
}

So now you need to call the user code to parse "2021-11-14" into some Date object so your initializer becomes even more complicated. All in all, this complexity isn't required as all the information is in the document already (+ allows to change the value server side)

[AnthonyMDev]

and send this over the wire:

{
  "query": "query myQuery($arg: MyType! = {})",
  "variables": {}
}

Yeah, that's definitely not what we are doing right now. And that does mean we end up parsing custom scalars when it's perhaps not necessary. But it also avoids this bug that you mentioned to me @martinbonnin.

Making the change to this approach for all fields would probably be a pretty significant change. It would mean we would need to mitigate the aforementioned caching bug. It would also technically be a breaking change. Because currently, we initialize an actual input value struct with all the default values you provided in the operation definition (See the generated model example here). You can actually inspect and even mutate those values. So this approach would mean we would make the entire input struct nil. The default values would be sent over the wire, but you wouldn't be able to interact with them at run time.

Given the PetSearchQuery generated here, you could no longer do this:

let query = PetSearchQuery()
query.filters.species = ["Bird"] // filters would be nil, and default value would only be interpreted at GraphQL execution time server-side.
client.fetch(query) { ... }

---

But, I think this my proposed Option B is viable strictly for fields on an input type that are omitted from a default argument for that input type and have a default value on the schema.

So given this input type:

input MyType {
    a: Float = 1.0
    b: String
}

And this definition:

query myQuery($arg: MyType! = {b: "Hello World"})

Here the user is creating a default value for the MyType input type. The a field on that type can be inferred to use the default value.

We could generate the models:

struct MyType {
    public init(
        a: GraphQLNullable<Double>,
        b: GraphQLNullable<String>
    ) {
        ...
    }
}

class MyQuery {
    public init(arg: MyType = MyType(
            a: GraphQLNullable<Double>,
            b: GraphQLNullable<String> = "Hello World"
        )
    ) {...}
}

// a is omitted from network request and uses schema's default value. 
// b is sent as "Hello World"
let query = MyQuery(arg: MyType(a: nil)) 

// a is sent as 10.5
// b is sent as "Goodbye"
let query = MyQuery(arg: MyType(a: 10.5, b: "Goodbye")) 

Does that seem like a reasonable solution here? Is the fact that this only applies to fields on an input type that have a default value confusing?

[martinbonnin]

Given the PetSearchQuery generated here, you could no longer do this query.filters.species = ["Bird"] (...)

Could you force a specific value of filter at runtime?

let query = PetSearchQuery()
query.filters = PetSearchFilters(species = ["Bird"])
client.fetch(query) { ... }

We could generate the models:

class MyQuery {
    public init(arg: MyType = MyType(
            a: GraphQLNullable<Double>,
            b: GraphQLNullable<String> = "Hello World"
        )
    ) {...}
}

I think that'd work.

The 2 limitations I can foresee:

1. it's adding some codegen complexity: getting the custom scalars and enums to work reliably can generate complex Swift/Kotlin code.
2. if the defaultValue is large, it's transmitted over the wire. Someone using persisted queries will not be able to save those bytes because variables are always transmitted.

But it also avoids apollographql/apollo-kotlin#5186 that you mentioned to me @martinbonnin.

Regarding caching for Apollo Kotlin, I'm currently planning to either:

• parse the defaultValue from the document. Requires a mini GraphQL value parser embedded in the runtime but maybe it's not too bad. But doesn't work for persisted queries where the document is not in the binary.
• more probably generate a Kotlin representation of the default value as a Map at compile time that is only used by cache code and can be tree-shaken for non-cache users (also saves the need to deal with enums and custom scalars, it's all done pre-coercion):

val argDefaultValue = mapOf(
  "key" to "value",
  "date" to "2023-11-15"
)

Now for more fun times, there's the question of what to do with default values that are defined on the server and are subject to change without the cache knowing about it. Should we include those in the cache key or not?

input Config {
  language: String = "fr"
}

query GetMessage($config: Config = {}) {
   localizedMessage(config: $config)
}

If we're using localizedMessage({config: fr}) as a cache key and the server changes the default to en, we start putting english messages in the french cache key, which may be surprising.

If we're using localizedMessage({}) as a cache key then another message specifying the language explicitly cannot reuse that cache entry, which is also surprising.

I'm not sure what to do there. Maybe Option A. and disabling this altogether makes more sense. But maybe only for cache users because there are legitimate usages for non-cache users? Not sure...

Is the fact that this only applies to fields on an input type that have a default value confusing?

Inputs with default values are confusing because GraphQL doesn't have the notion of undefined vs null but unfortunately, it's a sad reality we have to live with so I don't think there's a easy way out there. Once we solve nullability for output types we should do a working group for input types nullability 😄

[AnthonyMDev]

Hi @martinbonnin, I'm looking at this again for 2.0 since it possibly requires a breaking change to fix. Did Apollo Kotlin make a decision on how to handle this?

I'm wondering now if we should just default the value to nil, making the generated initializers look like:

struct MyType {
    public init(a: GraphQLNullable<Double> = nil) {
        ...
    }
}

class MyQuery {
    public init(arg: MyType = MyType()) {...}
}

This most accurately mirrors what the user is stating in their operation definition.

My initial concern with this approach was:

defaulting to nil would use the server's default value, which could change out from underneath the client.

And while it's still true that the default value on the server could change, changing behavior for existing clients, I now think that's actually intended here. If you are writing the graphql operation definition query myQuery($arg: MyType! = {}) { ... } and just sending it over HTTP yourself, you are telling the server to use it's default value. If the server changes its default value and then you send that same query again, it will get a different result (or at least it could since the resolved value of $arg is different).

I think I was trying to protect the users from something when I was actually preventing an explicitly allowed behavior of the GraphQL spec.

Correct me if I'm wrong, but this means we also avoid the issue you've encountered since we would still be initializing and sending the object with client defined default values (just not schema defined ones).

[AnthonyMDev]

This is what I've done for 2.0. The initial compile error in this issue is resolved. I've looked into the rest of the conversation, and I'm not convinced that we have a perfect solution for the other bug that was mentioned, but I don't think there is a right way to resolve this. It's a minor bug that people can likely workaround specifically deciding how to handle the edge case. I'm closing this now.