oxan / djangorestframework-dataclasses

Dataclasses serializer for Django REST framework
BSD 3-Clause "New" or "Revised" License
431 stars 28 forks source link

Optional string field causing TypeError during validation. #32

Closed pyarun closed 3 years ago

pyarun commented 3 years ago

Probably I am not using dataclass properly as this looks like a simple issue, here is my code

@dataclasses.dataclass
class OrderItem(TypedJsonMixin):
    product_id: typing.Union[uuid.UUID, str]
    unit_price: float
    quantity: int
    product_type: str
    discount: float
    status: typing.Optional[str] = ""
    description: typing.Optional[str] = None
    message: typing.Optional[str] = None  # Order status message.

class ItemSerializer(DataclassSerializer):
    event = serializers.SerializerMethodField()
    product_id = serializers.UUIDField(required=True)

    class Meta:
        dataclass = OrderItem
        # fields = "__all__"
        read_only_fields = ("description", "message")
        extra_kwargs = {
            # "product_id": {"max_length": 36, "required": True},
            "product_type": {"required": True},
            "status": {"default": "in-cart"},
        }

Now while creating an instance of ItemSerializer, I am not passing description and message.

data = {
          "product_id": "a503e2e0-ce0e-4556-bb58-08d4f99f199e",
          "product_type": "public-event",
          "unit_price": 500,
          "quantity": 1,
          "discount": 10
        }
s = ItemSerializer(data=data)
s.is_valid()

I am getting the following error, as empty_values is rest_framework.fields.empty

Error

serializer.is_valid(raise_exception=True)
  File "/Users/arun/.local/share/virtualenvs/djZipDate-ChKVyfQ3/lib/python3.8/site-packages/rest_framework/serializers.py", line 234, in is_valid
    self._validated_data = self.run_validation(self.initial_data)
  File "/Users/arun/.local/share/virtualenvs/djZipDate-ChKVyfQ3/lib/python3.8/site-packages/rest_framework/serializers.py", line 433, in run_validation
    value = self.to_internal_value(data)
  File "/Users/arun/.local/share/virtualenvs/djZipDate-ChKVyfQ3/lib/python3.8/site-packages/rest_framework/serializers.py", line 490, in to_internal_value
    validated_value = field.run_validation(primitive_value)
  File "/Users/arun/.local/share/virtualenvs/djZipDate-ChKVyfQ3/lib/python3.8/site-packages/rest_framework/serializers.py", line 621, in run_validation
    value = self.to_internal_value(data)
  File "/Users/arun/.local/share/virtualenvs/djZipDate-ChKVyfQ3/lib/python3.8/site-packages/rest_framework/serializers.py", line 657, in to_internal_value
    validated = self.child.run_validation(item)
  File "/Users/arun/.local/share/virtualenvs/djZipDate-ChKVyfQ3/lib/python3.8/site-packages/rest_framework/serializers.py", line 433, in run_validation
    value = self.to_internal_value(data)
  File "/Users/arun/.local/share/virtualenvs/djZipDate-ChKVyfQ3/lib/python3.8/site-packages/rest_framework_dataclasses/serializers.py", line 531, in to_internal_value
    instance = dataclass_type(**native_values, **empty_values)
  File "<string>", line 11, in __init__
  File "/Users/arun/.local/share/virtualenvs/djZipDate-ChKVyfQ3/lib/python3.8/site-packages/typed_json_dataclass/typed_json_dataclass.py", line 67, in __post_init__
    raise TypeError((f'{class_name}.{field_name} was '
TypeError: OrderItem.description was defined to be any of: (<class 'str'>, <class 'NoneType'>) but was found to be <class 'type'> instead

I read some existing issues about optional fields, but do not see a solution.

oxan commented 3 years ago

You shouldn't pass the data you want to deserialize as keyword arguments to the serializer, but as the data parameter.

oxan commented 3 years ago

The error you get seems to be because TypedJsonMixin does type validation on the values of a dataclass instance, and that conflicts with our handling of non-supplied values. I'd suggest to ditch the TypedJsonMixin, since the serializer does validation already.

pyarun commented 3 years ago

You shouldn't pass the data you want to deserialize as keyword arguments to the serializer, but as the data parameter.

Changed the example code according to feedback. It was a mistake while writing code sample here. Sorry about it!

I went on to remove TypedJsonMixin, and this error is resolved. But TypedJsonMixin is giving 2 useful methods to_json and to_dict which are used in many places!

After serializer validation, we are getting OrderItem instance in def create(self, validated_data) method, which is awesome. But after all processing when i have to save this data in django.contrib.postgres.fields.jsonb.JSONField i need to convert OrderItem instance to a dict where to_dict is very useful.

Still I replaced TypedJsonMixin with https://pypi.org/project/dataclasses-json/ which is a subset of TypedJson and do not deal with type conversion and validation.

order_item.to_json()    # order_item is OrderItem instance

We still have problem as OrderItem.description has value of rest_framework.fields.empty which cannot be converted to JSON and fails with

TypeError: Object of type type is not JSON serializable

image

oxan commented 3 years ago

I went on to remove TypedJsonMixin, and this error is resolved. But TypedJsonMixin is giving 2 useful methods to_json and to_dict which are used in many places!

FYI Python has a native method to convert dataclasses to dicts: https://docs.python.org/3/library/dataclasses.html#dataclasses.asdict.

We still have problem as OrderItem.description has value of rest_framework.fields.empty which cannot be converted to JSON and fails with

TypeError: Object of type type is not JSON serializable

You can call _strip_empty_sentinels() to get rid of those empty values. However, as that method is still internal (for now) and actually has a different signature in master than in v0.8, it's probably a better idea to do a super-call to the create() or update() methods:

   def create(self, validated_data):
      order_item = super(MyCustomSerializerName, self).create(validated_data)
      order_item.to_json()
oxan commented 3 years ago

I don't think there's anything actionable remaining here, so I'll close this issue for now.