Comments (10)
For your point #1 - It was a recently change that I authored as a part of Issue#11 which was fixed in [PR#12] #12. Is there a good way to find the related name of the reverse relationship without the related_name attribute? If yes, they this probably should be fixed.
For point #2, yes that the purpose of the fix in Issue#11. I don't see why now and what security issues it could bring about - Django admin allows that workflow as well, but I agree that there should be a way of removing such reverse fields from the mutation and I thought the 'exclude' option would just work? Can you give that a go?
from graphene-django-plus.
- The thing is that when no related_name field is defined - I'd say
son_set
name can be formed or it wouldn't be include at all. In other case this patch forces then to define allrelated_name
flags for all FKs. Even if it's the way how you want it implemented - it must be noted at least in README.md, but I wouldn't suggest as it's not forced by the Django itself - The problem I see is that mutations are dedicated for user interaction with system mostly and by default allowing to set some not-direct relations can lead to unexpected results which are not intuitive (for example in our application we will have to go through all our mutations and add related_names to exclude fields). Django admin allows related models creation only by adding tabular admins, am I right? So by only defining model in admin you only see fields from the model as well. For example DRF serializers also by default allows only model-field updates:
class FatherSerializer(serializers.ModelSerializer):
class Meta:
model = Father
fields = ['__all__'] # won't allow sons update neither will return on GET
while
class FatherSerializer(serializers.ModelSerializer):
class Meta:
model = Father
fields = ['id', 'name', 'sons'] # will return list of sons IDs on GET and will allow update
Just to be clear - the functionality you added is really very useful just my point is that maybe it shouldn't be included by default but by adding it to only_fields
definition.
@bellini666 what are your ideas?
from graphene-django-plus.
I understand your points and agree with you, specially when comparing this lib to DRF and django-admin alone.
But on the other hand, when we consider DjangoObjectType
, it will automatically add those relations to the type unless you specify them in the exclude
list or only list what you want in the fields
. Because of that I stopped relying on automatic field generation and I'll always add a fields
list to types and mutations so I can have total control of what is being expose, so I know what you mean when you say that "in your application we will have to go through all our mutations and add related_names to exclude fields".
So, there's the dilemma now. I would say that I'm more inclined to force the user to have to declare the field, but that makes the mutations behave different from graphene's queries.
from graphene-django-plus.
The purpose of the patch wasn't to force the user to define related_name for FK fields, it's just that the current patch has that shortcoming which I think can and should be fixed.
On point 2, I agree that maybe there should be a way of turning the auto-generation of indirect fields off. But it should be only_fields
- imagine a model with 20 fields, you loose the benefit of the library auto-generating input fields for you.
I would suggest another meta option which enables auto-generation of these reverse fields - say it's called indirect_fields
. It's disabled by default, so input fields are only generated for directly fields, but when enabled would add the indirect reverse fields to input and process them for mutations as well.
from graphene-django-plus.
@gghildyal I like the idea of having an option for that. I would say that maybe we should create a global option in django settings for that so you would not have to worry about defining your preference, if it is different from the default, in every type you create.
What do you think of (as the default):
GRAPHENE_DJANGO_PLUS = {
'MUTATIONS_INCLUDE_REVERSE_RELATIONS': True,
}
That way if anyone doesn't want the feature he can simply set that to False
.
cc @justinask7
from graphene-django-plus.
@bellini666 @gghildyal
For point 1: that makes sense, @gghildyal will you be able to update that functionality? I could try as well
For point 2: @bellini666 with global option we kind of state that "you will either include reverse relations in all your project nodes or not at all", right? I'd maybe agree with @gghildyal that indirect_fields
option might work as I'd imagine it as an array, for example:
class UpdateFather(ModelUpdateMutation):
class Meta:
model = Father
related_fields = ["sons"] # So we're still allowing to not define `only_fields`, but optionally add `related_fields` as well
In case that works for us all - I could try to create a PR with such updated logic implementation
from graphene-django-plus.
@bellini666 - For me it looks like having support for 'reverse' fields or not does seem like a global option, so I support your recommendation (Although @justinask7 doesn't). @justinask7 The one issue with my approach is that if you have a complex app with loads of models and relationship, doing a per-model configuration can be painful.
@justinask7 wrt Option 1 - Can you please give it a go?
from graphene-django-plus.
For point 2: @bellini666 with global option we kind of state that "you will either include reverse relations in all your project nodes or not at all", right? I'd maybe agree with @gghildyal that indirect_fields option might work as I'd imagine it as an array, for example:
@justinask7 the global option would be for you to change the behaviour of the mutations automatically generating input fields for your reverse relations. If you change this option they would not be automatically generated for you anymore, but you would still be able to add them manually when you choose to do so.
And I don't like the idea of having a related_fields
option, it just complicates an already complicated logic even more.
from graphene-django-plus.
@bellini666 that totally makes sense, cause previously I thought that you were suggesting whether to allow include of reverse fields or not.
So basically now even with turned off global option you will still be able to include reverse relation fields into only_fields
list, right?
Will try to make a PR for this change asap
@gghildyal will include that fix in this PR as well
from graphene-django-plus.
Thank you guys for cooperating on this issue!
from graphene-django-plus.
Related Issues (17)
- Make relay interface optional
- ModelCreateMutation generates required fields for CharFields without null=True. HOT 4
- BaseModelMutation doesn't generate input fields for reverse foreign key relationships. HOT 1
- Any plans on merging this functionality with graphene-django? HOT 2
- Check object_permissions argument before check permission inside get_node on ModelType class HOT 2
- #support #UploadType HOT 4
- Can you add where as input in the query too with this library as in graphene-django-crud library? HOT 1
- When and where to set some field value before mutation? HOT 6
- Why UUID is a Telephone Type? HOT 1
- 'FloatField' object has no attribute 'max_digits' HOT 2
- Use without relay? HOT 2
- ObjectPermissionChecker is undefined when launching without django-guardian installed
- This lib is DEPRECATED (read this)
- Project status - deprecated? HOT 2
- Version 2.1 change breaks current object permissions HOT 2
- Order of imports affects the operation of the library HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from graphene-django-plus.