Skip to content

when passing vars to an attribute with multiple args make sure parse_literal consumes at least 2 params #169

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

Closed

Conversation

fugal-dy
Copy link
Contributor

Problem:
This PR addresses the problem I encountered when querying with filter-variables with a list of filter arguments
of which at least one value is a scalar type that does not interpolate any variable. At the parse_literal() call site
the test for existence of variables is misleading resulting in a false return of Undefined because of a mismatch
of parse_literal() signatures of different types, some taking variables as a second parameter, some not (e. g. graphene.types.generic.GenericScalar . In any case the presence of variables is not a sufficient criteria.

Solution:
variables may be passed to a scalar type that does not do anything with them. We assume that the scalar is still a valid case that should not evaluate to Undefined. In order to disregard the passed variables we check for the implemented parse_literal() method to actually consume a second parameter.

Example case: evaluation of a GraphQLList type that recurringly invokes parse_literal
passing the same variables regardless of whether the variables passed actually
are relevant for the scalar currently evaluated.

`variables` may be passed to a scalar type that does not do anything
with them while it is still a valid case we do not want to evaluate
to `Undefined`.

Example case: evaluation of a `GraphQLList` type that recurringly invokes `parse_literal`
passing the same `variables` regardless of whether the variables passed actually
are relevant for the scalar currently evaluated.
@fugal-dy fugal-dy requested a review from Cito as a code owner April 29, 2022 16:18
@Cito
Copy link
Member

Cito commented Apr 30, 2022

Thanks @fugal-dy for bringing this up.

The API of GraphQL-core actually requires GraphQLScalarType.parse_literal to accept an optional second parameter. If Graphene does not respect that API, wouldn't it be better to fix it in Graphene? I believe that inspect.signature comes with a performance penalty and makes the code more complicated, so I would want to avoid it if not necessary.

Also, can you provide a simple runnable example that demonstrates the issue you want to solve? This will help me decide on the best way to solve this. Generally, I prefer if a PR is accompanied with an issue containing exaplanations and runnable example code. This also helps me to create unit tests if they are not part of the PR.

@fugal-dy
Copy link
Contributor Author

fugal-dy commented May 2, 2022

I agree, this here would mostly make concessions rather than sovle the problem. I'll open an issue in graphene and link it here

@fugal-dy
Copy link
Contributor Author

fugal-dy commented May 3, 2022

fyi @Cito here's the issue with code and the fix. Took me a while to find the most generic and simple example (which is quite a far cry from the code I ran when it occurred to me ;)

@fugal-dy fugal-dy marked this pull request as draft May 3, 2022 12:00
@fugal-dy fugal-dy changed the title When passing vars make sure parse_literal consumes at least 2 params when passing vars to an attribute with multiple args make sure parse_literal consumes at least 2 params May 3, 2022
@Cito
Copy link
Member

Cito commented May 6, 2022

Thank you @fugal-dy - your fix is now merged into the master branch of Graphene.

@Cito Cito closed this May 6, 2022
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

Successfully merging this pull request may close these issues.

2 participants