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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1.10.X] Fix field regex with StrictStr type annotation #4538

Merged
merged 3 commits into from Sep 20, 2022

Conversation

sisp
Copy link

@sisp sisp commented Sep 19, 2022

There's an inconsistency between constr(...) and ConstrainedStr, which is the base class of StrictStr, with regard to the expected type of the regex argument/attribute which causes a field of type StrictStr with a regex constraint specified via the Field(...) class to raise an error:

File "pydantic/main.py", line 340, in pydantic.main.BaseModel.__init__
File "pydantic/main.py", line 1076, in pydantic.main.validate_model
File "pydantic/fields.py", line 884, in pydantic.fields.ModelField.validate
File "pydantic/fields.py", line 1101, in pydantic.fields.ModelField._validate_singleton
File "pydantic/fields.py", line 1148, in pydantic.fields.ModelField._apply_validators
File "pydantic/class_validators.py", line 318, in pydantic.class_validators._generic_validator_basic.lambda13
File "pydantic/types.py", line 433, in pydantic.types.ConstrainedStr.validate
AttributeError: 'str' object has no attribute 'match'

I've fixed this inconsistency in a backwards compatible way by extending the type of regex in ConstrainedStr to also support a str value in addition to Pattern[str].

I hope this fix will be considered despite the ongoing efforts towards Pydantic v2. 馃檪

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise happy to accept this.

Comment on lines 433 to 435
regex = cls._regex()
if regex:
if not regex.match(value):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this might be slow since the compiled regex might not be cached.

Better to do the following which I think should work in both cases

Suggested change
regex = cls._regex()
if regex:
if not regex.match(value):
if cls.regex:
if not re.match(cls.regex, value):

strict = False

@classmethod
def __modify_schema__(cls, field_schema: Dict[str, Any]) -> None:
regex = cls._regex()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll therefore still need an isinstance check here.

@sisp
Copy link
Author

sisp commented Sep 20, 2022

@samuelcolvin Thanks for your review feedback! I think I've made the changes accordingly which should improve regex caching. Happy to receive your feedback again. 馃檪

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@hramezani please review and check you're happy with this.

Copy link
Member

@hramezani hramezani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 馃憤

Thanks @sisp

@samuelcolvin samuelcolvin merged commit b171a7e into pydantic:1.10.X-fixes Sep 20, 2022
@samuelcolvin
Copy link
Member

thanks so much for this.

@sisp sisp deleted the fix/strict-str-regex branch September 20, 2022 13:45
@sisp
Copy link
Author

sisp commented Sep 20, 2022

Sure thing, thanks for the review parallel to all the v2 dev work! Can't wait to see v2 and all its improvements! 馃槈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants