Django-rest-framework: Model unique_for_month changes ability to partially update ModelSerializer

[jambonrose]

If I have a model and a ModelSerializer, then I can partially update model instances using the serializer. For instance, given a Post model, and a basic PostSerializer, shown below, I can update a Post instance using the serializer.

class Post(Model):
    title = CharField(max_length=63)
    slug = SlugField(max_length=63)
    pub_date = DateField(default=date.today)

class PostSerializer(ModelSerializer):
    class Meta:
        model = Post
        fields = "__all__"

I use the code below to achieve the actual update.

post = Post.objects.create(title="first", slug="slug")
s_post = PostSerializer(instance=post, data={"title": "second"}, partial=True)
if s_post.is_valid():
    s_post.save()  # updates the title from first->second

The problem is that if I add a unique_for_month constraint on the SlugField, I lose the ability to use the code above.

class Post(Model):
    title = CharField(max_length=63)
    slug = SlugField(max_length=63, unique_for_month="pub_date")
    pub_date = DateField(default=date.today)

If I run the update code with the PostSerializer as shown above, then the serializer will have the following errors:

{
    'slug': [
        ErrorDetail(
            string='This field is required.',
            code='required'
        ),
    ],
    'pub_date': [
        ErrorDetail(
            string='This field is required.',
            code='required'
        )
    ],
}

I believe this is a bug, but apologize if I've misunderstood.

[jambonrose]

I've taken the liberty of creating an minimum-viable example on Github to demonstrate the difference in behavior and help evaluate the problem.

The relevant code is:

• models.py
• serializers.py
• tests.py

The tests are written to pass (demonstrate the failure successfully), but I've included code that will fail if uncommented.

The assertion that I would expect to pass is on line 61.

If I see confirmation that this is in-fact a bug rather than a mistake on my part, I am happy to write a test for DRF itself.

Hope this is helpful!

[jambonrose]

Hi, is there anything I can do to clarify the issue? Would love to get input and know how best to help here.

[tomchristie]

Okay, so the thing for us to do first is to break this down into two seperare issues:

1. What is the serializer class that ModelSerializer generates. Is that as expected?
2. Is the behavior of the serializer class as expected?

If you print an instance of Post you'll see the serializer class. I think that'll have a required=True on each of those two fields. Assuming that's the case the question is why does partial here not remove the required-ness?

[tomchristie]

Replicating the issue with a plain Serializer class will narrow down the behaviour that we need to consider here.

[jambonrose]

Hi Tom,

Thanks for taking a look at this.

I've taken the first step and printed out the serializer class from each of the models. Neither the slug field nor the date field gain the required argument, which I believe is what we consider correct behavior.

>>> from blog.serializers import PostSerializer, UniquePostSerializer

>>> PostSerializer()                                                                                                                     
PostSerializer():
    id = IntegerField(label='ID', read_only=True)
    title = CharField(max_length=63)
    slug = SlugField(max_length=63)
    pub_date = DateField(required=False)

>>> UniquePostSerializer()                                                                                                               
UniquePostSerializer():
    id = IntegerField(label='ID', read_only=True)
    title = CharField(max_length=63)
    slug = SlugField(max_length=63)
    pub_date = DateField(default=<built-in method today of type object>)
    class Meta:
        validators = [
            <UniqueForMonthValidator(
                queryset=UniquePost.objects.all(),
                field='slug',
                date_field='pub_date',
            )>
        ]

However, I do see that the serializer has a UniqueForMonthValidator and I'm therefore guessing (but please correct me if I'm missing something) that the problem lies with what the UniqueForMonthValidator is doing, rather than the ModelSerializer. That said, to be certain, I'll use a class with Serializer as base to narrow the problem down.

I note that the printout of DateField appears to be different for the two classes (despite the models being identical for both). I'll see if that's limited to the printout or to behavior.

I'll report back once I've got more info, and will keep updating the MVE as I go.

[jambonrose]

I confirm the same unexpected behavior using Serializer as base, which leads me to believe ModelSerializer is not at fault, but that serial validation or UniqueForMonthValidator is.

EDIT: For clarify, I went and tried the UniqueTogetherValidator, UniqueForDateValidator, UniqueForMonthValidator, UniqueForYearValidator on the UniquePostSerializer. The UniqueTogetherValidator does not exhibit the problem (when running the test suite), but all of the date validators do.

[jambonrose]

I believe the problem stems from the BaseUniqueForValidator.enforce_required_fields() method.

The Serializer calls run_validators() during the validation process, and this then calls the run_validators() method in Field. Within this method, Field will call the BaseUniqueForValidator.set_context() method, before calling the validator directly.

The validator will store the instance associated with the Serializer when the context is set. Stated differently: UniqueForMonthValidator has access to the post object specified in UniquePostSerializer(instance=post, data={"title": "second"}, partial=True).

BaseUniqueForValidator.__call__() invokes BaseUniqueForValidator.enforce_required_fields(). This is the method that raises the validation error for slug and pub_date. The method does so because it checks only the field values found in the data argument, and does not ever use the field values from the instance.

To fix this issue, I would therefore propose to change BaseUniqueForValidator.enforce_required_fields() such that it relies on data from the instance (if it is set) as well as the new data passed to the serializer.

I'm happy to open a PR if that sounds like the right direction.

[tomchristie]

To fix this issue, I would therefore propose to change BaseUniqueForValidator.enforce_required_fields() such that it relies on data from the instance (if it is set) as well as the new data passed to the serializer.

I think it's a little simpler than "relies on data from the instance". If we have an instance then we don't need enforce_required_fields to run anything else, because we neccessarily do have all the fields populated.

See the behaviour of UniqueTogetherValidator.enforce_required_fields - it simply returns if there is an instance: https://github.com/encode/django-rest-framework/blob/master/rest_framework/validators.py#L114

I assume that we want a pull request that does the same: simply return from BaseUniqueForValidator.enforce_required_fields if an instance is set.

Find out if that introduces any test failures, then take things from there. :)

[jambonrose]

😅 That solution makes much more sense than my suggestion! I'll have a go at this ASAP.