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

Using variables in scalars #1011

Open
cutsoy opened this issue Jan 24, 2023 · 8 comments
Open

Using variables in scalars #1011

cutsoy opened this issue Jan 24, 2023 · 8 comments

Comments

@cutsoy
Copy link

cutsoy commented Jan 24, 2023

I would like some clarification on using variables in scalars.

Given the following schema:

scalar Custom

type Query {
    hello(custom: Custom!): String!
}

We know that the following query could be valid (depending on Custom's wire format):

query Example {
    hello(custom: { name: "Tim" })
}

Would the following example also be valid?

query Example($name: String!) {
    hello(custom: { name: $name })
}

If so, does the executor first resolve $name and then call the scalar's deserializer with { name: "Tim" }?

Can someone shed some light on this?

@andimarek
Copy link
Contributor

Hi @cutsoy ,

while not explicitly mentioned in the GraphQL spec, custom Scalars are responsible for resolving variables references themself.

Almost all custom Scalars are simple enough that this is never an issue, but more complex one need to take care of resolving variables. For example you can see the logic resolving variable values for a JS JSON Scalar here: https://github.com/taion/graphql-type-json/blob/master/src/index.js#L39

or the same logic for a Java implementation here: https://github.com/graphql-java/graphql-java-extended-scalars/blob/master/src/main/java/graphql/scalars/object/ObjectScalar.java#L82

The reason why custom Scalars are responsible for resolving variables reference themself, is that it is actually up to the custom Scalars to define the semantics of a variable reference like $name. In theory you could define a custom Scalar which doesn't resolve variable reference, but interprets them differently. The AST element is called "variable reference", but the semantics are up to the custom Scalar to define.

Hope that helps.

@cutsoy
Copy link
Author

cutsoy commented Jan 24, 2023

Ah, interesting!

Does this also imply that scalar literals aren't checked for undefined variables? For example, would the following be valid (depending on the schema)?

query Example {
    hello(custom: { name: $name })
}

(note that $name isn't defined)

@benjie
Copy link
Member

benjie commented Jan 24, 2023

No, that wouldn't be valid because it fails https://spec.graphql.org/draft/#sec-All-Variable-Uses-Defined

@cutsoy
Copy link
Author

cutsoy commented Jan 24, 2023

Got it, thanks!

And also, are the variables already parsed before they are passed into the variables argument of parseLiteral?

Specifically, this line suggests that the variables are already parsed (hence unknown):
https://github.com/graphql/graphql-js/blob/8be83d8d528991aed04ca17434b7e26e56f649cf/src/type/definition.ts#L620

Example:

scalar Foo
scalar Bar

query Example($foo: Foo!) {
    greeting(bar: { foo: $foo })
}

In this case, would Bar.parseLiteral be called like this?

const $foo = Foo.parseValue("example");

Bar.parseLiteral(
    ast`{ foo: $foo }`, // ast
    { $foo },           // variables
)

... or like this?

Bar.parseLiteral(
    ast`{ foo: $foo }`,  // ast
    { $foo: "example" }, // variables
)

@cutsoy
Copy link
Author

cutsoy commented Jan 24, 2023

Hm, surprisingly, it looks like the graphql-js implementation calls parseLiteral twice in this example (assuming { foo: "example" } are passed as variables).

  1. Bar.parseLiteral is called without variables (which would probably result in a crash if Bar was reasonably implemented).
  2. Foo.parseValue is called without variables.
  3. Bar.parseLiteral is called a second time with variables being the result of Foo.parseValue.

Why is Bar.parseLiteral first called without variables?

@benjie
Copy link
Member

benjie commented Jan 24, 2023

I think you should redirect those questions to the graphql-js repo.

@martinbonnin
Copy link
Contributor

I think it's worth a clarification in the spec?

With the given example:

query Example($name: String!) {
    hello(custom: { name: $name })
}

Spec says:

* Let locationType be the expected type of the Argument, ObjectField or ListValue
entry where variableUsage is located.

But it's not really possible to know the type of the usage here.

Maybe add an explicit case for custom scalars?

* Let locationType be the expected type of the Argument, ObjectField or ListValue
entry where variableUsage is located or null if used in a custom scalar literal
* If locationType is null return true
* Else if locationType is a non-null type...

@benjie
Copy link
Member

benjie commented Feb 10, 2023

I agree a change to the spec may be necessary, perhaps you'd like to raise an RFC with your proposed edits and add it to an upcoming GraphQL Working Group? e.g. https://github.com/graphql/graphql-wg/blob/main/agendas/2023/03-Mar/02-wg-primary.md

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

4 participants