From 47650e760a4712dcdd38153b5a42cbf828b433a1 Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Wed, 29 Jan 2020 10:50:50 +0000 Subject: [PATCH 1/2] Only warn if a field doesn't exist on the Django model Also don't warn if the field name matches a custom field. --- graphene_django/tests/test_types.py | 21 +++++++++++-- graphene_django/types.py | 46 ++++++++++++++++++----------- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/graphene_django/tests/test_types.py b/graphene_django/tests/test_types.py index cb31a9c50..dcfabc42a 100644 --- a/graphene_django/tests/test_types.py +++ b/graphene_django/tests/test_types.py @@ -320,27 +320,42 @@ class Meta: @with_local_registry def test_django_objecttype_fields_exclude_exist_on_model(): - with pytest.raises(Exception, match=r"Field .* doesn't exist"): + with pytest.warns(UserWarning, match=r"Field name .* doesn't exist"): class Reporter(DjangoObjectType): class Meta: model = ReporterModel fields = ["first_name", "foo", "email"] - with pytest.raises(Exception, match=r"Field .* doesn't exist"): + with pytest.warns(UserWarning, match=r"Field name .* doesn't exist"): class Reporter2(DjangoObjectType): class Meta: model = ReporterModel exclude = ["first_name", "foo", "email"] - with pytest.raises(Exception, match=r".* exists on model .* but it's not a field"): + with pytest.warns( + UserWarning, + match=r"Field name .* exists on Django model .* but it's not a model field", + ): class Reporter3(DjangoObjectType): class Meta: model = ReporterModel fields = ["first_name", "some_method", "email"] + # Don't warn if selecting a custom field + with pytest.warns(None) as record: + + class Reporter4(DjangoObjectType): + custom_field = String() + + class Meta: + model = ReporterModel + fields = ["first_name", "custom_field", "email"] + + assert len(record) == 0 + class TestDjangoObjectType: @pytest.fixture diff --git a/graphene_django/types.py b/graphene_django/types.py index 4824c456b..1d2af713c 100644 --- a/graphene_django/types.py +++ b/graphene_django/types.py @@ -33,24 +33,6 @@ def construct_fields( ): _model_fields = get_model_fields(model) - # Validate the given fields against the model's fields. - model_field_names = set(field[0] for field in _model_fields) - for fields_list in (only_fields, exclude_fields): - if not fields_list: - continue - for name in fields_list: - if name in model_field_names: - continue - - if hasattr(model, name): - raise Exception( - '"{}" exists on model {} but it\'s not a field.'.format(name, model) - ) - else: - raise Exception( - 'Field "{}" doesn\'t exist on model {}.'.format(name, model) - ) - fields = OrderedDict() for name, field in _model_fields: is_not_in_only = only_fields and name not in only_fields @@ -80,6 +62,31 @@ def construct_fields( return fields +def validate_fields(model, fields, only_fields, exclude_fields): + # Validate the given fields against the model's fields and custom fields + all_field_names = set(fields.keys()) + for fields_list in (only_fields, exclude_fields): + if not fields_list: + continue + for name in fields_list: + if name in all_field_names: + continue + + if hasattr(model, name): + warnings.warn( + 'Field name "{}" exists on Django model {} but it\'s not a model field.'.format( + name, model + ) + ) + + else: + warnings.warn( + 'Field name "{}" doesn\'t exist on Django model {}.'.format( + name, model + ) + ) + + class DjangoObjectTypeOptions(ObjectTypeOptions): model = None # type: Model registry = None # type: Registry @@ -211,6 +218,9 @@ def __init_subclass_with_meta__( _meta=_meta, interfaces=interfaces, **options ) + # Validate fields + validate_fields(model, _meta.fields, fields, exclude) + if not skip_registry: registry.register(cls) From 948e501ad8002b7a72992f651ae5fa17d3ac2309 Mon Sep 17 00:00:00 2001 From: Jonathan Kim Date: Fri, 31 Jan 2020 15:20:33 +0000 Subject: [PATCH 2/2] Expand warning messages --- graphene_django/tests/test_types.py | 15 ++++----------- graphene_django/types.py | 25 +++++++++++++++++++------ 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/graphene_django/tests/test_types.py b/graphene_django/tests/test_types.py index dcfabc42a..a25383fca 100644 --- a/graphene_django/tests/test_types.py +++ b/graphene_django/tests/test_types.py @@ -327,19 +327,12 @@ class Meta: model = ReporterModel fields = ["first_name", "foo", "email"] - with pytest.warns(UserWarning, match=r"Field name .* doesn't exist"): - - class Reporter2(DjangoObjectType): - class Meta: - model = ReporterModel - exclude = ["first_name", "foo", "email"] - with pytest.warns( UserWarning, - match=r"Field name .* exists on Django model .* but it's not a model field", - ): + match=r"Field name .* matches an attribute on Django model .* but it's not a model field", + ) as record: - class Reporter3(DjangoObjectType): + class Reporter2(DjangoObjectType): class Meta: model = ReporterModel fields = ["first_name", "some_method", "email"] @@ -347,7 +340,7 @@ class Meta: # Don't warn if selecting a custom field with pytest.warns(None) as record: - class Reporter4(DjangoObjectType): + class Reporter3(DjangoObjectType): custom_field = String() class Meta: diff --git a/graphene_django/types.py b/graphene_django/types.py index 1d2af713c..129dbe150 100644 --- a/graphene_django/types.py +++ b/graphene_django/types.py @@ -62,7 +62,7 @@ def construct_fields( return fields -def validate_fields(model, fields, only_fields, exclude_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()) for fields_list in (only_fields, exclude_fields): @@ -74,15 +74,28 @@ def validate_fields(model, fields, only_fields, exclude_fields): if hasattr(model, name): warnings.warn( - 'Field name "{}" exists on Django model {} but it\'s not a model field.'.format( - name, model + ( + 'Field name "{field_name}" matches an attribute on Django model "{app_label}.{object_name}" ' + "but it's not a model field so Graphene cannot determine what type it should be. " + 'Either define the type of the field on DjangoObjectType "{type_}" or remove it from the "fields" list.' + ).format( + field_name=name, + app_label=model._meta.app_label, + object_name=model._meta.object_name, + type_=type_, ) ) else: warnings.warn( - 'Field name "{}" doesn\'t exist on Django model {}.'.format( - name, model + ( + '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_, ) ) @@ -219,7 +232,7 @@ def __init_subclass_with_meta__( ) # Validate fields - validate_fields(model, _meta.fields, fields, exclude) + validate_fields(cls, model, _meta.fields, fields, exclude) if not skip_registry: registry.register(cls)