New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Django integration proposal #48
Comments
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
|
Right now graphene is already transversing Django relations (only if the target model is in some I was thinking about adding support to django-filter for filtering the querysets (so you can define which attributes you can filter on). 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! |
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? |
@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. |
I've just knocked out a proof of concept for a |
Great work @adamcharnock! Once the Thoughts? |
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 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 |
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 |
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 |
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:
Points of concern:
|
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 Issues to be addressed (suggestions welcome):
|
@adamcharnock, after taking a deeper look at the gist here are some thoughts:
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
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! |
@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. |
Discussion can be found here: graphql-python#48 Original gist can be found here: https://gist.github.com/adamcharnock/ad051b419d4c613d40fe
Discussion can be found here: graphql-python#48 Original gist can be found here: https://gist.github.com/adamcharnock/ad051b419d4c613d40fe
I've created PR #60 as this seems like a sensible time to stop working with gists. @syrusakbary My reasoning for Your other two points sound like good suggestions. I'll investigate them further when get back to coding. |
Discussion can be found here: graphql-python#48 Original gist can be found here: https://gist.github.com/adamcharnock/ad051b419d4c613d40fe
Closing this issue as the main PR #60 is merged into master. |
…e-primary-keys Fix: objects with composite primary keys should have correct id
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[already supported]resolve_FIELD()
may be specified to customise this traversal, but if not then traversal will be done using the relation's manager'sall()
method.resolve_FIELD()
boilerplate code).Query:
ConnectionFields
which connect toDjangoNodes
without the need for aresolve_FIELD()
method.Query
class which inherits from multiple app-specificQuery
classes) [related: Queries do not support multiple inheritance #55]Mutations:
DjangoMutation
which provides standard functionality though which to update Django models.CreateMutation
,UpdateMutation
, andDeleteMutation
base classes.All/any thoughts very welcome indeed!
The text was updated successfully, but these errors were encountered: