Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3.15 is raising required error on model nullable fields #9378

Open
Enorio opened this issue Apr 11, 2024 · 19 comments
Open

3.15 is raising required error on model nullable fields #9378

Enorio opened this issue Apr 11, 2024 · 19 comments

Comments

@Enorio
Copy link

Enorio commented Apr 11, 2024

I have a model with nullable fields. Upgrading from 3.14 to 3.15 I'm getting this error:
AssertionError: You cannot call .save() on a serializer with invalid data.

When I print the serializer.errors after serializer.is_valid(), it says those fields are required. Is it possible that this is a bug, or do I have to manually override the field in the serializer to have the required=False kwarg?

@sevdog
Copy link
Contributor

sevdog commented Apr 23, 2024

Could you provide a code sample of your model, serializer and data for checking?

@Enorio
Copy link
Author

Enorio commented May 21, 2024

class Foo(...):
    ...
    value = models.IntegerField(null=True)

class FooSerializer(serializers.ModelSerializer):
    class Meta:
        model = Foo
        fields = '__all__'


serializer = FooSerializer(data=data) # This data does not have the "value" field
serializer.is_valid()
saved_data = serializer.save()

Raises this error AssertionError: You cannot call .save() on a serializer with invalid data.

@sevdog
Copy link
Contributor

sevdog commented May 22, 2024

The value field does not provide any default, so it is expected to fail if no value for that field is provided.

The real issue would be how did that ever worked without a default in the first place?
It should have worked if you had default=None or blank=True on your model's field (but I am not confident in the latter alone would work).

Usually a nullable field which is not required in forms is defined as:

class Foo(...):
    ...
    value = models.IntegerField(null=True, default=None, blank=True)

@Enorio
Copy link
Author

Enorio commented May 22, 2024

I'm using a django v3.y.z for now, don't know if that's the problem.
I have a lot of values with null=True without the default kwarg and never had a problem in the serializer. My guess is if you have only null, this counts as default=None?

@sevdog
Copy link
Contributor

sevdog commented May 22, 2024

My guess is if you have only null, this counts as default=None?

It is not documented nor in the codebase of django such behaviour for null or default. Django and the majority of DBMS does not suppose the default based on the assumption that a field is nullable.

I can only guess that there is something else which is providing a value for those fields in your project (a middleware? the frontend? custom views? custom serializers? custom base models?).

@Enorio
Copy link
Author

Enorio commented May 22, 2024

I don't think so, what I have it's pretty standard.
I'll look better into it and check the differences between these 2 DRF versions.

@BPC-AnonymousBeast
Copy link

I don't think so, what I have it's pretty standard. I'll look better into it and check the differences between these 2 DRF versions.

I tried to reproduce the error you are getting.
I have pretty much done what you have done. Infact, inspired by another similar issue, I have also added a foreign key constraint. I'm able to get back null even if I don't add that field in the body of the post request.

 class Color(models.Model):
      color_name = models.CharField(max_length=100)
  
      def __str__(self) -> str:
          return self.color_name

  class Person(models.Model):
      name = models.CharField(max_length=100)
      lastname = models.CharField(max_length=100,null=True)
      age = models.IntegerField(null=True)
      color = models.ForeignKey(Color, on_delete=models.CASCADE,related_name= 'color', null=True)
  class PeopleSerialiser(serializers.ModelSerializer):
      # my_id = serializers.PrimaryKeyRelatedField()
  
      class Meta:
          model = Person
          fields = "__all__"
@api_view(['GET','POST'])
 def person(request):
     if request.method == 'GET':
         objs = Person.objects.all()
         serializer = PeopleSerialiser(objs, many = True)
         
         return Response(serializer.data)
image

If you want to make some changes to my code and try again to reproduce the error. Please let me know.
Note : I'm using djangorestframework 3.15.1 : -

image

@Enorio
Copy link
Author

Enorio commented Jul 4, 2024

Can you try your view with the is_valid(), like this? Didn't try your code, but my error comes from the is_valid() method, that adds info to the self._errors

@api_view(['GET','POST'])
 def person(request):
     if request.method == 'GET':
         objs = Person.objects.all()
         serializer = PeopleSerialiser(objs, many = True)
         serializer.is_valid(raise_exception=True)
         
         return Response(serializer.data)

@BPC-AnonymousBeast
Copy link

serializer.is_valid(raise_exception=True)

Adding this line to my code would not make any difference but I added that line to the POST view. There is no change in the output.
Following is the view and output :

@api_view(['GET','POST'])
def person(request):
    if request.method == 'GET':
        objs = Person.objects.all()
        serializer = PeopleSerialiser(objs, many = True)

        return Response(serializer.data)
    
    else:
        data = request.data
        serializer = PeopleSerialiser(data=data)
        if serializer.is_valid(raise_exception=True):
            serializer.save()
            return Response(serializer.data)
        
        return Response(serializer.errors)
image

Postman output :
image

Feel free to let me know if anything!!

@Enorio
Copy link
Author

Enorio commented Jul 8, 2024

Ok, I apologize because the example that I posted was not complete, it was missing a uniqueConstraint 😅, I think its the same case as #9410

I've created a new project with your example to validate this.

Models

class Color(models.Model):
    color_name = models.CharField(max_length=100)

    class Meta:
        verbose_name = "Color"
        verbose_name_plural = "Colors"
        ordering = ('id',)

class Person(models.Model):
    name = models.CharField(max_length=100)
    lastname = models.CharField(max_length=100, null=True)
    age = models.IntegerField(null=True)
    color = models.ForeignKey(Color, on_delete=models.CASCADE, related_name='color', null=True)

    class Meta:
        verbose_name = "Person"
        verbose_name_plural = "Persons"
        ordering = ('id',)
        constraints = [models.UniqueConstraint(name='unique_constraint', fields=('color', 'age'), )]

Serializer

class ColorSerializer(serializers.ModelSerializer):
    class Meta:
        model = Color
        fields = '__all__'

class PersonSerializer(serializers.ModelSerializer):
    class Meta:
        model = Person
        fields = "__all__"

Viewset

class DRFBugViewset(viewsets.ModelViewSet):
    serializer_class = PersonSerializer

With DRF 3.14.0

image

With DRF 3.15.1

image

@rohanahata
Copy link

I am facing the same issue as @Enorio

@sevdog
Copy link
Contributor

sevdog commented Jul 11, 2024

If everyone here is having this issue when there are UniqueConstraints this may be a side effect of #7438 because the constraint adds a UniqueTogetherValidator for given fields and thus the validator is forcing those fields to be required.

@Enorio
Copy link
Author

Enorio commented Jul 11, 2024

If everyone here is having this issue when there are UniqueConstraints this may be a side effect of #7438 because the constraint adds a UniqueTogetherValidator for given fields and thus the validator is forcing those fields to be required.

So basically I have to set a default value in the given fields to "bypass" the constraint added?

@lucasreveal
Copy link

lucasreveal commented Aug 22, 2024

Ended up doing this

from django_json_api.rest_framework import ModelSerializer


class LegacyDRFModelSerializer(ModelSerializer):
    def get_validators(self):
        # If the validators have been declared explicitly then use that.
        validators = getattr(getattr(self, "Meta", None), "validators", None)
        if validators is not None:
            return list(validators)

        # Otherwise use the default set of validators.
        return self.get_unique_for_date_validators()

Then using LegacyDRFModelSerializer instead of ModelSerializer in affected serializers

@browniebroke
Copy link
Contributor

browniebroke commented Sep 11, 2024

I tried to take the example from @Enorio earlier and I can reporduce the problem.

However, if I change it as follows (which I think means the same thing, from Django perspective):

  class Color(models.Model):
      color_name = models.CharField(max_length=100)
  
  class Person(models.Model):
      name = models.CharField(max_length=100)
      lastname = models.CharField(max_length=100, null=True)
      age = models.IntegerField(null=True)
      color = models.ForeignKey(Color, on_delete=models.CASCADE, related_name='color', null=True)
  
      class Meta:
-         constraints = [models.UniqueConstraint(name='unique_constraint', fields=('color', 'age'))]
+         unique_together = ('color', 'age')

Then it fails on both 3.14 and 3.15: #9532. This is because Meta.contraints were completely ignored by DRF 3.14 and are now treated by 3.15+ as unique_together.

From what I can tell, it's a change of behaviour due to adding support for a newer Django syntax.

@znotdead
Copy link

I tried to take the example from @Enorio earlier and I can reporduce the problem.

However, if I change it as follows (which I think means the same thing, from Django perspective):

  class Color(models.Model):
      color_name = models.CharField(max_length=100)
  
  class Person(models.Model):
      name = models.CharField(max_length=100)
      lastname = models.CharField(max_length=100, null=True)
      age = models.IntegerField(null=True)
      color = models.ForeignKey(Color, on_delete=models.CASCADE, related_name='color', null=True)
  
      class Meta:
-         constraints = [models.UniqueConstraint(name='unique_constraint', fields=('color', 'age'))]
+         unique_together = ('color', 'age')

Then it fails on both 3.14 and 3.15: #9532. This is because Meta.contraints were completely ignored by DRF 3.14 and are now treated by 3.15+ as unique_together.

From what I can tell, it's a change of behaviour due to adding support for a newer Django syntax.

According to docs unique_together can be deprecated later and it is recommended to use constraints:

Use UniqueConstraint with the constraints option instead.

UniqueConstraint provides more functionality than unique_together. unique_together may be deprecated in the future.

@browniebroke
Copy link
Contributor

According to docs unique_together can be deprecated later and it is recommended to use constraints

Yes, preceisely. They were designed as a more powerful replacement, and DRF handles constraints the same way as unique_together.

@znotdead
Copy link

According to docs unique_together can be deprecated later and it is recommended to use constraints

Yes, preceisely. They were designed as a more powerful replacement, and DRF handles constraints the same way as unique_together.

Except it doesn't support conditional constraints + cases where field is set just before save or based on other data not in request (i.e. defined in pre_save signal).

Workaround for this now is to add in Meta of serializer next:

        extra_kwargs = {
            # By default "unique together" validation enforces that all fields be required=True
            # field1 is conditional unique
            # field2 can be set in pre_save
            'field1': {'required': False},
            'field2': {'required': False},
        }
        validators = []  # Remove a default "unique together" constraint

So had to turn off as DRF does NOT handles constraints the same way.

@browniebroke
Copy link
Contributor

Yes I agree conditional contraints aren't well supported, and I think there is another more appropriate issue for that case (#9358).

The cases reported so far in here aren't making use of conditional constraints however, and without conditions I was only saying that the behaviour is consistent with DRF 3.14 unique_together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants