Update django form mutation to more granularly handle input/output fields

[jtratner]

Example of how I'd like to change DjangoFormMutation as per #933. I think this should be backwards compatible with the existing one. Check out docstring on DjangoFormMutation for an overview of what the changes (ought to!) do :)

I also tried to add some docs about class behavior.

This still needs a test case and probably to be merged into the public graphene django docs but I figured this'd be good basis for discussion :)

Ultimately this is motivated by me having desire to be able to have a graphql mutation that just returns clientMutationId and a single object even tho I'm not using a DjangoModelFormMutation.

[jkimbo]

Thanks @jtratner ! I get the use case for this but I'm just wondering if this is the best API for it because it seems quite verbose. Could we simply it by just having 2 options: input_fields and output_fields where each is a list of fields or the string __all__ to select all fields? Is there much benefit to being able to specify only and exclude?

Related: for DjangoObjectType's we are planning on removing the only_fields and exclude_fields attributes in favour of only and exclude attributes: https://github.com/graphql-python/graphene-django/pull/691 We should probably maintain that naming convention elsewhere in the library as well.

[jtratner]

@jkimbo - totally agree that it’s wordy 😂😅 I was trying to follow the existing convention.

Honestly if I were designing this class from the start I prob would have users explicitly specify output fields for the mutation. I’m not clear on a use case where you’d want to post the fields from cleaned_data back. Then you’d only need only and include to manage the input fields.

If you wanted to do that string, I’d suggest defining a constant

import graphene_django

...
Meta:
    input = graphene_django.ALL_FIELDS
    output = ()

[jkimbo]

I think that api makes more sense @jtratner so let's move towards that implementation. We can first start by preferring input_fields (or maybe just input) and output_fields options and emitting deprecation warnings if only_fields or exclude_fields are defined. Then in a later release we can drop only_fields and exclude_fields completely and also raise exceptions if input_fields or output_fields are not defined. I would follow a similar logic to https://github.com/graphql-python/graphene-django/pull/691

Don't have a strong opinion on defining a constant verses just writing out __all__. They can be both be valid. (The __all__ came from DjangoRestFramework)

[jtratner]

What about getting rid of the automatic output fields in a backwards compatible way:

1. only_fields and exclude_fields keep the existing behavior (and warns about deprecation)
2. No fields specified keeps existing behavior (deprecation warning about required to specify)
3. Specifying fields or exclude will be required in future version (they only modify input fields). If specified in current version, it means that only output field is clientMutationId. exclude=graphene_django.ALL_FIELDS means no input fields at all.

This would make the API much simpler

[jkimbo]

Specifying fields or exclude will be required in future version (they only modify input fields). If specified in current version, it means that only output field is clientMutationId. exclude=graphene_django.ALL_FIELDS means no input fields at all.

DjangoFormMutations has an errors field defined on the output type which should stay there. Also I think we should just drop the clientMutationId field completely because it's no longer necessary for relay or any other frontend client.

I think I like the idea of defaulting to no output fields which means you have to be very explicit about what outputs you want. Unfortunately I don't have many usecases for DjangoFormMutation so I'm not that familiar with what people use it form. What kind of things are you using it form @jtratner ?

[jtratner]

Our app (for good or ill) has a number of forms that are pretty-close-but-not-quite ModelForms. (So they essentially construct objects, just the input is different than underlying DB representation).

This class is handy cuz it lets us reuse validation logic from our existing HTML front end (old school without JS)

That said, looking at Django Rest Framework, I wonder if write_only and read_only would be better names (with anything not specified assumed to be for both). This would match the serializer definitions...

(Apologies for the array of ideas - just hoping to get to a meaningful API)

[jtratner]

(See #933 for a simplifies version of our use case)

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[jtratner]

@jkimbo - circling back to this after a while :) Has your API break completed now? (I think that was a blocker in the past). If so, is there a set way of naming that you'd like me to use? If not, I can try to get back in the zone for this PR and finish it up.