tOgg1 / graphene-django-cud

Easy and painless CUD-mutations for graphene-django.
MIT License
81 stars 37 forks source link

Respect Django's own validation mechanism #118

Closed hansegucker closed 8 months ago

hansegucker commented 1 year ago

It seems that Django's own validation mechanism is currently ignored. https://docs.djangoproject.com/en/4.2/ref/models/instances/#validating-objects

Is there any option to do similar stuff like in the clean method? I saw that there is validate, but validate is executed before the object is known.

hansegucker commented 9 months ago

Does anyone have an idea?

JFer11 commented 9 months ago

I'm encountering a similar issue, particularly with fields like EmailField where Django's default validators are not being applied automatically because full_clean() is never invoked. This oversight leads to models being saved without proper input validation.

To address this, I've considered a couple of solutions:

  1. Using DRF Model Serializers: For each model, create a corresponding Django Rest Framework (DRF) model serializer and override the validation pipeline from django-graphene-cud. By unpacking all input values and passing them to the serializer, we can leverage the is_valid() method. This method automatically performs the four essential validation steps as outlined in the Django documentation:

    • Validate model fields (Model.clean_fields())
    • Validate the model as a whole (Model.clean())
    • Validate field uniqueness (Model.validate_unique())
    • Validate constraints (Model.validate_constraints())

    While this approach integrates Django's comprehensive validation mechanisms, it introduces additional boilerplate code for each model, which can be seen as a significant drawback.

  2. Overriding the Save Method: Another approach I considered (but ultimately decided against) involves overriding the model's save method to directly call full_clean(). However, there are several discussions on Stack Overflow advising against this due to potential issues and unintended side effects. Here are some relevant discussions that highlight why this might not be the best approach:

  3. Using Graphene-Django Form Mutations: Similar to the DRF serializers method where we override the validate method, Graphene-Django's DjangoFormMutation and DjangoModelFormMutation allow for Django's form validations to be integrated into GraphQL mutations. This approach utilizes the form's is_valid() for immediate and comprehensive validation, mirroring the DRF's handling of validation but within the GraphQL context. It simplifies validation by directly employing Django's built-in mechanisms, offering a streamlined alternative to the DRF solution with less need for custom validation code.

Considering the proposed solutions, the use of DRF Model Serializers offers a thorough integration of Django's validation mechanisms, ensuring that models adhere to Django's validation standards, albeit with the drawback of additional boilerplate. Alternatively, Graphene-Django Form Mutations provide a streamlined and efficient way to apply Django's form validations within GraphQL mutations, striking a balance between ease of implementation and comprehensive validation coverage as well.

Navigating this issue within the graphene-django-cud framework suggests that while there are viable paths to enhance validation, an entirely different approach could involve adopting graphene-django directly. However, understanding the desire to minimize project disruptions, these solutions offer practical starting points to bolster validation mechanisms keeping graphene-django-cud.

I welcome any further insights or alternative suggestions that could enhance our approach to handling validation within this specific context.

tOgg1 commented 9 months ago

Sorry for the delayed response here @hansegucker.

And thanks for the write-up @JFer11.

The validation mechanism of GDC is primarily concerned with validating the input to the mutation. There is currently no way to enable GDC to automatically also run the cleaning mechanisms of Django as well.

However, you can easily get the functionality you want by overriding one of two relevant methods: validate and before_save.

For create mutations, the signature of validate does not take the newly created object—the rationale being that no object should be created if input validation fails:

def validate(cls, root, info, input):

The easiest way to run full_clean on the object is to override the before_save method:

    @classmethod
    def before_save(cls, root, info, input, obj):
        obj.full_clean()

If full_clean fails with an exception, this will bubble up to the primary mutate function of the Mutation. The object creation is wrapped in a transaction. Thus, the exception will cause the object to be destroyed.

For update mutations, you can also use before_save. Overriding the validation pipeline directly is also an option here since it takes the resolved object as an argument:

    @classmethod
    def validate(cls, root, info, input, id, obj):
        obj.full_clean()
        return super().validate(root, info, input, id, obj)

I would, in this case, remember to call super's validate function as well, to make sure the regular validation pipeline of GDC runs in addition.

For the batch update and batch create mutations you should probably use the hooks after_update_obj and after_create_obj, respectively.

I will create a task to integrate some of this functionality more directly into the library.

I hope that makes things a bit clearer. Let me know if you have any additional questions.

JFer11 commented 8 months ago

@tOgg1, thank you for your guidance! I've implemented the before_save method in DjangoCreateMutation and DjangoPatchMutation with success. To ensure it works as intended, I conducted tests specifically on EmailField using various invalid email inputs. The results were positive, with all incorrect inputs being accurately caught by the validators.

@classmethod
def before_save(cls, root, info, input, obj):
    obj.full_clean()

This approach effectively ensures data integrity and aligns with Django's validation standards. Looking forward to seeing how this functionality might be integrated more seamlessly into graphene-django-cud.

Appreciate your support and openness to community contributions!

hansegucker commented 8 months ago

Thank you both for the input!

For the batch update and batch create mutations you should probably use the hooks after_update_obj and after_create_obj, respectively.

This is the point where I am running into problems because these methods are executed after the database operation has been done. So, any problems with uniqueness or database constraints will result in exceptions before. Isn't there something like before_create_obj?

tOgg1 commented 8 months ago

@hansegucker

You can use the validate method for the batch mutations. It is called for each entry in the input. The signature is:

@classmethod
def validate(cls, root, info, input, full_input):
    return super().validate(root, info, input, full_input)

It is called here in the source code.

From your initial message, I thought you wanted to clean the model after object creation.

If what you want to do instead is to literally clean and change the input data, the best way to do that currently in batch mutations is to override before_mutate:

    @classmethod
    def before_mutate(cls, root, info, input):
        # Alter the "input" and return it back. 
        # ...
        return input

If neither of these works, your current only option is to override the mutate method. I would start by copying the mutate method in the relevant base class, here for batch create, and alter it as needed.

Having an object-level method that allows you to alter the input data for that specific object is something I can put on the wishlist of new features.

tOgg1 commented 8 months ago

Gonna close this issue. Let me know if you have additional feedback, comments or issues.