Refactor: address PR #1457 review feedback (rebased on main, with tests)#1559
Open
firaskafri wants to merge 12 commits intographql-python:mainfrom
Open
Refactor: address PR #1457 review feedback (rebased on main, with tests)#1559firaskafri wants to merge 12 commits intographql-python:mainfrom
firaskafri wants to merge 12 commits intographql-python:mainfrom
Conversation
- 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
…compat shim The module was previously misspelled as ``formating``. The canonical file is now :mod:`graphene_django.debug.exception.formatting`, and ``middleware.py`` imports from the corrected path. A thin shim is left in place at the legacy ``formating`` import path re-exporting :func:`wrap_exception` so any third-party code importing the misspelled module continues to work. The shim is documented as deprecated. 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
…elds helpers ``validate_fields`` previously held two unrelated validation loops (for ``Meta.fields`` and ``Meta.exclude``) inside a single function with nested ``if/else`` branches. Extract each loop into a private helper (``_validate_only_fields`` and ``_validate_exclude_fields``) so each warning case can be reasoned about and tested in isolation. The public ``validate_fields`` signature and behaviour are preserved; ``DjangoObjectType.__init_subclass_with_meta__`` and any other callers are unaffected. Each new helper carries a docstring describing the cases it covers and the warning message it emits, making future maintenance easier. Made-with: Cursor
``get_filtering_args_from_filterset`` previously held all filter-type resolution logic in a single deeply-nested function. Extract it into small, individually-testable helpers: * ``get_field_type_from_registry`` (renamed from the misleadingly generic ``get_field_type``) — resolves a GraphQL type from the DjangoObjectType registry, unwrapping NonNull. * ``_is_foreign_key_form_field`` — predicate for FK-style form fields. * ``_get_field_type_from_model_field`` — routes FK fields through the related model's ``id``, plain fields through their owning model. * ``_get_form_field`` — resolves which form field to use to validate filter input (model formfield first, filter field second). * ``_get_field_type_and_form_field_for_implicit_filter`` — handles implicit (Meta.fields-derived) filters, special-casing ``isnull``. * ``_get_field_type_for_explicit_filter`` — converts a form field into a GraphQL type, used for explicitly-declared filters and the implicit fallback. * ``_is_filter_list_or_range`` — predicate identifying ``ListFilter`` / ``RangeFilter`` whose argument needs ``graphene.List`` wrapping. The convert_form_field import is moved into ``_get_field_type_for_explicit_filter`` so it stays lazy and avoids circular-import issues just like the previous in-function import. Behaviour is preserved end-to-end: existing filter test suite (64 tests in graphene_django/filter/, 36 in test_fields.py) still passes. The renamed ``get_field_type`` was a module-private symbol with no external callers, so the rename is non-breaking. 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
… tests-repeat target Add ``pytest-repeat`` and ``pytest-xdist`` to ``tests_require`` so they are installed via ``pip install -e ".[test]"`` (and transitively via ``[dev]``) without polluting ``install_requires`` — these are developer-tooling dependencies and library consumers must not be forced to install pytest plugins they will never use. Add a ``tests-repeat`` Makefile target that runs the suite 100 times in parallel using these plugins. Useful for surfacing flaky tests; the target docstring documents the install hint so the dependency on the test extras is discoverable. Made-with: Cursor
Add ``TestValidateFieldsHelpers`` exercising the new ``_validate_only_fields`` / ``_validate_exclude_fields`` helpers and the ``validate_fields`` orchestrator directly (i.e. without going through ``DjangoObjectType.__init_subclass_with_meta__``), so each warning branch fails a focused test rather than only being detected via integration coverage. Coverage: * ``_validate_only_fields``: known field (no warning), model attribute (attribute warning), unknown name (missing-field warning), ``None`` (no-op). * ``_validate_exclude_fields``: real field (no warning), custom field (no-effect warning), unknown name (missing-attribute warning), ``None`` (no-op). * ``validate_fields`` orchestrator: ``ALL_FIELDS`` sentinel normalisation; combined call delegating to both helpers. Each test follows the team docstring template (Name / Description / Assumptions / Expectations) and asserts on the warning message text via ``pytest.warns(..., match=...)`` so a regression in either the extraction or the message wording surfaces a focused failure. 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
Add ``TestAssertionAliases`` covering the new ``assert_response_no_errors`` / ``assert_response_has_errors`` aliases: * Identity check: each alias must be the **same callable** as its camelCase counterpart, so the two spellings cannot drift apart. * End-to-end behaviour for both spellings on a clean response and on an errors response, asserting both the silent-pass and ``AssertionError`` branches. Uses a tiny ``_FakeHttpResponse`` stand-in so the alias tests stay free of any dependency on routing, schemas, or the test client. Each test follows the team docstring template. 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR is a clean, rebased-on-
mainre-application of the work in #1457, with the regressions and breaking changes flagged in @sjdemartini's review fixed and full unit-test coverage added for every extracted helper.It supersedes the changes from #1457 that have not already been merged via the follow-up #1459.
What's in this PR
Bug fixes (real defects)
fix(tests):ArticleConnection.resolve_test()was missing its first argument and wouldTypeErrorif Graphene ever invoked it. Fixed to useparentper the implicit-static-method convention (#1457 review).Refactors with preserved public API
refactor(types): extract_validate_only_fields/_validate_exclude_fieldsfromvalidate_fields. Public signature unchanged.refactor(filter/utils): extract_get_field_type_from_model_field,_get_form_field,_get_field_type_and_form_field_for_implicit_filter,_get_field_type_for_explicit_filter,_is_filter_list_or_range, and_is_foreign_key_form_field. Rename module-privateget_field_type->get_field_type_from_registry.refactor(views): extract_should_short_circuit_get_requestand_is_atomic_mutationfromexecute_graphql_request. Three regressions from Reduce cognitive complexity and typo fixes #1457 are explicitly avoided and pinned by tests (see "Regression pins" below).Non-breaking renames / aliases
refactor(debug): renameformating.py->formatting.py, with a backwards-compat shim re-exportingwrap_exceptionfrom the legacy import path.feat(testing): add additiveassert_response_no_errors/assert_response_has_errorsaliases. camelCase methods remain canonical (matching Django'sassertEqual/assertNumQueriesconvention) so this is non-breaking (#1457 review).Tooling
chore(deps):pytest-repeatandpytest-xdistgo to thetests_require(and therefore thetest/devextras), notinstall_requires— so library consumers aren't forced to install pytest plugins.tests-repeatMakefile target (pytest --count 100 -n logical) for surfacing flaky tests.Content fixes
homeOne->home_one, etc.).filter/utils.pydocstring:Graphql->GraphQL;no explicitly declared->not explicitly declared.Dictonary,Confuesd,Millenium, etc. — were already merged via Typo fixes #1459 so they're not duplicated here.)Regression pins (from #1457 review)
if show_graphiql: return Noneafter extracting validation, dropping queries with GraphiQL silentlytest_should_short_circuit_returns_false_for_get_query,test_execute_graphql_request_get_with_query_and_graphiql_executesget_operation_astcall introduced inside the extracted validation methodtest_execute_graphql_request_calls_get_operation_ast_onceif middleware is not Noneguard removed from__init__, breakingMIDDLEWARE = Nonesettingstest_init_with_middleware_setting_none_does_not_raiseThe
views.pyonmainhas diverged significantly since #1457 was opened (post-#1439, post-#1475), so the refactor is freshly applied on top of the current code shape.Tests
54 new tests added (all green; full suite goes from 397 -> 451 passing):
TestValidateFieldsHelpersingraphene_django/tests/test_types.pygraphene_django/filter/tests/test_utils.py(new file) — including a regression test for filtering on a non-model fieldTestAssertionAliasesingraphene_django/utils/tests/test_testing.py(identity + 4 end-to-end behaviour cases)TestExecuteGraphqlRequestRefactoringraphene_django/tests/test_views.py(B1, B2, B3, plus parametrised_is_atomic_mutationover 6 settings combinations)Every test carries a docstring with
Name / Description / Assumptions / Expectations; class and module docstrings spell out scenarios and edge cases.Test plan
make tests(using project addopts) — 451 passedpytest -p random_order— 451 passedpytest --cov=graphene_django— 451 passed, 95% total coverageassertResponseNoErrors/assertResponseHasErrors/formatingimport path /validate_fieldssignature all still workCommits
12 logical commits, one per scope, ordered so each refactor's tests follow the corresponding code change for clean review.
Made with Cursor