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

_Any scalar not handling variable correctly #113

Open
v3i1y opened this issue Jun 25, 2021 · 4 comments
Open

_Any scalar not handling variable correctly #113

v3i1y opened this issue Jun 25, 2021 · 4 comments

Comments

@v3i1y
Copy link

v3i1y commented Jun 25, 2021

query($arg1: String!, $arg2: String!, $arg3: String!){
  _entities(representations: [{
    __typename: "SomeEntity",
    arg1: $arg1, 
    arg2: $arg2, 
    arg3: $arg3
  }]) {
    ... on SomeEntity {
      someField
    }
  }
}

fails on this line Looks like it's missing the case of VariableReference.

It's minor because gateway doesn't send this kind of query, but it can happen on manual testing and unit tests.

@sachindshinde
Copy link
Contributor

@v3i1r4in
This has to do with a limitation of the GraphQL spec with regards to scalars.

In your example, you are providing a literal value

{
    __typename: "SomeEntity",
    arg1: $arg1, 
    arg2: $arg2, 
    arg3: $arg3
}

for the scalar type _Any.

However, the rules for Input Coercion for Scalars do not make any mention of substitution of a variable's runtime value (this is in contrast to e.g. the rules for Input Coercion for Input Objects).

Accordingly, the APIs of GraphQL libraries usually don't support this kind of substitution in scalars. Even if we were to add case-handling for VariableReference in the line you've cited, we wouldn't be able to perform variable substitution with the variable's runtime value, as graphql-java doesn't provide us with those values during scalar input coercion. (Arguably though in such cases, _Any's parseLiteral() function should throw CoercingParseLiteralException instead of an AssertException in order to abide by the method contract, so I'll file a PR to fix that.)

The general alternative is to either fully embed the scalar in the GraphQL operation as a literal value, e.g.

{
    __typename: "SomeEntity",
    arg1: "foo", 
    arg2: "bar", 
    arg3: "baz"
}

or pass a variable reference for the scalar type _Any instead of a literal value.

@v3i1y
Copy link
Author

v3i1y commented Jun 29, 2021

as graphql-java doesn't provide us with those values during scalar input coercion

I think it does, https://github.com/graphql-java/graphql-java/blob/5396ab20f6aae315327c14a77ff2f66c8fd403c7/src/main/java/graphql/schema/Coercing.java#L98. Although not on the spec, this is supported by the javascript version, it would be nice to match the behavior.

@sachindshinde
Copy link
Contributor

@v3i1r4in
Ah, that's a good catch! It looks like it was added in graphql-java/graphql-java#1170 , which was released in graphql-java v9.7.

Given that graphql-js and graphql-java both support this, I'd be fine with doing the variable substitution in parseLiteral(), and will file a PR for the update.

@v3i1y
Copy link
Author

v3i1y commented Jun 29, 2021

that's perfect, thanks!

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