Creating a related object when using PATCH method raises a server error

[sommelon]

Let's say I have these models:

class Address(models.Model):
    street = models.CharField()
    city = models.CharField()
    postcode = models.CharField()

class Person(models.Model):
    address = models.ForeignKey('Address', null=True, blank=True, on_delete=models.SET_NULL)
    ... other fields ...

(note that all the fields in the Address model are required)

The address field is a NestedField in my serializer

class PersonSerializer(DynamicFieldsMixin, NestedModelSerializer):
    address = NestedField(AddressSerializer, allow_null=True, required=False)

When I create a Person object with address being null and then want to add the address later using a PATCH method with some of the fields missing, PATCH /api/person/1/

{
    "address": {"street": "Main street 1"}
}

I get a server error.

File "E:\Documents\Work\hriis-crm\backend\.venv\lib\site-packages\rest_framework\serializers.py", line 200, in save
    self.instance = self.update(self.instance, validated_data)
  File "E:\Documents\Work\hriis-crm\backend\.venv\lib\site-packages\django_restql\mixins.py", line 985, in update
    fields["foreignkey_related"]["writable"]
  File "E:\Documents\Work\hriis-crm\backend\.venv\lib\site-packages\django_restql\mixins.py", line 756, in update_writable_foreignkey_related
    obj = serializer.save()
  File "E:\Documents\Work\hriis-crm\backend\.venv\lib\site-packages\rest_framework\serializers.py", line 178, in save
    'You cannot call `.save()` on a serializer with invalid data.'
AssertionError: You cannot call `.save()` on a serializer with invalid data.

When I use a PUT method, I get a normal HTTP 400 response with validation messages.

{
  "address": {
    "city": [
      "This field is required."
    ],
    "postcode": [
      "This field is required."
    ]
  }
}

[yezyilomo]

Thank you for pointing this out, I've managed to reproduce it, it's definitely a bug, we should be raising validation error and not server error. I'm working on fixing it, if you have any information about the cause feel free to share it.

[gonzaloamadio]

Is this fixed? Because I am having same issue when making a put of a nested object with a unique together validation.

[yezyilomo]

It was fixed, can I see a serializer causing that error, maybe we missed something.

[gonzaloamadio]

I think it can be fixed adding replacing serializers.is_valid() into serializers.is_valid(raise_exception=True) inside mixins file. Cause it is failing in this lines:

def bulk_create_many_to_many_related(self, field, nested_obj, data):
        ....
        serializer.is_valid()               <--- Here I have a unique together validation that is failing
        obj = serializer.save()          <--- This is causing error
        ....
    return pks

I can make a toy example for you to look at.

[yezyilomo]

We are checking all nested fields in NestedField(Which is written in fields.py file) before hitting NestedCreateMixin and NestedUpdateMixin so what goes to NestedCreateMixin and NestedUpdateMixin should be error free, if something with error go past NestedField then we missed something when checking nested field.

So we have to figure out what check is missing in NestedField. A toy example would be helpful but before creating one let me explain what I've got from your explanations.

Your case is the same as of the origin author of this issue but you have a unique together constraint, and to reproduce the bug you are violating this constraint purposely(i.e repeating value of unique together fields) to see if django-restql will detect this constraint(i.e raise validation error) but it doesn't instead it raises server error.

Is that right?

[gonzaloamadio]

Yes you get it right.

class A(models.Model):
  name =  models.CharField()
class B(models.Model)
  name =  models.CharField()
  a = models.ForeignKey('A')
  class Meta:
     unique_together = (('name', 'a'),)

I am basically doing

POST api/a/

{
    “name”: “some name",
    “b_set”: {
        “create”: [{“name”: "b name"}]
}

Assume this returns both with id==1

PATCH api/a/1

{
    “name”: “some name",
    “b_set”: {
        “create”: [{“name”: "b name"}]        <-- This will violate unique constraint
}

[yezyilomo]

Okay, here we have to know that Django REST Framework doesn’t handle DB integrity errors, even if you don’t use django-restql you would still be required to handle that in your serializer. So that looks like something which you have to handle it yourself, I also think it makes sense to leave that to users because there’re many constraints some are custom so it would be impossible to handle all of them.

[sommelon]

I found out that this error also happens when you don't specify the many=True argument in the NestedField. Can you post your serializer? @gonzaloamadio

[gonzaloamadio]

I have created a repo where I played a bit with this. Here it is the repository @sommelon

[sommelon]

@gonzaloamadio I managed to get rid of the error by removing the product field from IngredientMixSerializer, so the problem is probably there, but I have no idea what's causing it.

EDIT: Although now that I think about it, it's probably because the unique validator is no longer running, since it doesn't have the required fields.

[gonzaloamadio]

Okay, here we have to know that Django REST Framework doesn’t handle DB integrity errors, even if you don’t use django-restql you would still be required to handle that in your serializer. So that looks like something which you have to handle it yourself, I also think it makes sense to leave that to users because there’re many constraints some are custom so it would be impossible to handle all of them.

That is true. But If I have unique constraints on the model (as you can see in the repository I created), are they "really" db constraints? I mean, they are, but they should be validated in Django.

I mean we should check why If we add raise_exeption=True in the is_valid methods, it works as expected, raising a ValidationError instead of breaking.

We should debug and see why that validation is not raising a ValidationError before getting where you say it should reach errors free (I mean to the bulk_create_many_to_many_related method).

Or can we just add the raise_execption and thats it?

[yezyilomo]

That is true. But If I have unique constraints on the model (as you can see in the repository I created), are they "really" db constraints? I mean, they are, but they should be validated in Django.

I mean we should check why If we add raise_exeption=True in the is_valid methods, it works as expected, raising a ValidationError instead of breaking.

We should debug and see why that validation is not raising a ValidationError before getting where you say it should reach errors free (I mean to the bulk_create_many_to_many_related method).

Or can we just add the raise_execption and thats it?

Adding raise_exeption=True to is_valid() won't help, because DRF serializer is not able to check custom model constraint, no wonder the check serializer.is_valid() passes, mind you what raises the error is this line https://github.com/yezyilomo/django-restql/blob/f8f859ef316f037dcac4affeb218e56957e7538e/django_restql/mixins.py#L843 And not this line https://github.com/yezyilomo/django-restql/blob/f8f859ef316f037dcac4affeb218e56957e7538e/django_restql/mixins.py#L842

In short DRF serializer is not aware of any custom model constraints, so if you define a custom constraint on your model you have to make a serializer aware of it for it not to crush when such constraint is violated, and this is not something which django-restql introduced, it's how django-restframework works.

Mind you am not saying it's not possible to raise ValidationError and tell user what went wrong, what am saying is, it's not possible to do so automatically(By automatically I mean to have a serializer handle it without you writing anything).

So as a developer it's your job to handle it and make sure your serializer doesn't crush because of a custom constraint which you introduced(since you are the one who wrote that custom constraint so chances are you know it better than a serializer)

I actually have a real world example with such constraint which doesn't raise server error when the constraint is violated instead it returns a nice error message which tells API user what went wrong.

Here is the model with such constraint which looks like

class PropertyRating(models.Model):
    id = models.AutoField(primary_key=True)
    owner = models.ForeignKey(User, on_delete=models.CASCADE, related_name="property_ratings")
    property = models.ForeignKey(Property, on_delete=models.CASCADE, related_name="ratings")
    score = models.SmallIntegerField(
        validators=[MaxValueValidator(5), MinValueValidator(0)]
    )

    class Meta:
        unique_together = ('owner', 'property')

And here is the way I've handled that constraint in it's serializer which looks like

class PropertyRatingSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    score = serializers.IntegerField(max_value=5, min_value=0, required=True)
    class Meta:
        model = PropertyRating
        fields = ['id', 'url', 'owner', 'property', 'score']
        read_only_fields = ('owner',)

    def create(self, validated_data):
        """function for creating property's ratings """
        request = self.context.get('request')
        user = request.user

        validated_data.update({"owner": user})
        try:
            return super().create(validated_data)
        except IntegrityError as e:
            raise serializers.ValidationError(
                {"Integrity Error": str(e)},
                "Integrity Error"
            )

    def update(self, instance, validated_data):
        """function for updating property's ratings """
        validated_data.pop("property", None)  # Remove property when updating

        try:
            return super().update(instance, validated_data)
        except IntegrityError as e:
            raise serializers.ValidationError(
                {"Integrity Error": str(e)},
                "Integrity Error"
            )

Below is the error message which API user gets when the constraint is violated(Note it's not server error, it's a normal 400 error coded, bad request response)

{
  "Integrity Error": "duplicate key value violates unique constraint \"api_propertyrating_owner_id_property_id_0d501183_uniq\"\nDETAIL:  Key (owner_id, property_id)=(1, 12) already exists.\n"
}

[gonzaloamadio]

Yes that is the other way we were thinking of handling it.

I have the same thought as you. BUT, and you can try it with the repository I made. If you add raise_exception to the line I told you. I am not sure why, it returns a nice validation error and not the db integrity error.

Proof:

# Post without raise_exception=True
❯ python manage.py runserver
Quit the server with CONTROL-C.
Internal Server Error: /api/products/10/
  File "/Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/django_restql/mixins.py", line 843, in bulk_create_many_to_one_related
    obj = serializer.save()
  File "/Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/rest_framework/serializers.py", line 178, in save
    'You cannot call `.save()` on a serializer with invalid data.'
AssertionError: You cannot call `.save()` on a serializer with invalid data.

❯ vi /Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/django_restql/mixins.py
# here I added raise_exception=True to is_valid()

❯ python manage.py runserver
Bad Request: /api/products/10/
[06/Aug/2021 19:52:28] "PATCH /api/products/10/ HTTP/1.1" 400 79

This is the error returned to the api client

{
    "non_field_errors": [
        "The fields ingredient, product must make a unique set."
    ]
}

[yezyilomo]

Yes that is the other way we were thinking of handling it.

I have the same thought as you. BUT, and you can try it with the repository I made. If you add raise_exception to the line I told you. I am not sure why, it returns a nice validation error and not the db integrity error.

Proof:

# Post without raise_exception=True
❯ python manage.py runserver
Quit the server with CONTROL-C.
Internal Server Error: /api/products/10/
  File "/Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/django_restql/mixins.py", line 843, in bulk_create_many_to_one_related
    obj = serializer.save()
  File "/Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/rest_framework/serializers.py", line 178, in save
    'You cannot call `.save()` on a serializer with invalid data.'
AssertionError: You cannot call `.save()` on a serializer with invalid data.

❯ vi /Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/django_restql/mixins.py
# here I added raise_exception=True to is_valid()

❯ python manage.py runserver
Bad Request: /api/products/10/
[06/Aug/2021 19:52:28] "PATCH /api/products/10/ HTTP/1.1" 400 79

This is the error returned to the api client

{
    "non_field_errors": [
        "The fields ingredient, product must make a unique set."
    ]
}

Ooooh yeah, I didn't realize it, I have tried it and it works, I thought serializer wasn't handling custom constrains, I guess I was wrong, thank your for bearing with me and taking your time to actually run tests.

Now we have to add raise_exeption=True to all is_valid() in both NestedUpdateMixin and NestedCreateMixin.

[sommelon]

And here is the way I've handled that constraint in it's serializer which looks like

class PropertyRatingSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    score = serializers.IntegerField(max_value=5, min_value=0, required=True)
    class Meta:
        model = PropertyRating
        fields = ['id', 'url', 'owner', 'property', 'score']
        read_only_fields = ('owner',)

    def create(self, validated_data):
        """function for creating property's ratings """
        request = self.context.get('request')
        user = request.user

        validated_data.update({"owner": user})
        try:
            return super().create(validated_data)
        except IntegrityError as e:
            raise serializers.ValidationError(
                {"Integrity Error": str(e)},
                "Integrity Error"
            )

    def update(self, instance, validated_data):
        """function for updating property's ratings """
        validated_data.pop("property", None)  # Remove property when updating

        try:
            return super().update(instance, validated_data)
        except IntegrityError as e:
            raise serializers.ValidationError(
                {"Integrity Error": str(e)},
                "Integrity Error"
            )

Below is the error message which API user gets when the constraint is violated(Note it's not server error, it's a normal 400 error coded, bad request response)

{
  "Integrity Error": "duplicate key value violates unique constraint \"api_propertyrating_owner_id_property_id_0d501183_uniq\"\nDETAIL:  Key (owner_id, property_id)=(1, 12) already exists.\n"
}

@yezyilomo Just a heads up (If you know about it, then I'm sorry). You can specify a global exception handler in the settings like this:

REST_FRAMEWORK = {
    'EXCEPTION_HANDLER': 'config.utils.helpers.functions.db_exception_handler',
}

and then catch all Integrity errors and return a custom response. Here is what I have done:

from rest_framework.views import exception_handler

def db_exception_handler(exc, context):
    response = exception_handler(exc, context)
    if isinstance(exc, IntegrityError):
        return Response({'detail': str(exc)}, status.HTTP_409_CONFLICT)

    return response

[sommelon]

Yes that is the other way we were thinking of handling it. I have the same thought as you. BUT, and you can try it with the repository I made. If you add raise_exception to the line I told you. I am not sure why, it returns a nice validation error and not the db integrity error. Proof:

# Post without raise_exception=True
❯ python manage.py runserver
Quit the server with CONTROL-C.
Internal Server Error: /api/products/10/
  File "/Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/django_restql/mixins.py", line 843, in bulk_create_many_to_one_related
    obj = serializer.save()
  File "/Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/rest_framework/serializers.py", line 178, in save
    'You cannot call `.save()` on a serializer with invalid data.'
AssertionError: You cannot call `.save()` on a serializer with invalid data.

❯ vi /Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/django_restql/mixins.py
# here I added raise_exception=True to is_valid()

❯ python manage.py runserver
Bad Request: /api/products/10/
[06/Aug/2021 19:52:28] "PATCH /api/products/10/ HTTP/1.1" 400 79

This is the error returned to the api client

{
    "non_field_errors": [
        "The fields ingredient, product must make a unique set."
    ]
}

Ooooh yeah, I didn't realize it, I have tried it and it works, I thought serializer wasn't handling custom constrains, I guess I was wrong, thank your for bearing with me and taking your time to actually run tests.

Now we have to add raise_exeption=True to all is_valid() in both NestedUpdateMixin and NestedCreateMixin.

Won't adding raise_exception=True to is_valid() in the Create and Update mixins introduce a problem with some objects being created or updated before the exception is thrown?

Let's say I have a serializer

class Serializer(DynamicFieldsMixin, NestedModelSerializer):
    field1 = NestedField(OtherSerializer1)
    field2 = NestedField(OtherSerializer2)

Let's say we are creating the fields in order field1, field2. We create the object in field1 and the field2 violates the unique constraint. It will raise an exception, but the object in field1 was already created.

[yezyilomo]

@yezyilomo Just a heads up (If you know about it, then I'm sorry). You can specify a global exception handler in the settings like this:

REST_FRAMEWORK = {
    'EXCEPTION_HANDLER': 'config.utils.helpers.functions.db_exception_handler',
}

and then catch all Integrity errors and return a custom response. Here is what I have done:

from rest_framework.views import exception_handler

def db_exception_handler(exc, context):
    response = exception_handler(exc, context)
    if isinstance(exc, IntegrityError):
        return Response({'detail': str(exc)}, status.HTTP_409_CONFLICT)

    return response

I didn't know about this, thanks for the heads up, will definitely use it next time.

[yezyilomo]

Won't adding raise_exception=True to is_valid() in the Create and Update mixins introduce a problem with some objects being created or updated before the exception is thrown?

Let's say I have a serializer

class Serializer(DynamicFieldsMixin, NestedModelSerializer):
    field1 = NestedField(OtherSerializer1)
    field2 = NestedField(OtherSerializer2)

Let's say we are creating the fields in order field1, field2. We create the object in field1 and the field2 violates the unique constraint. It will raise an exception, but the object in field1 was already created.

There's this problem already, it happens because when dealing with nested fields(parent-child), things have to be created/updated in order, we start by creating/updating parent first then we go to children, now if we encounter an error in creating/updating a child, there's no way to go back and undo what we have done to the parent or other preceding children.

[gonzaloamadio]

Yes that is the other way we were thinking of handling it. I have the same thought as you. BUT, and you can try it with the repository I made. If you add raise_exception to the line I told you. I am not sure why, it returns a nice validation error and not the db integrity error. Proof:

# Post without raise_exception=True
❯ python manage.py runserver
Quit the server with CONTROL-C.
Internal Server Error: /api/products/10/
  File "/Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/django_restql/mixins.py", line 843, in bulk_create_many_to_one_related
    obj = serializer.save()
  File "/Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/rest_framework/serializers.py", line 178, in save
    'You cannot call `.save()` on a serializer with invalid data.'
AssertionError: You cannot call `.save()` on a serializer with invalid data.

❯ vi /Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/django_restql/mixins.py
# here I added raise_exception=True to is_valid()

❯ python manage.py runserver
Bad Request: /api/products/10/
[06/Aug/2021 19:52:28] "PATCH /api/products/10/ HTTP/1.1" 400 79

This is the error returned to the api client

{
    "non_field_errors": [
        "The fields ingredient, product must make a unique set."
    ]
}

Ooooh yeah, I didn't realize it, I have tried it and it works, I thought serializer wasn't handling custom constrains, I guess I was wrong, thank your for bearing with me and taking your time to actually run tests. Now we have to add raise_exeption=True to all is_valid() in both NestedUpdateMixin and NestedCreateMixin.

Won't adding raise_exception=True to is_valid() in the Create and Update mixins introduce a problem with some objects being created or updated before the exception is thrown?

Let's say I have a serializer

class Serializer(DynamicFieldsMixin, NestedModelSerializer):
    field1 = NestedField(OtherSerializer1)
    field2 = NestedField(OtherSerializer2)

Let's say we are creating the fields in order field1, field2. We create the object in field1 and the field2 violates the unique constraint. It will raise an exception, but the object in field1 was already created.

I think if that if you run inside atomic requests, that won't happen. It will be rolled back.

[gonzaloamadio]

Yes that is the other way we were thinking of handling it. I have the same thought as you. BUT, and you can try it with the repository I made. If you add raise_exception to the line I told you. I am not sure why, it returns a nice validation error and not the db integrity error. Proof:

# Post without raise_exception=True
❯ python manage.py runserver
Quit the server with CONTROL-C.
Internal Server Error: /api/products/10/
  File "/Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/django_restql/mixins.py", line 843, in bulk_create_many_to_one_related
    obj = serializer.save()
  File "/Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/rest_framework/serializers.py", line 178, in save
    'You cannot call `.save()` on a serializer with invalid data.'
AssertionError: You cannot call `.save()` on a serializer with invalid data.

❯ vi /Users/gonzaloamadio/.virtualenvs/django-basic/lib/python3.7/site-packages/django_restql/mixins.py
# here I added raise_exception=True to is_valid()

❯ python manage.py runserver
Bad Request: /api/products/10/
[06/Aug/2021 19:52:28] "PATCH /api/products/10/ HTTP/1.1" 400 79

This is the error returned to the api client

{
    "non_field_errors": [
        "The fields ingredient, product must make a unique set."
    ]
}

Ooooh yeah, I didn't realize it, I have tried it and it works, I thought serializer wasn't handling custom constrains, I guess I was wrong, thank your for bearing with me and taking your time to actually run tests. Now we have to add raise_exeption=True to all is_valid() in both NestedUpdateMixin and NestedCreateMixin.

Won't adding raise_exception=True to is_valid() in the Create and Update mixins introduce a problem with some objects being created or updated before the exception is thrown?

Let's say I have a serializer

class Serializer(DynamicFieldsMixin, NestedModelSerializer):
    field1 = NestedField(OtherSerializer1)
    field2 = NestedField(OtherSerializer2)

Let's say we are creating the fields in order field1, field2. We create the object in field1 and the field2 violates the unique constraint. It will raise an exception, but the object in field1 was already created.

If the excpetion is not raised in the serializer validation, wont it be raised somewhere else anyway? At db level.. so it does not matter where it is raised, first object would be created anyway?

[yezyilomo]

If the excpetion is not raised in the serializer validation, wont it be raised somewhere else anyway? At db level.. so it does not matter where it is raised, first object would be created anyway?

Yeah, this is true, even if you don't add raise_exeption=True the exception will be raised the difference is that with raise_exeption=True you get ValidationError, and without it the serializer crushes.

[yezyilomo]

I think if that if you run inside atomic requests, that won't happen. It will be rolled back.

I have confirmed everything is rolled back if one step fails, You can use 'ATOMIC_REQUESTS': True, in DB config

[sommelon]

If the excpetion is not raised in the serializer validation, wont it be raised somewhere else anyway? At db level.. so it does not matter where it is raised, first object would be created anyway?

Yes, It would be raised at the DB level, but, assuming devs log server errors, it would be apparent that something went wrong and devs could act accordingly. If a validation error is thrown, then it wouldn't be so apparent.

[sommelon]

I think if that if you run inside atomic requests, that won't happen. It will be rolled back.

I have confirmed everything is rolled back if one step fails, You can use 'ATOMIC_REQUESTS': True, in DB config

I'm kinda hesitant to do this. I haven't done any profiling, but in django docs they say this:

Warning

While the simplicity of this transaction model is appealing, it also makes it inefficient when traffic increases. Opening a transaction for every view has some overhead. The impact on performance depends on the query patterns of your application and on how well your database handles locking.

[yezyilomo]

Yes, It would be raised at the DB level, but, assuming devs log server errors, it would be apparent that something went wrong and devs could act accordingly. If a validation error is thrown, then it wouldn't be so apparent.

I think when API user sends data which violates DB constraint that’s not devs fault, devs job is to make sure when something like this happen they tell API users what they have done wrong so that they can fix their input.

[yezyilomo]

I'm kinda hesitant to do this. I haven't done any profiling, but in django docs they say this:

Warning

While the simplicity of this transaction model is appealing, it also makes it inefficient when traffic increases. Opening a transaction for every view has some overhead. The impact on performance depends on the query patterns of your application and on how well your database handles locking.

That’s true, this feature should be used with caution, but apart from this way I can’t think of another way to undo changes if one step fails. I think people should treat creating/updating nested fields as individuals requests, even if this lib gives you the power to combine them in a single request.

[sommelon]

Yes, It would be raised at the DB level, but, assuming devs log server errors, it would be apparent that something went wrong and devs could act accordingly. If a validation error is thrown, then it wouldn't be so apparent.

I think when API user sends data which violates DB constraint that’s not devs fault, devs job is to make sure when something like this happen they tell API users what they have done wrong so that they can fix their input.

I see your point. Maybe it's fine to use raise_exception=True, but shouldn't we figure out why does the UniqueTogetherValidator not run in the first step of validation (in BaseNestedFieldSerializer) and runs in the second step (in create/update mixins)?