Skip to content

Possible Bug: Output fields from DjangoFormMutation #470

Closed
@oliviarves

Description

@oliviarves

In the initialization of the Meta class of a DjangoFormMutation, the output fields are declared similar to the input fields of the mutation, like:

input_fields = fields_for_form(form, only_fields, exclude_fields)
output_fields = fields_for_form(form, only_fields, exclude_fields)

For example, if we have a form for authentication, like the one provided by django:

class AuthenticationForm(forms.Form):
    """
    Base class for authenticating users. Extend this to get a form that accepts
    username/password logins.
    """
    username = UsernameField(
        max_length=254,
        widget=forms.TextInput(attrs={'autofocus': True}),
    )
    password = forms.CharField(
        label=_("Password"),
        strip=False,
        widget=forms.PasswordInput,
    )
    ...

And we link it to a mutation:

class AuthMutation(DjangoFormMutation):
    """
    Mutation to login a user
    """
    class Meta:
        form_class = AuthenticationForm
    ...

generates a mutation that requires a username and a password on the response.

AuthMutationPayload{
    username: String!
    password: String!
    clientMutationId: String
}

Is this right? Is sending back the password to the user secure? I think the output fields should be initialized as an OrderedDict().

Activity

maarcingebala

maarcingebala commented on Jul 23, 2018

@maarcingebala

I think by default all fields are serialized in the output. If I'm correct you should be able to use exclude_fields in mutation's Meta to exclude the unwanted fields.

oliviarves

oliviarves commented on Jul 23, 2018

@oliviarves
Author

@maarcingebala Thank you for the answer. When I use exclude_fields I am able to exclude the unwanted fields, but my question is: is it correct to force the output fields to include the input fields?

maarcingebala

maarcingebala commented on Jul 23, 2018

@maarcingebala

In your case input and output should probably be different. Input fields should include only username and password as you have it now. In the output of this mutation I would expect data of the authenticated user (in case of succesful authentication) or an error otherwise. Returning the password back sounds rather risky and probably there is no need in the application to do that.

maarcingebala

maarcingebala commented on Jul 23, 2018

@maarcingebala

I havent used those FormMutations that are currently in Graphene but I assume that the problem is that the output by default mirrors the input?

oliviarves

oliviarves commented on Jul 23, 2018

@oliviarves
Author

Exactly, the output fields mirrors the input, which I think it could be fine, but when it comes to cases like this authentication example, it makes me think if is necessary to declare the excluded fields all the time. I just feel like it is not right. That is why I proposing to initialize the output fields as an OrderedDict, but I realize that sometimes it could be useful to automatically include the input fields to the output ones. So, I suposse it is convenient someway.
Anyway, it is just a thought, and I wanted to share it, thinking that maybe it was a typing error or something like that. Please, excuse me if my English is not good.

oliviarves

oliviarves commented on Jul 23, 2018

@oliviarves
Author

Actually, exclude_fields does remove the fields, but in both input and output fields.

maarcingebala

maarcingebala commented on Jul 24, 2018

@maarcingebala

I think in case of authentication you'll be better off implementing the mutation yourself. You could use the AuthenticationForm in the mutate method, populate it with data from the input, do the authentication logic and manually return the output.

To return the same fields in the output as in the input is a fair idea, but in my opinion, it may work well only for simple, CRUD-like mutations such as creating or updating models. To make it more versatile, those form-based mutations should provide easy ways to override both the input and the output.

maarcingebala

maarcingebala commented on Jul 24, 2018

@maarcingebala

We tried those form-based mutations in our project before it was merged to master in graphene-django, but it turned out that some parts of our logic had to placed inside the form classes and some other parts in mutate functions. Also, when we had to include or exclude particular fields, we had to do it either at the form level or the mutation Meta-class level. Everything started to become a bit messy and we eventually gave up this approach and came up with our solution - model based mutations. We use it for CRUD-like mutations based on models and for all other cases such as authentication, upload etc we have simple BaseMutations that unify the way we return user errors. Although we reimplemented some logic of model forms, we find this approach more convenient so far.

jarcoal

jarcoal commented on Sep 19, 2018

@jarcoal

It would be nice to be able to specify output fields as there are many cases where values going out will not match values going in. Even just exposing separate methods like get_input_fields() and get_output_fields() would make it easy to do this.

stale

stale commented on Jun 11, 2019

@stale

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

jtratner

jtratner commented on Apr 15, 2020

@jtratner

see #933 and #934 where I posted up a fix to split out input and output field management :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @jarcoal@jtratner@maarcingebala@oliviarves

        Issue actions

          Possible Bug: Output fields from DjangoFormMutation · Issue #470 · graphql-python/graphene-django