Django-rest-framework: DRF wrongly moves validation business logic from models to views

[grjones]

I realize this was an intentional change in 3.0 where ModelSerializers no longer run model-level clean or full_clean, however I am submitting this as a bug because to me and others it is a violation of business logic residing in views instead of models. It's ok of course to close this out immediately, but I hope this behavior will be reconsidered one day. Here are some questions I would like us to think about:

• If validations happen in the serializers (at the view-level), how do I ensure the same validations occur at the model level if for example I am running one off scripts that have nothing to do with views or http? Do I have to implement that logic twice?
• Is there an expectation by developers that if you have implemented custom validations at the model level that these will be run by serializers, especially since ModelForm does?
• What happens in the case where in your models you are forcing a full_clean (perhaps by including it in the save method)? Will serializers know how to handle an exception from an explicit full_clean?

[tomchristie]

:)

I'll come back with a fuller answer to these points shortly, but in the meantime here's a couple of links relevant to my thoughts on this area.

• http://www.dabapps.com/blog/django-models-and-encapsulation/
• https://m.youtube.com/watch?v=3cSsbe-tA0E

[tomchristie]

So, point by point:

• Serializers are not tied to HTTP requests - you can use (and reuse) serializer validation for any input.
• It doesn't do that, no. We do document that the behavior is different, and the design reasons behind this http://www.django-rest-framework.org/api-guide/validators/#validation-in-rest-framework - In my view there's design issues with ModelForm that REST framework's serializers make an improvement over. We're moving towards providing a full form API as well so that you can reuse serializer in both API and HTML form use-cases.
• I'd recommend that application level validation should generally happen prior to save, not during it. Personally I'd avoid full_clean, and instead ensure that state changing operations on model instances are only ever made via method calls that can provide a boundary that ensures that only valid state changes may ever be made by the rest of the application.

Of course alternative serializer implementations are perfectly possible, and I'd welcome third party packages in this area.

The input is appreciated, but we had good reasons for making this switch in 3.x. While there are clearly trade-offs, the benefits of not having to handle partial-instantiated-objects-plus-some-relationships-that-haven't-yet-been-saved cases, and the direct transition between Serializerand ModelSerializer classes has been more than worth the break from the approach that ModelForm takes.

[rhfung]

@grjones I agree with Tom Christie that full_clean shouldn't be automatically called on a ModelSerializer, but a pre-save hook could be provided for calling full_clean in the default create and update methods. Here's what I came up with: https://github.com/rhfung/django-rest-framework/commit/7fd18fbe222654703fd8cbb36d4c7cc2b7efbb16

The only problem here is that if someone overrides create and save then they have to manually call full_clean and cannot rely on the pre-save hook.

[rrauenza]

I'm trying to implement a workaround for this -- I've started with a mixin that overrides validate() in the ModelSerializer.

The first problem I'm finding is that the attrs passed to validate() aren't complete enough to call Model(**attrs).full_clean() against if some of the attrs are read only -- they are omitted.

Can we reconsider @rhfung 's pre_save() hook? This would allow the ModelSerializer to make the unsaved instance and for us to inject a instance.full_clean() against the Model class...

We could alternately override create() and update() in the ModelSerializer, but for update we have to replicate the setting code since the method returns an instance -- i.e., we can't call super() first. Similar with create().

[swehba]

I am new to Django, and my opinions are therefore rather misinformed or "underinformed", but I tend to agree with @grjones. IMHO a model should be valid whether it is created by virtue of Django REST Framework, or by Django Admin, or by Django forms. This, to me, implies that all validation logic should reside at the model level. It doesn't make much sense to me to have validation code in the model for the sake of, say, Django Admin, and then have to replicate that code in a serializer. It also seems strange that a Django ValidationError is not the same as a DRF ValidationError, and within DRF you have to catch one and translate it into the other. Am I missing something here?

[jacek-jablonski]

I also find it strange to have two places where I can provide validation. It is fully understandable that in serializer I should provide validation for serializers that aren't bound to django's models. But when serializer is bound to django's model, then in which situations should I provide additional validation in serializer?

[tomchristie]

Welcome to talk this over amongst yourselves, but I don't see us having any change on this.
There's a chunk of context on the design considerations in this Django Under the Hood video.

[gonzaloamadio]

@tomchristie , If we put all the validation logic in the serializer. What happens if we want to reuse it when use the django admin for example? I mean, if we create some model using django admin

[xordoquy]

We didn't say it should be in serializer either. Have your business rules in some specific place that you can easily unit test and reuse it from serializers or model's full_clean if that's what you want though I'd rather agree with this statement:

ensure that state changing operations on model instances are only ever made via method calls that can provide a boundary that ensures that only valid state changes may ever be made by the rest of the application.

[gonzaloamadio]

We didn't say it should be in serializer either. Have your business rules in some specific place that you can easily unit test and reuse it from serializers or model's full_clean if that's what you want though I'd rather agree with this statement:

ensure that state changing operations on model instances are only ever made via method calls that can provide a boundary that ensures that only valid state changes may ever be made by the rest of the application.

So what that proposes, if I do not missunderstand, is that I do not use field validators, but I use a method for create/update, where I do all my validations? (something like here? https://medium.com/@hakibenita/bullet-proofing-django-models-c080739be4e)

So no matter if we create it via API or admin, validation will occur

[yhoiseth]

I have found django-fullclean useful when I need existing validations work with DRF.

It adds a pre_save signal that calls full_clean() every time a model is saved.

[reeshabhranjan]

I would still like the developers to rethink this after six years of this change because instead of making the application code simpler, all it is encouraging me to do is duplicate my code for Serializer and Form, or trying out hacky ways to reuse my validation code. One other design choice as mentioned by @swehba above is the different ValidationError class.

[Vektrat]

I have to comment on this again, because after years using DRF I stumbled upon this issue after a colleague put a validators=[...] arg in a model field.

While due to a responsibility principple of layers (domain/API) I defend the existence and distinction between Django.ValidationError and DRF.ValidationError (though the naming is asking for trouble... ^^), I have to say that the current behaviour seems to be forcing the devs to embark on a contradictory path in here...

If the model field specifies validators (which makes sense as pointed out by @swehba, since a model might _also_ need to validate in a more global scale), DRF is taking those into consideration, and doing so before any other DRF validation, and if an exception is raised, DRF wraps it with a str() over the original exception error, resulting in the API returning a string representation of an internal error, rather than an API specific one, not even giving the opportunity to manage this specific error, or provide an adequate error code or string rather than returning the internal error info straight away.

Not asking for a change nor I do have a solution, I suppose (as specified in the docs) this change is justified and the caveats are listed, plus as it is already said as well, we can always go back to using a Serializer rather than a ModelSerializer, but like the original poster, I wanted to leave it here that the way in which such exceptions are being handled is probably not ideal.

[auvipy]

Welcome to talk this over amongst yourselves, but I don't see us having any change on this.
There's a chunk of context on the design considerations in this Django Under the Hood video.

never mind, I can see lots of sentiment for this issue. we could consider moving this to discussion