diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 07c0766..a733c03 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -10,17 +10,17 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - name: Set up Python 3.9 - uses: actions/setup-python@v2 + - uses: actions/checkout@v3 + - name: Set up Python 3.10 + uses: actions/setup-python@v4 with: - python-version: 3.9 + python-version: '3.10' - name: Build wheel and source tarball run: | pip install wheel python setup.py sdist bdist_wheel - name: Publish a Python distribution to PyPI - uses: pypa/gh-action-pypi-publish@v1.1.0 + uses: pypa/gh-action-pypi-publish@v1.8.6 with: user: __token__ password: ${{ secrets.pypi_password }} diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 9f1c3ab..8cee90a 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -7,11 +7,11 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - name: Set up Python 3.9 - uses: actions/setup-python@v2 + - uses: actions/checkout@v3 + - name: Set up Python 3.10 + uses: actions/setup-python@v4 with: - python-version: 3.9 + python-version: '3.10' - name: Install dependencies run: | python -m pip install --upgrade pip diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index c2cdc99..31b479e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -14,9 +14,9 @@ jobs: - django: "3.2" python-version: "3.7" steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v2 + uses: actions/setup-python@v4 with: python-version: ${{ matrix.python-version }} - name: Install dependencies diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e64c4e1..adb54c7 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,8 +1,8 @@ default_language_version: - python: python3.9 + python: python3.10 repos: - repo: https://github.com/pre-commit/pre-commit-hooks - rev: v4.3.0 + rev: v4.4.0 hooks: - id: check-merge-conflict - id: check-json @@ -16,15 +16,15 @@ repos: - id: trailing-whitespace exclude: README.md - repo: https://github.com/asottile/pyupgrade - rev: v3.2.0 + rev: v3.3.2 hooks: - id: pyupgrade args: [--py37-plus] - repo: https://github.com/psf/black - rev: 22.10.0 + rev: 23.3.0 hooks: - id: black - repo: https://github.com/PyCQA/flake8 - rev: 5.0.4 + rev: 6.0.0 hooks: - id: flake8 diff --git a/Makefile b/Makefile index 391c454..29c412b 100644 --- a/Makefile +++ b/Makefile @@ -6,6 +6,7 @@ help: .PHONY: dev-setup ## Install development dependencies dev-setup: pip install -e ".[dev]" + python -m pre_commit install .PHONY: tests ## Run unit tests tests: diff --git a/docs/filtering.rst b/docs/filtering.rst index fb686a1..95576a0 100644 --- a/docs/filtering.rst +++ b/docs/filtering.rst @@ -2,8 +2,8 @@ Filtering ========= Graphene integrates with -`django-filter `__ to provide filtering of results. -See the `usage documentation `__ +`django-filter `__ to provide filtering of results. +See the `usage documentation `__ for details on the format for ``filter_fields``. This filtering is automatically available when implementing a ``relay.Node``. diff --git a/examples/cookbook-plain/cookbook/ingredients/migrations/0001_initial.py b/examples/cookbook-plain/cookbook/ingredients/migrations/0001_initial.py index 345cadb..23d71e8 100644 --- a/examples/cookbook-plain/cookbook/ingredients/migrations/0001_initial.py +++ b/examples/cookbook-plain/cookbook/ingredients/migrations/0001_initial.py @@ -5,7 +5,6 @@ from django.db import migrations, models class Migration(migrations.Migration): - initial = True dependencies = [] diff --git a/examples/cookbook-plain/cookbook/ingredients/migrations/0002_auto_20161104_0050.py b/examples/cookbook-plain/cookbook/ingredients/migrations/0002_auto_20161104_0050.py index 00fe255..5f9e7a0 100644 --- a/examples/cookbook-plain/cookbook/ingredients/migrations/0002_auto_20161104_0050.py +++ b/examples/cookbook-plain/cookbook/ingredients/migrations/0002_auto_20161104_0050.py @@ -4,7 +4,6 @@ from django.db import migrations, models class Migration(migrations.Migration): - dependencies = [ ("ingredients", "0001_initial"), ] diff --git a/examples/cookbook-plain/cookbook/ingredients/migrations/0003_auto_20181018_1746.py b/examples/cookbook-plain/cookbook/ingredients/migrations/0003_auto_20181018_1746.py index 8015d1f..e823a2e 100644 --- a/examples/cookbook-plain/cookbook/ingredients/migrations/0003_auto_20181018_1746.py +++ b/examples/cookbook-plain/cookbook/ingredients/migrations/0003_auto_20181018_1746.py @@ -4,7 +4,6 @@ from django.db import migrations class Migration(migrations.Migration): - dependencies = [ ("ingredients", "0002_auto_20161104_0050"), ] diff --git a/examples/cookbook-plain/cookbook/recipes/migrations/0001_initial.py b/examples/cookbook-plain/cookbook/recipes/migrations/0001_initial.py index fceeb9b..c415147 100644 --- a/examples/cookbook-plain/cookbook/recipes/migrations/0001_initial.py +++ b/examples/cookbook-plain/cookbook/recipes/migrations/0001_initial.py @@ -5,7 +5,6 @@ from django.db import migrations, models class Migration(migrations.Migration): - initial = True dependencies = [ diff --git a/examples/cookbook-plain/cookbook/recipes/migrations/0002_auto_20161104_0106.py b/examples/cookbook-plain/cookbook/recipes/migrations/0002_auto_20161104_0106.py index 0156920..f38bb69 100644 --- a/examples/cookbook-plain/cookbook/recipes/migrations/0002_auto_20161104_0106.py +++ b/examples/cookbook-plain/cookbook/recipes/migrations/0002_auto_20161104_0106.py @@ -4,7 +4,6 @@ from django.db import migrations, models class Migration(migrations.Migration): - dependencies = [ ("recipes", "0001_initial"), ] diff --git a/examples/cookbook-plain/cookbook/recipes/migrations/0003_auto_20181018_1728.py b/examples/cookbook-plain/cookbook/recipes/migrations/0003_auto_20181018_1728.py index c54855b..dacdb30 100644 --- a/examples/cookbook-plain/cookbook/recipes/migrations/0003_auto_20181018_1728.py +++ b/examples/cookbook-plain/cookbook/recipes/migrations/0003_auto_20181018_1728.py @@ -4,7 +4,6 @@ from django.db import migrations, models class Migration(migrations.Migration): - dependencies = [ ("recipes", "0002_auto_20161104_0106"), ] diff --git a/examples/cookbook/cookbook/ingredients/migrations/0001_initial.py b/examples/cookbook/cookbook/ingredients/migrations/0001_initial.py index 345cadb..23d71e8 100644 --- a/examples/cookbook/cookbook/ingredients/migrations/0001_initial.py +++ b/examples/cookbook/cookbook/ingredients/migrations/0001_initial.py @@ -5,7 +5,6 @@ from django.db import migrations, models class Migration(migrations.Migration): - initial = True dependencies = [] diff --git a/examples/cookbook/cookbook/ingredients/migrations/0002_auto_20161104_0050.py b/examples/cookbook/cookbook/ingredients/migrations/0002_auto_20161104_0050.py index 00fe255..5f9e7a0 100644 --- a/examples/cookbook/cookbook/ingredients/migrations/0002_auto_20161104_0050.py +++ b/examples/cookbook/cookbook/ingredients/migrations/0002_auto_20161104_0050.py @@ -4,7 +4,6 @@ from django.db import migrations, models class Migration(migrations.Migration): - dependencies = [ ("ingredients", "0001_initial"), ] diff --git a/examples/cookbook/cookbook/recipes/migrations/0001_initial.py b/examples/cookbook/cookbook/recipes/migrations/0001_initial.py index fceeb9b..c415147 100644 --- a/examples/cookbook/cookbook/recipes/migrations/0001_initial.py +++ b/examples/cookbook/cookbook/recipes/migrations/0001_initial.py @@ -5,7 +5,6 @@ from django.db import migrations, models class Migration(migrations.Migration): - initial = True dependencies = [ diff --git a/examples/cookbook/cookbook/recipes/migrations/0002_auto_20161104_0106.py b/examples/cookbook/cookbook/recipes/migrations/0002_auto_20161104_0106.py index 0156920..f38bb69 100644 --- a/examples/cookbook/cookbook/recipes/migrations/0002_auto_20161104_0106.py +++ b/examples/cookbook/cookbook/recipes/migrations/0002_auto_20161104_0106.py @@ -4,7 +4,6 @@ from django.db import migrations, models class Migration(migrations.Migration): - dependencies = [ ("recipes", "0001_initial"), ] diff --git a/graphene_django/__init__.py b/graphene_django/__init__.py index 7a413fc..12408a4 100644 --- a/graphene_django/__init__.py +++ b/graphene_django/__init__.py @@ -1,7 +1,7 @@ from .fields import DjangoConnectionField, DjangoListField from .types import DjangoObjectType -__version__ = "3.0.0" +__version__ = "3.0.2" __all__ = [ "__version__", diff --git a/graphene_django/converter.py b/graphene_django/converter.py index 386103a..9ad6c9d 100644 --- a/graphene_django/converter.py +++ b/graphene_django/converter.py @@ -315,26 +315,7 @@ def convert_field_to_djangomodel(field, registry=None): if not _type: return - class CustomField(Field): - def wrap_resolve(self, parent_resolver): - """ - Implements a custom resolver which go through the `get_node` method to ensure that - it goes through the `get_queryset` method of the DjangoObjectType. - """ - resolver = super().wrap_resolve(parent_resolver) - - def custom_resolver(root, info, **args): - fk_obj = resolver(root, info, **args) - if not isinstance(fk_obj, model): - # In case the resolver is a custom one that overwrites - # the default Django resolver - # This happens, for example, when using custom awaitable resolvers. - return fk_obj - return _type.get_node(info, fk_obj.pk) - - return custom_resolver - - return CustomField( + return Field( _type, description=get_django_field_description(field), required=not field.null, diff --git a/graphene_django/filter/tests/conftest.py b/graphene_django/filter/tests/conftest.py index a11831c..f8a65d7 100644 --- a/graphene_django/filter/tests/conftest.py +++ b/graphene_django/filter/tests/conftest.py @@ -87,7 +87,6 @@ def Query(EventType): events = DjangoFilterConnectionField(EventType) def resolve_events(self, info, **kwargs): - events = [ Event(name="Live Show", tags=["concert", "music", "rock"]), Event(name="Musical", tags=["movie", "music"]), diff --git a/graphene_django/forms/mutation.py b/graphene_django/forms/mutation.py index 3d59464..40d1d3c 100644 --- a/graphene_django/forms/mutation.py +++ b/graphene_django/forms/mutation.py @@ -82,7 +82,6 @@ class DjangoFormMutation(BaseDjangoFormMutation): def __init_subclass_with_meta__( cls, form_class=None, only_fields=(), exclude_fields=(), **options ): - if not form_class: raise Exception("form_class is required for DjangoFormMutation") @@ -129,7 +128,6 @@ class DjangoModelFormMutation(BaseDjangoFormMutation): exclude_fields=(), **options, ): - if not form_class: raise Exception("form_class is required for DjangoModelFormMutation") diff --git a/graphene_django/rest_framework/mutation.py b/graphene_django/rest_framework/mutation.py index c01d915..4062a44 100644 --- a/graphene_django/rest_framework/mutation.py +++ b/graphene_django/rest_framework/mutation.py @@ -72,7 +72,6 @@ class SerializerMutation(ClientIDMutation): _meta=None, **options ): - if not serializer_class: raise Exception("serializer_class is required for the SerializerMutation") diff --git a/graphene_django/static/graphene_django/graphiql.js b/graphene_django/static/graphene_django/graphiql.js index 106b470..5b9d96d 100644 --- a/graphene_django/static/graphene_django/graphiql.js +++ b/graphene_django/static/graphene_django/graphiql.js @@ -60,40 +60,27 @@ function trueLambda() { return true; }; - var fetcher = GraphiQL.createFetcher({ + var headers = {}; + var cookies = ("; " + document.cookie).split("; csrftoken="); + if (cookies.length == 2) { + csrftoken = cookies.pop().split(";").shift(); + } else { + csrftoken = document.querySelector("[name=csrfmiddlewaretoken]").value; + } + if (csrftoken) { + headers['X-CSRFToken'] = csrftoken + } + + var graphQLFetcher = GraphiQL.createFetcher({ url: fetchURL, wsClient: graphqlWs.createClient({ url: subscribeURL, shouldRetry: trueLambda, lazy: true, - }) + }), + headers: headers }) - function graphQLFetcher(graphQLParams, opts) { - if (typeof opts === 'undefined') { - opts = {}; - } - var headers = opts.headers || {}; - headers['Accept'] = headers['Accept'] || 'application/json'; - headers['Content-Type'] = headers['Content-Type'] || 'application/json'; - - // Parse the cookie value for a CSRF token - var csrftoken; - var cookies = ("; " + document.cookie).split("; csrftoken="); - if (cookies.length == 2) { - csrftoken = cookies.pop().split(";").shift(); - } else { - csrftoken = document.querySelector("[name=csrfmiddlewaretoken]").value; - } - if (csrftoken) { - headers['X-CSRFToken'] = csrftoken - } - - opts.headers = headers - - return fetcher(graphQLParams, opts) - } - // When the query and variables string is edited, update the URL bar so // that it can be easily shared. function onEditQuery(newQuery) { diff --git a/graphene_django/tests/schema_view.py b/graphene_django/tests/schema_view.py index 8ed2ecf..4d538ba 100644 --- a/graphene_django/tests/schema_view.py +++ b/graphene_django/tests/schema_view.py @@ -5,7 +5,6 @@ from .mutations import PetFormMutation, PetMutation class QueryRoot(ObjectType): - thrower = graphene.String(required=True) request = graphene.String(required=True) test = graphene.String(who=graphene.String()) diff --git a/graphene_django/tests/test_fields.py b/graphene_django/tests/test_fields.py index 835de78..8c7b78d 100644 --- a/graphene_django/tests/test_fields.py +++ b/graphene_django/tests/test_fields.py @@ -1,5 +1,6 @@ import datetime -from django.db.models import Count +import re +from django.db.models import Count, Prefetch import pytest @@ -7,8 +8,12 @@ from graphene import List, NonNull, ObjectType, Schema, String from ..fields import DjangoListField from ..types import DjangoObjectType -from .models import Article as ArticleModel -from .models import Reporter as ReporterModel +from .models import ( + Article as ArticleModel, + Film as FilmModel, + FilmDetails as FilmDetailsModel, + Reporter as ReporterModel, +) class TestDjangoListField: @@ -500,3 +505,145 @@ class TestDjangoListField: assert not result.errors assert result.data == {"reporters": [{"firstName": "Tara"}]} + + def test_select_related_and_prefetch_related_are_respected( + self, django_assert_num_queries + ): + class Article(DjangoObjectType): + class Meta: + model = ArticleModel + fields = ("headline", "editor", "reporter") + + class Film(DjangoObjectType): + class Meta: + model = FilmModel + fields = ("genre", "details") + + class FilmDetail(DjangoObjectType): + class Meta: + model = FilmDetailsModel + fields = ("location",) + + class Reporter(DjangoObjectType): + class Meta: + model = ReporterModel + fields = ("first_name", "articles", "films") + + class Query(ObjectType): + articles = DjangoListField(Article) + + @staticmethod + def resolve_articles(root, info): + # Optimize for querying associated editors and reporters, and the films and film + # details of those reporters. This is similar to what would happen using a library + # like https://github.com/tfoxy/graphene-django-optimizer for a query like the one + # below (albeit simplified and hardcoded here). + return ArticleModel.objects.select_related( + "editor", "reporter" + ).prefetch_related( + Prefetch( + "reporter__films", + queryset=FilmModel.objects.select_related("details"), + ), + ) + + schema = Schema(query=Query) + + query = """ + query { + articles { + headline + + editor { + firstName + } + + reporter { + firstName + + films { + genre + + details { + location + } + } + } + } + } + """ + + r1 = ReporterModel.objects.create(first_name="Tara", last_name="West") + r2 = ReporterModel.objects.create(first_name="Debra", last_name="Payne") + + ArticleModel.objects.create( + headline="Amazing news", + reporter=r1, + pub_date=datetime.date.today(), + pub_date_time=datetime.datetime.now(), + editor=r2, + ) + ArticleModel.objects.create( + headline="Not so good news", + reporter=r2, + pub_date=datetime.date.today(), + pub_date_time=datetime.datetime.now(), + editor=r1, + ) + + film1 = FilmModel.objects.create(genre="ac") + film2 = FilmModel.objects.create(genre="ot") + film3 = FilmModel.objects.create(genre="do") + FilmDetailsModel.objects.create(location="Hollywood", film=film1) + FilmDetailsModel.objects.create(location="Antarctica", film=film3) + r1.films.add(film1, film2) + r2.films.add(film3) + + # We expect 2 queries to be performed based on the above resolver definition: one for all + # articles joined with the reporters model (for associated editors and reporters), and one + # for the films prefetch (which includes its `select_related` JOIN logic in its queryset) + with django_assert_num_queries(2) as captured: + result = schema.execute(query) + + assert not result.errors + assert result.data == { + "articles": [ + { + "headline": "Amazing news", + "editor": {"firstName": "Debra"}, + "reporter": { + "firstName": "Tara", + "films": [ + {"genre": "AC", "details": {"location": "Hollywood"}}, + {"genre": "OT", "details": None}, + ], + }, + }, + { + "headline": "Not so good news", + "editor": {"firstName": "Tara"}, + "reporter": { + "firstName": "Debra", + "films": [ + {"genre": "DO", "details": {"location": "Antarctica"}}, + ], + }, + }, + ] + } + + assert len(captured.captured_queries) == 2 # Sanity-check + + # First we should have queried for all articles in a single query, joining on the reporters + # model (for the editors and reporters ForeignKeys) + assert re.match( + r'SELECT .* "tests_article" INNER JOIN "tests_reporter"', + captured.captured_queries[0]["sql"], + ) + + # Then we should have queried for all of the films of all reporters, joined with the film + # details for each film, using a single query + assert re.match( + r'SELECT .* FROM "tests_film" INNER JOIN "tests_film_reporters" .* LEFT OUTER JOIN "tests_filmdetails"', + captured.captured_queries[1]["sql"], + ) diff --git a/graphene_django/tests/test_get_queryset.py b/graphene_django/tests/test_get_queryset.py index 91bdc70..7cbaa54 100644 --- a/graphene_django/tests/test_get_queryset.py +++ b/graphene_django/tests/test_get_queryset.py @@ -16,6 +16,12 @@ class TestShouldCallGetQuerySetOnForeignKey: Check that the get_queryset method is called in both forward and reversed direction of a foreignkey on types. (see issue #1111) + + NOTE: For now, we do not expect this get_queryset method to be called for nested + objects, as the original attempt to do so prevented SQL query-optimization with + `select_related`/`prefetch_related` and caused N+1 queries. See discussions here + https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857 + and here https://github.com/graphql-python/graphene-django/pull/1401. """ @pytest.fixture(autouse=True) @@ -121,69 +127,6 @@ class TestShouldCallGetQuerySetOnForeignKey: assert not result.errors assert result.data == {"reporter": {"firstName": "Jane"}} - def test_get_queryset_called_on_foreignkey(self): - # If a user tries to access a reporter through an article they should get our authorization error - query = """ - query getArticle($id: ID!) { - article(id: $id) { - headline - reporter { - firstName - } - } - } - """ - - result = self.schema.execute(query, variables={"id": self.articles[0].id}) - assert len(result.errors) == 1 - assert result.errors[0].message == "Not authorized to access reporters." - - # An admin user should be able to get reporters through an article - query = """ - query getArticle($id: ID!) { - article(id: $id) { - headline - reporter { - firstName - } - } - } - """ - - result = self.schema.execute( - query, - variables={"id": self.articles[0].id}, - context_value={"admin": True}, - ) - assert not result.errors - assert result.data["article"] == { - "headline": "A fantastic article", - "reporter": {"firstName": "Jane"}, - } - - # An admin user should not be able to access draft article through a reporter - query = """ - query getReporter($id: ID!) { - reporter(id: $id) { - firstName - articles { - headline - } - } - } - """ - - result = self.schema.execute( - query, - variables={"id": self.reporter.id}, - context_value={"admin": True}, - ) - assert not result.errors - assert result.data["reporter"] == { - "firstName": "Jane", - "articles": [{"headline": "A fantastic article"}], - } - class TestShouldCallGetQuerySetOnForeignKeyNode: """ @@ -290,72 +233,3 @@ class TestShouldCallGetQuerySetOnForeignKeyNode: ) assert not result.errors assert result.data == {"reporter": {"firstName": "Jane"}} - - def test_get_queryset_called_on_foreignkey(self): - # If a user tries to access a reporter through an article they should get our authorization error - query = """ - query getArticle($id: ID!) { - article(id: $id) { - headline - reporter { - firstName - } - } - } - """ - - result = self.schema.execute( - query, variables={"id": to_global_id("ArticleType", self.articles[0].id)} - ) - assert len(result.errors) == 1 - assert result.errors[0].message == "Not authorized to access reporters." - - # An admin user should be able to get reporters through an article - query = """ - query getArticle($id: ID!) { - article(id: $id) { - headline - reporter { - firstName - } - } - } - """ - - result = self.schema.execute( - query, - variables={"id": to_global_id("ArticleType", self.articles[0].id)}, - context_value={"admin": True}, - ) - assert not result.errors - assert result.data["article"] == { - "headline": "A fantastic article", - "reporter": {"firstName": "Jane"}, - } - - # An admin user should not be able to access draft article through a reporter - query = """ - query getReporter($id: ID!) { - reporter(id: $id) { - firstName - articles { - edges { - node { - headline - } - } - } - } - } - """ - - result = self.schema.execute( - query, - variables={"id": to_global_id("ReporterType", self.reporter.id)}, - context_value={"admin": True}, - ) - assert not result.errors - assert result.data["reporter"] == { - "firstName": "Jane", - "articles": {"edges": [{"node": {"headline": "A fantastic article"}}]}, - } diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index 292d579..bbc2c90 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -789,7 +789,6 @@ def test_should_query_promise_connectionfields(): def test_should_query_connectionfields_with_last(): - r = Reporter.objects.create( first_name="John", last_name="Doe", email="johndoe@example.com", a_choice=1 ) @@ -827,7 +826,6 @@ def test_should_query_connectionfields_with_last(): def test_should_query_connectionfields_with_manager(): - r = Reporter.objects.create( first_name="John", last_name="Doe", email="johndoe@example.com", a_choice=1 ) diff --git a/setup.py b/setup.py index d9aefef..96da8ff 100644 --- a/setup.py +++ b/setup.py @@ -14,7 +14,7 @@ rest_framework_require = ["djangorestframework>=3.6.3"] tests_require = [ - "pytest>=7.1.3", + "pytest>=7.3.1", "pytest-cov", "pytest-random-order", "coveralls", @@ -26,10 +26,11 @@ tests_require = [ dev_requires = [ - "black==22.8.0", - "flake8==5.0.4", - "flake8-black==0.3.3", - "flake8-bugbear==22.9.11", + "black==23.3.0", + "flake8==6.0.0", + "flake8-black==0.3.6", + "flake8-bugbear==23.3.23", + "pre-commit", ] + tests_require setup(