Skip to content

Optimize views #1439

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Oct 29, 2023
Merged

Optimize views #1439

Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4b42599
Optimize execute_graphql_request
danthewildcat Aug 3, 2023
314c68d
Require operation_ast to be found by view handler
danthewildcat Aug 3, 2023
33b3426
Remove unused show_graphiql kwarg
danthewildcat Aug 3, 2023
94550ac
Old style if syntax
danthewildcat Aug 3, 2023
7d84e87
Merge branch 'main' into optimize-views
firaskafri Aug 4, 2023
ba70076
Merge branch 'main' into optimize-views
danthewildcat Aug 7, 2023
f8af0eb
Merge branch 'main' into optimize-views
kiendang Oct 26, 2023
b24ed31
Revert "Remove unused show_graphiql kwarg"
kiendang Oct 26, 2023
f48d54b
Add missing schema validation step
kiendang Oct 26, 2023
2e4c6a2
Pass args directly to improve clarity
kiendang Oct 29, 2023
352db54
Remove duplicated operation_ast not None check
kiendang Oct 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 66 additions & 32 deletions graphene_django/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,18 @@
from django.utils.decorators import method_decorator
from django.views.decorators.csrf import ensure_csrf_cookie
from django.views.generic import View
from graphql import OperationType, get_operation_ast, parse
from graphql import (
ExecutionResult,
OperationType,
execute,
get_operation_ast,
parse,
validate_schema,
)
from graphql.error import GraphQLError
from graphql.execution import ExecutionResult
from graphql.execution.middleware import MiddlewareManager
from graphql.language import OperationDefinitionNode
from graphql.validation import validate

from graphene import Schema
from graphene_django.constants import MUTATION_ERRORS_FLAG
Expand Down Expand Up @@ -295,56 +303,82 @@ def execute_graphql_request(
return None
raise HttpError(HttpResponseBadRequest("Must provide query string."))

schema_validation_errors = validate_schema(self.schema.graphql_schema)
if schema_validation_errors:
return ExecutionResult(data=None, errors=schema_validation_errors)

try:
document = parse(query)
except Exception as e:
return ExecutionResult(errors=[e])

if request.method.lower() == "get":
operation_ast = get_operation_ast(document, operation_name)
if operation_ast and operation_ast.operation != OperationType.QUERY:
if show_graphiql:
return None
operation_ast = get_operation_ast(document, operation_name)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the old logic would at some point call get_operation_ast so I hoisted it up


raise HttpError(
HttpResponseNotAllowed(
["POST"],
"Can only perform a {} operation from a POST request.".format(
operation_ast.operation.value
),
)
if not operation_ast:
ops_count = len(
[
x
for x in document.definitions
if isinstance(x, OperationDefinitionNode)
]
)
if ops_count > 1:
op_error = (
"Must provide operation name if query contains multiple operations."
)
try:
extra_options = {}
if self.execution_context_class:
extra_options["execution_context_class"] = self.execution_context_class
elif operation_name:
op_error = f"Unknown operation named '{operation_name}'."
else:
op_error = "Must provide a valid operation."

return ExecutionResult(errors=[GraphQLError(op_error)])
Copy link
Contributor Author

@danthewildcat danthewildcat Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If operation_ast is not found it will fail eventually anyway; throw it here to simplify other validation checks like for the GET request and for the transaction checks for mutation ops.

Copy link
Collaborator

@sjdemartini sjdemartini Oct 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a minute to realize why this new op_error logic above is included, and I see it's trying to provide a helpful error message to the user for the specific cases that get_operation_ast ends up returning None, which seems nice.

Question though: is it always true that execute will necessarily error in the cases that get_operation_ast returns None? If not, I worry that this new logic will introduce errors in some cases where we didn't used to error. Before, POST requests seemed to still call execute even if the AST result was None. Based on a search in the graphql-core repo, it seems that get_operation_ast is just a utility and not used internally, so I can't easily detect if there are similar scenarios that raise errors.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this logic is ported from here https://github.com/graphql-python/graphql-core/blob/0c93b8452eed38d4f800c7e71cf6f3f3758cd1c6/src/graphql/execution/execute.py#L708-L727. This is called during execute.

We also have a test

graphene-django/graphene_django/tests/test_views.py

Lines 121 to 139 in 36cf100

def test_errors_when_missing_operation_name(client):
response = client.get(
url_string(
query="""
query TestQuery { test }
mutation TestMutation { writeTest { test } }
"""
)
)
assert response.status_code == 400
assert response_json(response) == {
"errors": [
{
"message": "Must provide operation name if query contains multiple operations.",
}
]
}

Thus I don't think this will change the code behavior (throwing errors it didn't before). Though I do agree that not doing it ourselves is better since the checked is run inside execute anyway so this is basically unnecessary duplicated code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, good finds, agreed then that it doesn't make sense to reimplement here. Thanks!


if (
request.method.lower() == "get"
and operation_ast.operation != OperationType.QUERY
):
if show_graphiql:
return None

raise HttpError(
HttpResponseNotAllowed(
["POST"],
"Can only perform a {} operation from a POST request.".format(
operation_ast.operation.value
),
)
)

execute_args = (self.schema.graphql_schema, document)
validation_errors = validate(*execute_args)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: it seems slightly awkward/unnecessary to be using the execute_args when calling validate() here, since validate doesn't have the same signature as execute; they just happen to share the same first two args. How about just removing the execute_args var, passing those two arguments directly here to validate, and adding the schema and document as part of execute_options dictionary below?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree


if validation_errors:
return ExecutionResult(data=None, errors=validation_errors)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing version called self.schema.execute which is basically just a wrapper to calling graphql_impl which performs the following steps:

  1. validate_schema -> we are doing this here
  2. parse -> we already did this operation a few lines up. This operation is surprisingly expensive to we get a nice optimization by not having to redundantly call it
  3. validate -> This actually also calls validate_schema (though schema validation is cached so that particular piece isn't done redundantly)

So rather than calling self.schema.execute I am replacing the validate call and then calling execute rather than redundantly parsing and validating the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing version called self.schema.execute which is basically just a wrapper to calling graphql_impl which performs the following steps:

  1. validate_schema -> we are doing this here
  2. parse -> we already did this operation a few lines up. This operation is surprisingly expensive to we get a nice optimization by not having to redundantly call it
  3. validate -> This actually also calls validate_schema (though schema validation is cached so that particular piece isn't done redundantly)

Rather than calling self.schema.execute I am reimplementing the validate call and then calling execute directly rather than redundantly parsing and validating the request.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you put this part in a separate PR? I can review and merge this part now.


options = {
"source": query,
try:
execute_options = {
"root_value": self.get_root_value(request),
"context_value": self.get_context(request),
"variable_values": variables,
"operation_name": operation_name,
"context_value": self.get_context(request),
"middleware": self.get_middleware(request),
}
options.update(extra_options)
if self.execution_context_class:
execute_options[
"execution_context_class"
] = self.execution_context_class

operation_ast = get_operation_ast(document, operation_name)
if (
operation_ast
and operation_ast.operation == OperationType.MUTATION
and (
graphene_settings.ATOMIC_MUTATIONS is True
or connection.settings_dict.get("ATOMIC_MUTATIONS", False) is True
)
):
graphene_settings.ATOMIC_MUTATIONS is True
or connection.settings_dict.get("ATOMIC_MUTATIONS", False) is True
) and operation_ast.operation == OperationType.MUTATION:
with transaction.atomic():
result = self.schema.execute(**options)
result = execute(*execute_args, **execute_options)
if getattr(request, MUTATION_ERRORS_FLAG, False) is True:
transaction.set_rollback(True)
return result

return self.schema.execute(**options)
return execute(*execute_args, **execute_options)
except Exception as e:
return ExecutionResult(errors=[e])

Expand Down