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

QueryComplexityRule seems to attempt to parse input variables repeatedly #1468

Open
ingluisjimenez opened this issue Oct 10, 2023 · 2 comments

Comments

@ingluisjimenez
Copy link

I am trying to enable the QueryComplexity rule and noticing that when the rule executes, it attempts to parse the values of the inputs multiple times for each Field definition.

The issue I am running into is that I have a custom scalar type that does some validation when parseValue() or parseLiteral() are called and log when it fails validation, and this validation process seems to be running (hundreds) multiple times, while I would expect that code to run at most once while the QueryComplexity is traversing through the schema. This ends up generating hundreds of error log lines when the custom scalar type fails validation, but also, it seems sub-optimal in that the rule attempts to extract/parse the same values from the input over and over.

Is this intentional behavior? Should my custom scalar type memoize the return value of parseValue() and prevent triggering the validation repeatedly?

@spawnia
Copy link
Collaborator

spawnia commented Oct 11, 2023

I guess the following call is repeated:

$fieldArguments = $this->buildFieldArguments($node);

Perhaps we can apply memoization in the implementation of the QueryComplexity rule?

@ingluisjimenez
Copy link
Author

Thank you for following up @spawnia .

I am surprised this hasn't been brought up before as it seems something that would impact performance on a large graph and that's why I thought maybe I was doing something wrong.

It sounds like memoization would help to prevent calling Values::getVariableValues() multiple times, but buildFieldArguments() would still need to be called multiple times, as it is attempting to extract the arguments for a specific field/node, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants