Codecov Report

Merging #1974 into master will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##            master    #1974      +/-   ##
===========================================
- Coverage   100.00%   99.90%   -0.10%     
===========================================
  Files           21       21              
  Lines         3909     4035     +126     
  Branches       788      805      +17     
===========================================
+ Hits          3909     4031     +122     
- Misses           0        3       +3     
- Partials         0        1       +1     

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 bf9cc4a...13d01a2. Read the comment docs.

@samuelcolvin Looks like I found a bug, correct me in a case when I missed smth.

For instance, I define a custom type which inherits from str:

class CustomStr(str):
   ...

And I have to models:

class Foo(BaseModel):
    field: CustomStr # without constrain

class Bar(BaseModel):
    field: CustomStr = Field(..., min_length=10) # with constrain

Example of usage and results:

foo = Foo(field='foo')
bar = Bar(field='bar')

print(type(foo.field))
print(type(bar.field))

<class 'CustomStr'>
<class 'str'>

That's because of this code:

# schema.py L835
if issubclass(type_, (str, SecretStr)) and not issubclass(type_, (EmailStr, AnyUrl, ConstrainedStr)):
    attrs = ('max_length', 'min_length', 'regex')
    constraint_func = constr

So with my current implementation, SectertStr will be converted into str 😬

I have a future request and want to hear your opinion.

I propose to add __constraints__ custom attribute for custom-defined classes so the user can specify what kind of constraints supported by the class.

So, for instance, SecretStr class will look like:

class SecretStr:
    min_length: OptionalInt = None
    max_length: OptionalInt = None

    __constraints__: Set[str] = {'min_length', 'max_length'}

    @classmethod
    def __modify_schema__(cls, field_schema: Dict[str, Any]) -> None:
        update_not_none(
            field_schema,
            type='string',
            writeOnly=True,
            format='password',
            minLength=cls.min_length,
            maxLength=cls.max_length,
        )

With this approach, we can simplify schema.get_annotation_from_field_info function and allows us to write more extendable code.

For instance, we can define mixins:

class LengthValidation:
    min_length: OptionalInt = None
    max_length: OptionalInt = None

    __constraints__: Set[str] = {'min_length', 'max_length'}

    @classmethod
    def __modify_schema__(cls: Type[Sized], field_schema: Dict[str, Any]) -> None:
        update_not_none(field_schema, minLength=cls.min_length, maxLength=cls.max_length)

    @classmethod
    def __get_validators__(cls) -> 'CallableGenerator':
        yield constr_length_validator

class SecretStr(LengthValidation):
    @classmethod
    def __modify_schema__(cls, field_schema: Dict[str, Any]) -> None:
        super().__modify_schema__(field_schema)
        field_schema.update(type='string', writeOnly=True, format='password')

    def __len__(self) -> int:
        return len(self._secret_value)

@samuelcolvin What is your opinion regarding this feature?

Also, I thought about API like this:

class SecretBytes:
    min_length: Constraint[OptionalInt] = None
    max_length: Constraint[OptionalInt] = None

I have already updated my PR using this feature.

I think better to modify schema.py to add something like

if issubclass(type_, SecretStr) and (field_info.min_items is not None or field_info.max_items is not None):
    type_.min_length = field_info.min_items
    type_.max_length = field_info.max_items

Then allow yield the length validator from __get_validators__ if either of those properties is set.

I think that would require a smaller change?

@samuelcolvin I have updated PR, thanks for your suggestions.

And what about the bug and feature request which I described above?
Should I create issues regarding them?

And what about the bug and feature request which I described above?
Should I create issues regarding them?

if you have a bug or feature request, please create an issue or issues. But in general I think the current approach works pretty well, I don't want to change the interface or add more features until it's absolutely necessary.

I have updated PR.
I will create an issue regarding the bug which I found.

thanks so much.

nice,

I am still getting the original error message tho with version 1.7:

ValueError: On field "password" the following field constraints are set but not enforced: min_length. 
For more details see https://pydantic-docs.helpmanual.io/usage/schema/#unenforced-field-constraints

Hi @transfluxus, I just test it out, and it works as expected.

I have run this code:

from pydantic import BaseModel, Field, SecretStr


class Form(BaseModel):
    password: SecretStr = Field(min_length=6)

form = Form(password="hello world")

And I didn't receive any error.

Please, make sure that you are using the latest available version of a pydantic by running:

pip install -U pydantic

hey @uriyyo
indeed, there was a problem with my ide not updating pydantic to 1.7.

Would be nice if the constructor wouldn't have a warning about expecting a "SecretStr" instead of str.
Since

password=SecretStr("ceoofddffl")

actually fails with

password
  str type expected (type=type_error.str)

but this concerns all Secret Fieldtypes and is another issue.

thanks!