Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows nullable variables in non-null locations #3227

Closed
darylsze opened this issue Jul 9, 2021 · 3 comments · Fixed by #3879
Closed

Allows nullable variables in non-null locations #3227

darylsze opened this issue Jul 9, 2021 · 3 comments · Fixed by #3879

Comments

@darylsze
Copy link

darylsze commented Jul 9, 2021

Currently I am migrating AppSync sdk (although it embedded an internal Apollo sdk, it is way too outdated) to Apollo sdk v2.
However, I found that below query will be compile error:

query myQueryABCD(
    $page: Int = 1,
    $first: Int = 20
) {
      myQueryABCD(first: $first, page: $page) {
          ...MyFragmentABCD
      }
}

because in query definition, the field first is optional with default value, but in schema query first is required.
So i change the query to below:

query myQueryABCD(
    $page: Int = 1,
    $first: Int! = 20 // adding '!' as required
) {
      myQueryABCD(first: $first, page: $page) {
          ...MyFragmentABCD
      }
}

Then apollo client can generate query class successfully, and project got compile. Yeah!!
However, the problem is apollo client respond error because of Variable "$first" of type "Int!" is required and will not use the default value. Perhaps you meant to use type "Int".

I would suggest

  1. allowing required field to have default value, or
  2. do not return error. Just warn me in project
@darylsze darylsze changed the title Required field with default value should not be runtime error Required field with default value should not return error Jul 9, 2021
@darylsze darylsze changed the title Required field with default value should not return error Required field with default value should not return error but warning Jul 9, 2021
@martinbonnin
Copy link
Contributor

martinbonnin commented Jul 9, 2021

Hi 👋

I found that below query will be compile error:

This is most likely a bug in Apollo Android. The spec allows nullable variables in non-null locations when a default value exist:
http://spec.graphql.org/draft/#sec-All-Variable-Usages-are-Allowed.Allowing-optional-variables-when-default-values-exist
It's not going to be easy to fix but we should fix it.

Variable "$first" of type "Int!" is required and will not use the default value. Perhaps you meant to use type "Int"

This is most likely an error in your backend. There was a change in the GraphQL spec to support that use case: graphql/graphql-spec#418 that is most likely not ported there.

Overall both queries appear valid but trigger bugs in either Apollo Android or your backend validation logic. I'll look into the Apollo Android part. Until this is fixed, a workaround would be to remove the defaultValue and always set the variable from Kotlin code. Or if that's too many places to change, maybe add a Kotlin wrapper? Something like:

fun MyQueryABCD(
    page: Input<Int?> = Input.absent(),
    first: Int = 20
): MyQueryABCD {
  return MyQueryABCD(page, first)
}

@martinbonnin martinbonnin changed the title Required field with default value should not return error but warning Allows nullable variables in non-null locations Jul 26, 2021
@martinbonnin
Copy link
Contributor

@darylsze
Copy link
Author

darylsze commented Jul 26, 2021

thanks for the following up. On my client side, i have replaced all query in which contains both required type + default value combination into required type only, the default value, on the other hand are assigned through kotlin code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants