From 280b38f804f4619066062e28bcbaba9914e54ab2 Mon Sep 17 00:00:00 2001
From: Jonathan Kim <jkimbo@gmail.com>
Date: Fri, 7 Feb 2020 09:55:38 +0000
Subject: [PATCH] Only warn if a field doesn't exist on the Django model (#862)

* Only warn if a field doesn't exist on the Django model

Also don't warn if the field name matches a custom field.

* Expand warning messages
---
 graphene_django/tests/test_types.py | 18 ++++++---
 graphene_django/types.py            | 59 ++++++++++++++++++++---------
 2 files changed, 54 insertions(+), 23 deletions(-)

diff --git a/graphene_django/tests/test_types.py b/graphene_django/tests/test_types.py
index cb31a9c..a25383f 100644
--- a/graphene_django/tests/test_types.py
+++ b/graphene_django/tests/test_types.py
@@ -320,26 +320,34 @@ def test_django_objecttype_fields_exclude_type_checking():
 
 @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 .* matches an attribute on Django model .* but it's not a model field",
+    ) as record:
 
         class Reporter2(DjangoObjectType):
             class Meta:
                 model = ReporterModel
-                exclude = ["first_name", "foo", "email"]
+                fields = ["first_name", "some_method", "email"]
 
-    with pytest.raises(Exception, match=r".* exists on model .* but it's not a field"):
+    # Don't warn if selecting a custom field
+    with pytest.warns(None) as record:
 
         class Reporter3(DjangoObjectType):
+            custom_field = String()
+
             class Meta:
                 model = ReporterModel
-                fields = ["first_name", "some_method", "email"]
+                fields = ["first_name", "custom_field", "email"]
+
+    assert len(record) == 0
 
 
 class TestDjangoObjectType:
diff --git a/graphene_django/types.py b/graphene_django/types.py
index 4824c45..129dbe1 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,44 @@ 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())
+    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 "{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 "{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_,
+                    )
+                )
+
+
 class DjangoObjectTypeOptions(ObjectTypeOptions):
     model = None  # type: Model
     registry = None  # type: Registry
@@ -211,6 +231,9 @@ class DjangoObjectType(ObjectType):
             _meta=_meta, interfaces=interfaces, **options
         )
 
+        # Validate fields
+        validate_fields(cls, model, _meta.fields, fields, exclude)
+
         if not skip_registry:
             registry.register(cls)