Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added generic approach to strict type checking for constrained types #799

Merged
merged 14 commits into from Sep 17, 2019

Conversation

@DerRidda
Copy link
Contributor

DerRidda commented Sep 9, 2019

Change Summary

  • Use the arbitrary validator to build strict validators for ConstrainedInt, ConstrainedFloat, ConstrainedStr
  • Make StrictStr a derived class of ConstrainedStr
  • Add tests for new strict cases for ConstrainedInt and ConstrainedFloat

This PR adds configurable strictness validation to ConstrainedStr, ConstrainedInt and ConstrainedFloat.

This behaviour is desired for our use case as we generally never want coercion for anything,
especially those that loses information such as float -> int coercion.

Related issue number

This issue seems related but was not regarded when creating this PR: #780

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.rst file added describing change
    (see changes/README.md for details)
- Use the arbitrary validator to build strict validators for ConstrainedInt, ConstrainedFloat, ConstrainedStr
- Make StrictStr a derived class of ConstrainedStr
- Add tests for new strict cases for ConstrainedInt and ConstrainedFloat
@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 9, 2019

Codecov Report

Merging #799 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #799   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          17     17           
  Lines        2972   2984   +12     
  Branches      578    580    +2     
=====================================
+ Hits         2972   2984   +12
Impacted Files Coverage Δ
pydantic/types.py 100% <100%> (ø) ⬆️
pydantic/validators.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c57937...359b468. Read the comment docs.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Sep 9, 2019

I think this looks good.

Please can you

  • also export StrictFloat and StrictInt (same way you do StrictStr)
  • add something to the docs about this, perhaps by extending and changing the StrictBool section
  • add a change description.
@DerRidda

This comment has been minimized.

Copy link
Contributor Author

DerRidda commented Sep 9, 2019

I have added all requested changes.

Docs have their own section now.

@DerRidda DerRidda changed the title Added generic approach to strict type checking for constraint types Added generic approach to strict type checking for constrained types Sep 9, 2019
docs/index.rst Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Show resolved Hide resolved
DerRidda and others added 2 commits Sep 11, 2019
Co-Authored-By: Zaar Hai <haizaar@users.noreply.github.com>
Co-Authored-By: Zaar Hai <haizaar@users.noreply.github.com>
@@ -785,6 +785,14 @@ The SecretStr and SecretBytes will be formatted as either `'**********'` or `''`

(This script is complete, it should run "as is")

Strict Types

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 11, 2019

Owner

can you move the "StrictBool" section into here.

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 12, 2019

Owner

You still need to remove the "StrictBool" section: https://pydantic-docs.helpmanual.io/#strictbool

This comment has been minimized.

Copy link
@DerRidda

DerRidda Sep 16, 2019

Author Contributor

Done.

docs/index.rst Outdated Show resolved Hide resolved
These types will only pass validation when the validated value is of the respective type or is a subtype of that type.
This behavior is also exposed via the ``strict`` field of the ``ConstrainedStr``, ``ConstrainedFloat`` and
``ConstrainedInt`` classes and can be combined with a multitude of complex validation rules (please note that there is no ``ConstrainedBool`` class).

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 11, 2019

Owner

Would be good to add a code example demonstrating this behaviour.

This comment has been minimized.

Copy link
@DerRidda

DerRidda Sep 16, 2019

Author Contributor

Done

pydantic/types.py Outdated Show resolved Hide resolved
tests/test_types.py Outdated Show resolved Hide resolved
tests/test_types.py Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
DerRidda added 3 commits Sep 11, 2019
- Make ConstrainedInt and ConstrainedFloat use those validators instead of abusing arbitrary type validator for strictness
- Prevent double validaton of same conditions by only yielding either the strict or non-strict type validator for for those classes
- Added example for strict type usage
- Added note about caveats for StrictInt and StrictFloat
pydantic/validators.py Outdated Show resolved Hide resolved
@@ -785,6 +785,14 @@ The SecretStr and SecretBytes will be formatted as either `'**********'` or `''`

(This script is complete, it should run "as is")

Strict Types

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 12, 2019

Owner

You still need to remove the "StrictBool" section: https://pydantic-docs.helpmanual.io/#strictbool

pydantic/validators.py Show resolved Hide resolved
DerRidda and others added 4 commits Sep 16, 2019
faster method to check if a value is boolean for strict int validator

Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
- Moved the Strctbool code example into strict_types.py example file
…other constrained types
@@ -617,12 +617,9 @@ A standard ``bool`` field will raise a ``ValidationError`` if the value is not o
``'off', 'f', 'false', 'n', 'no', '1', 'on', 't', 'true', 'y', 'yes'``
* a ``bytes`` which is valid (per the previous rule) when decoded to ``str``

For stricter behavior, ``StrictBool`` can be used to require specifically ``True`` or ``False``;

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 16, 2019

Owner

Can we keep this line and add a link to the "Strict Types" section.

Sorry to mess you around.

pydantic/validators.py Show resolved Hide resolved

@classmethod
def __get_validators__(cls) -> 'CallableGenerator':
yield not_none_validator
yield str_validator
yield make_arbitrary_type_validator(str) if cls.strict else str_validator

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 16, 2019

Owner

does this work if you pass it an enum that inherits from str? Perhaps it should, but it will likely confuse many people since the __str__ method is different from normal I think.

We should at least add a note to the docs.

This comment has been minimized.

Copy link
@DerRidda

DerRidda Sep 16, 2019

Author Contributor

Naturally this should pass with all subclasses as we only make the exception for integers with boolean as that is a weird quirk of Python where Guido probably looked at C too much.

I can add the note to the docs.

@samuelcolvin samuelcolvin merged commit 1b467da into samuelcolvin:master Sep 17, 2019
10 checks passed
10 checks passed
Header rules No header rules processed
Details
Pages changed 3 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 100% (+0%) compared to 0c57937
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20190917.3 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Sep 17, 2019

great, thank you so much.

@DerRidda

This comment has been minimized.

Copy link
Contributor Author

DerRidda commented Sep 17, 2019

Thank you for merging, I was actually still touching on a few things you had commented on but if it is fine like that I don't mind.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Sep 17, 2019

sorry about that, I'm crashing about trying to get to v1. Feel free to submit another PR if you have any tweaks.

@skewty

This comment has been minimized.

Copy link
Contributor

skewty commented Nov 18, 2019

I deleted my previous posts because the solution I came up with in the end seems trivial.

I guess responses were created due to emails generated.. So here is a response to a question that disappeared :)

@dmontagu

Why can't you do IdentStr = typing.NewType('IdentStr', ConstrainedStr) or similar?

import pydantic, typing

IdentStr = typing.NewType('IdentStr', pydantic.ConstrainedStr)
IdentStr.min_length

Traceback (most recent call last):
  File "<input>", line 1, in <module>
AttributeError: 'function' object has no attribute 'min_length'

typing.NewType only fakes making a real class.


Since in real life, we control the code for IdentStr, I am playing around with:

if PYDANTIC_VERSION >= "1.0.0":
    from pydantic import StrictStr
else:  # min version is 0.32.2 in setup.py
    from pydantic import ConstrainedStr
    from pydantic.errors import StrError

    class StrictStr(ConstrainedStr):
        @classmethod
        def strict_str_validator(cls, v: str) -> str:
            if not isinstance(v, str):
                raise StrError()
            return v

        @classmethod
        def __get_validators__(cls):
            yield cls.strict_str_validator
            yield from super().__get_validators__()

class IdentStr(StrictStr):
    min_length = 1
@DerRidda DerRidda deleted the DerRidda:strict-constrained-types branch Nov 18, 2019
@skewty

This comment has been minimized.

Copy link
Contributor

skewty commented Dec 1, 2019

I am sorta lost as to why this isn't working as expected for me in v1.2:

from typing import Type, TypeVar
import pydantic


class ConnectionName(pydantic.StrictStr):
    min_length = 1


class EndpointStr(pydantic.StrictStr):
    min_length = 1


DEFAULT_ENDPOINT = EndpointStr("default")
T = TypeVar("T", bound="UpRequestEvent")


class UpRequestEvent(pydantic.BaseModel):
    connection_name: ConnectionName  # connection name
    connection_endpoint: EndpointStr  # connection name's endpoint (for server type connections)

    @classmethod
    def build_from(
        cls: Type[T],
        connection_name: ConnectionName,
        connection_endpoint: EndpointStr = DEFAULT_ENDPOINT,
        **kwargs
    ) -> T:
        return cls(
            connection_name=connection_name,
            connection_endpoint=connection_endpoint,
        )

    class Config:
        use_enum_values = True


# This works
UpRequestEvent.build_from(
    connection_name="test", connection_endpoint="default",
)

# This fails
UpRequestEvent.build_from(
    connection_name=ConnectionName("test"), connection_endpoint=EndpointStr("default"),
)
Traceback (most recent call last):
  File "/opt/pycharm-eap/plugins/python/helpers/pydev/pydevd.py", line 1415, in _exec
    pydev_imports.execfile(file, globals, locals)  # execute the script
  File "/opt/pycharm-eap/plugins/python/helpers/pydev/_pydev_imps/_pydev_execfile.py", line 18, in execfile
    exec(compile(contents+"\n", file, 'exec'), glob, loc)
  File "/home/spalmer1009/.PyCharm2019.3/config/scratches/scratch.py", line 44, in <module>
    connection_name=ConnectionName("test"), connection_endpoint=EndpointStr("default"),
  File "/home/spalmer1009/.PyCharm2019.3/config/scratches/scratch.py", line 30, in build_from
    connection_endpoint=connection_endpoint,
  File "pydantic/main.py", line 274, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 2 validation errors for UpRequestEvent
connection_name
  Expected unicode, got ConnectionName (type=type_error)
connection_endpoint
  Expected unicode, got EndpointStr (type=type_error)

My special types ConnectionName and EndpointStr are a subclass of str (through StrictStr -> ConstrainedStr)

Am I not able to subclass these two types?

@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Dec 1, 2019

This appears to be an issue with the compiled vs. uncompiled versions of the package, as it works for me when not compiled (and fails when compiled). I'm trying to figure out if there is a workaround/solution now.

@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Dec 1, 2019

@skewty I think I found the issue.

If you change the signature of ConstrainedStr.validate to

def validate(cls, value: Union[str]) -> Union[str]: ...

and of strict_str_validator to:

def strict_str_validator(v: Any) -> Union[str]:

your code works.

I think what is happening is something weird with how cython handles str; using Union[str] prevents cython from applying some compilation magic.

@skewty It would be great if you could submit a new (bug) issue about this. It would also be great if you could make a PR with these changes and adding an appropriate test, but we can get to it eventually if you create an issue.

@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Dec 1, 2019

Strangely, looking at the differences in the generated cython code, you can see cython is actually injecting this error:

+060:         return v

    __Pyx_XDECREF(__pyx_r);
    if (!(likely(PyString_CheckExact(__pyx_v_v))||((__pyx_v_v) == Py_None)||(PyErr_Format(PyExc_TypeError, "Expected %.16s, got %.200s", "str", Py_TYPE(__pyx_v_v)->tp_name), 0))) __PYX_ERR(0, 60, __pyx_L1_error)
    __Pyx_INCREF(__pyx_v_v);
    __pyx_r = ((PyObject*)__pyx_v_v);

(In the -> Union[str] version no type error is injected like this.)

It also makes another minor change in the code earlier on, but I can't tell if that is affecting anything meaningful at first glance (looks like it is just looking up the Union type).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.