Django integration proposal about graphene HOT 16 CLOSED

One cool thing would be to inspect field asts and generate the correct prefetch_related, select_related and only calls onto the queryset. I think syrus is working on something like this.

Sent from my iPhone

On Nov 23, 2015, at 11:12 AM, Adam Charnock [email protected] wrote:

I've been thinking a bit about Django integration for Graphene. This is in part because I think it could be really useful, and in part because this is something I'd be interested in contributing a pull request for. However, development seems to be moving quickly so I thought it best to discuss these ideas first in case there are already plans.

The main motivation here is reducing boilerplate code. My intention would be to make all automatic functionality overridable & customisable.

Nodes:

Automatically traverse Django relations. A resolve_FIELD() may be specified to customise this traversal, but if not then traversal will be done using the relation's manager's all() method.
Automatically support query args for all/specified model fields (reduces resolve_FIELD() boilerplate code).
Query:

Automatically resolve ConnectionFields which connect to DjangoNodes without the need for a resolve_FIELD() method.
Support for pulling in schema definitions from individual Django apps, rather than being defined centrally. (I currently do this by using a top-level Query class which inherits from multiple app-specific Query classes)
Mutations:

Provide a DjangoMutation which provides standard functionality though which to update Django models.
Consider a class-based views approach, with CreateMutation, UpdateMutation, and DeleteMutation base classes.
All/any thoughts very welcome indeed!

—
Reply to this email directly or view it on GitHub.

from graphene.

I agree @jhgg, I think that is #28.

As far as my ability to contribute is concerned, I would probably look to implement the basic ideas/functionality which could then be optimised.

from graphene.

Right now graphene is already transversing Django relations (only if the target model is in some ObjectType in the schema), and resolving to ConnectionFields whenever the target model is defined in some DjangoNode in the schema, without the need for a resolve_FIELD() method.

I was thinking about adding support to django-filter for filtering the querysets (so you can define which attributes you can filter on).
I agree that could be very useful having the possibility to autodiscover the schema by searching into the django INSTALLED_APPS (something similar to django.admin is doing).

About optimizing the hits to the dbs only fetching the data needed... I'm working on that! :)

The mutations will probably require a little bit more of work!

from graphene.

Probably adding custom permissions for querying/updating using django-guardian is also a good enhancement (and could be required when using automutation models?). Thoughts about that?

from graphene.

@syrusakbary I'll checkout the existing Django traversing, I think I mustn't have realised that.

Supporting third party packages such as django-filter and django-guardian sounds like a good idea, and I think you're right that some permission checking would be needed for most projects. I suggest we avoid requiring their use though, as people may already have other systems in place (especially for permissions). Which makes me wonder if a pluggable system of some form could make sense.

I haven't looked into how I'd actually implement the mutations, but I'd be open to suggestions.

from graphene.

I've just knocked out a proof of concept for a DjangoQuery class. I just took my existing Query class and wrote a metaclass to autogenerate the necessary attributes. I'd be willing to accept that this is somewhat crude, but I felt it was worth a shot. It also has the advantage of allowing one to extend/override the attributes/methods in the child class.

https://gist.github.com/adamcharnock/78dbc5d8e448b4e5b1d9

from graphene.

Great work @adamcharnock!
About creating a BaseQuery class for DjangoNodes I think is something very opinionated for the framework. Why? I think imposing the ConnectionField fields names as all_X should be done explicitly by the developer.

Once the django-filter integration is done, probably will be not strictly required to create resolve_FIELDNAME methods, as will be resolved by default with the DjangoFilter resolver.

Thoughts?

from graphene.

Thanks @syrusakbary, and I totally agree. That was something I meant to comment but clearly forgot. I couldn't see an obvious way of allowing the developer control of those names, so I just hard coded it for the sake of a simple proof-of-concept.

Good point on the resolve_FIELDNAME methods. I'll take a look at creating a resolver based on django-filter.

Given these comments, are you happy with the general direction of the above gist? If so I'll continue to develop it. If not, I'll hold off until we figure out a better idea.

EDIT: Added comments to gist

from graphene.

I think creating directly this fields by the developer will be better and less opinionated for the framework. (Maybe having this as some external package?)

I imagine doing something like this:

class Query(graphene.ObjectType):
    all_droids = DjangoFilterConnectionField(Droid)

(Note that might not be necessary to create the resolve_all_droids method)

from graphene.

Just checking in to say I haven't forgotten about this. My initial attempt – while a learning experience for me – is clearly not the way to go. I've spent some time getting on with my own project and in doing so I have definitely come around to the method you suggest above.

I'll knock up another gist in a bit.

EDIT: @syrusakbary Thank you very much for the suggestions and taking the time

from graphene.

Ok, here is attempt number two:

https://gist.github.com/adamcharnock/ad051b419d4c613d40fe

I have it loosely working at the moment, but no doubt they'll be issues. The main points are:

• The addition of a DjangoFilterConnectionField field
• ...which uses a FilterConnectionResolver to resolve the field to a queryset as provided by django-filter
• The contents of FilterConnectionResolver were largely taken from the django-filter FilterView class

Points of concern:

1. The dubious conversion of django-filter filters to Graphene types in DjangoFilterConnectionField.get_filtering_args()
2. The use of the on parameter to specify the model field upon which the filtering should be performed. Just a general feeling that this could be done better.

from graphene.

Ok, I've just rewritten the conversion to Graphene types and updated the gist. The conversion is now down based upon the the form fields graphene uses to parse & validate the incoming values. See converter.py in the gist. Maybe there is a smarter way of doing this, but doing lookups seems to be the way both graphene and django-filter already do this so I did the same.

Issues to be addressed (suggestions welcome):

• The ordering of fields in the root Query is important. If a filter can filter upon a relation, then Graphene must already be aware of that model's existence, and that model's entry in the root Query must therefore come first.
• I think I need to implement an extra filter (GlobalIDFilter?) in order to filter against global IDs. This would need to reference a GlobalIDField form field, which'll also need implementing.

from graphene.

@adamcharnock, after taking a deeper look at the gist here are some thoughts:

• Why convert_form_field_to_djangomodel returns an ID? (If I'm querying for a model inside a model, aka user.group.name, I want to get the whole instance for the group, not just the id)
• What you think about having SimpleQuerySetConnectionResolver,FilterConnectionResolver as function decorators? So you can also do:

class Query(ObjectType):
    # ...
    @filter_connection
    def resolve_all_accounts(self, args, info):
        return Account.objects.filter(user=info.context.request_context.user) # Only for my user

• We could also create a Filter class dynamically (aka django_filters.FilterSet) when using DjangoFilterConnectionField, similar as Django Rest Framework is doing, something like:

class Query(ObjectType):
    account = relay.NodeField(accounts_nodes.Account)
    all_accounts = DjangoFilterConnectionField(accounts_nodes.Account, fields=['name', 'slug', 'created'])

Overall I really like your solution!

from graphene.

@syrusakbary Great, those sounds like some very good points. It's 3:30am here so I should really get some sleep, but I'll probably take a look tomorrow.

from graphene.

I've created PR #60 as this seems like a sensible time to stop working with gists.

@syrusakbary My reasoning for convert_form_field_to_djangomodel() returning an ID type is that, if one is filtering for a model, one would do so by the model's ID. However, this decision was not arrived at after an abundance of reflection. Do you have thoughts on this?

Your other two points sound like good suggestions. I'll investigate them further when get back to coding.

from graphene.

Closing this issue as the main PR #60 is merged into master.

from graphene.