DRF wrongly moves validation business logic from models to views #7850
Replies: 26 comments 5 replies
-
|
:) 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. |
Beta Was this translation helpful? Give feedback.
-
|
So, point by point:
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 |
Beta Was this translation helpful? Give feedback.
-
|
@grjones I agree with Tom Christie that The only problem here is that if someone overrides |
Beta Was this translation helpful? Give feedback.
-
|
I'm trying to implement a workaround for this -- I've started with a mixin that overrides The first problem I'm finding is that the attrs passed to Can we reconsider @rhfung 's We could alternately override |
Beta Was this translation helpful? Give feedback.
-
|
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 |
Beta Was this translation helpful? Give feedback.
-
|
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? |
Beta Was this translation helpful? Give feedback.
-
|
Welcome to talk this over amongst yourselves, but I don't see us having any change on this. |
Beta Was this translation helpful? Give feedback.
-
|
@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 |
Beta Was this translation helpful? Give feedback.
-
|
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
|
Beta Was this translation helpful? Give feedback.
-
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 |
Beta Was this translation helpful? Give feedback.
-
|
I have found django-fullclean useful when I need existing validations work with DRF. It adds a |
Beta Was this translation helpful? Give feedback.
-
|
@yhoiseth how do you handle |
Beta Was this translation helpful? Give feedback.
-
|
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 |
Beta Was this translation helpful? Give feedback.
-
|
I have to comment on this again, because after years using DRF I stumbled upon this issue after a colleague put a While due to a responsibility principple of layers (domain/API) I defend the existence and distinction between If the model field specifies 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 |
Beta Was this translation helpful? Give feedback.
-
never mind, I can see lots of sentiment for this issue. we could consider moving this to discussion |
Beta Was this translation helpful? Give feedback.
-
|
I too wish we could have a single model validation rule that was respected by all interfaces (DRF serializers, admin, etc.) The alternatives we are left with as I understand them are:
All solutions create a cascade of complexity that inevitably burden the developer. It should be easier than this to ensure that my model does not allow instances to be in garbage states. I shouldn't be drifting anywhere else than the model to do this. |
Beta Was this translation helpful? Give feedback.
-
|
Was very interesting to see how many people got the same issues around DRF vs. Django model validation. models.py serializers.py |
Beta Was this translation helpful? Give feedback.
-
|
After a long search, I found an elegant way to retrieve the validations of our models. We generate our custom exception handling (https://www.django-rest-framework.org/api-guide/exceptions/#custom-exception-handling) so that it catches django errors (ValidationError) Our model# models.py
from django.db import models
from django.core.exceptions import ValidationError
class MyModel(models.Model):
start = ...
finish = ...
...
def clean(self):
if self.finish < self.start:
raise ValidationError("Finish must occur after start")
def save(self, *args, **kwargs):
self.full_clean()
super().save(*args, **kwargs)Our custom exception handling# myapp/exceptions.py
from django.core.exceptions import ValidationError
from rest_framework.views import exception_handler
from rest_framework.response import Response
from rest_framework import status
def django_error_handler(exc, context):
"""Handle django core's errors."""
# Call REST framework's default exception handler first,
# to get the standard error response.
response = exception_handler(exc, context)
if response is None and isinstance(exc, ValidationError):
return Response(status=status.HTTP_400_BAD_REQUEST, data=exc.message_dict)
return response# settings.py
REST_FRAMEWORK = {
...,
'EXCEPTION_HANDLER': 'myapp.exceptions.django_error_handler'
}I have relied on an answer from this post https://stackoverflow.com/questions/65912181/how-to-validate-against-full-model-data-in-django-rest-framework. |
Beta Was this translation helpful? Give feedback.
-
|
I have tried both solutions proposed by @tsepulvedacaroca01 and @d3lav3ga, but I cannot get the Browsable API to handle the field errors correctly. The only way I can get the Browsable API to handle the field errors correctly, is to throw Solution 1: Throw
|
Beta Was this translation helpful? Give feedback.
-
|
I understand your problem, when generating the validator in the serializer, the Django admin will not detect it, because it gets the validations from the model ( The idea of the solution implemented is to avoid duplicating the validation in the model and the serializer. Validate the following:
If everything is correct, when triggering a model error from the serializer it should be received by the |
Beta Was this translation helpful? Give feedback.
-
|
The best solution to this issue as written up by HackSoftware: from rest_framework import exceptions as rest_exceptions
def get_first_matching_attr(obj, *attrs, default=None):
for attr in attrs:
if hasattr(obj, attr):
return getattr(obj, attr)
return default
def get_error_message(exc):
""" Used for DRF error message extraction """
if hasattr(exc, 'message_dict'):
return exc.message_dict
error_msg = get_first_matching_attr(exc, 'message', 'messages')
if isinstance(error_msg, list):
error_msg = ', '.join(error_msg)
if error_msg is None:
error_msg = str(exc)
return error_msg
class ApiErrorsMixin:
"""
Mixin that transforms Django and Python exceptions into rest_framework ones.
without the mixin, they return 500 status code which is not desired.
"""
expected_exceptions = {
ValueError: rest_exceptions.ValidationError,
ValidationError: rest_exceptions.ValidationError,
PermissionError: rest_exceptions.NotFound # PermissionDenied
}
def handle_exception(self, exc):
if isinstance(exc, tuple(self.expected_exceptions.keys())):
drf_exception_class = self.expected_exceptions[exc.__class__]
drf_exception = drf_exception_class(get_error_message(exc))
return super().handle_exception(drf_exception)
return super().handle_exception(exc)Simply add the mixin to your |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for the reference, @alfonsrv! The link didn't work, but I found a fork of the style guide here. You also forgot to include the required functions. I'll add them here for reference: def get_first_matching_attr(obj, *attrs, default=None):
for attr in attrs:
if hasattr(obj, attr):
return getattr(obj, attr)
return defaultdef get_error_message(exc):
if hasattr(exc, 'message_dict'):
return exc.message_dict
error_msg = get_first_matching_attr(exc, 'message', 'messages')
if isinstance(error_msg, list):
error_msg = ', '.join(error_msg)
if error_msg is None:
error_msg = str(exc)
return error_msgfrom rest_framework import exceptions as rest_exceptions
from django.core.exceptions import ValidationError
from project.common.utils import get_error_message
class ApiErrorsMixin:
"""
Mixin that transforms Django and Python exceptions into rest_framework ones.
Without the mixin, they return 500 status code which is not desired.
"""
expected_exceptions = {
ValueError: rest_exceptions.ValidationError,
ValidationError: rest_exceptions.ValidationError,
PermissionError: rest_exceptions.PermissionDenied
}
def handle_exception(self, exc):
if isinstance(exc, tuple(self.expected_exceptions.keys())):
drf_exception_class = self.expected_exceptions[exc.__class__]
drf_exception = drf_exception_class(get_error_message(exc))
return super().handle_exception(drf_exception)
return super().handle_exception(exc)However, including the |
Beta Was this translation helpful? Give feedback.
-
|
Ha, cheers! Was in a rush this morning, but thought I'd finally share it. Try registering the Mixin as the first class of your view – e.g. |
Beta Was this translation helpful? Give feedback.
-
|
If I define a class PersonSerializer(serializers.ModelSerializer):
...
def validate_name(self, name):
raise serializers.ValidationError("This works as expected")
...it works as expected:
|
Beta Was this translation helpful? Give feedback.
-
|
I did not know this - the lack of model validation support - was still a thing in DRF. ProblemI need validation of my data, regardless of the point of ingress. Solution?
That's No Good
|
Beta Was this translation helpful? Give feedback.
-
|
I only came to say, thank you all for talking about this. I only found out just now that my model validation was not being implemented at all. 🙃 😩 |
Beta Was this translation helpful? Give feedback.
-
|
Welp, if only i found this issue earlier. I really like @tsepulvedacaroca01 's approach, but as i came up with my own solution already i thought i'd share it, too. As many, i didn't want to maintain separate validators for forms and serializers, so i've written a validator that can be used by both: class ValidateFuzzyUnique:
"""Matches the validation value with all existing target_field values and raises
an ValidationError exception if it is too similar.
Args:
queryset: The queryset to be used.
target_field: The field of the queryset that should be compared.
source: Where the validator is called from: 'serializer' or 'form'.
value: The value to be validated on calling.
Raises:
ValidationError: Raised if SequenceMatcher ratio is > 0.85.
"""
def __init__(self, queryset, target_field, source="form"):
self.queryset = queryset
self.target_field = target_field
self.source = source
self.model = self.queryset.model.__name__
def __call__(self, value):
values = [getattr(row, self.target_field)
for row in self.queryset]
value = value.lower()
for val in values:
if SequenceMatcher(None, val.lower(), value).ratio() > 0.85:
message = (
f"Name too similar to existing {self.model} {val}.")
if self.source.lower() == "serializer":
raise serializers.ValidationError(message)
else:
raise ValidationError(message)This can then be used in forms and serializers: class TagModelSerializer(serializers.ModelSerializer):
name = serializers.CharField(validators=[
validators.ValidateFuzzyUnique(
queryset=models.Tag.objects.all(),
target_field="name",
source="serializer"
)
])
class Meta:
model = models.Tag
fields = "__all__"Let me know if there are any issues or downsides with this that i might have missed of course ;) |
Beta Was this translation helpful? Give feedback.
-
|
I feel there is a bit of an asymmetry that if I write validation logic in the model for a single field DRF picks it up automatically and there is no opinionated way to do this when there is a dependency between fields. I could imagine having a validators field at the model level, which takes a list of functions get an attr dict. Something like (bear with me I'm a bit new to Python & Django): # model_validators_mixin.py
from django.forms import model_to_dict
class ModelValidatorsMixin(object):
validators = []
fields_to_include_in_validation = []
def clean(self):
attr = model_to_dict(instance=self, fields=self.fields_to_include_in_validation if len(self.fields_to_include_in_validation) > 0 else None)
for validator in self.validators:
validator(attr)
super(ModelValidatorsMixin, self).clean()
# model_serializer_validators_mixin.py
from django.core.exceptions import ValidationError as DjangoValidationError
from rest_framework.exceptions import ValidationError
class ModelSerializerValidatorsMixin(object):
def validate(self, attrs):
for validator in self.Meta.model.validators:
try:
validator(attrs)
except DjangoValidationError as exc:
raise ValidationError(exc)
return attrs
# some_model.py
from django.core.exceptions import ValidationError
from django.db import models
from shared.model_validators import ModelValidatorsMixin
def validate_a(attr: dict):
a = attr.get("a")
b = attr.get("b")
if a == b:
raise ValidationError("a cannot be equal to b")
def validate_b(attr: dict):
a = attr.get("a")
b = attr.get("b")
c = attr.get("c")
if len(a) - len(b) - len(c) < 0:
raise ValidationError("a - b - c cannot be less than 0")
class SomeModel(models.Model, ModelValidatorsMixin):
a = models.CharField(max_length=255)
b = models.CharField(max_length=255)
c = models.CharField(max_length=255)
validators = [validate_a, validate_b]
fields_to_include_in_validation = ["a", "b", "c"] |
Beta Was this translation helpful? Give feedback.
-
|
I can see important staff in both approaches. I really like Tom Christie's article: http://www.dabapps.com/blog/django-models-and-encapsulation/. Also, as @swehba mentioned, I need some common validation, no matter if I use Serializer, django admin, django forms, management command, invoke model method from Celery task or use shell. This almost matches Tom Christie's approach - I can set model field validators. The only problem here - how will I will make a multi field validation? ================ So in my situation, I was thinking about the following:
The idea was:
Now point by point. 1.1. DRF call business logic this for create. I.e. DRF calls Model.objects.create(), which I customize.
And here it brakes. I have now double validation: one from serializer and much the same from business logic layer. @tomchristie, can you help me to understand this? I greatly appreciate your article and work, but still cannot understand this moment. |
Beta Was this translation helpful? Give feedback.
-
|
Hello everyone, THE PHILOSOPHY I think, that, developers must write Model validation into ONE SINGLE PLACE. Django PROBLEM I know that there is a lot of confusion about Model Validation:
WORKAROUD: Django Community
So when i call WORKAROUD: Django Rest Framework He says: don't call never save() model method directly, but use model manager/instance to create methods that calls save(). Django Rest Framework PROBLEM WORKAROUND: So the idea is that serializer's validation will be failed if Model.full_clean() trown a ValidationException. PATCH: Sample Implementations: So if i do: This have limited compatibility with nested serializer -> https://github.com/giuseppenovielli/django_dry_project/blob/main/src/motorizations/views.py#L25 What do you think about this approach? |
Beta Was this translation helpful? Give feedback.
-
|
can someone eli5 me on this, I'm just getting started and want to make sure my validation goes through. what validations aren't run and in which case? |
Beta Was this translation helpful? Give feedback.
and others
-
I realize this was an intentional change in 3.0 where
ModelSerializersno longer run model-levelcleanorfull_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:ModelFormdoes?full_clean(perhaps by including it in thesavemethod)? Will serializers know how to handle an exception from an explicitfull_clean?Beta Was this translation helpful? Give feedback.
All reactions