Discussion: Addressing reasonable `model.full_clean()` behavior.

[brendan-morin]

Original ninja issue: vitalik/django-ninja#443

Overview

Basically there are two parts to this issue:

1. model.full_clean() is not called by default (known Django decision for backwards compatibility)
2. model_full_clean() is not async compatible, so it would be possible to call this in an unsafe manner when left to users to decide how or when to call.

Background

In a nutshell, model.full_clean() performs model-level validation checks. Django has historically disabled this by default, but recommends manually calling it on save(), which has been a fairly contentious decision and infamous footgun, as not doing so can lead to data corruption.

Discussion

First off, I think that the Django decision to not call model.full_clean() by default is somewhat defensible for Django core, where legacy apps may have existed for many years. However, ninja and shinobi are new frameworks (and generally new use cases) on top of Django and it would be perfectly reasonable to call model.full_clean() by default and instead allow people to opt out.

The reason I recommend enabling by default is Django and ninja's "just works no surprises" approach, which is widely loved. However, this is at odds with being able to specify validations at the model level that you reasonably could assume would be enforced but default to not enforcing them. IMO, it is more surprising that you would define validation at the model level and then choose to ignore it, especially given we already have the ability to use Schema-level validation for more bespoke validation use cases that would not be applied to all interactions with a particular model.

Possible Solutions

While enabling by default addresses reasonable expectations, given the async focus on Django Ninja, it is possible this could lead to issues with the model.full_clean() behavior not being async compatible. One idea, from the original ninja issue:

One option Is there possibly some way forward using a flag as described above + one the async adapter functions in conjunction with an asyncio.get_running_loop() test to see if the code is running async? E.g. do nothing in sync context, and in async call the adapter to ensure it's a safe/sync operation?

Would love to hear more thoughts on this - thank you!

[pmdevita]

Been thinking about this for a little bit, here are my thoughts:

At the moment, Ninja doesn't call .save() either, its direct involvement with incoming data ends with it presenting your Schema/ModelSchema object. This makes calling .full_clean() tricky, it would be hard to call it by itself, without an actual model instance involved.

But I think this part of Ninja/Shinobi could be improved. The current user story of setattr(key, value) for key, value in dict, while a Django classic, feels like more of a practical necessity than a polished solution to me. We already have an easy way to reflect data out of a model and into a schema, so being able to go the other way easily too would be nice. And if we have an excuse to call .save(), it would also make sense to call .full_clean() while we're at it.

I think for any fields that are automatically defined by model_fields, those should (unless someone has a specific use case I haven't seen yet, please let me know!) be pretty straightforward to copy back. For model fields that are manually defined, I think they fall into two categories.

1. I needed to define this field manually because Shinobi doesn't construct it correctly on its own. This is probably an opportunity to improve ModelSchema, maybe if it picked up some other metadata it would be constructed correctly.
2. It's some other data that I didn't want going into the model.

So I think I'd want two separate functions for this, create and patch essentially. I don't think it's possible to create a solution for all use cases but hopefully we could capture the most common.

For create, we could maybe have a function that fully produces a model. You could optionally pass in an exclude list of model_fields you don't want to copy over, and an include list of manually defined model fields you do want to copy over. This might not work in some use cases where a user would want more control over how the model is constructed though, maybe there's some other way to do this?

Patch would work similarly except it would take a model instance you supply. Finally for both of these, after copying the data in, we'd be in a good place to call full_clean().

As for handling async, we can supply an a variant of the functions for calling under async, and they can do the sync_to_async underneath.

As an alternative or potentially supplemental idea, some of the Django field validators, like length or range, could be passed in directly to a Pydantic field. This would then both validate in Pydantic and enhance the OpenAPI spec. Many of the other validators also have Pydantic equivalents, and custom ones could just be wrapped as is.

If we do both things (mimic validators, call full_clean()), maybe we do a modified full_clean() call where if a schema was able to represent all of its field's validators in Pydantic, it could skip clean_fields() and then call clean() and validate_unique() after, although it might depend on whether the slight gain in speed is worth testing the correctness of the ModelSchema implementation.