From 64bc6c7c2e3f81dbb22f1f4c46b4c2a8dc627f3e Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Tue, 25 Jun 2019 16:41:28 +0100 Subject: [PATCH 01/11] Create new fields and exclude options that are aliased to exclude_fields and only_fields --- graphene_django/tests/test_types.py | 46 ++++++++++++++++++++++++++++- graphene_django/types.py | 22 ++++++++++---- 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/graphene_django/tests/test_types.py b/graphene_django/tests/test_types.py index 6f5ab7ed7..06a9e3d69 100644 --- a/graphene_django/tests/test_types.py +++ b/graphene_django/tests/test_types.py @@ -220,17 +220,61 @@ class Meta: assert fields == ["id", "email", "films"] +@with_local_registry +def test_django_objecttype_fields(): + class Reporter(DjangoObjectType): + class Meta: + model = ReporterModel + fields = ("id", "email", "films") + + fields = list(Reporter._meta.fields.keys()) + assert fields == ["id", "email", "films"] + + +@with_local_registry +def test_django_objecttype_only_fields_and_fields(): + with pytest.raises(Exception): + + class Reporter(DjangoObjectType): + class Meta: + model = ReporterModel + only_fields = ("id", "email", "films") + fields = ("id", "email", "films") + + @with_local_registry def test_django_objecttype_exclude_fields(): class Reporter(DjangoObjectType): class Meta: model = ReporterModel - exclude_fields = "email" + exclude_fields = ["email"] fields = list(Reporter._meta.fields.keys()) assert "email" not in fields +@with_local_registry +def test_django_objecttype_exclude(): + class Reporter(DjangoObjectType): + class Meta: + model = ReporterModel + exclude = ["email"] + + fields = list(Reporter._meta.fields.keys()) + assert "email" not in fields + + +@with_local_registry +def test_django_objecttype_exclude_fields_and_exclude(): + with pytest.raises(Exception): + + class Reporter(DjangoObjectType): + class Meta: + model = ReporterModel + exclude = ["email"] + exclude_fields = ["email"] + + class TestDjangoObjectType: @pytest.fixture def PetModel(self): diff --git a/graphene_django/types.py b/graphene_django/types.py index 6c100ef0e..1492f386d 100644 --- a/graphene_django/types.py +++ b/graphene_django/types.py @@ -74,8 +74,10 @@ def __init_subclass_with_meta__( model=None, registry=None, skip_registry=False, - only_fields=(), - exclude_fields=(), + only_fields=(), # deprecated in favour of `fields` + fields=(), + exclude_fields=(), # deprecated in favour of `exclude` + exclude=(), filter_fields=None, filterset_class=None, connection=None, @@ -109,10 +111,20 @@ def __init_subclass_with_meta__( ) ) + # Alias only_fields -> fields + if only_fields and fields: + raise Exception("Can't set both only_fields and fields") + if only_fields: + fields = only_fields + + # Alias exclude_fields -> exclude + if exclude_fields and exclude: + raise Exception("Can't set both exclude_fields and exclude") + if exclude_fields: + exclude = exclude_fields + django_fields = yank_fields_from_attrs( - construct_fields( - model, registry, only_fields, exclude_fields, convert_choices_to_enum - ), + construct_fields(model, registry, fields, exclude, convert_choices_to_enum), _as=Field, ) From 09402441fd0e740d2181e5fe8993455d538fdd31 Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Tue, 25 Jun 2019 16:50:43 +0100 Subject: [PATCH 02/11] Update docs --- docs/queries.rst | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/docs/queries.rst b/docs/queries.rst index 7aff57216..12071cf88 100644 --- a/docs/queries.rst +++ b/docs/queries.rst @@ -41,14 +41,18 @@ Full example return Question.objects.get(pk=question_id) -Fields ------- +Specifying which fields to include +---------------------------------- By default, ``DjangoObjectType`` will present all fields on a Model through GraphQL. -If you don't want to do this you can change this by setting either ``only_fields`` and ``exclude_fields``. +If you only want a subset of fields to be present, you can do so using +``fields`` or ``exclude``. It is strongly recommended that you explicitly set +all fields that should be exposed using the fields attribute. +This will make it less likely to result in unintentionally exposing data when +your models change. -only_fields -~~~~~~~~~~~ +``fields`` +~~~~~~~~~~ Show **only** these fields on the model: @@ -57,24 +61,24 @@ Show **only** these fields on the model: class QuestionType(DjangoObjectType): class Meta: model = Question - only_fields = ('question_text') + fields = ('id', 'question_text') -exclude_fields -~~~~~~~~~~~~~~ +``exclude`` +~~~~~~~~~~~ -Show all fields **except** those in ``exclude_fields``: +Show all fields **except** those in ``exclude``: .. code:: python class QuestionType(DjangoObjectType): class Meta: model = Question - exclude_fields = ('question_text') + exclude = ('question_text') -Customised fields -~~~~~~~~~~~~~~~~~ +Customising fields +------------------ You can completely overwrite a field, or add new fields, to a ``DjangoObjectType`` using a Resolver: @@ -178,7 +182,7 @@ When ``Question`` is published as a ``DjangoObjectType`` and you want to add ``C class QuestionType(DjangoObjectType): class Meta: model = Question - only_fields = ('category',) + fields = ('category',) Then all query-able related models must be defined as DjangoObjectType subclass, or they will fail to show if you are trying to query those relation fields. You only From a8584febade36344386ee06dfdeffb1750677564 Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Tue, 25 Jun 2019 16:55:34 +0100 Subject: [PATCH 03/11] Add some checking around fields and exclude definitions --- graphene_django/tests/test_types.py | 28 ++++++++++++++++++++++++++++ graphene_django/types.py | 15 +++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/graphene_django/tests/test_types.py b/graphene_django/tests/test_types.py index 06a9e3d69..0cd074083 100644 --- a/graphene_django/tests/test_types.py +++ b/graphene_django/tests/test_types.py @@ -275,6 +275,34 @@ class Meta: exclude_fields = ["email"] +@with_local_registry +def test_django_objecttype_exclude_and_only(): + with pytest.raises(AssertionError): + + class Reporter(DjangoObjectType): + class Meta: + model = ReporterModel + exclude = ["email"] + fields = ["id"] + + +@with_local_registry +def test_django_objecttype_fields_exclude_type_checking(): + with pytest.raises(TypeError): + + class Reporter(DjangoObjectType): + class Meta: + model = ReporterModel + fields = "foo" + + with pytest.raises(TypeError): + + class Reporter2(DjangoObjectType): + class Meta: + model = ReporterModel + fields = "foo" + + class TestDjangoObjectType: @pytest.fixture def PetModel(self): diff --git a/graphene_django/types.py b/graphene_django/types.py index 1492f386d..145e9a92c 100644 --- a/graphene_django/types.py +++ b/graphene_django/types.py @@ -111,17 +111,32 @@ def __init_subclass_with_meta__( ) ) + assert not (fields and exclude), ( + "Cannot set both 'fields' and 'exclude' options on " + "DjangoObjectType {class_name}.".format(class_name=cls.__name__) + ) + # Alias only_fields -> fields if only_fields and fields: raise Exception("Can't set both only_fields and fields") if only_fields: fields = only_fields + if fields and not isinstance(fields, (list, tuple)): + raise TypeError( + 'The `fields` option must be a list or tuple or "__all__". ' + "Got %s." % type(fields).__name__ + ) # Alias exclude_fields -> exclude if exclude_fields and exclude: raise Exception("Can't set both exclude_fields and exclude") if exclude_fields: exclude = exclude_fields + if exclude and not isinstance(exclude, (list, tuple)): + raise TypeError( + "The `exclude` option must be a list or tuple. Got %s." + % type(exclude).__name__ + ) django_fields = yank_fields_from_attrs( construct_fields(model, registry, fields, exclude, convert_choices_to_enum), From 4c10afba885faa0b950782dcb1570507835aa603 Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Tue, 25 Jun 2019 17:01:04 +0100 Subject: [PATCH 04/11] Add all fields option --- graphene_django/tests/test_types.py | 21 +++++++++++++++++++++ graphene_django/types.py | 8 +++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/graphene_django/tests/test_types.py b/graphene_django/tests/test_types.py index 0cd074083..c74a6b950 100644 --- a/graphene_django/tests/test_types.py +++ b/graphene_django/tests/test_types.py @@ -242,6 +242,27 @@ class Meta: fields = ("id", "email", "films") +@with_local_registry +def test_django_objecttype_all_fields(): + class Reporter(DjangoObjectType): + class Meta: + model = ReporterModel + fields = "__all__" + + fields = list(Reporter._meta.fields.keys()) + assert fields == [ + "id", + "first_name", + "last_name", + "email", + "pets", + "a_choice", + "reporter_type", + "films", + "articles", + ] + + @with_local_registry def test_django_objecttype_exclude_fields(): class Reporter(DjangoObjectType): diff --git a/graphene_django/types.py b/graphene_django/types.py index 145e9a92c..855761cf1 100644 --- a/graphene_django/types.py +++ b/graphene_django/types.py @@ -24,6 +24,9 @@ from typing import Type +ALL_FIELDS = "__all__" + + def construct_fields( model, registry, only_fields, exclude_fields, convert_choices_to_enum ): @@ -121,12 +124,15 @@ def __init_subclass_with_meta__( raise Exception("Can't set both only_fields and fields") if only_fields: fields = only_fields - if fields and not isinstance(fields, (list, tuple)): + if fields and fields != ALL_FIELDS and not isinstance(fields, (list, tuple)): raise TypeError( 'The `fields` option must be a list or tuple or "__all__". ' "Got %s." % type(fields).__name__ ) + if fields == ALL_FIELDS: + fields = None + # Alias exclude_fields -> exclude if exclude_fields and exclude: raise Exception("Can't set both exclude_fields and exclude") From 5662a040b9e47eb2bcc6f9d33950ce710c05cf4d Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Tue, 25 Jun 2019 17:02:21 +0100 Subject: [PATCH 05/11] Update docs to include `__all__` option --- docs/queries.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/queries.rst b/docs/queries.rst index 12071cf88..862dd327f 100644 --- a/docs/queries.rst +++ b/docs/queries.rst @@ -63,6 +63,17 @@ Show **only** these fields on the model: model = Question fields = ('id', 'question_text') +You can also set the ``fields`` attribute to the special value ``'__all__'`` to indicate that all fields in the model should be used. + +For example: + +.. code:: python + + class QuestionType(DjangoObjectType): + class Meta: + model = Question + fields = '__all__' + ``exclude`` ~~~~~~~~~~~ From 5f05bb6f07b71d1f66a5825c0aa00cf2b457fea6 Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Tue, 25 Jun 2019 17:18:47 +0100 Subject: [PATCH 06/11] Actual order of fields is not stable --- graphene_django/tests/test_types.py | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/graphene_django/tests/test_types.py b/graphene_django/tests/test_types.py index c74a6b950..ced0bfc38 100644 --- a/graphene_django/tests/test_types.py +++ b/graphene_django/tests/test_types.py @@ -250,17 +250,7 @@ class Meta: fields = "__all__" fields = list(Reporter._meta.fields.keys()) - assert fields == [ - "id", - "first_name", - "last_name", - "email", - "pets", - "a_choice", - "reporter_type", - "films", - "articles", - ] + assert len(fields) == len(ReporterModel._meta.get_fields()) @with_local_registry From 877692072377c0d99ca766fb4fac042b99553cd4 Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Wed, 26 Jun 2019 10:00:18 +0100 Subject: [PATCH 07/11] Update docs/queries.rst Co-Authored-By: Semyon Pupkov --- docs/queries.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/queries.rst b/docs/queries.rst index 862dd327f..7d2dc9c10 100644 --- a/docs/queries.rst +++ b/docs/queries.rst @@ -85,7 +85,7 @@ Show all fields **except** those in ``exclude``: class QuestionType(DjangoObjectType): class Meta: model = Question - exclude = ('question_text') + exclude = ('question_text', ) Customising fields From aa04fd7f8f06c92e7adb6c1c6375d4e012311327 Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Wed, 26 Jun 2019 10:02:41 +0100 Subject: [PATCH 08/11] Fix example code --- docs/queries.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/queries.rst b/docs/queries.rst index 7d2dc9c10..67ebb0681 100644 --- a/docs/queries.rst +++ b/docs/queries.rst @@ -85,7 +85,7 @@ Show all fields **except** those in ``exclude``: class QuestionType(DjangoObjectType): class Meta: model = Question - exclude = ('question_text', ) + exclude = ('question_text',) Customising fields @@ -99,7 +99,7 @@ You can completely overwrite a field, or add new fields, to a ``DjangoObjectType class Meta: model = Question - exclude_fields = ('question_text') + fields = ('id', 'question_text') extra_field = graphene.String() From 730e17f23ca62af71e1fe935f1906d45ea745d73 Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Tue, 9 Jul 2019 13:32:04 +0100 Subject: [PATCH 09/11] Format code --- graphene_django/rest_framework/serializer_converter.py | 4 +++- .../tests/test_multiple_model_serializers.py | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/graphene_django/rest_framework/serializer_converter.py b/graphene_django/rest_framework/serializer_converter.py index 35c8dc8bc..c419419c0 100644 --- a/graphene_django/rest_framework/serializer_converter.py +++ b/graphene_django/rest_framework/serializer_converter.py @@ -57,7 +57,9 @@ def convert_serializer_field(field, is_input=True): def convert_serializer_to_input_type(serializer_class): - cached_type = convert_serializer_to_input_type.cache.get(serializer_class.__name__, None) + cached_type = convert_serializer_to_input_type.cache.get( + serializer_class.__name__, None + ) if cached_type: return cached_type serializer = serializer_class() diff --git a/graphene_django/rest_framework/tests/test_multiple_model_serializers.py b/graphene_django/rest_framework/tests/test_multiple_model_serializers.py index 4504610f2..c1f462614 100644 --- a/graphene_django/rest_framework/tests/test_multiple_model_serializers.py +++ b/graphene_django/rest_framework/tests/test_multiple_model_serializers.py @@ -18,8 +18,12 @@ class MyFakeChildModel(models.Model): class MyFakeParentModel(models.Model): name = models.CharField(max_length=50) created = models.DateTimeField(auto_now_add=True) - child1 = models.OneToOneField(MyFakeChildModel, related_name='parent1', on_delete=models.CASCADE) - child2 = models.OneToOneField(MyFakeChildModel, related_name='parent2', on_delete=models.CASCADE) + child1 = models.OneToOneField( + MyFakeChildModel, related_name="parent1", on_delete=models.CASCADE + ) + child2 = models.OneToOneField( + MyFakeChildModel, related_name="parent2", on_delete=models.CASCADE + ) class ParentType(DjangoObjectType): From 022748cc0f094bcff5345d2252aff3a1221fa95b Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Tue, 9 Jul 2019 13:47:35 +0100 Subject: [PATCH 10/11] Start raising PendingDeprecationWarnings for using only_fields and exclude_fields --- graphene_django/types.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/graphene_django/types.py b/graphene_django/types.py index 855761cf1..ec426f19a 100644 --- a/graphene_django/types.py +++ b/graphene_django/types.py @@ -1,3 +1,4 @@ +import warnings from collections import OrderedDict import six @@ -123,6 +124,11 @@ def __init_subclass_with_meta__( if only_fields and fields: raise Exception("Can't set both only_fields and fields") if only_fields: + warnings.warn( + "Defining `only_fields` is deprecated in favour of `fields`.", + PendingDeprecationWarning, + stacklevel=2, + ) fields = only_fields if fields and fields != ALL_FIELDS and not isinstance(fields, (list, tuple)): raise TypeError( @@ -137,6 +143,11 @@ def __init_subclass_with_meta__( if exclude_fields and exclude: raise Exception("Can't set both exclude_fields and exclude") if exclude_fields: + warnings.warn( + "Defining `exclude_fields` is deprecated in favour of `exclude`.", + PendingDeprecationWarning, + stacklevel=2, + ) exclude = exclude_fields if exclude and not isinstance(exclude, (list, tuple)): raise TypeError( From 084aa0120fb5b1f6ec53a4eaded2ea29017a5796 Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Tue, 9 Jul 2019 13:48:07 +0100 Subject: [PATCH 11/11] Update tests --- graphene_django/filter/tests/test_fields.py | 2 +- graphene_django/tests/test_query.py | 8 ++++---- graphene_django/tests/test_schema.py | 2 +- graphene_django/tests/test_types.py | 20 ++++++++++++-------- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/graphene_django/filter/tests/test_fields.py b/graphene_django/filter/tests/test_fields.py index d163ff33e..99876b6ab 100644 --- a/graphene_django/filter/tests/test_fields.py +++ b/graphene_django/filter/tests/test_fields.py @@ -774,7 +774,7 @@ class Meta: model = Pet interfaces = (Node,) filter_fields = {"age": ["exact"]} - only_fields = ["age"] + fields = ("age",) class Query(ObjectType): pets = DjangoFilterConnectionField(PetType) diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index f4661221c..f24f84bca 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -28,7 +28,7 @@ def test_should_query_only_fields(): class ReporterType(DjangoObjectType): class Meta: model = Reporter - only_fields = ("articles",) + fields = ("articles",) schema = graphene.Schema(query=ReporterType) query = """ @@ -44,7 +44,7 @@ def test_should_query_simplelazy_objects(): class ReporterType(DjangoObjectType): class Meta: model = Reporter - only_fields = ("id",) + fields = ("id",) class Query(graphene.ObjectType): reporter = graphene.Field(ReporterType) @@ -289,7 +289,7 @@ class ReporterType(DjangoObjectType): class Meta: model = Reporter interfaces = (Node,) - only_fields = ("articles",) + fields = ("articles",) class Query(graphene.ObjectType): all_reporters = DjangoConnectionField(ReporterType) @@ -329,7 +329,7 @@ class ReporterType(DjangoObjectType): class Meta: model = Reporter interfaces = (Node,) - only_fields = ("articles",) + fields = ("articles",) class ArticleType(DjangoObjectType): class Meta: diff --git a/graphene_django/tests/test_schema.py b/graphene_django/tests/test_schema.py index 452449b5d..2c2f74b67 100644 --- a/graphene_django/tests/test_schema.py +++ b/graphene_django/tests/test_schema.py @@ -48,6 +48,6 @@ def test_should_map_only_few_fields(): class Reporter2(DjangoObjectType): class Meta: model = Reporter - only_fields = ("id", "email") + fields = ("id", "email") assert list(Reporter2._meta.fields.keys()) == ["id", "email"] diff --git a/graphene_django/tests/test_types.py b/graphene_django/tests/test_types.py index ced0bfc38..6cbaae0bb 100644 --- a/graphene_django/tests/test_types.py +++ b/graphene_django/tests/test_types.py @@ -211,10 +211,12 @@ def inner(*args, **kwargs): @with_local_registry def test_django_objecttype_only_fields(): - class Reporter(DjangoObjectType): - class Meta: - model = ReporterModel - only_fields = ("id", "email", "films") + with pytest.warns(PendingDeprecationWarning): + + class Reporter(DjangoObjectType): + class Meta: + model = ReporterModel + only_fields = ("id", "email", "films") fields = list(Reporter._meta.fields.keys()) assert fields == ["id", "email", "films"] @@ -255,10 +257,12 @@ class Meta: @with_local_registry def test_django_objecttype_exclude_fields(): - class Reporter(DjangoObjectType): - class Meta: - model = ReporterModel - exclude_fields = ["email"] + with pytest.warns(PendingDeprecationWarning): + + class Reporter(DjangoObjectType): + class Meta: + model = ReporterModel + exclude_fields = ["email"] fields = list(Reporter._meta.fields.keys()) assert "email" not in fields