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

Validation spec: fields implementation with default values #3214

Closed
vepanimas opened this issue Jul 21, 2021 · 3 comments
Closed

Validation spec: fields implementation with default values #3214

vepanimas opened this issue Jul 21, 2021 · 3 comments

Comments

@vepanimas
Copy link

Hi! I'm the maintainer of the GraphQL for IntelliJ IDEA plugin. I have a question about validating field implementations when a default value is specified. I looked through the specification and found no explicit mention of checking default values in the field validation process.

Should I check default values at all? If so, I know three cases in which it is unclear to me whether the implementation of the interface is correct or should I display an error:

  1. Only an interface specifies a default value, but the implementor doesn't.
interface Interface {
    field(a: String = "1"): String
}

type Query implements Interface {
    field(a: String): String
}
  1. Ony an implementation specifies a default value:
interface Interface {
    field(a: String): String
}

type Query implements Interface {
    field(a: String = "1"): String
}
  1. Both specify default values but their values are different:
interface Interface {
    field(a: String = "ABC"): String
}

type Query implements Interface {
    field(a: String = "1"): String
}
@yaacovCR
Copy link
Contributor

I think this is a great question, and possibly exposes a problem with the specification, but not the one mentioned exactly. As far as I can tell, default values on interfaces have no effect at runtime, as we always use the default from the concrete runtime type, and so all of the above are valid, because the default on the interface is kind of meaningless.

Even in this case:

{
  ... on SomeInterface {
    someFieldWithDefaultValueForArg
  }
}

We always use the individual default for the concrete runtime type. One might think we could suggest a change in the grammar to disallow setting a default on the field arguments for interfaces, or a separate validation rule.

On the other hand, I think I have spotted one case in which validation looks at the default on the interface, presumably when it shouldn't, i.e. the Required Arguments are Provided Rule : https://spec.graphql.org/draft/#sec-Required-Arguments which as written and implemented seems to refer to the interface fields themselves in the above scenario, so that if an interface type and an implementing object both had a field with an argument with a non-nullable type, but within the interface field, a default was specified, then it would be optional in the above case, even though it would be without a value at runtime based on my understanding.

E.g.:

interface Interface {
    field(a: String! = "ABC"): String
}

type Query implements Interface {
    field(a: String!): String
}

# The below should pass validation, but crash at runtime
query {
  ... on Interface {
    field
  }
}

I have to verify this behavior within the reference implementation, but that's my best guess at the moment.

@yaacovCR
Copy link
Contributor

Added a test where the interface field passes validation but we get a runtime error:

@yaacovCR
Copy link
Contributor

Closing this issue here. I think progress can be better tracked at graphql/graphql-spec#1121 as the reference implementation is in line with the current specification. This definitely seems to be a spec issue => if I understand it right.

Thanks so much @vepanimas for bringing this up.

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

No branches or pull requests

2 participants