Pylons / colander

A serialization/deserialization/validation library for strings, mappings and lists.
https://docs.pylonsproject.org/projects/colander/en/latest/
Other
447 stars 145 forks source link

Boolean type should fallback to False instead of True!!! #250

Open daveoncode opened 8 years ago

daveoncode commented 8 years ago

If I deserialize a dictionary containing None as value for a boolean node, Colander falls back to True, which is absolutely wrong... a default for a boolean must always be False by definition. I currently subclassed Boolean in this way to make it work as expected:

class SmartBoolean(Boolean):
    def deserialize(self, node, cstruct):
        if cstruct in (null, None, ''):
            return null
        return super().deserialize(node, cstruct)

But I hope you will consider to replace True with False in the last line of original Boolean deserialize() method. Thanks!

digitalresistor commented 8 years ago

Please create a PR with tests and sign CONTRIBUTORS.txt if you haven't, and we can pull it into colander!

mmerickel commented 8 years ago

a default for a boolean must always be False by definition

Not to be too pedantic but I don't think that's actually in the definition of a boolean. I can't find anything about what the default should be on wikipedia. Most values actually default to True exception 0, None and a few empty collections in python.

ANYWAY I'm not against changing this, but someone who cares (you) will need to submit a PR with tests as Bret said. :-)

tisdall commented 8 years ago

Looking at the source for Boolean it looks like you can define how you want to serialize/deserialize values. It says:

The behaviour for values not contained in :attr:`false_choices`
depends on :attr:`true_choices`: if it's empty, any value is considered
``True``; otherwise, only values contained in :attr:`true_choices`
are considered ``True``, and an Invalid exception would be raised
for values outside of both :attr:`false_choices` and :attr:`true_choices`.

So, because the default for false values is 'false' or 0, None ends up being considered True.

Like most of colander, it's probably a good idea to translate None values from your database into colander.null and then you'll get true, false, and null values.

daveoncode commented 8 years ago

I will submit a PR soon... in the meanwhile, just google "default boolen value" or try this in a python shell: assert bool() is False then... assert bool() is True boom! :D

digitalresistor commented 5 years ago

@daveoncode why not add str(None) as a value to false_choices?

For example:

class Boolean(colander.Boolean):
    """Overrides the default false_choices to include 'None' unless false_choices is provided."""

    def __init__(self, **kwargs):
        kwargs["false_choices"] = kwargs.get("false_choices", ('false', '0', str(None)))
        super().__init__(
            **kwargs,
        )

In the code sample you supplied in https://github.com/Pylons/colander/issues/250#issue-127900218 you return colander.null which may do what you expect it to do only if you set missing=False on your SchemaNode, so it will still not return False by default.

twillis commented 2 years ago

this is very surprising behavior that just bit me in the ass in production no less.