Reduce cognitive complexity and typo fixes by lilac-supernova-2 · Pull Request #1457 · graphql-python/graphene-django

lilac-supernova-2 · 2023-08-26T07:27:58Z

Hello there!

Just wanna say I love this project so I went ahead and did some small contributions.
Pretty much just splitting out some functions that were too big, and fixed some typos.

While doing this I got paranoid with unit tests so I added a tests-repeat command to the Makefile that will allow you to run all unit tests, 100 times, in parallel - this can be useful in the future to catch bugs and/or flaky tests that can fail from time to time (which, after implementing, got some occasions where the tests failed but worked nicely after re-running - so maybe there are either undiscovered bugs and/or some flaky tests whose configuration - could be due to using some randomness on some tests - fail, well, randomly)

The only function with a pretty high cognitive complexity that I didn't tackle (currently at 29) is the graphene_django.types.DjangoObjectType.__init_subclass_with_meta__ because it already looks pretty sequential and IMO not too hard to understand, just a lot of things that need to happen in it.

Tested and formatted the code as per the contributing guidelines. Let me know if you have any questions and if you're ok with this contribution!

…uce cognitive complexity

… multiple times + in parallel in one shot

lilac-supernova-2 · 2023-08-26T08:06:30Z


        if middleware is None:
            middleware = graphene_settings.MIDDLEWARE
+        if isinstance(middleware, MiddlewareManager):


Let me know if I should've kept the check for middleware is not None here - my thought process was that we already check above if it's none, so probably we don't need to check if now it is not none.

Unless graphene_settings.MIDDLEWARE can be None - then this is needed and I can re-add it back in.

sjdemartini

Thanks for making improvements throughout graphene-django! Would it be possible to break this up into a couple different PRs: one with just the typo/content fixes, and one with actual code changes? I haven't taken the time to go through meticulously and validate that the refactored code matches the original behavior, and personally less sure how necessary that is (I think complexity scores only tell you so much). The typo fixes and whatnot seem like an easy thing to merge first. (On the code changes, I just left a couple comments where there are breaking changes to method names, which I don't think we should merge as is. If there are other breaking changes to graphene-django method signatures we should avoid those as well.)

sjdemartini · 2023-08-28T15:09:59Z

    test = String()

-    def resolve_test():
+    def resolve_test(self):


The convention for the resolve_* methods for an ObjectType (which Connection is) is to use parent as the first argument, not self, since these are "implicit static methods": https://docs.graphene-python.org/en/latest/types/objecttypes/#naming-convention

sjdemartini · 2023-08-28T15:10:41Z

        self.client = client

-    def assertResponseNoErrors(self, resp, msg=None):
+    def assert_response_no_errors(self, resp, msg=None):


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

sjdemartini · 2023-08-28T15:10:49Z

        self.assertNotIn("errors", list(content.keys()), msg or content)

-    def assertResponseHasErrors(self, resp, msg=None):
+    def assert_response_has_errors(self, resp, msg=None):


Same here, avoid breaking changes

@sjdemartini thanks for the replies and for your very important suggestions - I'll take care of these soon enough.

- examples/starwars/data.py: rename camelCase locals (homeOne, tieFighter, tieInterceptor) to snake_case to match Python style. - graphene_django/filter/utils.py: fix "Graphql" -> "GraphQL" in the get_field_type docstring and "no explicitly declared" -> "not explicitly declared" inline comment. Other typo fixes from PR graphql-python#1457 (Dictonary, Confuesd, Millenium, firtsnaMe, perserved, compatability, choises, inheritence, bellow) were already merged into main since 2023 via the follow-up PR graphql-python#1459. Made-with: Cursor

``resolve_*`` methods on a Graphene ``ObjectType`` are implicit static methods and are invoked by graphene-core with ``(parent, info, **args)``. The previous ``def resolve_test():`` declaration accepted zero arguments, so any code path that actually invoked it would raise ``TypeError``. Use ``parent`` (the documented convention for implicit static methods on ObjectType subclasses, of which Connection is one) rather than ``self``. Originally flagged by sjdemartini in PR graphql-python#1457: graphql-python#1457 (comment) Made-with: Cursor

Some users prefer PEP 8 snake_case for test-helper names, but the existing camelCase methods are part of the public API documented in README.md and docs/testing.rst, and renaming them would be a hard breaking change for every downstream test suite. Add ``assert_response_no_errors`` and ``assert_response_has_errors`` as additive aliases pointing at the same underlying callables. The camelCase methods remain canonical (matching Django's own ``assertEqual`` / ``assertNumQueries`` convention); both spellings now work identically with no deprecation warning on either one. Originally requested as a rename in PR graphql-python#1457; addressed here as non-breaking aliases per the reviewer feedback: graphql-python#1457 (comment) graphql-python#1457 (comment) Made-with: Cursor

…pers ``execute_graphql_request`` previously held two unrelated decision trees (the GET-vs-non-query operation check and the atomic-mutation check) inline alongside the rest of the execution flow. Extract each into a focused static helper: * ``_should_short_circuit_get_request(request, operation_ast, show_graphiql)`` returns a boolean signalling whether the caller should return ``None`` (GraphiQL rendering a non-query GET), and raises ``HttpError`` for the disallowed GET-of-a-mutation/subscription case. By returning a sentinel rather than ``None``, the public semantics are preserved exactly: queries on GET requests still execute normally, and the GraphiQL no-op short-circuit is still gated on the operation actually being a non-query. * ``_is_atomic_mutation(operation_ast)`` consolidates the mutation + ATOMIC_MUTATIONS check so it can be reasoned about and unit-tested in isolation. Both helpers receive the already-parsed ``operation_ast`` rather than re-parsing the document, so ``get_operation_ast`` is still called exactly once per request. This addresses the regressions identified in the review of PR graphql-python#1457: * B1: the original PR introduced ``if show_graphiql: return None`` unconditionally after extracting the validation, breaking the GET-with-query-and-GraphiQL execution path. The sentinel-return design avoids that. * B2: the original PR added a second ``get_operation_ast`` call. The AST is now computed once and passed into both helpers. * B3: ``__init__``'s ``if middleware is not None`` guard is preserved on main and remains untouched here. Made-with: Cursor

…ield regression Add ``graphene_django/filter/tests/test_utils.py`` with focused unit tests for every helper extracted from ``get_filtering_args_from_filterset``: * ``get_field_type_from_registry`` (registered model, NonNull unwrapping, missing model, missing field). * ``_is_foreign_key_form_field`` (parametrised over the four FK-related form-field classes plus a non-FK negative case). * ``_get_form_field`` (model_field.formfield wins, falls back to filter_field.field when model_field is None or the factory yields falsy). * ``_get_field_type_from_model_field`` (FK -> related-model id, non-FK -> owning model + field name). * ``_get_field_type_and_form_field_for_implicit_filter`` (isnull short-circuit, real model field, missing model field). * ``_get_field_type_for_explicit_filter`` (uses passed form_field, falls back to filter_field.field). * ``_is_filter_list_or_range`` (ListFilter, RangeFilter, plain CharFilter). Plus an integration regression test that exercises ``get_filtering_args_from_filterset`` end-to-end for a method-backed filter targeting a non-model field — pinning the subtle behavioural-parity concern flagged in PR graphql-python#1457 review (the implicit-filter path must still allow the explicit-filter fallback to build the Argument when the filter has no underlying model field). Each test follows the team docstring template (Name / Description / Assumptions / Expectations); the file's module docstring spells out the assumptions and scenarios that apply file-wide. All 23 tests pass; existing 64 filter tests continue to pass. Made-with: Cursor

…ingle AST parse Add ``TestExecuteGraphqlRequestRefactor`` pinning every behaviour the ``execute_graphql_request`` extraction in views.py is meant to preserve, including the three regressions identified in the PR graphql-python#1457 review. * ``_should_short_circuit_get_request`` (5 tests): - POST never short-circuits. - GET + query never short-circuits regardless of show_graphiql (B1 regression pin). - GET + mutation + show_graphiql=True returns True so the caller short-circuits. - GET + mutation + show_graphiql=False raises HttpError carrying a 405 with the documented message. - operation_ast=None never short-circuits. * ``_is_atomic_mutation`` (7 tests via parametrize + None case): - All 6 combinations of (operation kind, ATOMIC_MUTATIONS global, ATOMIC_MUTATIONS per-db). - None operation_ast returns False. * ``execute_graphql_request`` end-to-end: - GET + query + show_graphiql=True executes the query and returns the result (B1 pin). - GET + mutation + show_graphiql=True returns None (existing behaviour preserved). - get_operation_ast called exactly once per request (B2 pin). * ``GraphQLView.__init__``: - graphene_settings.MIDDLEWARE = None succeeds and yields view.middleware is None (B3 pin). Each test follows the team docstring template (Name / Description / Assumptions / Expectations); the class docstring spells out which scenarios and edge cases are covered. All 16 new tests pass. Made-with: Cursor

lilac-supernova-2 added 21 commits August 26, 2023 00:19

Remove unnecessary if statement

ad447d9

Keep 'if' above same as before, fix lint issue

0e1b605

Fix Star Wars spaceship name

cd6e042

Fix some typos in comments

ea77bc7

Rename filename with typo

6580228

Update import

1b00e67

Snake case fixes

5c52096

Add possibly missing import of DjangoFilterConnectionField

2a888ac

Actually, let's not do this to avoid circular imports

b639034

Remove nesting for readability

d873220

Rename function

67373c7

Typo fixes

722daf1

More typo fixes

34e61c6

Refactor get_filtering_args_from_filterset into more functions to red…

988b308

…uce cognitive complexity

Add new tests-repeat command to Makefile to be able to run unit tests…

cb92d6e

… multiple times + in parallel in one shot

Add 'self' argument

19d5558

Snake case formatting fix

34db851

Refactor validation function to reduce complexity

98fd498

Linting

a94a89d

Refactor execute_graphql_request function to reduce complexity

59c0117

Linting

e263840

lilac-supernova-2 marked this pull request as ready for review August 26, 2023 08:02

lilac-supernova-2 commented Aug 26, 2023

View reviewed changes

sjdemartini requested changes Aug 28, 2023

View reviewed changes

lilac-supernova-2 mentioned this pull request Sep 6, 2023

Typo fixes #1459

Merged

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

firaskafri mentioned this pull request Apr 19, 2026

Refactor: address PR #1457 review feedback (rebased on main, with tests) #1559

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce cognitive complexity and typo fixes#1457

Reduce cognitive complexity and typo fixes#1457
lilac-supernova-2 wants to merge 21 commits intographql-python:mainfrom
lilac-supernova-2:reduce-cognitive-complexity-and-typo-fixes

lilac-supernova-2 commented Aug 26, 2023 •

edited

Loading

Uh oh!

lilac-supernova-2 Aug 26, 2023

Uh oh!

sjdemartini left a comment

Uh oh!

sjdemartini Aug 28, 2023

Uh oh!

sjdemartini Aug 28, 2023

Uh oh!

sjdemartini Aug 28, 2023

Uh oh!

lilac-supernova-2 Sep 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lilac-supernova-2 commented Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lilac-supernova-2 Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

sjdemartini left a comment

Choose a reason for hiding this comment

Uh oh!

sjdemartini Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

sjdemartini Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

sjdemartini Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

lilac-supernova-2 Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lilac-supernova-2 commented Aug 26, 2023 •

edited

Loading