From 0b103417f9e85e060787c9aaaeaf15693d2872c9 Mon Sep 17 00:00:00 2001 From: Jonathan Mares Date: Sun, 14 Jan 2018 22:36:52 -0500 Subject: [PATCH 1/3] on_delete param where needed due to django 2 requirements added support for querying a model with objects that may be a proxy model, fixed and added tests a few style changes --- examples/starwars/models.py | 19 +++- graphene_django/tests/models.py | 44 ++++++++- graphene_django/tests/test_query.py | 143 ++++++++++++++++++++++++++- graphene_django/tests/test_schema.py | 1 + graphene_django/tests/test_types.py | 8 +- graphene_django/types.py | 3 +- 6 files changed, 209 insertions(+), 9 deletions(-) diff --git a/examples/starwars/models.py b/examples/starwars/models.py index 2f80e27..3964acb 100644 --- a/examples/starwars/models.py +++ b/examples/starwars/models.py @@ -5,7 +5,13 @@ from django.db import models class Character(models.Model): name = models.CharField(max_length=50) - ship = models.ForeignKey('Ship', blank=True, null=True, related_name='characters') + ship = models.ForeignKey( + 'Ship', + blank=True, + null=True, + on_delete=models.SET_NULL, + related_name='characters' + ) def __str__(self): return self.name @@ -13,7 +19,10 @@ class Character(models.Model): class Faction(models.Model): name = models.CharField(max_length=50) - hero = models.ForeignKey(Character) + hero = models.ForeignKey( + Character, + on_delete=models.SET_NULL, + ) def __str__(self): return self.name @@ -21,7 +30,11 @@ class Faction(models.Model): class Ship(models.Model): name = models.CharField(max_length=50) - faction = models.ForeignKey(Faction, related_name='ships') + faction = models.ForeignKey( + Faction, + on_delete=models.SET_NULL, + related_name='ships' + ) def __str__(self): return self.name diff --git a/graphene_django/tests/models.py b/graphene_django/tests/models.py index 0c62f28..747e485 100644 --- a/graphene_django/tests/models.py +++ b/graphene_django/tests/models.py @@ -15,7 +15,11 @@ class Pet(models.Model): class FilmDetails(models.Model): location = models.CharField(max_length=30) - film = models.OneToOneField('Film', related_name='details') + film = models.OneToOneField( + 'Film', + on_delete=models.CASCADE, + related_name='details' + ) class Film(models.Model): @@ -30,15 +34,49 @@ class Reporter(models.Model): pets = models.ManyToManyField('self') a_choice = models.CharField(max_length=30, choices=CHOICES) + reporter_type = models.IntegerField( + 'Reporter Type', + null=True, + blank=True, + choices=[(1, u'Regular'), (2, u'CNN Reporter')] ) + def __str__(self): # __unicode__ on Python 2 return "%s %s" % (self.first_name, self.last_name) + def __init__(self, *args, **kwargs): + """ + Override the init method so that during runtime, Django + can know that this object can be a CNNReporter by casting + it to the proxy model. Otherwise, as far as Django knows, + when a CNNReporter is pulled from the database, it is still + of type Reporter. This was added to test proxy model support. + """ + super(Reporter, self).__init__(*args, **kwargs) + if self.reporter_type == 2: # quick and dirty way without enums + self.__class__ = CNNReporter + +class CNNReporter(Reporter): + """ + This class is a proxy model for Reporter, used for testing + proxy model support + """ + class Meta: + proxy = True + class Article(models.Model): headline = models.CharField(max_length=100) pub_date = models.DateField() - reporter = models.ForeignKey(Reporter, related_name='articles') - editor = models.ForeignKey(Reporter, related_name='edited_articles_+') + reporter = models.ForeignKey( + Reporter, + on_delete=models.SET_NULL, + related_name='articles' + ) + editor = models.ForeignKey( + Reporter, + on_delete=models.SET_NULL, + related_name='edited_articles_+' + ) lang = models.CharField(max_length=2, help_text='Language', choices=[ ('es', 'Spanish'), ('en', 'English') diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index 0dece3f..a20824d 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -13,7 +13,11 @@ from ..compat import MissingType, JSONField from ..fields import DjangoConnectionField from ..types import DjangoObjectType from ..settings import graphene_settings -from .models import Article, Reporter +from .models import ( + Article, + CNNReporter, + Reporter, +) pytestmark = pytest.mark.django_db @@ -689,6 +693,7 @@ def test_should_query_dataloader_fields(): email='johndoe@example.com', a_choice=1 ) + Article.objects.create( headline='Article Node 1', pub_date=datetime.date.today(), @@ -780,3 +785,139 @@ def test_should_handle_inherited_choices(): ''' result = schema.execute(query) assert not result.errors + + +def test_proxy_model_support(): + """ + This test asserts that we can query for all Reporters, + even if some are of a proxy model type at runtime. + """ + class ReporterType(DjangoObjectType): + + class Meta: + model = Reporter + interfaces = (Node, ) + use_connection = True + + reporter_1 = Reporter.objects.create( + first_name='John', + last_name='Doe', + email='johndoe@example.com', + a_choice=1 + ) + + reporter_2 = CNNReporter.objects.create( + first_name='Some', + last_name='Guy', + email='someguy@cnn.com', + a_choice=1, + reporter_type=2, # set this guy to be CNN + ) + + class Query(graphene.ObjectType): + all_reporters = DjangoConnectionField(ReporterType) + + schema = graphene.Schema(query=Query) + query = ''' + query ProxyModelQuery { + allReporters { + edges { + node { + id + } + } + } + } + ''' + + expected = { + 'allReporters': { + 'edges': [{ + 'node': { + 'id': 'UmVwb3J0ZXJUeXBlOjE=', + }, + }, + { + 'node': { + 'id': 'UmVwb3J0ZXJUeXBlOjI=', + }, + } + ] + } + } + + result = schema.execute(query) + assert not result.errors + assert result.data == expected + + +def test_proxy_model_fails(): + """ + This test asserts that if you try to query for a proxy model, + that query will fail with: + GraphQLError('Expected value of type "CNNReporterType" but got: + CNNReporter.',) + + This is because a proxy model has the identical model definition + to its superclass, and defines its behavior at runtime, rather than + at the database level. Currently, filtering objects of the proxy models' + type isn't supported. It would require a field on the model that would + represent the type, and it doesn't seem like there is a clear way to + enforce this pattern across all projects + """ + class CNNReporterType(DjangoObjectType): + + class Meta: + model = CNNReporter + interfaces = (Node, ) + use_connection = True + + reporter_1 = Reporter.objects.create( + first_name='John', + last_name='Doe', + email='johndoe@example.com', + a_choice=1 + ) + + reporter_2 = CNNReporter.objects.create( + first_name='Some', + last_name='Guy', + email='someguy@cnn.com', + a_choice=1, + reporter_type=2, # set this guy to be CNN + ) + + class Query(graphene.ObjectType): + all_reporters = DjangoConnectionField(CNNReporterType) + + schema = graphene.Schema(query=Query) + query = ''' + query ProxyModelQuery { + allReporters { + edges { + node { + id + } + } + } + } + ''' + + expected = { + 'allReporters': { + 'edges': [{ + 'node': { + 'id': 'UmVwb3J0ZXJUeXBlOjE=', + }, + }, + { + 'node': { + 'id': 'UmVwb3J0ZXJUeXBlOjI=', + }, + } + ] + } + } + + result = schema.execute(query) + assert result.errors diff --git a/graphene_django/tests/test_schema.py b/graphene_django/tests/test_schema.py index 32db172..904c043 100644 --- a/graphene_django/tests/test_schema.py +++ b/graphene_django/tests/test_schema.py @@ -35,6 +35,7 @@ def test_should_map_fields_correctly(): 'email', 'pets', 'a_choice', + 'reporter_type' ] assert sorted(fields[-2:]) == [ diff --git a/graphene_django/tests/test_types.py b/graphene_django/tests/test_types.py index 83d9b40..6985026 100644 --- a/graphene_django/tests/test_types.py +++ b/graphene_django/tests/test_types.py @@ -58,7 +58,7 @@ def test_django_get_node(get): def test_django_objecttype_map_correct_fields(): fields = Reporter._meta.fields fields = list(fields.keys()) - assert fields[:-2] == ['id', 'first_name', 'last_name', 'email', 'pets', 'a_choice'] + assert fields[:-2] == ['id', 'first_name', 'last_name', 'email', 'pets', 'a_choice', 'reporter_type'] assert sorted(fields[-2:]) == ['articles', 'films'] @@ -124,6 +124,7 @@ type Reporter { email: String! pets: [Reporter] aChoice: ReporterAChoice! + reporterType: ReporterReporterType articles(before: String, after: String, first: Int, last: Int): ArticleConnection } @@ -132,6 +133,11 @@ enum ReporterAChoice { A_2 } +enum ReporterReporterType { + A_1 + A_2 +} + type RootQuery { node(id: ID!): Node } diff --git a/graphene_django/types.py b/graphene_django/types.py index 684863a..889620c 100644 --- a/graphene_django/types.py +++ b/graphene_django/types.py @@ -108,7 +108,8 @@ class DjangoObjectType(ObjectType): raise Exception(( 'Received incompatible instance "{}".' ).format(root)) - model = root._meta.model + + model = root._meta.model._meta.concrete_model return model == cls._meta.model @classmethod From bc1d47efb5af0a4def9fdc748872afbe70e846b3 Mon Sep 17 00:00:00 2001 From: Jonathan Mares Date: Sat, 3 Feb 2018 14:36:25 -0500 Subject: [PATCH 2/3] fix a line break --- graphene_django/tests/models.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/graphene_django/tests/models.py b/graphene_django/tests/models.py index 747e485..534f1f9 100644 --- a/graphene_django/tests/models.py +++ b/graphene_django/tests/models.py @@ -38,7 +38,8 @@ class Reporter(models.Model): 'Reporter Type', null=True, blank=True, - choices=[(1, u'Regular'), (2, u'CNN Reporter')] ) + choices=[(1, u'Regular'), (2, u'CNN Reporter')] + ) def __str__(self): # __unicode__ on Python 2 return "%s %s" % (self.first_name, self.last_name) From bfcfccfc8daa5e4b85b291d2b6a7fae935edd592 Mon Sep 17 00:00:00 2001 From: Jonathan Mares Date: Sat, 3 Feb 2018 22:51:25 -0500 Subject: [PATCH 3/3] undo cascade changes --- examples/starwars/models.py | 19 +++---------------- graphene_django/tests/models.py | 18 +++--------------- 2 files changed, 6 insertions(+), 31 deletions(-) diff --git a/examples/starwars/models.py b/examples/starwars/models.py index 3964acb..2f80e27 100644 --- a/examples/starwars/models.py +++ b/examples/starwars/models.py @@ -5,13 +5,7 @@ from django.db import models class Character(models.Model): name = models.CharField(max_length=50) - ship = models.ForeignKey( - 'Ship', - blank=True, - null=True, - on_delete=models.SET_NULL, - related_name='characters' - ) + ship = models.ForeignKey('Ship', blank=True, null=True, related_name='characters') def __str__(self): return self.name @@ -19,10 +13,7 @@ class Character(models.Model): class Faction(models.Model): name = models.CharField(max_length=50) - hero = models.ForeignKey( - Character, - on_delete=models.SET_NULL, - ) + hero = models.ForeignKey(Character) def __str__(self): return self.name @@ -30,11 +21,7 @@ class Faction(models.Model): class Ship(models.Model): name = models.CharField(max_length=50) - faction = models.ForeignKey( - Faction, - on_delete=models.SET_NULL, - related_name='ships' - ) + faction = models.ForeignKey(Faction, related_name='ships') def __str__(self): return self.name diff --git a/graphene_django/tests/models.py b/graphene_django/tests/models.py index 534f1f9..03d19d4 100644 --- a/graphene_django/tests/models.py +++ b/graphene_django/tests/models.py @@ -15,11 +15,7 @@ class Pet(models.Model): class FilmDetails(models.Model): location = models.CharField(max_length=30) - film = models.OneToOneField( - 'Film', - on_delete=models.CASCADE, - related_name='details' - ) + film = models.OneToOneField('Film', related_name='details') class Film(models.Model): @@ -68,16 +64,8 @@ class CNNReporter(Reporter): class Article(models.Model): headline = models.CharField(max_length=100) pub_date = models.DateField() - reporter = models.ForeignKey( - Reporter, - on_delete=models.SET_NULL, - related_name='articles' - ) - editor = models.ForeignKey( - Reporter, - on_delete=models.SET_NULL, - related_name='edited_articles_+' - ) + reporter = models.ForeignKey(Reporter, related_name='articles') + editor = models.ForeignKey(Reporter, related_name='edited_articles_+') lang = models.CharField(max_length=2, help_text='Language', choices=[ ('es', 'Spanish'), ('en', 'English')