oxan / djangorestframework-dataclasses

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

Validation error with "" in string lists. #30

Closed tolomea closed 3 years ago

tolomea commented 3 years ago

I get a validation error when passing "values": [""] into values: List[Optional[str]], "ErrorDetail(string='This field may not be blank.'", "code='blank')

Also is there a way to express list can't be empty?

oxan commented 3 years ago

This happens because the allow_blank option of CharField is False by default (see https://www.django-rest-framework.org/api-guide/fields/#charfield). You can workaround this by setting that option (either with the extra_kwargs mechanism or by explicitly specifying the field), or replacing the CharField class in the fieldmapping with a subclass that defaults allow_blank to True.

Also is there a way to express list can't be empty?

Same issue, you have to set the allow_empty option for ListField explicitly.

I'm closing this issue, as the serializer will never be able to handle constraints (such as these) that aren't expressable in Pythons type system without explicit configuration. I'm not interested in inventing a meta-language to express them, there's other libraries already that do that better.

tolomea commented 3 years ago

replacing the CharField class in the fieldmapping thanks I will give that a shot

I'm closing this issue, as the serializer will never be able to handle constraints (such as these) that aren't expressable in Pythons type system without explicit configuration.

That makes sense.

If you manage to get dataclasses in serializer_field_mapping working sometime that would make the explicit configuration much easier in cases using generated structures.

tolomea commented 3 years ago

I take it that serializer_field_mapping doesn't carry into nested Dataclasses.

It would be handy if this line https://github.com/oxan/djangorestframework-dataclasses/blob/6b8496386db6e045742737bd88ae5c34d237eda5/rest_framework_dataclasses/serializers.py#L439 used a class level property for the serializer class. Obviously I can override the whole function, but that increases my exposure to future breaking changes on your side.

oxan commented 3 years ago

If you manage to get dataclasses in serializer_field_mapping working sometime that would make the explicit configuration much easier in cases using generated structures.

That should be relatively easy using something like this (untested) here:

field_class = self.serializer_field_mapping[type_info.base_type] if type_info.base_type in self.serializer_field_mapping else DataclassSerializer

I'll try to get this merged (and the other changes I suggested in your issues) soon.

It would be handy if this line https://github.com/oxan/djangorestframework-dataclasses/blob/6b8496386db6e045742737bd88ae5c34d237eda5/rest_framework_dataclasses/serializers.py#L439 used a class level property for the serializer class.

Yes, I agree, and I already tried that. However, Python doesn't allow you to reference the class itself in the class body, because the class isn't defined until after the body is evaluated.

tolomea commented 3 years ago

Ah, that's annoying, I guess you could have a one line function that does only that, having only one very simple purpose it'd be less likely to change over time.

Maybe you could walk up the MRO and find the first thing with no Meta, but I'm not sure about the extra complexity.

intgr commented 3 years ago

I've thought about this a bit. It would be helpful and much cleaner if one could pass custom field kwargs overrides using dataclass field(metadata=...), e.g.:

@dataclass
class Example:
    string: str = field(metadata={"drf_field_args": {"allow_blank": True}})
    lst: List[str] = field(metadata={"drf_field_args": {"child": serializers.CharField(allow_blank=True)}})

Does this seem like a mechanism you want to support @oxan?

intgr commented 3 years ago

Or perhaps a metadata key that allows overriding the field as a whole

@dataclass
class Example:
    string: str = field(metadata={"drf_field": serializers.CharField(allow_blank=True)})
    lst: List[str] = field(metadata={
        "drf_field": serializers.ListField(child=serializers.CharField(allow_blank=True))
    })
oxan commented 3 years ago

Ah, that's annoying, I guess you could have a one line function that does only that, having only one very simple purpose it'd be less likely to change over time.

Maybe you could walk up the MRO and find the first thing with no Meta, but I'm not sure about the extra complexity.

I'll just create the property using the @property decorator.

I've thought about this a bit. It would be helpful and much cleaner if one could pass custom field kwargs overrides using dataclass field(metadata=...), e.g.:

Does this seem like a mechanism you want to support @oxan?

Yeah, I also thought about that, but not sure whether I like it. I'll have to think about it.

oxan commented 3 years ago

If you manage to get dataclasses in serializer_field_mapping working sometime that would make the explicit configuration much easier in cases using generated structures.

This landed in 175e210.

I take it that serializer_field_mapping doesn't carry into nested Dataclasses.

Side note: it does if you change the dict on DataclassSerializer directly.

It would be handy if this line used a class level property for the serializer class.

This landed in 8d3038b.

I've thought about this a bit. It would be helpful and much cleaner if one could pass custom field kwargs overrides using dataclass field(metadata=...), e.g.:

I've split this out to #31.

tolomea commented 3 years ago

Side note: it does if you change the dict on DataclassSerializer directly.

yes, but if one day I picked up a library that uses this that would create a likely very obscure bug