From b7e4937775a951c6d3990db58689bd9acee8a222 Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Tue, 9 Jul 2019 14:03:11 +0100 Subject: [PATCH] Alias `only_fields` as `fields` and `exclude_fields` as `exclude` (#691) * Create new fields and exclude options that are aliased to exclude_fields and only_fields * Update docs * Add some checking around fields and exclude definitions * Add all fields option * Update docs to include `__all__` option * Actual order of fields is not stable * Update docs/queries.rst Co-Authored-By: Semyon Pupkov * Fix example code * Format code * Start raising PendingDeprecationWarnings for using only_fields and exclude_fields * Update tests --- docs/queries.rst | 45 ++++++--- graphene_django/filter/tests/test_fields.py | 2 +- .../rest_framework/serializer_converter.py | 4 +- .../tests/test_multiple_model_serializers.py | 8 +- graphene_django/tests/test_query.py | 8 +- graphene_django/tests/test_schema.py | 2 +- graphene_django/tests/test_types.py | 99 +++++++++++++++++-- graphene_django/types.py | 54 +++++++++- 8 files changed, 187 insertions(+), 35 deletions(-) diff --git a/docs/queries.rst b/docs/queries.rst index 7aff572..67ebb06 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,35 @@ Show **only** these fields on the model: class QuestionType(DjangoObjectType): class Meta: model = Question - only_fields = ('question_text') + 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. -exclude_fields -~~~~~~~~~~~~~~ - -Show all fields **except** those in ``exclude_fields``: +For example: .. code:: python class QuestionType(DjangoObjectType): class Meta: model = Question - exclude_fields = ('question_text') + fields = '__all__' -Customised fields -~~~~~~~~~~~~~~~~~ +``exclude`` +~~~~~~~~~~~ + +Show all fields **except** those in ``exclude``: + +.. code:: python + + class QuestionType(DjangoObjectType): + class Meta: + model = Question + exclude = ('question_text',) + + +Customising fields +------------------ You can completely overwrite a field, or add new fields, to a ``DjangoObjectType`` using a Resolver: @@ -84,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() @@ -178,7 +193,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 diff --git a/graphene_django/filter/tests/test_fields.py b/graphene_django/filter/tests/test_fields.py index d163ff3..99876b6 100644 --- a/graphene_django/filter/tests/test_fields.py +++ b/graphene_django/filter/tests/test_fields.py @@ -774,7 +774,7 @@ def test_integer_field_filter_type(): model = Pet interfaces = (Node,) filter_fields = {"age": ["exact"]} - only_fields = ["age"] + fields = ("age",) class Query(ObjectType): pets = DjangoFilterConnectionField(PetType) diff --git a/graphene_django/rest_framework/serializer_converter.py b/graphene_django/rest_framework/serializer_converter.py index 35c8dc8..c419419 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 4504610..c1f4626 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): diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index f466122..f24f84b 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 @@ def test_should_query_connectionfields(): class Meta: model = Reporter interfaces = (Node,) - only_fields = ("articles",) + fields = ("articles",) class Query(graphene.ObjectType): all_reporters = DjangoConnectionField(ReporterType) @@ -329,7 +329,7 @@ def test_should_keep_annotations(): 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 452449b..2c2f74b 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 6f5ab7e..6cbaae0 100644 --- a/graphene_django/tests/test_types.py +++ b/graphene_django/tests/test_types.py @@ -211,26 +211,113 @@ def with_local_registry(func): @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"] @with_local_registry -def test_django_objecttype_exclude_fields(): +def test_django_objecttype_fields(): class Reporter(DjangoObjectType): class Meta: model = ReporterModel - exclude_fields = "email" + 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_all_fields(): + class Reporter(DjangoObjectType): + class Meta: + model = ReporterModel + fields = "__all__" + + fields = list(Reporter._meta.fields.keys()) + assert len(fields) == len(ReporterModel._meta.get_fields()) + + +@with_local_registry +def test_django_objecttype_exclude_fields(): + 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 +@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"] + + +@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 6c100ef..ec426f1 100644 --- a/graphene_django/types.py +++ b/graphene_django/types.py @@ -1,3 +1,4 @@ +import warnings from collections import OrderedDict import six @@ -24,6 +25,9 @@ if six.PY3: from typing import Type +ALL_FIELDS = "__all__" + + def construct_fields( model, registry, only_fields, exclude_fields, convert_choices_to_enum ): @@ -74,8 +78,10 @@ class DjangoObjectType(ObjectType): 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 +115,48 @@ class DjangoObjectType(ObjectType): ) ) + 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: + 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( + '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") + 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( + "The `exclude` option must be a list or tuple. Got %s." + % type(exclude).__name__ + ) + 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, )