Skip to content
New issue

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

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

Already on GitHub? # to your account

GraphQLScalarType.parse_literal is called with two arguments instead of one when Query variables are present. #93

Closed
rafalp opened this issue May 18, 2020 · 7 comments

Comments

@rafalp
Copy link
Contributor

rafalp commented May 18, 2020

It has been reported to us that when custom scalar with parse_literal is given literal value in query that has been called with variables, function passed to parse_literal will be called with two arguments instead of one: ast of literal and dict with query variables:

query TestQuery($variable: ID) {
  field(scalar: "STRING")
  otherField(id: $variable)
}

This produces tricky to debug error, eg.:

Argument 'scalar' has invalid value "STRING"

However actual error is TypeError for function being called with two arguments instead of one.

However GraphQL.js documentation doesn't mention this behaviour:
https://graphql.org/graphql-js/type/#graphqlscalartype

I've found that value_from_ast_untyped is called with two arguments in situation when query variables are present, but couldn't find anything about custom parse_literal.

Is this intended?

@Cito
Copy link
Member

Cito commented May 20, 2020

That variables parameter is intended, it exists since GraphQL.js v0.12. So I think the GraphQL.js documentation is outdated and I have just reported this. Maybe we should do something about the misleading error message nevertheless.

@Cito
Copy link
Member

Cito commented May 20, 2020

The problem at runtime is that value_from_ast catches all exceptions from parse_literal and maps them to Undefined, i.e. it does not discern between exceptions raised inside the method and those caused by a wrong signature. But I don't know any simple way of discerning these. If you have an idea, let me know.

I have tried to mitigate the problem by adding better type hints in 0f81d95 (not sure why that comment said this was not possible - maybe because of the impedance mismatch when using the callable as a method, but that warning has been silenced anyway).

@rafalp
Copy link
Contributor Author

rafalp commented Jun 14, 2020

I've did some experimenting with this and found that literal_parser is called two times. First time its called without variables, and if it raises error, this error points to location in query. Second time its called with variables (but those are already parsed values), and if it errors, it points to field in query that failed.

Could you please explain this behaviour? I'm updating Ariadne's docs on custom scalars knowing why this happens would help me a lot here.

Thanks!

@Cito
Copy link
Member

Cito commented Jun 15, 2020

The first time the parse_literal function is called happens during validation when the ValueOfCorrectType validator is executed. The whole validation is based on the document alone, the variable values are disregarded by the validator, and therefore are not passed to the function.

The second time the parse_literal function is called happens when the query is actually executed. This time the variable values are evaluated and passed to the function.

This is all ported 1:1 from GraphQL.js. If your question is why they are actually passing the variables to parse_literal at all, I currently cannot give a good answer. The built-in scalars do not make use of this in any case.

@Cito
Copy link
Member

Cito commented Jun 16, 2020

I have now created graphql-js#2657 to clarify this issue.

Cito added a commit that referenced this issue Jul 5, 2020
@Cito
Copy link
Member

Cito commented Jul 5, 2020

To sum up the current state: The variables parameter is in fact needed to support complex scalar types. The problem that it is not passed during validation seems to be a oversight and should be fixed in GraphQL.js. For compatibility sake, I did not change the behavior. I have added a test_custom_scalar test module to demonstrate the usage of the parameter. This will be hopefully improved in GraphQL.js and then ported to GraphQL-core.

@Cito Cito closed this as completed Jul 5, 2020
@rafalp
Copy link
Contributor Author

rafalp commented Jul 5, 2020

Makes sense. Thanks for investigating this!

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

No branches or pull requests

2 participants