diff --git a/Makefile b/Makefile index 633c83f3b..c81078954 100644 --- a/Makefile +++ b/Makefile @@ -12,6 +12,10 @@ dev-setup: tests: PYTHONPATH=. pytest graphene_django --cov=graphene_django -vv +.PHONY: tests-repeat ## Run unit tests 100 times in parallel to surface flaky tests (requires pip install -e ".[test]") +tests-repeat: + PYTHONPATH=. pytest graphene_django --cov=graphene_django -vv --count 100 -n logical + .PHONY: format ## Format code format: ruff format graphene_django examples setup.py diff --git a/examples/starwars/data.py b/examples/starwars/data.py index bfac78b19..f5d266757 100644 --- a/examples/starwars/data.py +++ b/examples/starwars/data.py @@ -31,14 +31,14 @@ def initialize(): falcon = Ship(id="4", name="Millennium Falcon", faction=rebels) falcon.save() - homeOne = Ship(id="5", name="Home One", faction=rebels) - homeOne.save() + home_one = Ship(id="5", name="Home One", faction=rebels) + home_one.save() - tieFighter = Ship(id="6", name="TIE Fighter", faction=empire) - tieFighter.save() + tie_fighter = Ship(id="6", name="TIE Fighter", faction=empire) + tie_fighter.save() - tieInterceptor = Ship(id="7", name="TIE Interceptor", faction=empire) - tieInterceptor.save() + tie_interceptor = Ship(id="7", name="TIE Interceptor", faction=empire) + tie_interceptor.save() executor = Ship(id="8", name="Executor", faction=empire) executor.save() diff --git a/graphene_django/debug/exception/formating.py b/graphene_django/debug/exception/formating.py index 0d477b330..90d5504e6 100644 --- a/graphene_django/debug/exception/formating.py +++ b/graphene_django/debug/exception/formating.py @@ -1,17 +1,11 @@ -import traceback +"""Backwards-compatibility shim for the misspelled ``formating`` module. -from django.utils.encoding import force_str +The canonical module is now :mod:`graphene_django.debug.exception.formatting`. +This shim re-exports its public symbols so that any third-party code still +importing the legacy name continues to work. It is deprecated and will be +removed in a future major release. +""" -from .types import DjangoDebugException +from .formatting import wrap_exception # noqa: F401 - re-exported for backwards compatibility - -def wrap_exception(exception): - return DjangoDebugException( - message=force_str(exception), - exc_type=force_str(type(exception)), - stack="".join( - traceback.format_exception( - exception, value=exception, tb=exception.__traceback__ - ) - ), - ) +__all__ = ["wrap_exception"] diff --git a/graphene_django/debug/exception/formatting.py b/graphene_django/debug/exception/formatting.py new file mode 100644 index 000000000..0d477b330 --- /dev/null +++ b/graphene_django/debug/exception/formatting.py @@ -0,0 +1,17 @@ +import traceback + +from django.utils.encoding import force_str + +from .types import DjangoDebugException + + +def wrap_exception(exception): + return DjangoDebugException( + message=force_str(exception), + exc_type=force_str(type(exception)), + stack="".join( + traceback.format_exception( + exception, value=exception, tb=exception.__traceback__ + ) + ), + ) diff --git a/graphene_django/debug/middleware.py b/graphene_django/debug/middleware.py index de0d72d18..6c18b1a1e 100644 --- a/graphene_django/debug/middleware.py +++ b/graphene_django/debug/middleware.py @@ -1,6 +1,6 @@ from django.db import connections -from .exception.formating import wrap_exception +from .exception.formatting import wrap_exception from .sql.tracking import unwrap_cursor, wrap_cursor from .types import DjangoDebug diff --git a/graphene_django/filter/tests/test_utils.py b/graphene_django/filter/tests/test_utils.py new file mode 100644 index 000000000..fe4cfcd7b --- /dev/null +++ b/graphene_django/filter/tests/test_utils.py @@ -0,0 +1,613 @@ +"""Focused unit tests for the filter-type resolution helpers. + +These tests cover the helpers extracted from +:func:`graphene_django.filter.utils.get_filtering_args_from_filterset`: + +* :func:`get_field_type_from_registry` +* :func:`_is_foreign_key_form_field` +* :func:`_get_field_type_from_model_field` +* :func:`_get_form_field` +* :func:`_get_field_type_and_form_field_for_implicit_filter` +* :func:`_get_field_type_for_explicit_filter` +* :func:`_is_filter_list_or_range` + +Each helper has at least one test per documented branch so that a +regression in any single branch fails a focused test rather than only +being detected via the integration-level ``test_fields.py`` suite. + +The file also pins one **integration** scenario that exercises the +``get_filtering_args_from_filterset`` flow end-to-end for the +"non-model field" path — this protects against the subtle behavioural +parity concern flagged in the PR review (the implicit-filter helper +must still allow the explicit-filter fallback to take over when the +filter targets something that is not a real model field). + +Assumptions that apply to the file as a whole: + +* ``django-filter`` is installed (the whole module is skipped via + ``pytestmark`` otherwise, matching the convention used in the rest of + ``graphene_django/filter/tests``). +* The shared :class:`graphene_django.tests.models.Pet` / + :class:`graphene_django.tests.models.Person` / + :class:`graphene_django.tests.models.Reporter` models are available + for use in test fixtures. +""" + +import pytest +from django import forms +from django_filters import CharFilter, FilterSet, NumberFilter + +import graphene +from graphene import NonNull + +from graphene_django import DjangoObjectType +from graphene_django.forms import GlobalIDFormField, GlobalIDMultipleChoiceField +from graphene_django.registry import Registry +from graphene_django.tests.models import Person, Pet, Reporter +from graphene_django.utils import DJANGO_FILTER_INSTALLED + +from ..filters import ListFilter, RangeFilter +from ..utils import ( + _get_field_type_and_form_field_for_implicit_filter, + _get_field_type_for_explicit_filter, + _get_field_type_from_model_field, + _get_form_field, + _is_filter_list_or_range, + _is_foreign_key_form_field, + get_field_type_from_registry, + get_filtering_args_from_filterset, +) + +pytestmark = [] +if not DJANGO_FILTER_INSTALLED: + pytestmark.append( + pytest.mark.skipif( + True, reason="django_filters not installed or not compatible" + ) + ) + + +@pytest.fixture +def isolated_registry(): + """A fresh :class:`Registry` that does not pollute the global one.""" + return Registry() + + +@pytest.fixture +def reporter_type(isolated_registry): + """A :class:`DjangoObjectType` for Reporter registered in the isolated registry. + + Used by the ``get_field_type_from_registry`` tests to assert that the + helper correctly looks up the Graphene type for a given model + field + combination. Returns the type class. + """ + + class ReporterFilterType(DjangoObjectType): + class Meta: + model = Reporter + fields = "__all__" + registry = isolated_registry + + return ReporterFilterType + + +@pytest.fixture +def pet_type(isolated_registry): + """A :class:`DjangoObjectType` for Pet registered in the isolated registry. + + Pet has a foreign key (``owner``) to Person, used by the FK-routing + branch tests of :func:`_get_field_type_from_model_field`. + """ + + class PetFilterType(DjangoObjectType): + class Meta: + model = Pet + fields = "__all__" + registry = isolated_registry + + return PetFilterType + + +@pytest.fixture +def person_type(isolated_registry): + """A :class:`DjangoObjectType` for Person registered in the isolated registry. + + Required so the FK-routing branch can resolve ``Person.id`` via the + registry when the related model is queried for its ``id`` field. + """ + + class PersonFilterType(DjangoObjectType): + class Meta: + model = Person + fields = "__all__" + registry = isolated_registry + + return PersonFilterType + + +# --------------------------------------------------------------------------- +# get_field_type_from_registry +# --------------------------------------------------------------------------- + + +class TestGetFieldTypeFromRegistry: + """Coverage for :func:`get_field_type_from_registry`. + + The helper is the only public renamed function in the refactor; it + must continue to: + + * resolve the Graphene type for a registered model + field pair, + * unwrap a ``NonNull`` wrapper so callers always receive the named + type underneath, + * return ``None`` when the model isn't registered, and + * return ``None`` when the field name doesn't exist on the type. + """ + + def test_returns_field_type_for_registered_model( + self, isolated_registry, reporter_type + ): + """ + Name: registered model resolves field type + Description: When the registry has a DjangoObjectType for the model + and that type exposes the requested field, return its Graphene + type. + Assumptions: ``ReporterFilterType`` is registered in the isolated + registry and exposes ``first_name``. + Expectations: The returned type is non-None. + """ + result = get_field_type_from_registry( + isolated_registry, Reporter, "first_name" + ) + assert result is not None + + def test_unwraps_non_null(self, isolated_registry, reporter_type): + """ + Name: NonNull wrapper unwrapped + Description: When the resolved type is a ``NonNull`` wrapper, the + helper must unwrap it so the caller can compose its own + wrappers without double-wrapping. + Assumptions: ``Reporter.first_name`` is a non-null CharField, which + converts to ``NonNull(String)`` in graphene-django. + Expectations: ``isinstance(result, NonNull)`` is False but + ``isinstance(NonNull, ...)`` would have been True before + unwrapping. + """ + result = get_field_type_from_registry( + isolated_registry, Reporter, "first_name" + ) + assert not isinstance(result, NonNull), ( + f"expected unwrapped type, got NonNull-wrapped: {result!r}" + ) + + def test_returns_none_for_unregistered_model(self, isolated_registry): + """ + Name: unregistered model returns None + Description: When the requested model has no DjangoObjectType in + the registry, the helper must short-circuit to ``None`` so the + caller can fall back to converting the form field instead. + Assumptions: ``Pet`` is not registered in this isolated registry. + Expectations: Returns ``None``. + """ + assert get_field_type_from_registry(isolated_registry, Pet, "name") is None + + def test_returns_none_for_unknown_field( + self, isolated_registry, reporter_type + ): + """ + Name: unknown field returns None + Description: When the model is registered but the requested field + name doesn't exist on its type, the helper must return ``None``. + Assumptions: ``ReporterFilterType`` does not declare a field + named ``"nonexistent"``. + Expectations: Returns ``None``. + """ + assert ( + get_field_type_from_registry(isolated_registry, Reporter, "nonexistent") + is None + ) + + +# --------------------------------------------------------------------------- +# _is_foreign_key_form_field +# --------------------------------------------------------------------------- + + +class TestIsForeignKeyFormField: + """Coverage for :func:`_is_foreign_key_form_field`. + + The helper centralises the "form field maps to a foreign-key relation" + predicate so the FK-routing logic in + :func:`_get_field_type_from_model_field` stays readable. + """ + + @pytest.mark.parametrize( + "form_field_class", + [ + forms.ModelChoiceField, + forms.ModelMultipleChoiceField, + GlobalIDFormField, + GlobalIDMultipleChoiceField, + ], + ) + def test_recognises_fk_form_fields(self, form_field_class): + """ + Name: FK-style form fields recognised + Description: All four FK-related form-field classes used by the + implicit-filter resolution must be reported as foreign-key + form fields so they get routed through the related model's id. + Assumptions: The four parameterised classes are the canonical FK + form fields handled by the resolver. + Expectations: ``_is_foreign_key_form_field`` returns ``True``. + """ + if form_field_class is forms.ModelChoiceField: + instance = form_field_class(queryset=Person.objects.none()) + elif form_field_class is forms.ModelMultipleChoiceField: + instance = form_field_class(queryset=Person.objects.none()) + else: + instance = form_field_class() + + assert _is_foreign_key_form_field(instance) is True + + def test_plain_form_field_is_not_fk(self): + """ + Name: plain form field is not FK + Description: A plain ``CharField`` (or any non-FK form field) must + be reported as a non-FK form field so the resolver routes it + through the regular model-field path instead. + Assumptions: ``forms.CharField`` is not in the FK-recognised set. + Expectations: ``_is_foreign_key_form_field`` returns ``False``. + """ + assert _is_foreign_key_form_field(forms.CharField()) is False + + +# --------------------------------------------------------------------------- +# _get_form_field +# --------------------------------------------------------------------------- + + +class TestGetFormField: + """Coverage for :func:`_get_form_field` resolution rules. + + Resolution order: + + 1. ``model_field.formfield(required=...)`` if the model field exposes + a ``formfield`` factory and it returns a truthy value. + 2. ``filter_field.field`` otherwise (and when ``model_field`` is + ``None``). + """ + + def test_uses_model_field_formfield_when_available(self): + """ + Name: prefers model_field.formfield + Description: When the underlying model field exposes a ``formfield`` + factory that returns a form field, that form field is used. + Assumptions: ``Reporter._meta.get_field("first_name").formfield()`` + returns a non-None form field. + Expectations: The returned form field is the one produced by + ``model_field.formfield`` (a CharField for first_name), not the + filter's own ``filter_field.field``. + """ + model_field = Reporter._meta.get_field("first_name") + filter_field = CharFilter() + + result = _get_form_field(model_field, filter_field, required=False) + assert result is not filter_field.field + assert isinstance(result, forms.CharField) + + def test_falls_back_to_filter_field_field_when_model_field_is_none(self): + """ + Name: falls back to filter_field.field when model_field is None + Description: When the resolver cannot find a model field for a + filter (e.g. the filter targets an annotation or method-only + attribute), the helper must use ``filter_field.field`` so + validation does not lose its form field. + Assumptions: ``CharFilter().field`` is a usable form field. + Expectations: The returned form field is exactly ``filter_field.field``. + """ + filter_field = CharFilter() + result = _get_form_field(None, filter_field, required=False) + assert result is filter_field.field + + def test_falls_back_when_model_field_formfield_returns_falsy(self): + """ + Name: falls back when model_field.formfield returns falsy + Description: Even when a model field has a ``formfield`` factory, + the resolver must still fall back to ``filter_field.field`` if + the factory returns a falsy value (e.g. for fields that are + not user-editable). + Assumptions: We can simulate a model field whose ``formfield`` + returns ``None`` using a tiny fake object. + Expectations: The returned form field is exactly ``filter_field.field``. + """ + + class FakeModelField: + def formfield(self, required): + return None + + filter_field = CharFilter() + result = _get_form_field(FakeModelField(), filter_field, required=False) + assert result is filter_field.field + + +# --------------------------------------------------------------------------- +# _get_field_type_from_model_field +# --------------------------------------------------------------------------- + + +class TestGetFieldTypeFromModelField: + """Coverage for :func:`_get_field_type_from_model_field`. + + For FK form fields the resolver must look up the related model's + ``id`` in the registry; for non-FK form fields it must look up the + owning model + field name directly. + """ + + def test_foreign_key_routes_through_related_model_id( + self, isolated_registry, pet_type, person_type + ): + """ + Name: foreign key uses related model id + Description: For an FK model field combined with an FK form field, + the helper must resolve the type via the **related model**'s + ``id`` field (since GraphQL filters on relations target the + related node's identifier). + Assumptions: ``Pet.owner`` is an FK to ``Person``; both types are + registered in the isolated registry. + Expectations: The returned type is non-None (Person.id resolves) + and matches the type that ``get_field_type_from_registry`` + would return for ``Person.id`` directly. + """ + owner_field = Pet._meta.get_field("owner") + fk_form_field = forms.ModelChoiceField(queryset=Person.objects.none()) + + result = _get_field_type_from_model_field( + owner_field, fk_form_field, isolated_registry + ) + expected = get_field_type_from_registry(isolated_registry, Person, "id") + assert result is not None + assert result == expected + + def test_non_foreign_key_routes_through_owning_model_field( + self, isolated_registry, pet_type + ): + """ + Name: non-FK uses owning model field + Description: For a non-FK model field, the helper must resolve the + type via the owning model + the field name directly. + Assumptions: ``Pet.name`` is a CharField on the registered + ``PetFilterType``. + Expectations: The returned type matches what + ``get_field_type_from_registry`` returns for ``Pet.name``. + """ + name_field = Pet._meta.get_field("name") + plain_form_field = forms.CharField() + + result = _get_field_type_from_model_field( + name_field, plain_form_field, isolated_registry + ) + expected = get_field_type_from_registry(isolated_registry, Pet, "name") + assert result == expected + + +# --------------------------------------------------------------------------- +# _get_field_type_and_form_field_for_implicit_filter +# --------------------------------------------------------------------------- + + +class TestGetFieldTypeAndFormFieldForImplicitFilter: + """Coverage for :func:`_get_field_type_and_form_field_for_implicit_filter`. + + Branches: + + * ``filter_type == "isnull"`` short-circuits to ``(graphene.Boolean, None)``. + * Real model field present -> ``(field_type_from_registry, form_field)``. + * Model field absent -> ``(None, form_field)`` so the caller falls + back to the explicit-filter path. + """ + + def test_isnull_short_circuits_to_boolean(self, isolated_registry, reporter_type): + """ + Name: isnull -> Boolean + Description: The ``isnull`` lookup is always boolean and needs no + form field, so the helper short-circuits without touching the + model. + Assumptions: ``filter_type`` of ``"isnull"`` is the canonical key + used by django-filter for null checks. + Expectations: Returns ``(graphene.Boolean, None)``. + """ + result = _get_field_type_and_form_field_for_implicit_filter( + Reporter, "isnull", CharFilter(field_name="first_name"), isolated_registry, False + ) + assert result == (graphene.Boolean, None) + + def test_returns_field_type_and_form_field_for_real_model_field( + self, isolated_registry, reporter_type + ): + """ + Name: real model field returns (field_type, form_field) + Description: For a filter targeting a real model field, the helper + must return both the resolved Graphene type and the resolved + form field so the caller can reuse them. + Assumptions: ``Reporter.first_name`` exists and ``ReporterFilterType`` + is registered. + Expectations: ``field_type`` is non-None and ``form_field`` is a + ``forms.CharField`` (since first_name is a CharField). + """ + field_type, form_field = _get_field_type_and_form_field_for_implicit_filter( + Reporter, + "exact", + CharFilter(field_name="first_name"), + isolated_registry, + False, + ) + assert field_type is not None + assert isinstance(form_field, forms.CharField) + + def test_returns_none_for_missing_model_field( + self, isolated_registry, reporter_type + ): + """ + Name: missing model field returns (None, form_field) + Description: When the filter targets a name that is not a real + model field, the helper must return ``(None, form_field)`` so + the caller falls back to the explicit-filter conversion path. + Assumptions: ``Reporter`` has no field named + ``"nonexistent_attribute"``. + Expectations: ``field_type`` is ``None`` and ``form_field`` is the + filter's own field (since model_field is None). + """ + char_filter = CharFilter(field_name="nonexistent_attribute") + field_type, form_field = _get_field_type_and_form_field_for_implicit_filter( + Reporter, "exact", char_filter, isolated_registry, False + ) + assert field_type is None + assert form_field is char_filter.field + + +# --------------------------------------------------------------------------- +# _get_field_type_for_explicit_filter +# --------------------------------------------------------------------------- + + +class TestGetFieldTypeForExplicitFilter: + """Coverage for :func:`_get_field_type_for_explicit_filter`. + + The helper converts a Django form field into a Graphene type via + :func:`graphene_django.forms.converter.convert_form_field`. It is + used both for explicitly-declared filters and as the fallback when + the implicit-filter path could not resolve a registry type. + """ + + def test_uses_passed_form_field_when_truthy(self): + """ + Name: prefers passed form_field when truthy + Description: When the caller has already resolved a form field, + the helper should use it directly rather than falling back to + ``filter_field.field``. + Assumptions: ``forms.IntegerField()`` converts to a usable + Graphene type via the form-field converter. + Expectations: The returned type matches what + ``convert_form_field(IntegerField())`` would yield. + """ + from ...forms.converter import convert_form_field + + explicit = forms.IntegerField() + filter_field = CharFilter() + + result = _get_field_type_for_explicit_filter(filter_field, explicit) + expected = convert_form_field(explicit).get_type() + assert result == expected + + def test_falls_back_to_filter_field_field_when_form_field_is_falsy(self): + """ + Name: falls back to filter_field.field when form_field is falsy + Description: When ``form_field`` is ``None``/falsy, the helper must + fall back to ``filter_field.field`` so the explicit-filter path + never converts a missing form field. + Assumptions: ``CharFilter().field`` is a usable form field. + Expectations: The returned type matches what + ``convert_form_field(filter_field.field)`` would yield. + """ + from ...forms.converter import convert_form_field + + filter_field = CharFilter() + result = _get_field_type_for_explicit_filter(filter_field, None) + expected = convert_form_field(filter_field.field).get_type() + assert result == expected + + +# --------------------------------------------------------------------------- +# _is_filter_list_or_range +# --------------------------------------------------------------------------- + + +class TestIsFilterListOrRange: + """Coverage for :func:`_is_filter_list_or_range`. + + Identifies ``ListFilter`` and ``RangeFilter`` so their argument type + gets wrapped in ``graphene.List`` (the ``in`` and ``range`` lookups + accept multiple values). + """ + + def test_list_filter_recognised(self): + """ + Name: ListFilter recognised + Description: A :class:`ListFilter` instance must be reported as a + list/range filter so its argument gets wrapped in ``graphene.List``. + Assumptions: ``ListFilter`` is the canonical implementation of + list-typed filter arguments in graphene-django. + Expectations: Returns ``True``. + """ + assert _is_filter_list_or_range(ListFilter()) is True + + def test_range_filter_recognised(self): + """ + Name: RangeFilter recognised + Description: A :class:`RangeFilter` instance must be reported as a + list/range filter so its argument gets wrapped in ``graphene.List``. + Assumptions: ``RangeFilter`` is the canonical implementation of + range-typed filter arguments in graphene-django. + Expectations: Returns ``True``. + """ + assert _is_filter_list_or_range(RangeFilter()) is True + + def test_plain_char_filter_not_recognised(self): + """ + Name: plain CharFilter not list-or-range + Description: A regular ``CharFilter`` must not be reported as a + list/range filter so its argument stays scalar. + Assumptions: ``django_filters.CharFilter`` is not a subclass of + ``ListFilter`` or ``RangeFilter``. + Expectations: Returns ``False``. + """ + assert _is_filter_list_or_range(CharFilter()) is False + + +# --------------------------------------------------------------------------- +# Regression: get_filtering_args_from_filterset routes non-model fields +# correctly through the explicit-filter fallback. +# --------------------------------------------------------------------------- + + +class TestGetFilteringArgsFromFilterset: + """Integration regression coverage for :func:`get_filtering_args_from_filterset`. + + This pins the subtle behavioural-parity concern flagged in the PR + review: the implicit-filter helper now always calls ``_get_form_field`` + even when the model field is missing (returning ``(None, form_field)``), + and the caller must still fall through to the explicit-filter helper + so the Graphene Argument is built. + """ + + def test_filter_on_non_model_field_uses_explicit_fallback( + self, isolated_registry, reporter_type + ): + """ + Name: filter on non-model field falls back to form-field conversion + Description: When a FilterSet declares a filter targeting a name + that is not a real model field (here, an annotation-style + ``method`` filter), the resolver must still produce a Graphene + ``Argument`` for it via the explicit-filter conversion path. + Assumptions: ``django_filters.NumberFilter(method=...)`` declared on + a FilterSet without a corresponding model field is a valid way + to expose a method-backed filter; ``reporter_type`` registers a + ``Reporter`` :class:`DjangoObjectType` in the isolated registry. + Expectations: The returned arguments dict contains the filter + name and yields a non-None Graphene ``Argument``. + """ + + class ReporterFilterSet(FilterSet): + articles_count = NumberFilter(method="filter_by_articles_count") + + class Meta: + model = Reporter + fields = [] + + def filter_by_articles_count(self, queryset, _name, value): + return queryset + + args = get_filtering_args_from_filterset(ReporterFilterSet, reporter_type) + assert "articles_count" in args + argument = args["articles_count"] + assert isinstance(argument, graphene.Argument) + assert argument._type is not None diff --git a/graphene_django/filter/utils.py b/graphene_django/filter/utils.py index 9ffcc5cf3..8c7cce925 100644 --- a/graphene_django/filter/utils.py +++ b/graphene_django/filter/utils.py @@ -8,19 +8,154 @@ from .filterset import custom_filterset_factory, setup_filterset -def get_field_type(registry, model, field_name): - """ - Try to get a model field corresponding Graphql type from the DjangoObjectType. +def get_field_type_from_registry(registry, model, field_name): + """Resolve the GraphQL type for ``model.field_name`` from the registry. + + Looks up the ``DjangoObjectType`` registered for ``model`` and returns + the GraphQL type associated with ``field_name`` on it, unwrapping a + ``NonNull`` wrapper when present so callers always receive the underlying + named type. + + Returns ``None`` when either the model is not registered as a + ``DjangoObjectType`` or the type does not expose ``field_name``. A + ``None`` return is the caller's signal to fall back to converting the + Django form field instead. """ object_type = registry.get_type_for_model(model) - if object_type: - object_type_field = object_type._meta.fields.get(field_name) - if object_type_field: - field_type = object_type_field.type - if isinstance(field_type, graphene.NonNull): - field_type = field_type.of_type - return field_type - return None + if not object_type: + return None + + object_type_field = object_type._meta.fields.get(field_name) + if not object_type_field: + return None + + field_type = object_type_field.type + if isinstance(field_type, graphene.NonNull): + field_type = field_type.of_type + return field_type + + +def _is_foreign_key_form_field(form_field): + """Return ``True`` when ``form_field`` represents a foreign-key relation. + + Foreign-key form fields are routed through the related model's ``id`` in + :func:`_get_field_type_from_model_field` because filtering on a relation + in GraphQL means filtering on the related node's identifier rather than + on the relation column itself. + """ + return isinstance( + form_field, + ( + forms.ModelChoiceField, + forms.ModelMultipleChoiceField, + GlobalIDFormField, + GlobalIDMultipleChoiceField, + ), + ) + + +def _get_field_type_from_model_field(model_field, form_field, registry): + """Resolve the GraphQL type for ``model_field`` via the registry. + + For foreign-key form fields, lookup is performed on the related model's + ``id`` (since filtering on a foreign key in GraphQL means filtering on + the related node identifier). For all other fields, lookup is performed + on the owning model + the model field name directly. + + Delegates the actual registry lookup to + :func:`get_field_type_from_registry`. + """ + if _is_foreign_key_form_field(form_field): + return get_field_type_from_registry(registry, model_field.related_model, "id") + + return get_field_type_from_registry(registry, model_field.model, model_field.name) + + +def _get_form_field(model_field, filter_field, required): + """Resolve which Django form field to use to validate the filter input. + + Resolution order: + + 1. The form field corresponding to the model field (``model_field.formfield(required=...)``) + when the model field exposes a ``formfield`` factory. + 2. The form field declared on the filter itself (``filter_field.field``) + when the model field's factory yields a falsy value or when the + model field has no ``formfield`` factory at all. + + ``model_field`` may be ``None`` (when the filter targets something that + is not a real model field, e.g. an annotation or a method-only filter). + In that case we always fall through to ``filter_field.field``. + """ + form_field = None + if hasattr(model_field, "formfield"): + form_field = model_field.formfield(required=required) + if not form_field: + form_field = filter_field.field + return form_field + + +def _get_field_type_and_form_field_for_implicit_filter( + model, filter_type, filter_field, registry, required +): + """Resolve the GraphQL type for a filter that is not explicitly declared. + + Implicit filters are those generated from ``Meta.fields = {...}`` on the + ``FilterSet`` (rather than declared as class attributes). Their type is + derived from the underlying model field. + + Returns a ``(field_type, form_field)`` tuple where: + + * ``field_type`` is the resolved GraphQL type, or ``None`` if it could + not be derived from the model — in which case the caller falls back + to converting the form field via :func:`_get_field_type_for_explicit_filter`. + * ``form_field`` is the Django form field that the caller may reuse for + the explicit-filter fallback so it does not need to resolve it twice. + + Special-case: ``isnull`` lookups are always boolean and need no form + field, so the helper short-circuits with ``(graphene.Boolean, None)``. + """ + if filter_type == "isnull": + return graphene.Boolean, None + + model_field = get_model_field(model, filter_field.field_name) + form_field = _get_form_field(model_field, filter_field, required) + + if model_field: + field_type = _get_field_type_from_model_field(model_field, form_field, registry) + return field_type, form_field + + return None, form_field + + +def _get_field_type_for_explicit_filter(filter_field, form_field): + """Resolve the GraphQL type for an explicitly-declared filter via form-field conversion. + + Used when: + + * The filter was declared explicitly on the ``FilterSet`` class (so its + type cannot be derived from a model field), or + * The implicit-filter path could not resolve a registry type (e.g. the + target model is not exposed via a ``DjangoObjectType``). + + Falls back to ``filter_field.field`` when ``form_field`` is falsy, then + runs the form field through :func:`graphene_django.forms.converter.convert_form_field` + to obtain the final GraphQL type. + """ + from ..forms.converter import convert_form_field + + form_field = form_field or filter_field.field + return convert_form_field(form_field).get_type() + + +def _is_filter_list_or_range(filter_field): + """Return ``True`` when the filter is a ``ListFilter`` or ``RangeFilter``. + + Both filter classes accept a list of values for the ``in`` and ``range`` + lookups, so their argument type must be wrapped in ``graphene.List``. + See :func:`replace_csv_filters` for context on why these custom filter + classes exist. + """ + return isinstance(filter_field, (ListFilter, RangeFilter)) def get_filtering_args_from_filterset(filterset_class, type): @@ -28,8 +163,6 @@ def get_filtering_args_from_filterset(filterset_class, type): Inspect a FilterSet and produce the arguments to pass to a Graphene Field. These arguments will be available to filter against in the GraphQL API. """ - from ..forms.converter import convert_form_field - args = {} model = filterset_class._meta.model registry = type._meta.registry @@ -49,47 +182,16 @@ def get_filtering_args_from_filterset(filterset_class, type): if name not in filterset_class.declared_filters or isinstance( filter_field, TypedFilter ): - # Get the filter field for filters that are no explicitly declared. - if filter_type == "isnull": - field_type = graphene.Boolean - else: - model_field = get_model_field(model, filter_field.field_name) - - # Get the form field either from: - # 1. the formfield corresponding to the model field - # 2. the field defined on filter - if hasattr(model_field, "formfield"): - form_field = model_field.formfield(required=required) - if not form_field: - form_field = filter_field.field - - # First try to get the matching field type from the GraphQL DjangoObjectType - if model_field: - if ( - isinstance(form_field, forms.ModelChoiceField) - or isinstance(form_field, forms.ModelMultipleChoiceField) - or isinstance(form_field, GlobalIDMultipleChoiceField) - or isinstance(form_field, GlobalIDFormField) - ): - # Foreign key have dynamic types and filtering on a foreign key actually means filtering on its ID. - field_type = get_field_type( - registry, model_field.related_model, "id" - ) - else: - field_type = get_field_type( - registry, model_field.model, model_field.name - ) + field_type, form_field = _get_field_type_and_form_field_for_implicit_filter( + model, filter_type, filter_field, registry, required + ) if not field_type: - # Fallback on converting the form field either because: - # - it's an explicitly declared filters - # - we did not manage to get the type from the model type - form_field = form_field or filter_field.field - field_type = convert_form_field(form_field).get_type() - - if isinstance(filter_field, ListFilter) or isinstance( - filter_field, RangeFilter - ): + field_type = _get_field_type_for_explicit_filter( + filter_field, form_field + ) + + if _is_filter_list_or_range(filter_field): # Replace InFilter/RangeFilter filters (`in`, `range`) argument type to be a list of # the same type as the field. See comments in `replace_csv_filters` method for more details. field_type = graphene.List(field_type) @@ -127,7 +229,7 @@ def replace_csv_filters(filterset_class): This is because those BaseCSVFilter are expecting a string as input with comma separated values. - But with GraphQl we can actually have a list as input and have a proper + But with GraphQL we can actually have a list as input and have a proper type verification of each value in the list. See issue https://github.com/graphql-python/graphene-django/issues/1068. diff --git a/graphene_django/tests/test_types.py b/graphene_django/tests/test_types.py index 0491bcd30..d0ff8c4d3 100644 --- a/graphene_django/tests/test_types.py +++ b/graphene_django/tests/test_types.py @@ -31,7 +31,7 @@ class ArticleConnection(Connection): test = String() - def resolve_test(): + def resolve_test(parent): return "test" class Meta: @@ -832,3 +832,266 @@ class Query(ObjectType): assert "type Reporter implements Node {" not in schema assert "type ReporterConnection {" not in schema assert "type ReporterEdge {" not in schema + + +class TestValidateFieldsHelpers: + """Direct unit tests for the ``validate_fields`` extraction in + :mod:`graphene_django.types`. + + These tests exercise :func:`_validate_only_fields`, + :func:`_validate_exclude_fields`, and the + :func:`validate_fields` orchestrator in isolation — without going + through ``DjangoObjectType.__init_subclass_with_meta__`` — so each + warning case fails a focused test rather than only being detected + via integration coverage on type construction. + + Scenarios covered: + + * ``_validate_only_fields`` + - happy path: requested name is in ``all_field_names`` (no warning) + - requested name resolves to a non-field model attribute (warning + about "matches an attribute") + - requested name is unknown to the model (warning about "doesn't + exist on Django model") + - ``only_fields=None`` is a no-op + - ``only_fields=ALL_FIELDS`` is normalised to a no-op by the + orchestrator and is therefore exercised through ``validate_fields`` + * ``_validate_exclude_fields`` + - happy path: excluded name corresponds to a real model field + (no warning) + - excluded name is a custom field on the type (warning about + "Excluding the custom field") + - excluded name is unknown to the model (warning about "does not + have a field or attribute") + - ``exclude_fields=None`` is a no-op + * ``validate_fields`` orchestrator + - ``ALL_FIELDS`` sentinel for ``only_fields`` is normalised to + empty iteration (and therefore emits no warning) + - delegates to both helpers (combined-warning happy path) + + The tests assert on the message text via ``pytest.warns(..., match=...)`` + so a regression in the error wording also fails a focused test. + """ + + @staticmethod + def _all_field_names(): + """Reusable set of "real" converted-field names for ReporterModel.""" + return {"first_name", "last_name", "email", "pets", "custom_field"} + + def test_only_fields_known_field_emits_no_warning(self): + """ + Name: only_fields, known field, no warning + Description: A name present in ``all_field_names`` short-circuits via + ``continue`` and emits no warning. + Assumptions: ``first_name`` is one of the converted fields supplied + in ``all_field_names``. + Expectations: ``warnings.simplefilter("error")`` does not raise — i.e. + no warning is emitted. + """ + from ..types import _validate_only_fields + + with warnings.catch_warnings(): + warnings.simplefilter("error") + _validate_only_fields( + ["first_name"], self._all_field_names(), ReporterModel, "TestType" + ) + + def test_only_fields_model_attribute_warns_about_attribute(self): + """ + Name: only_fields, attribute-not-field, attribute warning + Description: A name that exists as a Python attribute on the model but + is not a model field (e.g. ``Reporter.some_method``) must produce + the "matches an attribute" warning so the user knows to declare + the field on the type. + Assumptions: ``Reporter.some_method`` is defined on the model and is + not in ``all_field_names``. + Expectations: A ``UserWarning`` whose message matches the + "matches an attribute on Django model" wording is emitted. + """ + from ..types import _validate_only_fields + + with pytest.warns( + UserWarning, + match=r"matches an attribute on Django model .* but it's not a model field", + ): + _validate_only_fields( + ["some_method"], self._all_field_names(), ReporterModel, "TestType" + ) + + def test_only_fields_unknown_name_warns_about_missing_field(self): + """ + Name: only_fields, unknown name, missing-field warning + Description: A name that is neither a converted field nor any + attribute on the model must produce the "doesn't exist on + Django model" warning so the user knows it is a typo or stale + entry in ``Meta.fields``. + Assumptions: ``"foo"`` is not a field of ReporterModel and is not + in ``all_field_names``. + Expectations: A ``UserWarning`` whose message matches the + "doesn't exist on Django model" wording is emitted. + """ + from ..types import _validate_only_fields + + with pytest.warns( + UserWarning, + match=r"Field name \"foo\" doesn't exist on Django model", + ): + _validate_only_fields( + ["foo"], self._all_field_names(), ReporterModel, "TestType" + ) + + def test_only_fields_none_is_a_noop(self): + """ + Name: only_fields, None argument, no-op + Description: Passing ``None`` for ``only_fields`` must iterate over + an empty sequence and emit no warning. + Assumptions: The helper uses ``only_fields or ()`` so ``None`` is + normalised at call site. + Expectations: ``warnings.simplefilter("error")`` does not raise. + """ + from ..types import _validate_only_fields + + with warnings.catch_warnings(): + warnings.simplefilter("error") + _validate_only_fields( + None, self._all_field_names(), ReporterModel, "TestType" + ) + + def test_exclude_fields_real_model_field_emits_no_warning(self): + """ + Name: exclude_fields, real model field, no warning + Description: Excluding a name that exists on the model and is not + also a custom field on the type is the happy path and produces + no warning. + Assumptions: ``"first_name"`` is on ReporterModel and is not in the + ``all_field_names`` set passed in (we deliberately use a set + without it to model "this is a real field, not a custom one"). + Expectations: ``warnings.simplefilter("error")`` does not raise. + """ + from ..types import _validate_exclude_fields + + with warnings.catch_warnings(): + warnings.simplefilter("error") + _validate_exclude_fields( + ["first_name"], + {"custom_field"}, + ReporterModel, + "TestType", + ) + + def test_exclude_fields_custom_field_warns_about_no_effect(self): + """ + Name: exclude_fields, custom field, "no effect" warning + Description: Excluding a name that maps to a custom field declared + directly on the ``DjangoObjectType`` has no effect (since + ``Meta.exclude`` only filters auto-converted model fields), so + a warning must be emitted. + Assumptions: ``"custom_field"`` is in the supplied ``all_field_names`` + (i.e. it was added by the user as a custom field on the type). + Expectations: A ``UserWarning`` whose message matches the + "Excluding the custom field" wording is emitted. + """ + from ..types import _validate_exclude_fields + + with pytest.warns( + UserWarning, + match=r"Excluding the custom field \"custom_field\" on DjangoObjectType", + ): + _validate_exclude_fields( + ["custom_field"], + self._all_field_names(), + ReporterModel, + "TestType", + ) + + def test_exclude_fields_unknown_name_warns_about_missing_attribute(self): + """ + Name: exclude_fields, unknown name, missing-attribute warning + Description: Excluding a name that is neither a custom field nor any + attribute of the model must produce the "does not have a field + or attribute" warning to flag the typo or stale entry. + Assumptions: ``"foo"`` does not exist on ReporterModel and is not in + ``all_field_names``. + Expectations: A ``UserWarning`` matching the "does not have a field + or attribute" wording is emitted. + """ + from ..types import _validate_exclude_fields + + with pytest.warns( + UserWarning, + match=r"does not have a field or attribute named \"foo\"", + ): + _validate_exclude_fields( + ["foo"], self._all_field_names(), ReporterModel, "TestType" + ) + + def test_exclude_fields_none_is_a_noop(self): + """ + Name: exclude_fields, None argument, no-op + Description: Passing ``None`` for ``exclude_fields`` must iterate + over an empty sequence and emit no warning. + Assumptions: The helper uses ``exclude_fields or ()`` so ``None`` is + normalised at call site. + Expectations: ``warnings.simplefilter("error")`` does not raise. + """ + from ..types import _validate_exclude_fields + + with warnings.catch_warnings(): + warnings.simplefilter("error") + _validate_exclude_fields( + None, self._all_field_names(), ReporterModel, "TestType" + ) + + def test_validate_fields_normalises_all_fields_sentinel(self): + """ + Name: validate_fields, ALL_FIELDS sentinel + Description: The orchestrator must treat ``only_fields=ALL_FIELDS`` + as an empty iteration so that the request "all fields" never + triggers a per-name warning loop. + Assumptions: :data:`graphene_django.types.ALL_FIELDS` is the sentinel + used to mean "all fields". + Expectations: No warning is emitted even though the + ``ALL_FIELDS`` string itself is not in ``all_field_names``. + """ + from ..types import ALL_FIELDS, validate_fields + + fake_fields = {"first_name": object(), "last_name": object()} + with warnings.catch_warnings(): + warnings.simplefilter("error") + validate_fields( + "TestType", + ReporterModel, + fake_fields, + only_fields=ALL_FIELDS, + exclude_fields=None, + ) + + def test_validate_fields_delegates_to_both_helpers(self): + """ + Name: validate_fields, delegates to both helpers + Description: The orchestrator must forward both ``only_fields`` and + ``exclude_fields`` to their respective helpers, so an invalid + entry in either list still surfaces a warning. + Assumptions: ``"foo"`` is unknown to the model in both contexts. + Expectations: Two warnings are emitted in a single call: one for + the missing-from-fields name and one for the + missing-from-exclude name. + """ + from ..types import validate_fields + + fake_fields = {"first_name": object()} + with warnings.catch_warnings(record=True) as recorded: + warnings.simplefilter("always") + validate_fields( + "TestType", + ReporterModel, + fake_fields, + only_fields=["foo"], + exclude_fields=["bar"], + ) + + messages = [str(w.message) for w in recorded] + assert any("Field name \"foo\" doesn't exist" in m for m in messages), messages + assert any( + "does not have a field or attribute named \"bar\"" in m for m in messages + ), messages diff --git a/graphene_django/tests/test_views.py b/graphene_django/tests/test_views.py index 2f9a1c32e..b99007e26 100644 --- a/graphene_django/tests/test_views.py +++ b/graphene_django/tests/test_views.py @@ -922,3 +922,376 @@ def test_exceeds_max_validation_errors(client, url): error_messages = (error["message"].lower() for error in json_response["errors"]) assert any(MAX_VALIDATION_ERRORS_EXCEEDED_MESSAGE in msg for msg in error_messages) + + +class TestExecuteGraphqlRequestRefactor: + """Pin the ``execute_graphql_request`` extraction in + :mod:`graphene_django.views`. + + The refactor introduced two private helpers, + :meth:`GraphQLView._should_short_circuit_get_request` and + :meth:`GraphQLView._is_atomic_mutation`, and the tests in this class + exercise the previously-implicit branches that those helpers now + cover so that any regression fails a focused test. + + Scenarios covered: + + * ``_should_short_circuit_get_request`` + - POST never short-circuits regardless of operation kind. + - GET + query never short-circuits (the B1 regression: an earlier + rewrite of this branch caused queries with ``show_graphiql=True`` + to be silently dropped). + - GET + non-query + ``show_graphiql=True`` short-circuits to + ``True`` so the caller returns ``None``. + - GET + non-query + ``show_graphiql=False`` raises ``HttpError`` + carrying a 405 ``HttpResponseNotAllowed``. + - GET with ``operation_ast=None`` (e.g. an unparseable document) + does not short-circuit; the executor sees the error. + * ``_is_atomic_mutation`` + - Parametrised over the 8 combinations of (operation kind, + ATOMIC_MUTATIONS setting, db ATOMIC_MUTATIONS setting), + asserting the boolean outcome for each. + * ``execute_graphql_request`` end-to-end + - GET + query + ``show_graphiql=True`` invoked directly returns + a non-None ``ExecutionResult`` with the query's data (B1 pin). + - GET + mutation + ``show_graphiql=True`` invoked directly + returns ``None`` (existing behaviour preserved). + - ``get_operation_ast`` is invoked exactly once per request (B2 + pin: the original PR introduced a second call). + * ``GraphQLView.__init__`` + - Constructing the view with ``graphene_settings.MIDDLEWARE = None`` + succeeds and yields ``view.middleware is None`` (B3 pin: the + original PR dropped the ``if middleware is not None`` guard, + which would have raised ``TypeError``). + + Assumptions: the existing :data:`graphene_django.tests.schema_view.schema` + exposes a ``test`` query and a ``writeTest`` mutation, both used as + minimal exercises of the query and mutation branches respectively. + """ + + @staticmethod + def _make_view(): + """Build a :class:`GraphQLView` against the existing test schema.""" + from graphene_django.views import GraphQLView + + from .schema_view import schema + + return GraphQLView(schema=schema) + + @staticmethod + def _build_request(method, **get_params): + """Build a tiny stand-in for a Django HttpRequest. + + ``execute_graphql_request`` only reads ``request.method`` (in the + ``_should_short_circuit_get_request`` helper) and is otherwise + agnostic to the request object, so a SimpleNamespace is enough + for these unit tests and avoids spinning up the full request + factory. + """ + from types import SimpleNamespace + + return SimpleNamespace(method=method, GET={}, META={}) + + def test_should_short_circuit_returns_false_for_post(self): + """ + Name: short-circuit, POST is never short-circuited + Description: ``_should_short_circuit_get_request`` must return + ``False`` for POST requests so all operation kinds (queries, + mutations, subscriptions) are routed to the executor. + Assumptions: A POST mutation is the canonical "do not + short-circuit" case. + Expectations: Returns ``False``. + """ + from graphql import OperationType, parse, get_operation_ast + + from graphene_django.views import GraphQLView + + document = parse("mutation { writeTest { test } }") + op_ast = get_operation_ast(document, None) + assert op_ast.operation == OperationType.MUTATION + + request = self._build_request("post") + assert ( + GraphQLView._should_short_circuit_get_request(request, op_ast, True) + is False + ) + + def test_should_short_circuit_returns_false_for_get_query(self): + """ + Name: short-circuit, GET + query never short-circuits (B1 pin) + Description: A query operation on a GET request must always be + executed normally, even when GraphiQL is rendering. This is + the regression the PR refactor introduced and that the + ``_should_short_circuit_get_request`` helper is designed to + prevent. + Assumptions: ``test`` is a query in :data:`schema_view.schema`. + Expectations: Returns ``False`` regardless of ``show_graphiql``. + """ + from graphql import OperationType, parse, get_operation_ast + + from graphene_django.views import GraphQLView + + document = parse("{ test }") + op_ast = get_operation_ast(document, None) + assert op_ast.operation == OperationType.QUERY + + request = self._build_request("get") + assert ( + GraphQLView._should_short_circuit_get_request(request, op_ast, True) + is False + ) + assert ( + GraphQLView._should_short_circuit_get_request(request, op_ast, False) + is False + ) + + def test_should_short_circuit_returns_true_for_get_mutation_with_graphiql(self): + """ + Name: short-circuit, GET + mutation + show_graphiql=True + Description: The only case that short-circuits to ``True`` is a + non-query operation on a GET request when GraphiQL is + rendering, so the caller can return ``None`` without raising. + Assumptions: ``writeTest`` is a mutation in + :data:`schema_view.schema`. + Expectations: Returns ``True``. + """ + from graphql import OperationType, parse, get_operation_ast + + from graphene_django.views import GraphQLView + + document = parse("mutation { writeTest { test } }") + op_ast = get_operation_ast(document, None) + assert op_ast.operation == OperationType.MUTATION + + request = self._build_request("get") + assert ( + GraphQLView._should_short_circuit_get_request(request, op_ast, True) + is True + ) + + def test_should_short_circuit_raises_for_get_mutation_without_graphiql(self): + """ + Name: short-circuit, GET + mutation + no GraphiQL raises 405 + Description: A non-query GET request without GraphiQL must raise + ``HttpError`` carrying a 405 ``HttpResponseNotAllowed`` so the + client gets the documented "Can only perform a {} operation + from a POST request" error. + Assumptions: The operation is a mutation. + Expectations: ``HttpError`` is raised; the wrapped response is a + 405 with the expected message. + """ + from http import HTTPStatus + + from graphql import OperationType, parse, get_operation_ast + + from graphene_django.views import GraphQLView, HttpError + + document = parse("mutation { writeTest { test } }") + op_ast = get_operation_ast(document, None) + assert op_ast.operation == OperationType.MUTATION + + request = self._build_request("get") + with pytest.raises(HttpError) as excinfo: + GraphQLView._should_short_circuit_get_request(request, op_ast, False) + assert excinfo.value.response.status_code == HTTPStatus.METHOD_NOT_ALLOWED + assert ( + "Can only perform a mutation operation from a POST request." + in excinfo.value.message + ) + + def test_should_short_circuit_returns_false_when_operation_ast_is_none(self): + """ + Name: short-circuit, missing operation_ast + Description: When ``get_operation_ast`` couldn't resolve an + operation (e.g. operation_name not found on the document), the + helper must not short-circuit; downstream validation will + surface the appropriate error to the user. + Assumptions: ``operation_ast`` is allowed to be ``None``. + Expectations: Returns ``False`` for both GET and POST. + """ + from graphene_django.views import GraphQLView + + for method in ("get", "post"): + request = self._build_request(method) + assert ( + GraphQLView._should_short_circuit_get_request(request, None, True) + is False + ) + assert ( + GraphQLView._should_short_circuit_get_request(request, None, False) + is False + ) + + @pytest.mark.parametrize( + "operation_str,atomic_global,atomic_db,expected", + [ + # Queries are never atomic regardless of settings. + ("{ test }", True, True, False), + ("{ test }", False, False, False), + # Mutations require at least one of the two flags. + ("mutation { writeTest { test } }", False, False, False), + ("mutation { writeTest { test } }", True, False, True), + ("mutation { writeTest { test } }", False, True, True), + ("mutation { writeTest { test } }", True, True, True), + ], + ) + def test_is_atomic_mutation_combinations( + self, monkeypatch, operation_str, atomic_global, atomic_db, expected + ): + """ + Name: _is_atomic_mutation, parametrised settings + Description: ``_is_atomic_mutation`` must return ``True`` only when + the operation is a mutation **and** at least one of the two + ATOMIC_MUTATIONS flags (graphene setting or per-connection db + setting) is enabled. + Assumptions: The two-flag rule mirrors the original inline logic + in ``execute_graphql_request``. + Expectations: For each ``(operation_kind, atomic_global, atomic_db)`` + combination, the helper returns the documented boolean. + """ + from django.db import connection as db_connection + from graphql import parse, get_operation_ast + + from graphene_django.views import GraphQLView + + monkeypatch.setattr( + "graphene_django.views.graphene_settings.ATOMIC_MUTATIONS", + atomic_global, + ) + monkeypatch.setitem( + db_connection.settings_dict, "ATOMIC_MUTATIONS", atomic_db + ) + + document = parse(operation_str) + op_ast = get_operation_ast(document, None) + assert GraphQLView._is_atomic_mutation(op_ast) is expected + + def test_is_atomic_mutation_returns_false_for_none_operation(self): + """ + Name: _is_atomic_mutation, missing operation_ast + Description: With no operation AST, the helper must return ``False`` + so the caller does not wrap an unparseable document in a + transaction. + Assumptions: ``operation_ast`` is allowed to be ``None``. + Expectations: Returns ``False``. + """ + from graphene_django.views import GraphQLView + + assert GraphQLView._is_atomic_mutation(None) is False + + def test_execute_graphql_request_get_with_query_and_graphiql_executes(self): + """ + Name: execute_graphql_request, GET + query + show_graphiql=True (B1 pin) + Description: Calling ``execute_graphql_request`` directly with + ``show_graphiql=True`` and a query must still execute the + query and return a non-None ``ExecutionResult``. The earlier + rewrite of this code path returned ``None`` unconditionally + on ``show_graphiql=True``, dropping the result silently. + Assumptions: ``test`` is a String query on :data:`schema_view.schema` + that returns ``"Hello World"`` when called without arguments. + Expectations: The returned ``ExecutionResult`` has no errors and + ``result.data == {"test": "Hello World"}``. + """ + view = self._make_view() + request = self._build_request("get") + + result = view.execute_graphql_request( + request, + data={}, + query="{ test }", + variables=None, + operation_name=None, + show_graphiql=True, + ) + assert result is not None + assert result.errors is None + assert result.data == {"test": "Hello World"} + + def test_execute_graphql_request_get_with_mutation_and_graphiql_returns_none( + self, + ): + """ + Name: execute_graphql_request, GET + mutation + show_graphiql=True + Description: A non-query operation on a GET request with GraphiQL + rendering must short-circuit to ``None`` so GraphiQL can be + displayed without executing the mutation. This mirrors the + existing public behaviour and protects it across the + extraction. + Assumptions: ``writeTest`` is a mutation on :data:`schema_view.schema`. + Expectations: Returns ``None``. + """ + view = self._make_view() + request = self._build_request("get") + + result = view.execute_graphql_request( + request, + data={}, + query="mutation { writeTest { test } }", + variables=None, + operation_name=None, + show_graphiql=True, + ) + assert result is None + + def test_execute_graphql_request_calls_get_operation_ast_once(self, monkeypatch): + """ + Name: execute_graphql_request, single get_operation_ast call (B2 pin) + Description: The refactor must continue to call ``get_operation_ast`` + exactly once per request — the original PR introduced a second + call inside the extracted validation helper, which this test + prevents from regressing. + Assumptions: ``graphene_django.views.get_operation_ast`` is the + single import the view uses. + Expectations: The wrapped function is invoked exactly one time + during a single ``execute_graphql_request`` call. + """ + from graphene_django import views as views_module + + original = views_module.get_operation_ast + call_count = {"n": 0} + + def counting_get_operation_ast(*args, **kwargs): + call_count["n"] += 1 + return original(*args, **kwargs) + + monkeypatch.setattr( + views_module, "get_operation_ast", counting_get_operation_ast + ) + + view = self._make_view() + request = self._build_request("post") + view.execute_graphql_request( + request, + data={}, + query="{ test }", + variables=None, + operation_name=None, + show_graphiql=False, + ) + assert call_count["n"] == 1, ( + f"expected exactly one get_operation_ast call, got {call_count['n']}" + ) + + def test_init_with_middleware_setting_none_does_not_raise(self, monkeypatch): + """ + Name: __init__ with MIDDLEWARE=None (B3 pin) + Description: ``GraphQLView.__init__`` must tolerate + ``graphene_settings.MIDDLEWARE = None`` without raising + ``TypeError``. The original PR dropped the ``if middleware is + not None`` guard, which would have caused + ``list(instantiate_middleware(None))`` to fail. + Assumptions: ``graphene_settings.MIDDLEWARE`` is a publicly-overridable + setting that may legitimately be ``None``. + Expectations: Constructing the view succeeds and ``view.middleware`` + equals the class default (``None``). + """ + from graphene_django.views import GraphQLView + + from .schema_view import schema + + monkeypatch.setattr( + "graphene_django.views.graphene_settings.MIDDLEWARE", None + ) + + view = GraphQLView(schema=schema) + assert view.middleware is None diff --git a/graphene_django/types.py b/graphene_django/types.py index e310fe478..c9daf9107 100644 --- a/graphene_django/types.py +++ b/graphene_django/types.py @@ -62,10 +62,32 @@ def construct_fields( return fields -def validate_fields(type_, model, fields, only_fields, exclude_fields): - # Validate the given fields against the model's fields and custom fields - all_field_names = set(fields.keys()) - only_fields = only_fields if only_fields is not ALL_FIELDS else () +def _validate_only_fields(only_fields, all_field_names, model, type_): + """Validate the names listed in ``Meta.fields`` (a.k.a. ``only_fields``). + + For each requested name, emit a warning when it cannot be matched to a + converted model field. Two cases are distinguished so that the message + can guide the user toward the right fix: + + * The name resolves to a Python attribute on the model (e.g. a property + or method) but is not a model field — Graphene cannot infer a type, so + the user must either declare the field on the ``DjangoObjectType`` or + drop it from ``Meta.fields``. + * The name does not exist on the model at all — it should simply be + removed from ``Meta.fields``. + + Names that already correspond to a converted field are silently skipped. + + Args: + only_fields: Iterable of field names requested via ``Meta.fields``. + ``None`` and the ``ALL_FIELDS`` sentinel are normalised by the + caller and arrive here as an empty iterable. + all_field_names: Set of field names that were successfully converted + from the model and any custom fields declared on the type. + model: The Django model class being introspected. + type_: The ``DjangoObjectType`` subclass name, used in warning + messages to help locate the offending definition. + """ for name in only_fields or (): if name in all_field_names: continue @@ -83,12 +105,57 @@ def validate_fields(type_, model, fields, only_fields, exclude_fields): type_=type_, ) ) + continue - else: + warnings.warn( + ( + 'Field name "{field_name}" doesn\'t exist on Django model "{app_label}.{object_name}". ' + 'Consider removing the field from the "fields" list of DjangoObjectType "{type_}" because it has no effect.' + ).format( + field_name=name, + app_label=model._meta.app_label, + object_name=model._meta.object_name, + type_=type_, + ) + ) + + +def _validate_exclude_fields(exclude_fields, all_field_names, model, type_): + """Validate the names listed in ``Meta.exclude``. + + Two warning cases are emitted: + + * The excluded name corresponds to a custom field declared directly on + the ``DjangoObjectType``. ``Meta.exclude`` only filters auto-converted + model fields, so excluding a custom field has no effect. + * The excluded name is neither a model field nor any attribute of the + model — almost certainly a typo or stale name. + + A name that maps to a real model field/attribute (and is not also a + custom field on the type) is the happy path and produces no warning. + + Args: + exclude_fields: Iterable of field names requested via ``Meta.exclude``. + ``None`` results in a no-op iteration. + all_field_names: Set of field names that were successfully converted + from the model and any custom fields declared on the type. + model: The Django model class being introspected. + type_: The ``DjangoObjectType`` subclass name, used in warning + messages to help locate the offending definition. + """ + for name in exclude_fields or (): + if name in all_field_names: + warnings.warn( + f'Excluding the custom field "{name}" on DjangoObjectType "{type_}" has no effect. ' + 'Either remove the custom field or remove the field from the "exclude" list.' + ) + continue + + if not hasattr(model, name): warnings.warn( ( - 'Field name "{field_name}" doesn\'t exist on Django model "{app_label}.{object_name}". ' - 'Consider removing the field from the "fields" list of DjangoObjectType "{type_}" because it has no effect.' + 'Django model "{app_label}.{object_name}" does not have a field or attribute named "{field_name}". ' + 'Consider removing the field from the "exclude" list of DjangoObjectType "{type_}" because it has no effect' ).format( field_name=name, app_label=model._meta.app_label, @@ -97,27 +164,22 @@ def validate_fields(type_, model, fields, only_fields, exclude_fields): ) ) - # Validate exclude fields - for name in exclude_fields or (): - if name in all_field_names: - # Field is a custom field - warnings.warn( - f'Excluding the custom field "{name}" on DjangoObjectType "{type_}" has no effect. ' - 'Either remove the custom field or remove the field from the "exclude" list.' - ) - else: - if not hasattr(model, name): - warnings.warn( - ( - 'Django model "{app_label}.{object_name}" does not have a field or attribute named "{field_name}". ' - 'Consider removing the field from the "exclude" list of DjangoObjectType "{type_}" because it has no effect' - ).format( - field_name=name, - app_label=model._meta.app_label, - object_name=model._meta.object_name, - type_=type_, - ) - ) + +def validate_fields(type_, model, fields, only_fields, exclude_fields): + """Validate ``Meta.fields`` and ``Meta.exclude`` against the model. + + Thin orchestrator over :func:`_validate_only_fields` and + :func:`_validate_exclude_fields` that normalises ``only_fields`` + (treating the ``ALL_FIELDS`` sentinel as ``()``) and passes the set of + converted field names to both helpers. The public signature is + preserved so downstream callers (e.g. ``DjangoObjectType``) are + unaffected by the refactor. + """ + all_field_names = set(fields.keys()) + only_fields = only_fields if only_fields is not ALL_FIELDS else () + + _validate_only_fields(only_fields, all_field_names, model, type_) + _validate_exclude_fields(exclude_fields, all_field_names, model, type_) class DjangoObjectTypeOptions(ObjectTypeOptions): diff --git a/graphene_django/utils/testing.py b/graphene_django/utils/testing.py index 2ca1de941..ebea874fe 100644 --- a/graphene_django/utils/testing.py +++ b/graphene_django/utils/testing.py @@ -153,6 +153,15 @@ def assertResponseHasErrors(self, resp, msg=None): content = json.loads(resp.content) self.assertIn("errors", list(content.keys()), msg or content) + # PEP 8 / snake_case aliases. The camelCase methods above remain the + # canonical names (matching Django's own convention for assertion + # helpers such as ``assertEqual`` / ``assertNumQueries``); these + # aliases are provided for users who prefer snake_case in their test + # suites. Both names point at the same callable, so behaviour is + # identical and there is no deprecation warning on either spelling. + assert_response_no_errors = assertResponseNoErrors + assert_response_has_errors = assertResponseHasErrors + class GraphQLTestCase(GraphQLTestMixin, TestCase): pass diff --git a/graphene_django/utils/tests/test_testing.py b/graphene_django/utils/tests/test_testing.py index 801708ec8..0c35eabdb 100644 --- a/graphene_django/utils/tests/test_testing.py +++ b/graphene_django/utils/tests/test_testing.py @@ -1,9 +1,12 @@ +import json + import pytest from django.test import Client from ...settings import graphene_settings from ...tests.test_types import with_local_registry from .. import GraphQLTestCase +from ..testing import GraphQLTestMixin @with_local_registry @@ -52,3 +55,147 @@ def test_graphql_test_case_imports_endpoint(): """ assert GraphQLTestCase.GRAPHQL_URL == graphene_settings.TESTING_ENDPOINT + + +class _FakeHttpResponse: + """Minimal stand-in for ``django.http.HttpResponse``. + + The ``assertResponse*`` helpers only touch ``.status_code`` and + ``.content``, so we don't need the full Django response machinery to + exercise them. Building this stand-in keeps the alias tests free of + any dependency on routing, schemas, or the test client. + """ + + def __init__(self, content_dict, status_code=200): + self.content = json.dumps(content_dict).encode("utf-8") + self.status_code = status_code + + +class TestAssertionAliases: + """Coverage for the snake_case aliases in :mod:`graphene_django.utils.testing`. + + The PR adds ``assert_response_no_errors`` / + ``assert_response_has_errors`` as additive aliases for the existing + camelCase ``assertResponseNoErrors`` / ``assertResponseHasErrors``. + These tests pin three properties of that contract: + + 1. The aliases must point at the **same callable** as the + camelCase originals (no copy-paste re-implementation that could + drift out of sync over time). + 2. The aliases must work end-to-end on a "no errors" response. + 3. The aliases must work end-to-end on a "with errors" response. + + The end-to-end checks build a tiny ``GraphQLTestCase`` subclass and + invoke each alias against a synthesised :class:`_FakeHttpResponse`, + so neither GraphiQL routing nor a real schema needs to be involved. + """ + + def test_alias_is_identical_to_camel_case_original(self): + """ + Name: alias identity check + Description: ``assert_response_no_errors`` and + ``assert_response_has_errors`` must be the **same** function + objects as their camelCase counterparts so behaviour cannot + drift between spellings. + Assumptions: Both spellings are defined on + :class:`GraphQLTestMixin`. + Expectations: ``is`` identity holds for both pairs. + """ + assert ( + GraphQLTestMixin.assert_response_no_errors + is GraphQLTestMixin.assertResponseNoErrors + ) + assert ( + GraphQLTestMixin.assert_response_has_errors + is GraphQLTestMixin.assertResponseHasErrors + ) + + def test_assert_response_no_errors_alias_passes_for_clean_response(self): + """ + Name: snake_case no-errors helper, clean response + Description: ``assert_response_no_errors`` should silently accept a + 200 response whose JSON body has no ``errors`` key, exactly + like its camelCase counterpart. + Assumptions: A bare ``{"data": {...}}`` body with status 200 is the + canonical "clean" GraphQL response. + Expectations: The call returns without raising. + """ + + class _TC(GraphQLTestCase): + GRAPHQL_SCHEMA = True + + def runTest(self): + pass + + tc = _TC() + resp = _FakeHttpResponse({"data": {"hello": "world"}}, status_code=200) + tc.assert_response_no_errors(resp) + + def test_assert_response_no_errors_alias_fails_when_errors_present(self): + """ + Name: snake_case no-errors helper, errors present + Description: ``assert_response_no_errors`` should fail when the + response contains an ``errors`` key, exactly like its + camelCase counterpart. + Assumptions: ``unittest`` assertion failures raise + :class:`AssertionError`. + Expectations: ``AssertionError`` is raised. + """ + + class _TC(GraphQLTestCase): + GRAPHQL_SCHEMA = True + + def runTest(self): + pass + + tc = _TC() + resp = _FakeHttpResponse( + {"errors": [{"message": "boom"}]}, status_code=200 + ) + with pytest.raises(AssertionError): + tc.assert_response_no_errors(resp) + + def test_assert_response_has_errors_alias_passes_when_errors_present(self): + """ + Name: snake_case has-errors helper, errors present + Description: ``assert_response_has_errors`` should silently accept + a response whose body has an ``errors`` key, regardless of + status code (GraphQL returns 200 even on errors). + Assumptions: An ``{"errors": [...]}``-shaped response represents + a failing GraphQL query. + Expectations: The call returns without raising. + """ + + class _TC(GraphQLTestCase): + GRAPHQL_SCHEMA = True + + def runTest(self): + pass + + tc = _TC() + resp = _FakeHttpResponse( + {"errors": [{"message": "boom"}]}, status_code=200 + ) + tc.assert_response_has_errors(resp) + + def test_assert_response_has_errors_alias_fails_for_clean_response(self): + """ + Name: snake_case has-errors helper, clean response + Description: ``assert_response_has_errors`` should fail when the + response has no ``errors`` key, exactly like its camelCase + counterpart. + Assumptions: ``unittest`` assertion failures raise + :class:`AssertionError`. + Expectations: ``AssertionError`` is raised. + """ + + class _TC(GraphQLTestCase): + GRAPHQL_SCHEMA = True + + def runTest(self): + pass + + tc = _TC() + resp = _FakeHttpResponse({"data": {"hello": "world"}}, status_code=200) + with pytest.raises(AssertionError): + tc.assert_response_has_errors(resp) diff --git a/graphene_django/views.py b/graphene_django/views.py index 1ec659881..a1b04598a 100644 --- a/graphene_django/views.py +++ b/graphene_django/views.py @@ -319,22 +319,8 @@ def execute_graphql_request( operation_ast = get_operation_ast(document, operation_name) - if ( - request.method.lower() == "get" - and operation_ast is not None - 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 - ), - ) - ) + if self._should_short_circuit_get_request(request, operation_ast, show_graphiql): + return None validation_errors = validate( schema, @@ -359,14 +345,7 @@ def execute_graphql_request( "execution_context_class" ] = self.execution_context_class - if ( - operation_ast is not None - 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(operation_ast): with transaction.atomic(): result = execute(schema, document, **execute_options) if getattr(request, MUTATION_ERRORS_FLAG, False) is True: @@ -377,6 +356,64 @@ def execute_graphql_request( except Exception as e: return ExecutionResult(errors=[e]) + @staticmethod + def _should_short_circuit_get_request(request, operation_ast, show_graphiql): + """Decide whether a GET request with a non-query operation should short-circuit. + + For GET requests, only ``query`` operations are allowed: mutations + and subscriptions must be POSTed. This helper enforces that rule and + signals to the caller whether to short-circuit (return ``None``) + instead of executing the operation. + + Returns ``True`` only when GraphiQL is rendering a non-query GET + request — in that case ``execute_graphql_request`` should return + ``None`` so GraphiQL can be displayed without executing the + operation. Returns ``False`` for the happy path (POST, or GET with a + ``query`` operation), so execution proceeds normally. + + Raises :class:`HttpError` (with a 405 ``HttpResponseNotAllowed``) for + a non-query GET request when GraphiQL is *not* being rendered, + preserving the prior public behaviour. + """ + if request.method.lower() != "get": + return False + if operation_ast is None: + return False + if operation_ast.operation == OperationType.QUERY: + return False + + if show_graphiql: + return True + + raise HttpError( + HttpResponseNotAllowed( + ["POST"], + "Can only perform a {} operation from a POST request.".format( + operation_ast.operation.value + ), + ) + ) + + @staticmethod + def _is_atomic_mutation(operation_ast): + """Return ``True`` when the operation is a mutation that must be wrapped in a transaction. + + Mutations are wrapped in ``transaction.atomic()`` when either the + global ``ATOMIC_MUTATIONS`` graphene setting is ``True`` or the + per-connection ``settings_dict["ATOMIC_MUTATIONS"]`` is ``True``. + Anything else (queries, subscriptions, mutations without an atomic + flag set) returns ``False`` so the caller executes them outside a + transaction. + """ + if operation_ast is None: + return False + if operation_ast.operation != OperationType.MUTATION: + return False + return ( + graphene_settings.ATOMIC_MUTATIONS is True + or connection.settings_dict.get("ATOMIC_MUTATIONS", False) is True + ) + @classmethod def can_display_graphiql(cls, request, data): raw = "raw" in request.GET or "raw" in data diff --git a/setup.py b/setup.py index 0c4a4cc74..d1ab11d27 100644 --- a/setup.py +++ b/setup.py @@ -17,6 +17,8 @@ "pytest>=7.3.1", "pytest-cov", "pytest-random-order", + "pytest-repeat>=0.9.1", + "pytest-xdist>=3.3.1", "coveralls", "mock", "pytz",