search

closeio / cleancat

Validation and serialization library for Python designed to be used with JSON REST frameworks
MIT License
52 stars 8 forks source link

Fix serializing None values #29

Closed wojcikstefan closed 6 years ago

wojcikstefan commented 6 years ago

Before this PR, serializing an object could fail even if its data was clean. For example, if you had an optional DateTime field and tried to serialize {'dt_field_name': None}, you'd get AttributeError: 'NoneType' object has no attribute 'isoformat'. That's because the overridden DateTime.serialize method expects a valid datetime.datetime value: https://github.com/closeio/cleancat/blob/48b99263e89e7909d305e8d861e6098fdba6476d/cleancat/base.py#L177-L178

All the other overridden serialize methods (e.g. List.serialize, Enum.serialize, etc.) follow the same pattern. I figured this is an issue we can tackle generically and never call field.serialize(None).

An alternative would be to fix the overridden methods by prefixing them all with if value is None: return, but I think that would lead to a lot of copy-pasting as we create/override more serialize methods in the future.

thomasst commented 6 years ago

Is there ever a case where we'd want e.g. empty lists to be serialized as [] instead of None?

tsx commented 6 years ago

We could have another overridable method like serialize_empty?

wojcikstefan commented 6 years ago

I'd try to avoid such a method, especially since "empty" might mean many different things (e.g. people might expect [], None, '', {}, etc. to be treated as empty). Having to guess/check documentation to figure out which serialization method a given value will be piped through doesn't sound great.

I guess @thomasst has a point here... In order to make serialization completely flexible and remove as much "magic" (aka special implicit behavior) as possible, we may want to treat all values as equal and always pipe them through field.serialize... We lose some DRYness, but now that I've pondered it some more I think it's a fair price to pay.

Agreed @thomasst @tsx ?

thomasst commented 6 years ago

That works for me.

tsx commented 6 years ago

I don't care too strongly. Although there's one more related topic to think about: required constraint, which is also tied to the definition of empty-ness and is taken care of at the generic level with an extra overridable method has_value.

wojcikstefan commented 6 years ago

Yea, for a while I thought about only serializing values for which field.has_value returns True, and either passing the others as-is or hard-setting them to None, but I now believe it would only make things more confusing.