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

Coercing Variable Values when hasValue is not true and defaultValue does NOT exist #1044

Closed
NikiAtSalesforce opened this issue Sep 8, 2023 · 7 comments

Comments

@NikiAtSalesforce
Copy link

NikiAtSalesforce commented Sep 8, 2023

Edited to fix the example and provide some additional clarification.

As far as I can tell the spec is missing the case when a variable does not have a value and does not have a default value.

For example with this spec

type Object {
    id: ID
    name: String
}

input Where {
    id: ID
    name: String
}

type Query {
    object(where: Where): Object
}

And this query:

query byName($name: String) {
    object(where: { name: $name }) {
        id
    }
}

What should be the expected behavior if no value is provided for $name?

In the graphql-java implementation the behavior is to omit the field entirely from the input object. So the query ends up resolving to:

query {
 object(where: { }) {
  id
 }
}

Here is what the spec says:

CoerceVariableValues(schema, operation, variableValues)
Let coercedValues be an empty unordered Map.
Let variableDefinitions be the variables defined by operation.
For each variableDefinition in variableDefinitions:
Let variableName be the name of variableDefinition.
Let variableType be the expected type of variableDefinition.
Assert: IsInputType(variableType) must be true.
Let defaultValue be the default value for variableDefinition.
Let hasValue be true if variableValues provides a value for the name variableName.
Let value be the value provided in variableValues for the name variableName.
If hasValue is not true and defaultValue exists (including null):
Add an entry to coercedValues named variableName with the value defaultValue.
Otherwise if variableType is a Non-Nullable type, and either hasValue is not true or value is null, raise a request error.
Otherwise if hasValue is true:
If value is null:
Add an entry to coercedValues named variableName with the value null.
Otherwise:
If value cannot be coerced according to the input coercion rules of variableType, raise a request error.
Let coercedValue be the result of coercing value according to the input coercion rules of variableType.
Add an entry to coercedValues named variableName with the value coercedValue.
Return coercedValues.

But there is no case for no value supplied and no default value for a nullable input.

@jangko
Copy link

jangko commented Sep 11, 2023

null is value too, it should be coerced into no-value or empty.

@martinbonnin
Copy link
Contributor

In the graphql-java implementation the behavior is to use the default value of the input object that contains the field referencing the variable.

In pure GraphQL Java, you have DataFetchingEnvironment.containsArgument(String) to detect such cases. But maybe higher level abstractions like Spring integrations the behaviour is different?

All in all, I'd say this doesn't belong in the spec. Different resolver handle the absence of an argument differently but resolver implementations are outside the spec.

@NikiAtSalesforce
Copy link
Author

NikiAtSalesforce commented Sep 11, 2023

@martinbonnin Sorry, I got the example I posted slightly wrong. Here is an updated version that illustrates the issue:

type Object {
    id: ID
    name: String
}

input Where {
    id: ID
    name: String
}

type Query {
    object(where: Where): Object
}

query byName($name: String) {
    object(where: { name: $name }) {
        id
    }
}

If no variables are suppled in the request, this gets resolved to:

query byName($name: String) {
    object(where: { }) {
        id
    }
}

It is now not possible to determine if the query should be filtered by id or by name. DataFetchingEnvironment.containsArgument(String) isn't adequate here because the argument exists, but the field on the argument does not.

That said, my actual point here is that the spec does not define how to resolve an input that uses a variable when the variable does not have a value and the input does not have a default value. This leaves the behavior undefined. The two reference implementations I used both do the same thing, but it seems like it wouldn't be out of spec to use null or for the server to return an error.

@martinbonnin
Copy link
Contributor

It's the same thing. The handling of absent input values is left to your resolver. In your GraphQL Java resolver, you'll have something like so:

Map<String, Object> map = environment.getArgument("where") as Map<String, Object>;
map.containsKey("id") // false
map.containsKey("name") // false

So your resolver has to decide what to do and in your specific case, certainly return an error if it doesn't not know if it should filter by name or id. (BTW, you ca model this with enums instead to avoid this case)

All in all, undefined and null are different. Take this exemple:

input UserInput {
  name: String
  phoneNumber: String
  email: String
}

mutation DeleteEmail {
  # delete email, leave name and phoneNumber unchanged
  updateUser(userInput: {email: null}) {
    status
  }
}

It's up to your resolver to know how to handle the different cases, the spec doesn't know how to interpret them.

@NikiAtSalesforce
Copy link
Author

NikiAtSalesforce commented Sep 12, 2023

@martinbonnin But based on my reading of the spec it doesn't say that what the value should be if hasValue is false and there is no default. Since it is not specified in the spec, a resolver could reasonable resolve the value to 5 for anything that is undefined and that would not violate the spec. If the intention is that the value should remain undefined the spec should explicitly say so.

@martinbonnin
Copy link
Contributor

I think it's in the spec? Your initial post is about variable coercion but what you're looking for is more like argument coercion?

https://spec.graphql.org/October2021/#sel-NANRHHCJFTDFBDCAACGBq0B:

Let coercedValue be the result of coercing value according to the input coercion rules of argumentType.

https://spec.graphql.org/October2021/#sel-HAHhBVHBABAB2KhtV:

If no value is provided for a defined input object field and that field definition provides a default value, 
the default value should be used. If no default value is provided and the input object field’s type is 
non-null, an error should be raised. Otherwise, if the field is not required, then no entry is added 
to the coerced unordered map.

@NikiAtSalesforce
Copy link
Author

@martinbonnin yes thank you! I missed that part of the spec. I will close this issue.

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

3 participants