-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Update pydantic to >= 2 #15979
Update pydantic to >= 2 #15979
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Update the `pydantic` dependency to `>= 2`. Contributed by @dvzrv. |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,8 +29,7 @@ | |
| overload, | ||
| ) | ||
|
|
||
| from pydantic import BaseModel, MissingError, PydanticValueError, ValidationError | ||
| from pydantic.error_wrappers import ErrorWrapper | ||
| from pydantic import BaseModel, ValidationError | ||
| from typing_extensions import Literal | ||
|
|
||
| from twisted.web.server import Request | ||
|
|
@@ -786,20 +785,20 @@ def validate_json_object(content: JsonDict, model_type: Type[Model]) -> Model: | |
| if it wasn't a JSON object. | ||
| """ | ||
| try: | ||
| instance = model_type.parse_obj(content) | ||
| instance = model_type.model_validate(content, strict=True) | ||
| except ValidationError as e: | ||
| # Choose a matrix error code. The catch-all is BAD_JSON, but we try to find a | ||
| # more specific error if possible (which occasionally helps us to be spec- | ||
| # compliant) This is a bit awkward because the spec's error codes aren't very | ||
| # clear-cut: BAD_JSON arguably overlaps with MISSING_PARAM and INVALID_PARAM. | ||
| errcode = Codes.BAD_JSON | ||
|
|
||
| raw_errors = e.raw_errors | ||
| if len(raw_errors) == 1 and isinstance(raw_errors[0], ErrorWrapper): | ||
| raw_error = raw_errors[0].exc | ||
| if isinstance(raw_error, MissingError): | ||
| raw_errors = e.errors() | ||
| if len(raw_errors) == 1: | ||
| error_type = raw_errors[0].get("type") | ||
| if error_type == "missing": | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some kind of constant we can use in place of
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also only found https://github.com/pydantic/pydantic-core/blob/f5ef7afbf3d0db1126f12557c10a4c1f6cf6b6c6/python/pydantic_core/core_schema.py#L3824 and https://docs.pydantic.dev/2.0/usage/validation_errors/#missing, so I am not sure. Can the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Frankly, I have a hard time understanding how to better look into this. Other than querying the |
||
| errcode = Codes.MISSING_PARAM | ||
| elif isinstance(raw_error, PydanticValueError): | ||
| else: | ||
| errcode = Codes.INVALID_PARAM | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a behaviour change to me; it seems like you can compare to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (though as noted below, maybe this is an improvement) |
||
|
|
||
| raise SynapseError(HTTPStatus.BAD_REQUEST, str(e), errcode=errcode) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,9 +11,16 @@ | |
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| from typing import TYPE_CHECKING, Dict, Optional | ||
| from typing import TYPE_CHECKING, Any, Dict, Optional | ||
|
|
||
| from pydantic import Extra, StrictInt, StrictStr, constr, validator | ||
| from pydantic import ( | ||
| ConfigDict, | ||
| StrictInt, | ||
| StrictStr, | ||
| constr, | ||
| field_validator, | ||
| model_validator, | ||
| ) | ||
|
|
||
| from synapse.rest.models import RequestBodyModel | ||
| from synapse.util.threepids import validate_email | ||
|
|
@@ -29,8 +36,7 @@ class AuthenticationData(RequestBodyModel): | |
| `.dict(exclude_unset=True)` to access them. | ||
| """ | ||
|
|
||
| class Config: | ||
| extra = Extra.allow | ||
| model_config = ConfigDict(extra="allow") | ||
|
|
||
| session: Optional[StrictStr] = None | ||
| type: Optional[StrictStr] = None | ||
|
|
@@ -41,7 +47,7 @@ class Config: | |
| else: | ||
| # See also assert_valid_client_secret() | ||
| ClientSecretStr = constr( | ||
| regex="[0-9a-zA-Z.=_-]", # noqa: F722 | ||
| pattern="[0-9a-zA-Z.=_-]", # noqa: F722 | ||
| min_length=1, | ||
| max_length=255, | ||
| strict=True, | ||
|
|
@@ -50,18 +56,18 @@ class Config: | |
|
|
||
| class ThreepidRequestTokenBody(RequestBodyModel): | ||
| client_secret: ClientSecretStr | ||
| id_server: Optional[StrictStr] | ||
| id_access_token: Optional[StrictStr] | ||
| next_link: Optional[StrictStr] | ||
| id_server: Optional[StrictStr] = None | ||
| id_access_token: Optional[StrictStr] = None | ||
| next_link: Optional[StrictStr] = None | ||
| send_attempt: StrictInt | ||
|
|
||
| @validator("id_access_token", always=True) | ||
| def token_required_for_identity_server( | ||
| cls, token: Optional[str], values: Dict[str, object] | ||
| ) -> Optional[str]: | ||
| if values.get("id_server") is not None and token is None: | ||
| @model_validator(mode="before") | ||
| def token_required_for_identity_server(cls, data: Dict[str, Any]) -> Dict[str, Any]: | ||
| """Ensure that an access token is provided when a server is provided.""" | ||
| if data.get("id_server") is not None and data.get("id_access_token") is None: | ||
| raise ValueError("id_access_token is required if an id_server is supplied.") | ||
| return token | ||
|
|
||
| return data | ||
|
|
||
|
|
||
| class EmailRequestTokenBody(ThreepidRequestTokenBody): | ||
|
|
@@ -72,14 +78,14 @@ class EmailRequestTokenBody(ThreepidRequestTokenBody): | |
| # know the exact spelling (eg. upper and lower case) of address in the database. | ||
| # Without this, an email stored in the database as "[email protected]" would cause | ||
| # user requests for "[email protected]" to raise a Not Found error. | ||
| _email_validator = validator("email", allow_reuse=True)(validate_email) | ||
| email_validator = field_validator("email")(validate_email) | ||
|
|
||
|
|
||
| if TYPE_CHECKING: | ||
| ISO3116_1_Alpha_2 = StrictStr | ||
| else: | ||
| # Per spec: two-letter uppercase ISO-3166-1-alpha-2 | ||
| ISO3116_1_Alpha_2 = constr(regex="[A-Z]{2}", strict=True) | ||
| ISO3116_1_Alpha_2 = constr(pattern="[A-Z]{2}", strict=True) | ||
|
|
||
|
|
||
| class MsisdnRequestTokenBody(ThreepidRequestTokenBody): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -807,21 +807,21 @@ def test_add_valid_email_second_time_canonicalise(self) -> None: | |
| def test_add_email_no_at(self) -> None: | ||
| self._request_token_invalid_email( | ||
| "address-without-at.bar", | ||
| expected_errcode=Codes.BAD_JSON, | ||
| expected_errcode=Codes.INVALID_PARAM, | ||
| expected_error="Unable to parse email address", | ||
| ) | ||
|
Comment on lines
807
to
812
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a nice improvement, and presumably corresponds to the changes in
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My hope is that I didn't break any valid assumptions with this, but looking at the tests it seemed correct. |
||
|
|
||
| def test_add_email_two_at(self) -> None: | ||
| self._request_token_invalid_email( | ||
| "foo@[email protected]", | ||
| expected_errcode=Codes.BAD_JSON, | ||
| expected_errcode=Codes.INVALID_PARAM, | ||
| expected_error="Unable to parse email address", | ||
| ) | ||
|
|
||
| def test_add_email_bad_format(self) -> None: | ||
| self._request_token_invalid_email( | ||
| "[email protected]@good.example.com", | ||
| expected_errcode=Codes.BAD_JSON, | ||
| expected_errcode=Codes.INVALID_PARAM, | ||
| expected_error="Unable to parse email address", | ||
| ) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd need to consult with other packagers here. From https://pkgs.org/search/?q=pydantic I would expect that
>=2will cause pain for other distrosThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of our rebuild on Arch Linux I have opened a few PRs to fix the remaining projects that I am aware of (also my own).
I realize that for some this might be a bigger change. However, the alternative is using the
v1, which in part has changed nonetheless and is not the same as using pydantic v1.From a packager perspective I fear that not upgrading will leave us with a
python-pydantic1package, that needs to conflict with thepython-pydanticpackage (so software that uses either can not be installed with the respective other).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted elsewhere, introducing a
python-pydantic1package seems out of the question, as it has to conflict withpython-pydanticand synapse pulls inpydantic>2via twisted -> inflect.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far no other packager of any other distribution has chimed in to raise concerns.
From my perspective I do not see any issues with moving to pydantic v2, as distributions can adapt/update all other pydantic dependents (that I was able to check) appear to be compatible with v2 now (or are about to drop its use).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re. other distributions:
I am the new pydantic maintainer in Fedora Linux. I am working on updating our development branch (Fedora 40) to pydantic v2, but Fedora 38 and 39 will stay on pydantic v1. It would be nice to add support for pydantic v2 while keeping support for pydantic v1 a little while longer so we can continue to update those releases.
/cc @V02460 who maintains our matrix-synapse package in case they have something to add.