Django-rest-framework: Model's "clean" method raises ValidationError with specific fields, but DRF calls them non-field errors

[ataylor32]

Checklist

• [x] I have verified that that issue exists against the master branch of Django REST framework.
• [x] I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• [x] This is not a usage question. (Those should be directed to the discussion group instead.)
• [x] This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
• [x] I have reduced the issue to the simplest possible case.
• [ ] I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

1. Create a fresh Django project
2. Create an app called "widgets"

3. Use this code for the "widgets" app's "models.py" file:

   from django.core.exceptions import ValidationError
   from django.db import models
   
   
   class Widget(models.Model):
       STATUS_INCOMPLETE = 'i'
       STATUS_IN_PROGRESS = 'p'
       STATUS_COMPLETE = 'c'
       STATUS_CHOICES = (
           (STATUS_INCOMPLETE, 'Incomplete'),
           (STATUS_IN_PROGRESS, 'In Progress'),
           (STATUS_COMPLETE, 'Complete'),
       )
   
       name = models.CharField(max_length=255, unique=True)
       foo_status = models.CharField(choices=STATUS_CHOICES, max_length=1)
       bar_status = models.CharField(choices=STATUS_CHOICES, max_length=1)
       baz_status = models.CharField(choices=STATUS_CHOICES, max_length=1)
   
       class Meta:
           ordering = ('name',)
   
       def __str__(self):
           return self.name
   
       def clean(self):
           in_progress = 0
           excess_in_progress_status_fields = []
   
           for status_field in (
               'foo_status',
               'bar_status',
               'baz_status',
           ):
               if getattr(self, status_field) == self.STATUS_IN_PROGRESS:
                   in_progress += 1
                   if in_progress > 1:
                       excess_in_progress_status_fields.append(status_field)
   
           if excess_in_progress_status_fields:
               errors = {}
   
               for status_field in excess_in_progress_status_fields:
                   errors[status_field] = (
                       'You cannot have multiple status dropdown menus set to '
                       '"In Progress".'
                   )
   
               raise ValidationError(errors)
   

4. Use this code for the "widgets" app's "admin.py" file:

   from django.contrib import admin
   
   from .models import Widget
   
   admin.site.register(Widget)
   

5. Use this code for the project's "urls.py" file:

   from django.conf.urls import url, include
   from django.contrib import admin
   from rest_framework import routers, serializers, viewsets
   
   from widgets.models import Widget
   
   
   class WidgetSerializer(serializers.ModelSerializer):
       class Meta:
           model = Widget
           fields = '__all__'
   
       def validate(self, attrs):
           instance = Widget(**attrs)
           instance.clean()
           return attrs
   
   
   class WidgetViewSet(viewsets.ModelViewSet):
       queryset = Widget.objects.all()
       serializer_class = WidgetSerializer
   
   router = routers.DefaultRouter()
   router.register(r'widgets', WidgetViewSet)
   
   urlpatterns = [
       url(r'^', include(router.urls)),
       url(r'^api-auth/', include('rest_framework.urls', namespace='rest_framework')),
       url(r'^admin/', admin.site.urls),
   ]
   

6. Try to create a record with all three status fields set to "In Progress"

Expected behavior

The "bar_status" and "baz_status" fields should each be given an error message. This is how it's handled in the admin:

Actual behavior

The two error messages intended for the "bar_status" and "baz_status" fields are added to "non_field_errors" instead:

[nikhil96sher]

Hi @rpkilby
I wanted to work on this issue. So, I explored the framework and figured out that for this to work fields.get_error_details() should preserve the keys and thus return a mapping instead of a list (the current behavior).
The same has also been discussed in #4787
So, should this also be flagged as an improper usage (or) should we proceed with modifying serializers.as_serializer_error() to first check if it has key-value information, then convert detail to a mapping. I can submit a pull request for the latter.

[rpkilby]

Hi @nikhil96sher. This issue is relatively low priority for me, and at this point, I can't provide any specific guidance. First, some thought needs to be put into what the correct behavior should be. ie, as you stated, is this invalid usage, or should instance-level validation support a ValidationError with a messages dict...

One thing to check is if dict for serializers.ValidationError is supported, but not forms.ValidationError

Note to future self - this is not a duplicate of #3144.

[nwaxiomatic]

+1 running into same issue with "unique_together" validation

[jonashaag]

I just hit this too. I followed the official recommendation for having the validation in Model.clean be considered by DRF.

[jf248]

+1 Having to do this to work around:

from django.core.exceptions import ValidationError
...

    def validate(self, data):
        instance = Model(**data)
        try:
            instance.clean()
        except ValidationError as e:
            raise serializers.ValidationError(e.args[0])

[jonashaag]

If you have additional fields in your data (that aren't model fields), use this

class ValidatingModelSerializer(rest_framework.serializers.ModelSerializer):
    def validate(self, attrs):
        instance = self.Meta.model(
            **{
                field: value
                for field, value in attrs.items()
                if field in self.Meta.model._meta.fields_map
            })
        instance.clean()
        return attrs

[rrauenza]

@jonashaag , @jf248 Why .clean() and not .full_clean()? Are you intentionally skipping the uniqueness check?

[th0th]

Calling model's clean() method from serializer's validate() method causes trouble if model's clean() method validates some of the model object's existing fields (for example, request tries to update an object with PATCH method, with some of the existing values missing). As a workaround I used this:

def validate(self, attrs):
    serializer = ModelSerializer(
        instance=self.instance,
    )

    model_data = {
        **serializer.data,
        **attrs,
    }

    self.Meta.model(**model_data).clean()

    return attrs

[dennishylau]

This method seems to work for me for both instance creation and update, YMMV though

    def validate(self, data):
        # get the current user if needed
        user = getattr(self.context['request'], 'user', None)
        if self.instance is None:
            # api is called to create model instance
            instance = self.Meta.model(user=user,**data)
            instance.clean()
        else:
            instance = self.instance
            [setattr(instance, x, data[x]) for x in data]
            instance.clean()
        return data

[Muhammadinaam]

It is open since 2017??

[expresspotato]

Yes, I need to comment on this as well just because it seems so absurd that clean() is not called from DRF. Clean is the best place to return validation errors to be presented in the Django admin, it's called during any shell based object creation / updating and it keeps things consistent. Some DRF developer made the change in 3.0 and it's wrecked the DRY principle completely. If you wanted to force some other validation in special cases fine, but this should not be default behavior at all.