Optimize views #1439

danthewildcat · 2023-08-03T13:49:49Z

This PR is a no-op optimization:

Makes it so that the expensive parse call for parsing the request body into valid graphql is not called redundantly
Fails faster if the request does not have a valid operation_ast
Removes passing (and handling) the show_graphiql kwarg between the view methods

danthewildcat · 2023-08-03T14:44:54Z

graphene_django/views.py

-            if operation_ast and operation_ast.operation != OperationType.QUERY:
-                if show_graphiql:
-                    return None
+        operation_ast = get_operation_ast(document, operation_name)


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

danthewildcat · 2023-08-03T16:38:28Z

graphene_django/views.py

+        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)])


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.

danthewildcat · 2023-08-03T16:49:07Z

graphene_django/views.py

+        execute_args = (self.schema.graphql_schema, document)
+        validation_errors = validate(*execute_args)
+
+        if validation_errors:
+            return ExecutionResult(data=None, errors=validation_errors)


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

validate_schema -> we are doing this here

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

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.

danthewildcat · 2023-08-03T16:56:40Z

graphene_django/views.py

@@ -186,7 +186,7 @@ def dispatch(self, request, *args, **kwargs):
                    or 200
                )
            else:
-                result, status_code = self.get_response(request, data, show_graphiql)
+                result, status_code = self.get_response(request, data)


The preceding code does not allow show_graphiql to ever be True here:

graphene-django/graphene_django/views.py

Lines 153 to 156 in 10793be

show_graphiql = self.graphiql and self.can_display_graphiql(request, data)

if show_graphiql:

return self.render_graphiql(

Ergo, there is no reason to pass it around or pivot any other behavior on whether show_graphiql is True or not so I have removed this pivot to simplify downstream logic.

danthewildcat · 2023-08-03T17:02:19Z

graphene_django/views.py

+        execute_args = (self.schema.graphql_schema, document)
+
+        if validation_errors := validate(*execute_args):
+            return ExecutionResult(data=None, errors=validation_errors)


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

validate_schema -> we are doing this here

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

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.

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

kiendang · 2023-08-03T18:38:32Z

@danthewildcat is this related to #1393 (comment)?

danthewildcat · 2023-08-03T21:31:24Z

@danthewildcat is this related to #1393 (comment)?

Yes, this comment addresses the redundant parse logic mentioned by @erikwrede

This reverts commit 33b3426.

kiendang · 2023-10-26T12:09:47Z

Pushed 2 commits to add some small changes

Added the missing schema validation step in graphql_impl
Keep the show_graphiql arg. Agree that this arg is somewhat redundant but removing it is a non-backward compatible change.

Thanks @danthewildcat this will speed up execute_graphql_request and also paves the way for implementing support for validations rules (#1342, #1472)

sjdemartini

Overall this looks really good! Thanks both of you for putting this together! 🙌 Basically just one question and one minor code suggestion.

sjdemartini · 2023-10-29T01:35:55Z

graphene_django/views.py

+        execute_args = (self.schema.graphql_schema, document)
+        validation_errors = validate(*execute_args)


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?

sjdemartini · 2023-10-29T01:55:23Z

graphene_django/views.py

+            else:
+                op_error = "Must provide a valid operation."
+
+            return ExecutionResult(errors=[GraphQLError(op_error)])


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.

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.

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

sjdemartini

Sweet, LGTM!

* Optimize execute_graphql_request * Require operation_ast to be found by view handler * Remove unused show_graphiql kwarg * Old style if syntax * Revert "Remove unused show_graphiql kwarg" This reverts commit 33b3426. * Add missing schema validation step * Pass args directly to improve clarity * Remove duplicated operation_ast not None check --------- Co-authored-by: Firas Kafri <[email protected]> Co-authored-by: Kien Dang <[email protected]>

danthewildcat closed this Aug 3, 2023

danthewildcat reopened this Aug 3, 2023

danthewildcat force-pushed the optimize-views branch 2 times, most recently from 861c50a to 09556b6 Compare August 3, 2023 14:12

danthewildcat commented Aug 3, 2023

View reviewed changes

danthewildcat force-pushed the optimize-views branch 2 times, most recently from 9c5bca6 to 10793be Compare August 3, 2023 16:36

danthewildcat commented Aug 3, 2023

View reviewed changes

danthewildcat added 3 commits August 3, 2023 12:59

Optimize execute_graphql_request

4b42599

Require operation_ast to be found by view handler

314c68d

Remove unused show_graphiql kwarg

33b3426

danthewildcat force-pushed the optimize-views branch from 10793be to 33b3426 Compare August 3, 2023 16:59

danthewildcat commented Aug 3, 2023

View reviewed changes

Old style if syntax

94550ac

firaskafri and others added 2 commits August 4, 2023 11:16

Merge branch 'main' into optimize-views

7d84e87

Merge branch 'main' into optimize-views

ba70076

firaskafri requested review from firaskafri, kiendang and jaw9c September 7, 2023 07:24

firaskafri approved these changes Sep 7, 2023

View reviewed changes

This was referenced Oct 25, 2023

Query Limiting Depth #1472

Closed

Optimize execute_graphql_request #1474

Closed

kiendang added 3 commits October 26, 2023 20:03

Merge branch 'main' into optimize-views

f8af0eb

Revert "Remove unused show_graphiql kwarg"

b24ed31

This reverts commit 33b3426.

Add missing schema validation step

f48d54b

kiendang requested a review from sjdemartini October 26, 2023 12:10

sjdemartini reviewed Oct 29, 2023

View reviewed changes

kiendang added 2 commits October 29, 2023 12:08

Pass args directly to improve clarity

2e4c6a2

Remove duplicated operation_ast not None check

352db54

sjdemartini approved these changes Oct 29, 2023

View reviewed changes

kiendang merged commit e735f5d into graphql-python:main Oct 29, 2023

firaskafri mentioned this pull request Oct 29, 2023

Remove redundant call to validate #1393

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize views #1439

Optimize views #1439

danthewildcat commented Aug 3, 2023 •

edited

Loading

danthewildcat Aug 3, 2023

danthewildcat Aug 3, 2023 •

edited

Loading

danthewildcat Aug 3, 2023

danthewildcat Aug 3, 2023

danthewildcat Aug 3, 2023

kiendang Aug 27, 2023

kiendang commented Aug 3, 2023

danthewildcat commented Aug 3, 2023

kiendang commented Oct 26, 2023

sjdemartini left a comment

sjdemartini Oct 29, 2023

kiendang Oct 29, 2023

sjdemartini Oct 29, 2023 •

edited

Loading

kiendang Oct 29, 2023

sjdemartini Oct 29, 2023

sjdemartini left a comment

	show_graphiql = self.graphiql and self.can_display_graphiql(request, data)

	if show_graphiql:
	return self.render_graphiql(

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

	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.",
	}
	]
	}

Optimize views #1439

Optimize views #1439

Conversation

danthewildcat commented Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

danthewildcat Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiendang commented Aug 3, 2023

danthewildcat commented Aug 3, 2023

kiendang commented Oct 26, 2023

sjdemartini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjdemartini Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjdemartini left a comment

Choose a reason for hiding this comment

danthewildcat commented Aug 3, 2023 •

edited

Loading

danthewildcat Aug 3, 2023 •

edited

Loading

sjdemartini Oct 29, 2023 •

edited

Loading