3.15(.1?) regression: optional fields in serializers are suddenly required (or need explicit None passed) about django-rest-framework

[rnshaikh]

I have checked your test case on 3.15.1 it is working without passing optional field.

[tomchristie]

Closing based on feedback from @rnshaikh.

[AlexanderNeilson]

I think there might be something more to this. Possibly some interaction deeper between some other package / setting / default and how this works.

I upgraded from 3.14.0 to 3.15.1 last night (some other changes were made to the system as well so the interaction may come from that side but the "fix" I tried in my app should have worked and is the bit I think might be the residual issue)

Background:
One function of my system is to issue Purchase Orders on behalf of the company. Part of issuing a new PO involves generating the PO Number to be used. This is generated using django-sequences so the user doesn't submit the PO Number for the create. However when they create a "revision" as part of checking they provided the correct details we verify the po number they submit matches the purchase order they have asked to revise.

When upgraded to 3.15.1 Purchase Orders have stopped creating and started to return a response saying that PO Number is required. Now in investigating this I discovered that the web frontend had not been providing the PO Number field empty on create (the model specifies blank=True but not null=True) so through some combination of code / defaults it was incorrectly accepting it beforehand.

Realising that this was potentially a change to fix a "hole" that was being allowed before I went into the serialised and overrode the automatic serializer for that field and set required=False. However when tested / deployed this was still rejecting my submission with the same "validation error" response.

I added some debug printing into my serialised (I have added a general "validate" and the "create" and "update" methods) and neither a debug print during validation or creation is triggered before the response is generated.

So it seems to be some code change, coincidence, other setting, interaction that while the validator is now properly checking for the presence of blank=true keys in the submitted object but it seems like some combination of setting I have and changes in the code has caused something to be skipped.

I have not yet been able to trace a likely cause and I don't yet have a reliable isolated test that I can give. But I wanted to put in that I have seen a similar but enough different details as the above and so even if it turns out to be bad code elsewhere that has interacted with another fix you have done between 3.14.0 and 3.15.1 that could maybe suggest a note in documentation that guides users like myself what we might have done wrong.

Regards
Alexander

[tomchristie]

So it seems to be some code change, coincidence, other setting, interaction that while the validator is now properly checking for the presence of blank=true keys in the submitted object but it seems like some combination of setting I have and changes in the code has caused something to be skipped.

Hmm... Sounds a little tricky. A good starting point would be examining the repr of the serializer class both before and after the upgrade. That would show all the validation fields that are being generated for it. If you'd like a second pair of eyes on it you're welcome to share some code if that might be helpful.

I have not yet been able to trace a likely cause and I don't yet have a reliable isolated test that I can give. But I wanted to put in that I have seen a similar but enough different details as the above and so even if it turns out to be bad code elsewhere that has interacted with another fix you have done between 3.14.0 and 3.15.1 that could maybe suggest a note in documentation that guides users like myself what we might have done wrong.

You almost certainly haven't done anything wrong here. The responsibility is on us to ensure that upgrades really are upgrades, not sidewaysgrades or downwardsgrades.

[AlexanderNeilson]

@tomchristie thank you for your support to this.

Beside some format differences (related to how I executed the code in 3.14.0 and 3.15.1 - using the staging server and my dev machine) the two changes are as follows:

3.14.0:

    po_number = IntegerField(max_value=2147483647, min_value=0, required=False)
    revision = IntegerField(max_value=32767, min_value=0, required=False)

3.15.1

    po_number = IntegerField(max_value=2147483647, min_value=0, required=True)
    revision = IntegerField(default=0, max_value=32767, min_value=0, required=False)
...
    class Meta:
        validators = [<UniqueTogetherValidator(queryset=PurchaseOrder.objects.all(), fields=('po_number', 'revision'))>]

These are both generated from the following model extract:

    po_number = models.PositiveIntegerField(
        blank=True,
    )
    revision = models.PositiveSmallIntegerField(
        blank=True,
        default=0,
    )
...
    class Meta:
        constraints = [
            models.UniqueConstraint(fields=['po_number', 'revision'], name='gl_purchase_order_unique_po_revision'),
        ]
        ordering = ['-po_number', '-revision', ]

As in the documentation where it states "Note: The UniqueTogetherValidator class always imposes an implicit constraint that all the fields it applies to are always treated as required. Fields with default values are an exception to this as they always supply a value even when omitted from user input." Unique Together Validator Documentation

However it appears that 3.14.0 wasn't enforcing this #7173 and was them implemented in #7438
Now I accept that the documentation says it implies required=True however this did work before. I am not sure what should have happened but hopefully now if people google for this they might see this note and find the cause and might be able to look into this quicker.

I will now look and see if I can / am willing to use the default to call the issuance of the new PO number from django-sequences or if I really want to generate this myself deeper in my code and possibly push this down to use the ORM constraints that Django have added to implement this and allow my continued use of the method in existance.

Does DRF have a policy for including notes about periods where things were not working as documented and about when fixes are installed (in the sense that should I add in a pull request to update the docs to note that this wasn't working for "some" versions and starting on 3.15.0 it is now enforced in ModelSerializer to help others like (potentially @PureTryOut and me) to spot this as a change. (I have only found the changelog entry matching this after your hint to check the repr - I always forget to - and then using that as a source then find it) so while not everyone has to be as silly as me it may be worth adding a note in.

Thank you for your guidance in finding what has occurred (in my case at least) and help me now go find my solution to this situation.

Regards
Alexander

[tomchristie]

Oof... this is a case study right here.

In terms of an immediate resolution, you can revert to the prior behavior by explicitly declaring those fields on your serializer class, rather than allowing them to be auto-generated.

In terms of what we should have done better (or to rephrase more productively, how we can improve in the future) there's plenty to point towards.

[PureTryOut]

Ah I think you found the cause @AlexanderNeilson. I do indeed use a UniqueConstraint over multiple fields including house_number_addition, I omitted that from my original post and it seems that was a mistake.

A bit annoying because I definitely want it to be part of that unique constraint, and it being None is still a valid value. House number 5 with addition None is not the same as house number 5 with addition a and they should both be treated as unique. I'll explicitly declare the field for now so we can at least upgrade DRF.

I guess this warranted a major upgrade rather than a minor one if the new behaviour is intended 😅

[AlexanderNeilson]

@PureTryOut I think the behaviour was always intended but before 3.15.0 it wasn't actually working (looking at the tickets from the time it appears the problem being solved by this code change was to actually enforce this in validation instead of just letting the database constraint cause a 500 error when it was violated - I think their report was that Django itself was returning a 400)

I also note, slightly funny, that Django appears to have had this issue (or a related one) prior to 4.1 as they have in their constraints page:
"
Changed in Django 4.1:
In older versions, constraints were not checked during model validation.
"

@tomchristie Thank you, my first attempt (when this first hit me the other day) was to specify this explicitly and it appeared to be failing to work. I have just tried it now and am seeing this:
Serializer:
po_number = serializers.IntegerField(max_value=2147483647, min_value=0, required=False)
Serializer repr:

    po_number = IntegerField(max_value=2147483647, min_value=0, required=False)
...
    class Meta:
        validators = [<UniqueTogetherValidator(queryset=PurchaseOrder.objects.all(), fields=('po_number', 'revision'))>]

Request Body:
{"job":7845,"supplier":70,"po_external_references":[],"po_notes":"","internal_po_notes":"","delivery_option":1,"po_lines":[{"oncharge_line":false,"description":"bought some board","quantity":"2"}],"send_as_copy":true}
Response:

{
    "po_number": [
        "This field is required."
    ]
}

So the Validator is still included in the serializer now under 3.15.1 despite the explicit required=False (which was what started me down this path that led me to this "issue" and these investigations)

I am wondering if the validators being added are then being called with a "check_validators" or "full_clean" somewhere in the flow and their presence is triggering the alert by failing because the field is missing (or is something running the validation and re introducing the required=True further down the line). However as I am not making it to my "def validate(self, data)" step (debug print never fires) then I am thinking something must be overriding my explicit not required (or I really screwed up).

Thank you

Regards
Alexander

[PureTryOut]

I think the behaviour was always intended but before 3.15.0 it wasn't actually working

I realize this, it's a note I completely missed in the documentation, but seeing how long the "incorrect" behaviour was active people had started relying on it and I think it warranted a major version bump. I also just don't understand the behaviour, why is not passing it not fine but passing explicit "None" is?

Anyway I'm fine with the workaround (I just tested it, it works), so for me the issue is closed.

[buchannon]

I am having this issue with a simple ModelSerializer, overwriting the ID field to make it optional in the request.

id = serializers.UUIDField(required=False)

Updating to djangorestframework to v3.15.0 or v3.15.1 breaks optional field behavior for me, starting to make this field throw a validation error when not present.

rest_framework.exceptions.ValidationError: {'question': {'id': [ErrorDetail(string='This field is required.', code='required')]}}

Reverting back to djangorestframework version 3.14.0 fixes this issue for me.

[AlexanderNeilson]

@buchannon

Is this a default "primary key unique" requirement or have you got a manually specified constraint on the model requiring this to be unique?

If you open up a Django debugging shell "python manage.py shell" and then import your serializer and print(repr(())) of it is that showing the validator at the bottom while showing the id field as required=False?

I think what's happening here is that the forced "required=False" is being clobbered as the validation is running the constraints and the constraint requires the field so it still triggers the validation error as a field error when the unique or unique together is being run. (my theory only, not confirmed)

I'm currently considering setting default=0 (for my int field - you may want to set default=uuid.uuid4 or whatever format you chose to use (remember it should be a callable so no brackets at the end) so it can check the uniqueness of that value in the validation step and allow the code to continue to run.

That way we bypass the serializer validation step even though its not checking the final value and allow the database validation for primary key (unique enforced) (I still need to make sure postgresql is getting the unique together pushed down from the model into database spec to make sure this will work for me).

Its a partial skipping of the validation code but until DRF / Django drops support for pre 4.1 and the option to use the new constraints could push that down to the database tier / later in the process.

What's caught me out is I specified the unique together on the model to try push this enforcement down to the database as I am running my own code to generate the po number in the create method and increase the revision count when checked in the update method (so "change" generates new instance).

Regards
Alexander