encode / django-rest-framework

Web APIs for Django. 🎸
https://www.django-rest-framework.org
Other
28.26k stars 6.82k forks source link

Missing default value from ModelSerializer #2683

Open duduklein opened 9 years ago

duduklein commented 9 years ago

When creating a serializer from a model, the default value is not included in the serializer. I thought it was a bug and I was going to try to fix it and make a pull request, but I ran into the test suite and may be this is the expected behavior.

In the file django-rest-framework/tests/test_model_serializer.py, the FieldOptionsModel has a field declared as default_field = models.IntegerField(default=0).

In the test TestRegularFieldMappings.test_field_options, the expected serializer should contain default_field = IntegerField(required=False), which does not include the default value of 0.

I would expect to have default_field = IntegerField(default=0) (which would also imply required=False) or default_field = IntegerField(default=0, required=False)

Is this the expected behavior or a bug?

tomchristie commented 9 years ago

The default is set by the model, so the serializer can simply set use required=False and not pass a value.

duduklein commented 9 years ago

Thanks for your quick response. This is true but it's annoying to generate the documentation. When using django-rest-swagger, for example, if the default value is not passed to the serializer, swagger can not have acces to it.

I understand that funcionality-wise, this maybe useless as you pointed out, but for documentation it is not.

Can you please reconsider it? I can't see any disadvantage. If you agree with me, just let me know what do you prefer (IntegerField(default=0) or IntegerField(default=0, required=False)) and I'll make a pull request.

Thanks in advance.

grjones commented 9 years ago

:+1: on reopening this. I'm using a custom metadata class that produces JSON Schema. It would be very helpful to be able to set a default value, especially when using something like Angular Schema Form for form generation.

tomchristie commented 9 years ago

I'd consider reopening this but only in the face of an example PR and solid proposal.

Model defaults are model defaults. The serializer should omit the value and defer responsibility to the Model.object.create() to handle this. As things currently stand makes sense to be, but open to further review if someone takes it on seriously.

birdonfire commented 8 years ago

For what it's worth, DRF 2.x actually used the default values set on the model fields. DRF stopped using them as of 3.0.x.

This actually bit me when upgrading our app, because we have some use cases where the serializer gets initialized with a null instance::

from django.db import models
from rest_framework import serializers

class Progress(models.Model):
    visits = models.PositiveIntegerField(default=0)
    completed = models.BooleanField(default=False)
    started_time = models.DateTimeField(blank=True, null=True)
    finished_time = models.DateTimeField(blank=True, null=True)
    last_visit = models.DateTimeField(blank=True, null=True)

class ProgressSerializer(serializers.ModelSerializer):
    class Meta:
        model = Progress
        fields = (
            'visits',
            'completed',
            'started_time',
            'finished_time',
            'last_visit'
        )

# Instantiate the serializer with a null instance
serializer = ProgressSerializer(None)

# Expect the serialized data to use the defaults defined in the model
expected = {
    'visits': 0,
    'completed': False,
    'started_time': None,
    'finished_time': None,
    'last_visit': None
}
assert expected == serializer.data

# What we will actually get...
actual = {
    'visits': None,
    'completed': False,
    'started_time': None,
    'finished_time': None,
    'last_visit': None
}

The same thing would happen if you deserialized data that is missing the field.

On the one hand, it's sorta nice that the serializer reflects the data that was actually passed to it. On the other, it feels out of sync with the model; if we'd instantiated the model class with the same data, we'd see the default value.

cristianoccazinsp commented 2 years ago

I've faced this issue as well.

In my use case, it is related to UniqueTogetherValidator. The validator makes all fields required by default, which is not 100% correct. What if we have part of the unique_together constraint to be charfield, and the charfield defaults to an empty string? The validator should default to the empty string (or w/e the model default is) if the data is not available, otherwise, clients must always send the default value, which is annoying.

mangelozzi commented 2 years ago

On the one hand, it's sorta nice that the serializer reflects the data that was actually passed to it. On the other, it feels out of sync with the model; if we'd instantiated the model class with the same data, we'd see the default value.

Do you know why it was removed from DRF 3.0? I'm sure they had a good reason. To me it is surprising behaviour:

from django.db import models
from rest_framework import serializers

class Foo(models.Model)
    time_is_null = models.BooleanField(default=True, blank=True)

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

print(FooSerializer().data)  # {'time_is_null': False}    <--- would expect True

The serializer will initialise the time_is_null value as False.

alexburner commented 1 year ago

Another 👍 on reopening this. I'm facing a similar issue as this commenter: https://github.com/encode/django-rest-framework/issues/2683#issuecomment-169162906

We're trying to instantiate a serializer with a subset of the overall data, run some operations based on that data + model defaults, and then save the final result.

We found that we CAN get the "complete" data with default values — if we call .save() first. So, our current workaround is to call that twice. But this is clunky, as we also need to fetch the resulting model object and pass it in to the second save, to avoid an "id already exists" error:

try:
    # initial serialization + save for complete data
    serializer = MySerializer(data=request.data)
    serializer.is_valid(raise_exception=True)
    serializer.save()
    complete_data = serializer.data
    # operate on complete data
    updated_data = do_stuff(complete_data)
    # save updated data
    object = MyModel.objects.get(id=updated_data.get("id"))
    serializer = MySerializer(object, data=updated_data)
    serializer.is_valid(raise_exception=True)
    serializer.save()
    return Response(serializer.data, status=status.HTTP_200_OK)
except ValidationError:
    return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
george-miller commented 1 year ago

Its ridiculous to say Model.object.create() should handle this, especially when it was working in 2.x and people relied on it. Sometimes I need to parse data without immediately saving it to the database, in my case its a viz app like Grafana and a user is working on the unpersisted Query in the UI and they want to test out what a run of that Query would look like before saving it. Anyways, here's the shitty solution that we can use until they implement it for real.

class MyModelSerializer(ModelSerializer):
  class Meta:
    model = MyModel
    extra_kwargs = {'field_with_default': {'default': MyModel.field_with_default.field.default}}
george-miller commented 1 year ago

ok here's an even better solution for now

from django.db.models.fields import NOT_PROVIDED
class ModelSerializerDefaultsMixin():
  def __init__(self, *args, **kwargs):
    extra_kwargs = {}
    for field in self.Meta.model._meta.fields:
      if field.default != NOT_PROVIDED:
        extra_kwargs[field.name] = {'default': field.default}
    self.Meta.extra_kwargs = extra_kwargs
    super(ModelSerializerDefaultsMixin, self).__init__(*args, **kwargs)

class MyModelSerializer(ModelSerializerDefaultsMixin, ModelSerializer):
  class Meta:
    model = MyModel
shaleh commented 8 months ago

This is still frustrating. https://github.com/encode/django-rest-framework/issues/2683#issuecomment-956375654 is a use case that hurts. I have a model with several fields that are unique together but have sensible defaults. Those defaults are not applied. Works just fine in pure Django however serializers are not happy.

Like the Mixin suggestion above, the plan is to force the consumption of those defaults.....

+    def to_internal_value(self, data):
+        # Fun times. Because the model defines a unique together on the fields
+        # the generated validator for DRF forces all of the fields that are unique
+        # together to have values but ignores the defaults.... so this does the work
+        # DRF should be doing.
+        for name, field in self.fields.items():
+            if field.default == empty:
+                continue
+            if not (name in data or getattr(self.instance, name, None)):
+                data[name] = field.default
+
+        return super().to_internal_value(data)
auvipy commented 7 months ago

I would be happy to review a PR addressing this issue

shaleh commented 7 months ago

@auvipy I would be happy to post a PR. Is my approach sensible? Or should https://github.com/encode/django-rest-framework/issues/2683#issuecomment-1599329137 be explored? It did not work for me but that could have been a mistake I made.

auvipy commented 7 months ago

I can't say anything definitive right now. I would like to discuss further on a PR where we can iterate over several approach. and of course with good amount of usecase examples as test cases

mangelozzi commented 5 months ago

Do forms not use the defaults from the models? Are serializers not similar to forms? If it makes sense for forms to use the defaults, especially when provided default objects for creation, why does it not make sense for serializers to do same.

A solid example is when making a create object interface, and you wish to serialize a new "blank" object.

hitiksha-patel commented 3 months ago

In Django REST Framework (DRF), when a model field has a default value, the serializer does not automatically include this default value. Instead, the default is applied by the model during save operations.

Example For a model with a default value:

class FieldOptionsModel(models.Model):
    default_field = models.IntegerField(default=0)

The serializer:

class FieldOptionsModelSerializer(serializers.ModelSerializer):
    class Meta:
        model = FieldOptionsModel
        fields = '__all__'

Results in:

default_field = serializers.IntegerField(required=False)

Why This Happens Model-Level Default: The model handles default values when saving data. Serializer Validation: The serializer focuses on validating the provided input data.

Customizing Serializer To explicitly show the default value in the serializer:

class FieldOptionsModelSerializer(serializers.ModelSerializer):
    default_field = serializers.IntegerField(default=0, required=False)
    class Meta:
        model = FieldOptionsModel
        fields = '__all__'

This behavior is expected. Serializers validate input data, while models handle default values during save operations.

When creating a serializer from a model, the default value is not included in the serializer. I thought it was a bug and I was going to try to fix it and make a pull request, but I ran into the test suite and may be this is the expected behavior.

In the file django-rest-framework/tests/test_model_serializer.py, the FieldOptionsModel has a field declared as default_field = models.IntegerField(default=0).

In the test TestRegularFieldMappings.test_field_options, the expected serializer should contain default_field = IntegerField(required=False), which does not include the default value of 0.

I would expect to have default_field = IntegerField(default=0) (which would also imply required=False) or default_field = IntegerField(default=0, required=False)

Is this the expected behavior or a bug?