Cottonwood-Technology / ValidX

Fast, powerful, and flexible validator with sane syntax.
BSD 2-Clause "Simplified" License
20 stars 4 forks source link

Type() does not raise error if `nullable=False` and `None` is provided #6

Closed darkdreamingdan closed 1 year ago

darkdreamingdan commented 3 years ago

https://github.com/Cottonwood-Technology/ValidX/blob/0d34ece03f0ae7461189f0077f65e5fd7b2b30f2/src/validx/py/special.py#L194

Seems there's a bug here - this should raise a InvalidTypeError but instead seems to return the original value (and hence pass checks).

I'm trying to implement a "Any type except NULL" check and it seems not possible. I was using Type(tp=object, nullable=False)

darkdreamingdan commented 3 years ago

It seems like this bug in prevelant throughout the codebase. The nullable behaviour only works for the truthy case, but no exception is raised in the falsy case.

kr41 commented 3 years ago

It works as intended.

  1. Once validator encounters None and None is a valid value (i.e. self.nullable is True), there is no need to further validation, so the value is returned as it is.
  2. If None is not a valid value, it fails type check and InvalidTypeError(expected=..., actual=<type 'NoneType'>) is raised.

Your "Anything but None" validator doesn't work, because object is base class for all Python types, including NoneType. So the None passes type check.

Consider to use abstract base class with a __subclasshook__ classmethod.

>>> import abc
>>> class AnythingButNone(abc.ABC):
...     @classmethod
...     def __subclasshook__(cls, C):
...         return C is not type(None)
... 
>>> isinstance(object(), AnythingButNone)
True
>>> isinstance(True, AnythingButNone)
True
>>> isinstance(False, AnythingButNone)
True
>>> isinstance(None, AnythingButNone)
False

See Python docs for details.

darkdreamingdan commented 3 years ago

Hey, thanks for the speedy response!

I'm aware that None is a subclass of object. We're keen to use Validator objects for all of our checks (which is why we hadn't gone for a pure python approach) because of our existing abstractions, and I suppose i had expected that nullable=False invalidates whatever value is provided, but reading the documentation it's more geared towards the truthy case instead. We currently just implemented a really simple validator to achieve this:

    class NotNull(Validator):
        def __call__(self, value, __context=None):
            if value is None:
                raise exc.InvalidTypeError(expected=type(object), actual=type(None))
            return value

Really great shout with the subclasshook though, will give that a shot so we can take advantage of Cython in existing checks.

Related to #5, if you set tp to str with coerce=True you'll also get this edge case (and here None is not a subclass of str). Recognise that is probably not the intended use of Type though.

Happy for you to close, thanks for the tip!

kr41 commented 3 years ago

Related to #5, if you set tp to str with coerce=True you'll also get this edge case (and here None is not a subclass of str).

Yes, because it works like this:

  1. Is None a valid value? No.
  2. Is None instance of str? No.
  3. Can it be coerced to str? Yes, str(None) doesn't raise nor ValueError neither TypeError.
kr41 commented 3 years ago

I will consider such edge cases in future versions.