Issues with new 2.3 release

[justinask7]

As I've installed new 2.3 release I've faces several issues, which firstly I want to bring up for a discussion and upon an approval - I could create a PR for fixing. Issues are as following:

1. Schema is unable to generate related fields in case they are not defined, I mean in case we have ForeignKey without related_name definition:

class Father(models.Model):
   name = CharField(max_length=256, db_index=True)

class Son(models.Model):
   name = CharField(max_length=256, db_index=True)
   father = ForeignKey(Father, on_delete=CASCADE)

As far as I've debugged - in case FK field does not have related_name attribute it's not added as son_set field either (I think it comes up from this line https://github.com/0soft/graphene-django-plus/blob/master/graphene_django_plus/mutations.py#L97). I believe this can bring a very big confuse as Django does not force you to put related_name attribute
2. Basically now by default related son objects can be assigned to Father (following the previous example) while you wouldn't even think about that since it's not directly in the model when creating a mutation, right?I believe this might bring some logic-related security issues for some projects. What I'd suggest is that this possibility is really great, but I'd expect that to be added only when I explicitly add related_name in fields of mutation:

class UpdateFather(ModelUpdateMutation):
   class Meta:
      model = Father
      only_fields = ["name", "sons"]  # While it wouldn't be possible to update sons relation in case it's not in `only_fields` (`only_fields` are not defined at all)

What are your ideas about that?

[gghildyal]

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?

[justinask7]

@gghildyal

1. 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 all related_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
2. 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?

[bellini666]

@justinask7

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.

[gghildyal]

@justinask7 @bellini666

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.

[bellini666]

@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

[justinask7]

@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

[gghildyal]

@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?

[bellini666]

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.

[justinask7]

@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

[justinask7]

Thank you guys for cooperating on this issue!