-
Notifications
You must be signed in to change notification settings - Fork 765
Reduce cognitive complexity and typo fixes #1457
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
base: main
Are you sure you want to change the base?
Changes from all commits
ad447d9
0e1b605
cd6e042
ea77bc7
6580228
1b00e67
5c52096
2a888ac
b639034
d873220
67373c7
722daf1
34e61c6
988b308
cb92d6e
19d5558
34db851
98fd498
a94a89d
59c0117
e263840
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,7 +128,7 @@ def _client(self, client): | |
) | ||
self.client = client | ||
|
||
def assertResponseNoErrors(self, resp, msg=None): | ||
def assert_response_no_errors(self, resp, msg=None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a breaking change for consumers of this package. I would recommend at least preserving an alias with the original camelCase name. I imagine it was named this way for consistency with Django (and Python's) camelCase assertion methods: https://docs.djangoproject.com/en/4.2/topics/testing/tools/#assertions |
||
""" | ||
Assert that the call went through correctly. 200 means the syntax is ok, if there are no `errors`, | ||
the call was fine. | ||
|
@@ -138,7 +138,7 @@ def assertResponseNoErrors(self, resp, msg=None): | |
self.assertEqual(resp.status_code, 200, msg or content) | ||
self.assertNotIn("errors", list(content.keys()), msg or content) | ||
|
||
def assertResponseHasErrors(self, resp, msg=None): | ||
def assert_response_has_errors(self, resp, msg=None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, avoid breaking changes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sjdemartini thanks for the replies and for your very important suggestions - I'll take care of these soon enough. |
||
""" | ||
Assert that the call was failing. Take care: Even with errors, GraphQL returns status 200! | ||
:resp HttpResponse: Response | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,16 +103,15 @@ def __init__( | |
): | ||
if not schema: | ||
schema = graphene_settings.SCHEMA | ||
self.schema = schema or self.schema | ||
|
||
if middleware is None: | ||
middleware = graphene_settings.MIDDLEWARE | ||
if isinstance(middleware, MiddlewareManager): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know if I should've kept the check for Unless |
||
self.middleware = middleware | ||
else: | ||
self.middleware = list(instantiate_middleware(middleware)) | ||
|
||
self.schema = schema or self.schema | ||
if middleware is not None: | ||
if isinstance(middleware, MiddlewareManager): | ||
self.middleware = middleware | ||
else: | ||
self.middleware = list(instantiate_middleware(middleware)) | ||
self.root_value = root_value | ||
self.pretty = pretty or self.pretty | ||
self.graphiql = graphiql or self.graphiql | ||
|
@@ -285,6 +284,25 @@ def parse_body(self, request): | |
|
||
return {} | ||
|
||
def validate_query_request_type( | ||
self, request, document, operation_name, show_graphiql | ||
): | ||
if request.method.lower() == "get": | ||
operation_ast = get_operation_ast(document, operation_name) | ||
if ( | ||
operation_ast | ||
and operation_ast.operation != OperationType.QUERY | ||
and not show_graphiql | ||
): | ||
raise HttpError( | ||
HttpResponseNotAllowed( | ||
["POST"], | ||
"Can only perform a {} operation from a POST request.".format( | ||
operation_ast.operation.value | ||
), | ||
) | ||
) | ||
|
||
def execute_graphql_request( | ||
self, request, data, query, variables, operation_name, show_graphiql=False | ||
): | ||
|
@@ -298,20 +316,12 @@ def execute_graphql_request( | |
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 | ||
self.validate_query_request_type( | ||
request, document, operation_name, show_graphiql | ||
) | ||
if show_graphiql: | ||
return None | ||
|
||
raise HttpError( | ||
HttpResponseNotAllowed( | ||
["POST"], | ||
"Can only perform a {} operation from a POST request.".format( | ||
operation_ast.operation.value | ||
), | ||
) | ||
) | ||
try: | ||
extra_options = {} | ||
if self.execution_context_class: | ||
|
@@ -328,14 +338,7 @@ def execute_graphql_request( | |
options.update(extra_options) | ||
|
||
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 | ||
) | ||
): | ||
if self.is_atomic_mutation_enabled(operation_ast, connection): | ||
with transaction.atomic(): | ||
result = self.schema.execute(**options) | ||
if getattr(request, MUTATION_ERRORS_FLAG, False) is True: | ||
|
@@ -400,3 +403,14 @@ def get_content_type(request): | |
meta = request.META | ||
content_type = meta.get("CONTENT_TYPE", meta.get("HTTP_CONTENT_TYPE", "")) | ||
return content_type.split(";", 1)[0].lower() | ||
|
||
@staticmethod | ||
def is_atomic_mutation_enabled(operation_ast, connection): | ||
return ( | ||
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 | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention for the
resolve_*
methods for anObjectType
(whichConnection
is) is to useparent
as the first argument, notself
, since these are "implicit static methods": https://docs.graphene-python.org/en/latest/types/objecttypes/#naming-convention