From 35bfde3ee17844e9eaed646c94223e6134070f7d Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Mon, 11 Sep 2023 18:08:04 -0700 Subject: [PATCH 01/10] Convert to Pydantic v2 Update all Pydantic code and examples to Pydantic v2. Use validation_alias instead of env for configuring the environment variables in examples that use BaseSettings. Convert validate_exactly_one_of to a model validator and require that it be called with mode="after". Pydantic now distinguishes between str and int, and the GitHub IDs all appear to be ints, so change the GitHub App and webhook models to type those fields as int rather than str. --- changelog.d/20230911_180355_rra_DM_40744.md | 4 ++ docs/user-guide/arq.rst | 9 ++- docs/user-guide/datetime.rst | 5 +- docs/user-guide/fastapi-errors.rst | 2 +- .../github-apps/create-a-github-client.rst | 13 +++-- .../github-apps/handling-webhooks.rst | 7 ++- docs/user-guide/pydantic.rst | 33 +++++++---- pyproject.toml | 2 +- src/safir/github/models.py | 34 +++++------ src/safir/github/webhooks.py | 4 +- src/safir/metadata.py | 10 ++-- src/safir/models.py | 6 +- src/safir/pydantic.py | 58 +++++++++---------- src/safir/redis.py | 8 +-- src/safir/slack/blockkit.py | 5 +- tests/fastapi_test.py | 2 +- tests/github/data/webhooks/push_event.json | 2 +- tests/github/webhooks_test.py | 40 ++++++++----- tests/models_test.py | 4 +- tests/pydantic_test.py | 38 ++++++------ 20 files changed, 160 insertions(+), 126 deletions(-) create mode 100644 changelog.d/20230911_180355_rra_DM_40744.md diff --git a/changelog.d/20230911_180355_rra_DM_40744.md b/changelog.d/20230911_180355_rra_DM_40744.md new file mode 100644 index 00000000..a953ecfa --- /dev/null +++ b/changelog.d/20230911_180355_rra_DM_40744.md @@ -0,0 +1,4 @@ +### Backwards-incompatible changes + +- Safir now depends on Pydantic v2. Python code that uses any part of Safir related to Pydantic will also need to update to Pydantic v2, since the API is significantly different. See the [Pydantic migration guide](https://docs.pydantic.dev/latest/migration/) for more information. +- `safir.pydantic.validate_exactly_one_of` is now a Pydantic model validator. It must be called with `mode="after"`, since it operates in the model rather than on a raw dictionary. diff --git a/docs/user-guide/arq.rst b/docs/user-guide/arq.rst index 5410548e..b892f233 100644 --- a/docs/user-guide/arq.rst +++ b/docs/user-guide/arq.rst @@ -45,16 +45,19 @@ If your app uses a configuration system like ``pydantic.BaseSettings``, this exa from urllib.parse import urlparse from arq.connections import RedisSettings - from pydantic import BaseSettings, Field, RedisDsn + from pydantic import Field, RedisDsn + from pydantic_settings import BaseSettings from safir.arq import ArqMode class Config(BaseSettings): arq_queue_url: RedisDsn = Field( - "redis://localhost:6379/1", env="APP_ARQ_QUEUE_URL" + "redis://localhost:6379/1", validation_alias="APP_ARQ_QUEUE_URL" ) - arq_mode: ArqMode = Field(ArqMode.production, env="APP_ARQ_MODE") + arq_mode: ArqMode = Field( + ArqMode.production, validation_alias="APP_ARQ_MODE" + ) @property def arq_redis_settings(self) -> RedisSettings: diff --git a/docs/user-guide/datetime.rst b/docs/user-guide/datetime.rst index 89bb4a22..5c96354a 100644 --- a/docs/user-guide/datetime.rst +++ b/docs/user-guide/datetime.rst @@ -53,15 +53,14 @@ To use this format as the serialized representation of any `~datetime.datetime` from datetime import datetime - from pydantic import BaseModel + from pydantic import BaseModel, field_serializer from safir.datetime import isodatetime class Example(BaseModel): some_time: datetime - class Config: - json_encoders = {datetime: lambda v: isodatetime(v)} + _serialize_some_time = field_serializer("some_time")(isodatetime) Also see the Pydantic validation function `safir.pydantic.normalize_isodatetime`, discussed further at :ref:`pydantic-datetime`. diff --git a/docs/user-guide/fastapi-errors.rst b/docs/user-guide/fastapi-errors.rst index 4eeadbae..285aa3fe 100644 --- a/docs/user-guide/fastapi-errors.rst +++ b/docs/user-guide/fastapi-errors.rst @@ -127,7 +127,7 @@ The code to raise ``fastapi.HTTPException`` should therefore look something like ) raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, - detail=[error.dict(exclude_none=True)], + detail=[error.model_dump(exclude_none=True)], ) Declaring the error model diff --git a/docs/user-guide/github-apps/create-a-github-client.rst b/docs/user-guide/github-apps/create-a-github-client.rst index 6cf86bd6..66471aa6 100644 --- a/docs/user-guide/github-apps/create-a-github-client.rst +++ b/docs/user-guide/github-apps/create-a-github-client.rst @@ -31,7 +31,8 @@ For information about creating a GitHub App, retrieving its App ID and generatin .. code-block:: python - from pydantic import BaseSettings, Field, SecretStr + from pydantic import Field, SecretStr + from pydantic_settings import BaseSettings from safir.github import GitHubAppClientFactory @@ -41,11 +42,15 @@ For information about creating a GitHub App, retrieving its App ID and generatin GitHub App functionality. """ - github_app_id: str = Field(env="GITHUB_APP_ID") + github_app_id: str = Field(validation_alias="GITHUB_APP_ID") - github_webhook_secret: SecretStr = Field(env="GITHUB_WEBHOOK_SECRET") + github_webhook_secret: SecretStr = Field( + validation_alias="GITHUB_WEBHOOK_SECRET" + ) - github_app_private_key: SecretStr = Field(env="GITHUB_APP_PRIVATE_KEY") + github_app_private_key: SecretStr = Field( + validation_alias="GITHUB_APP_PRIVATE_KEY" + ) config = Config() diff --git a/docs/user-guide/github-apps/handling-webhooks.rst b/docs/user-guide/github-apps/handling-webhooks.rst index d56c59a5..f21b5dc9 100644 --- a/docs/user-guide/github-apps/handling-webhooks.rst +++ b/docs/user-guide/github-apps/handling-webhooks.rst @@ -41,8 +41,9 @@ The URL path for this endpoint corresponds to the webhook callback URL you set u body = await request.body() event = Event.from_http(request.headers, body, secret=webhook_secret) - # Bind the X-GitHub-Delivery header to the logger context; this identifies - # the webhook request in GitHub's API and UI for diagnostics + # Bind the X-GitHub-Delivery header to the logger context; this + # identifies the webhook request in GitHub's API and UI for + # diagnostics logger = logger.bind(github_delivery=event.delivery_id) logger.debug("Received GitHub webhook", payload=event.data) @@ -138,7 +139,7 @@ You can parse the ``event.data`` attribute into a Pydantic model using the ``par f"Received {event.event} {event.data.action} event", event=event.event, action=event.data.action, - payload=pull_request_event.dict(), + payload=pull_request_event.model_dump(), number=pull_request_event.number, ) diff --git a/docs/user-guide/pydantic.rst b/docs/user-guide/pydantic.rst index 1b9dc340..e177aa72 100644 --- a/docs/user-guide/pydantic.rst +++ b/docs/user-guide/pydantic.rst @@ -20,17 +20,21 @@ Here's an example of how to use it: .. code-block:: python + from pydantic import BaseModel, field_validator + from safir.pydantic import normalize_datetime + + class Info(BaseModel): last_used: Optional[datetime] = Field( None, title="Last used", description="When last used in seconds since epoch", - example=1614986130, + examples=[1614986130], ) - _normalize_last_used = validator( - "last_used", allow_reuse=True, pre=True - )(normalize_datetime) + _normalize_last_used = field_validator("last_used", pre=True)( + normalize_datetime + ) Multiple attributes can be listed as the initial arguments of `~pydantic.validator` if there are multiple fields that need to be checked. @@ -56,19 +60,23 @@ To use it, add a configuration block to any Pydantic model that has snake-case a .. code-block:: python + from pydantic import BaseModel, ConfigDict + from safir.pydantic import to_camel_case + + class Model(BaseModel): some_field: str - class Config: - alias_generator = to_camel_case - allow_population_by_field_name = True + model_config = ConfigDict( + alias_generator=to_camel_case, populate_by_name=True + ) By default, only the generated aliases (so, in this case, only the camel-case form of the attribute, ``someField``) are supported. The additional setting ``allow_population_by_field_name``, tells Pydantic to allow either ``some_field`` or ``someField`` in the input. As a convenience, you can instead inherit from `~safir.pydantic.CamelCaseModel`, which is a derived class of `~pydantic.BaseModel` with those settings added. This is somewhat less obvious when reading the classes and thus less self-documenting, but is less tedious if you have numerous models that need to support camel-case. -`~safir.pydantic.CamelCaseModel` also overrides ``dict`` and ``json`` to change the default of ``by_alias`` to `True` so that this model exports in camel-case by default. +`~safir.pydantic.CamelCaseModel` also overrides ``model_dump`` and ``model_dump_json`` to change the default of ``by_alias`` to `True` so that this model exports in camel-case by default. Requiring exactly one of a list of attributes ============================================= @@ -86,19 +94,22 @@ The intent here is that only one of those two configurations will be present: ei However, Pydantic has no native way to express that, and the above model will accept input where neither or both of those attributes are set. Safir provides a function, `~safir.pydantic.validate_exactly_one_of`, designed for this case. -It takes a list of fields, of which exactly one must be set, and builds a root validator function that checks this property of the model. +It takes a list of fields, of which exactly one must be set, and builds a model validator function that checks this property of the model. So, in the above example, the full class would be: .. code-block:: python + from pydantic import BaseModel, model_validator + from safir.pydantic import validate_exactly_one_of + + class Model(BaseModel): docker: Optional[DockerConfig] = None ghcr: Optional[GHCRConfig] = None - _validate_type = root_validator(allow_reuse=True)( + _validate_type = model_validator(mode="after")( validate_exactly_one_of("docker", "ghcr") ) Note the syntax, which is a little odd since it is calling a decorator on the results of a function builder. -``allow_reuse=True`` must be set due to limitations in Pydantic. diff --git a/pyproject.toml b/pyproject.toml index 2261f54b..f7fa6d8c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -27,7 +27,7 @@ dependencies = [ "fastapi<1", "gidgethub<6", "httpx>=0.20.0,<1", - "pydantic<2", + "pydantic>2,<3", "starlette<1", "structlog>=21.2.0,<24", ] diff --git a/src/safir/github/models.py b/src/safir/github/models.py index 4c214571..91be0cb5 100644 --- a/src/safir/github/models.py +++ b/src/safir/github/models.py @@ -40,7 +40,7 @@ class GitHubRepoOwnerModel(BaseModel): description=( "Login name of the owner (either a user or an organization)." ), - example="lsst-sqre", + examples=["lsst-sqre"], ) @@ -76,7 +76,7 @@ class GitHubRepositoryModel(BaseModel): name: str = Field( title="Repository name", description="Excludes owner prefix.", - example="times-square-demo", + examples=["times-square-demo"], ) full_name: str = Field( @@ -85,46 +85,46 @@ class GitHubRepositoryModel(BaseModel): "Full name, including owner prefix " "(e.g. ``lsst-sqre/times-square-demo``).)" ), - example="lsst-sqre/times-square-demo", + examples=["lsst-sqre/times-square-demo"], ) owner: GitHubRepoOwnerModel = Field(description="The repository's owner.") default_branch: str = Field( - description="The default branch (e.g. main).", example="main" + description="The default branch (e.g. main).", examples=["main"] ) html_url: HttpUrl = Field( description="URL of the repository for browsers.", - example="https://github.com/lsst-sqre/times-square-demo", + examples=["https://github.com/lsst-sqre/times-square-demo"], ) branches_url: str = Field( description="URI template for the repo's branches endpoint.", - example=( + examples=[ "https://github.com/lsst-sqre/times-square-demo/branches{/branch}" - ), + ], ) contents_url: str = Field( description="URI template for the contents endpoint.", - example=( + examples=[ "https://github.com/lsst-sqre/times-square-demo/contents/{+path}" - ), + ], ) trees_url: str = Field( description="URI template for the Git tree API.", - example=( + examples=[ "https://github.com/lsst-sqre/times-square-demo/git/trees{/sha}" - ), + ], ) blobs_url: str = Field( description="URI template for the Git blobs API.", - example=( + examples=[ "https://github.com/lsst-sqre/times-square-demo/git/blobs{/sha}" - ), + ], ) @@ -181,7 +181,7 @@ class GitHubBranchModel(BaseModel): https://docs.github.com/en/rest/branches/branches#get-a-branch """ - name: str = Field(description="Branch name (e.g. main)", example="main") + name: str = Field(description="Branch name (e.g. main)", examples=["main"]) commit: GitHubBranchCommitModel = Field(description="HEAD commit info.") @@ -274,7 +274,7 @@ class GitHubCheckSuiteModel(BaseModel): webhook (`~safir.github.webhooks.GitHubCheckSuiteEventModel`). """ - id: str = Field(description="Identifier for this check run.") + id: int = Field(description="Identifier for this check run.") head_branch: str = Field( description="Name of the branch the changes are on.", @@ -353,7 +353,7 @@ class GitHubCheckRunAnnotationLevel(str, Enum): class GitHubCheckSuiteId(BaseModel): """Brief information about a check suite in the `GitHubCheckRunModel`.""" - id: str = Field(description="Check suite ID") + id: int = Field(description="Check suite ID") class GitHubCheckRunOutput(BaseModel): @@ -383,7 +383,7 @@ class GitHubCheckRunModel(BaseModel): payload (`~safir.github.webhooks.GitHubCheckRunEventModel`). """ - id: str = Field(description="Identifier for this check run.") + id: int = Field(description="Identifier for this check run.") external_id: str | None = Field( description="Identifier set by the check runner." diff --git a/src/safir/github/webhooks.py b/src/safir/github/webhooks.py index 5815679f..d17c77cf 100644 --- a/src/safir/github/webhooks.py +++ b/src/safir/github/webhooks.py @@ -35,7 +35,7 @@ class GitHubAppInstallationModel(BaseModel): payloads for GitHub Apps. """ - id: str = Field(description="The installation ID.") + id: int = Field(description="The installation ID.") class GitHubPushEventModel(BaseModel): @@ -58,7 +58,7 @@ class GitHubPushEventModel(BaseModel): "The full git ref that was pushed. Example: refs/heads/main or " "refs/tags/v3.14.1." ), - example="refs/heads/main", + examples=["refs/heads/main"], ) before: str = Field( diff --git a/src/safir/metadata.py b/src/safir/metadata.py index a36d3c30..c3829a36 100644 --- a/src/safir/metadata.py +++ b/src/safir/metadata.py @@ -14,20 +14,20 @@ class Metadata(BaseModel): """Metadata about a package.""" - name: str = Field(..., title="Application name", example="myapp") + name: str = Field(..., title="Application name", examples=["myapp"]) - version: str = Field(..., title="Version", example="1.0.0") + version: str = Field(..., title="Version", examples=["1.0.0"]) description: str | None = Field( - None, title="Description", example="string" + None, title="Description", examples=["Some package description"] ) repository_url: str | None = Field( - None, title="Repository URL", example="https://example.com/" + None, title="Repository URL", examples=["https://example.com/"] ) documentation_url: str | None = Field( - None, title="Documentation URL", example="https://example.com/" + None, title="Documentation URL", examples=["https://example.com/"] ) diff --git a/src/safir/models.py b/src/safir/models.py index ee68241c..5ae300f1 100644 --- a/src/safir/models.py +++ b/src/safir/models.py @@ -35,12 +35,12 @@ class ErrorDetail(BaseModel): """The detail of the error message.""" loc: list[str] | None = Field( - None, title="Location", example=["area", "field"] + None, title="Location", examples=[["area", "field"]] ) - msg: str = Field(..., title="Message", example="Some error messge") + msg: str = Field(..., title="Message", examples=["Some error messge"]) - type: str = Field(..., title="Error type", example="some_code") + type: str = Field(..., title="Error type", examples=["some_code"]) class ErrorModel(BaseModel): diff --git a/src/safir/pydantic.py b/src/safir/pydantic.py index a0f41829..210e5bcf 100644 --- a/src/safir/pydantic.py +++ b/src/safir/pydantic.py @@ -6,7 +6,7 @@ from datetime import UTC, datetime from typing import Any, ParamSpec, TypeVar -from pydantic import BaseModel +from pydantic import BaseModel, ConfigDict P = ParamSpec("P") T = TypeVar("T") @@ -50,12 +50,12 @@ class Info(BaseModel): None, title="Last used", description="When last used in seconds since epoch", - example=1614986130, + examples=[1614986130], ) - _normalize_last_used = validator( - "last_used", allow_reuse=True, pre=True - )(normalize_datetime) + _normalize_last_used = field_validator("last_used", pre=True)( + normalize_datetime + ) """ if v is None: return v @@ -99,12 +99,12 @@ class Info(BaseModel): None, title="Last used", description="Date and time last used", - example="2023-01-25T15:44:34Z", + examples=["2023-01-25T15:44:34Z"], ) - _normalize_last_used = validator( - "last_used", allow_reuse=True, pre=True - )(normalize_isodatetime) + _normalize_last_used = field_validator("last_used", pre=True)( + normalize_isodatetime + ) """ if v is None: return None @@ -142,9 +142,9 @@ def to_camel_case(string: str) -> str: class Model(BaseModel): some_field: str - class Config: - alias_generator = to_camel_case - allow_population_by_field_name = True + model_config = ConfigDict( + alias_generator=to_camel_case, populate_by_name=True + ) This must be added to every class that uses ``snake_case`` for an attribute and that needs to be initialized from ``camelCase``. @@ -182,16 +182,16 @@ class CamelCaseModel(BaseModel): This is a convenience class identical to `~pydantic.BaseModel` except with an alias generator configured so that it can be initialized with either - camel-case or snake-case keys. Model exports with ``dict`` or ``json`` - also default to exporting in camel-case. + camel-case or snake-case keys. Model exports with ``model_dump`` or + ``model_dump_json`` also default to exporting in camel-case. """ - class Config: - alias_generator = to_camel_case - allow_population_by_field_name = True + model_config = ConfigDict( + alias_generator=to_camel_case, populate_by_name=True + ) - @_copy_type(BaseModel.dict) - def dict(self, **kwargs: Any) -> dict[str, Any]: + @_copy_type(BaseModel.model_dump) + def model_dump(self, **kwargs: Any) -> dict[str, Any]: """Export the model as a dictionary. Overridden to change the default of ``by_alias`` from `False` to @@ -199,10 +199,10 @@ def dict(self, **kwargs: Any) -> dict[str, Any]: """ if "by_alias" not in kwargs: kwargs["by_alias"] = True - return super().dict(**kwargs) + return super().model_dump(**kwargs) - @_copy_type(BaseModel.json) - def json(self, **kwargs: Any) -> str: + @_copy_type(BaseModel.model_dump_json) + def model_dump_json(self, **kwargs: Any) -> str: """Export the model as JSON. Overridden to change the default of ``by_alias`` from `False` to @@ -210,12 +210,12 @@ def json(self, **kwargs: Any) -> str: """ if "by_alias" not in kwargs: kwargs["by_alias"] = True - return super().json(**kwargs) + return super().model_dump_json(**kwargs) def validate_exactly_one_of( *settings: str, -) -> Callable[[type, dict[str, Any]], dict[str, Any]]: +) -> Callable[[BaseModel], BaseModel]: """Generate a validator imposing a one and only one constraint. Sometimes, models have a set of attributes of which one and only one may @@ -233,7 +233,7 @@ def validate_exactly_one_of( Returns ------- Callable - The root validator. + Resulting model validator. Examples -------- @@ -246,7 +246,7 @@ class Foo(BaseModel): bar: Optional[str] = None baz: Optional[str] = None - _validate_options = root_validator(allow_reuse=True)( + _validate_options = model_validator(mode="after")( validate_exactly_one_of("foo", "bar", "baz") ) @@ -263,15 +263,15 @@ class Foo(BaseModel): else: options = ", ".join(settings[:-1]) + ", and " + settings[-1] - def validator(cls: type, values: dict[str, Any]) -> dict[str, Any]: + def validator(model: T) -> T: seen = False for setting in settings: - if setting in values and values[setting] is not None: + if getattr(model, setting, None) is not None: if seen: raise ValueError(f"only one of {options} may be given") seen = True if not seen: raise ValueError(f"one of {options} must be given") - return values + return model return validator diff --git a/src/safir/redis.py b/src/safir/redis.py index 854568a5..ee901f5e 100644 --- a/src/safir/redis.py +++ b/src/safir/redis.py @@ -198,7 +198,7 @@ def _serialize(self, obj: S) -> bytes: bytes The serialized object. """ - return obj.json().encode() + return obj.model_dump_json().encode() def _deserialize(self, data: bytes) -> S: """Deserialize bytes into a Pydantic object. @@ -213,7 +213,7 @@ def _deserialize(self, data: bytes) -> S: S The deserialized Pydantic object. """ - return self._datatype.parse_raw(data.decode()) + return self._datatype.model_validate_json(data.decode()) class EncryptedPydanticRedisStorage(PydanticRedisStorage[S]): @@ -249,9 +249,9 @@ def __init__( self._fernet = Fernet(encryption_key.encode()) def _serialize(self, obj: S) -> bytes: - data = obj.json().encode() + data = obj.model_dump_json().encode() return self._fernet.encrypt(data) def _deserialize(self, data: bytes) -> S: data = self._fernet.decrypt(data) - return self._datatype.parse_raw(data.decode()) + return self._datatype.model_validate_json(data.decode()) diff --git a/src/safir/slack/blockkit.py b/src/safir/slack/blockkit.py index 8216df18..610e5755 100644 --- a/src/safir/slack/blockkit.py +++ b/src/safir/slack/blockkit.py @@ -7,7 +7,7 @@ from typing import Any, ClassVar, Self from httpx import HTTPError, HTTPStatusError -from pydantic import BaseModel, validator +from pydantic import BaseModel, field_validator from safir.datetime import current_datetime, format_datetime_for_logging @@ -179,7 +179,8 @@ class SlackMessage(BaseModel): for long text for want of a better alternative. """ - @validator("fields") + @field_validator("fields") + @classmethod def _validate_fields(cls, v: list[SlackBaseField]) -> list[SlackBaseField]: """Check constraints on fields. diff --git a/tests/fastapi_test.py b/tests/fastapi_test.py index 31ffcc66..23d27f56 100644 --- a/tests/fastapi_test.py +++ b/tests/fastapi_test.py @@ -58,7 +58,7 @@ async def permission_error() -> dict[str, str]: r = await client.get("/address") assert r.status_code == 422 - error = ErrorModel.parse_obj(r.json()) + error = ErrorModel.model_validate(r.json()) assert error.detail[0].loc == [ErrorLocation.body, "user", "address"] assert error.detail[0].msg == "Invalid address" assert error.detail[0].type == "invalid_address" diff --git a/tests/github/data/webhooks/push_event.json b/tests/github/data/webhooks/push_event.json index 5a9f79d4..9d6d2ff0 100644 --- a/tests/github/data/webhooks/push_event.json +++ b/tests/github/data/webhooks/push_event.json @@ -1,6 +1,6 @@ { "installation": { - "id": "123xyz" + "id": 123 }, "ref": "refs/tags/simple-tag", "before": "0000000000000000000000000000000000000000", diff --git a/tests/github/webhooks_test.py b/tests/github/webhooks_test.py index 68efed17..0487c374 100644 --- a/tests/github/webhooks_test.py +++ b/tests/github/webhooks_test.py @@ -4,12 +4,22 @@ from pathlib import Path -from safir.github import webhooks from safir.github.models import ( GitHubCheckRunStatus, GitHubCheckSuiteConclusion, GitHubCheckSuiteStatus, ) +from safir.github.webhooks import ( + GitHubAppInstallationEventAction, + GitHubAppInstallationEventModel, + GitHubAppInstallationRepositoriesEventModel, + GitHubCheckRunEventAction, + GitHubCheckRunEventModel, + GitHubCheckSuiteEventModel, + GitHubPullRequestEventAction, + GitHubPullRequestEventModel, + GitHubPushEventModel, +) def read_webhook_data(filename: str) -> str: @@ -20,7 +30,7 @@ def read_webhook_data(filename: str) -> str: def test_push_event() -> None: """Test parsing a push event webhook payload.""" - data = webhooks.GitHubPushEventModel.parse_raw( + data = GitHubPushEventModel.model_validate_json( read_webhook_data("push_event.json") ) @@ -30,18 +40,18 @@ def test_push_event() -> None: def test_installation_event() -> None: """Test parsing an installation event webhook payload.""" - data = webhooks.GitHubAppInstallationEventModel.parse_raw( + data = GitHubAppInstallationEventModel.model_validate_json( read_webhook_data("installation.json") ) - assert data.action == webhooks.GitHubAppInstallationEventAction.deleted + assert data.action == GitHubAppInstallationEventAction.deleted assert data.repositories[0].name == "Hello-World" assert data.repositories[0].owner_name == "octocat" def test_installation_repositories_event() -> None: """Test parsing an installation_repositories event webhook payload.""" - data = webhooks.GitHubAppInstallationRepositoriesEventModel.parse_raw( + data = GitHubAppInstallationRepositoriesEventModel.model_validate_json( read_webhook_data("installation_repositories.json") ) @@ -52,24 +62,24 @@ def test_installation_repositories_event() -> None: def test_pull_request_event() -> None: """Test parsing a pull_request event webhook payload.""" - data = webhooks.GitHubPullRequestEventModel.parse_raw( + data = GitHubPullRequestEventModel.model_validate_json( read_webhook_data("pull_request_event.json") ) assert data.number == 2 - assert data.action == webhooks.GitHubPullRequestEventAction.opened + assert data.action == GitHubPullRequestEventAction.opened assert data.pull_request.number == 2 assert data.pull_request.title == "Update the README with new information." def test_check_suite_completed_event() -> None: """Test parsing a check_suite completed event webhook payload.""" - data = webhooks.GitHubCheckSuiteEventModel.parse_raw( + data = GitHubCheckSuiteEventModel.model_validate_json( read_webhook_data("check_suite_completed.json") ) assert data.action == "completed" - assert data.check_suite.id == "118578147" + assert data.check_suite.id == 118578147 assert data.check_suite.head_branch == "changes" assert data.check_suite.head_sha == ( "ec26c3e57ca3a959ca5aad62de7213c562f8c821" @@ -80,19 +90,19 @@ def test_check_suite_completed_event() -> None: def test_check_run_created_event() -> None: """Test parsing a check_run created event webhook payload.""" - data = webhooks.GitHubCheckRunEventModel.parse_raw( + data = GitHubCheckRunEventModel.model_validate_json( read_webhook_data("check_run_created.json") ) - assert data.action == webhooks.GitHubCheckRunEventAction.created - assert data.check_run.id == "128620228" + assert data.action == GitHubCheckRunEventAction.created + assert data.check_run.id == 128620228 assert data.check_run.external_id == "" - assert data.check_run.url == ( + assert str(data.check_run.url) == ( "https://api.github.com/repos/Codertocat/Hello-World" "/check-runs/128620228" ) - assert data.check_run.html_url == ( + assert str(data.check_run.html_url) == ( "https://github.com/Codertocat/Hello-World/runs/128620228" ) assert data.check_run.status == GitHubCheckRunStatus.queued - assert data.check_run.check_suite.id == "118578147" + assert data.check_run.check_suite.id == 118578147 diff --git a/tests/models_test.py b/tests/models_test.py index f405d83b..88d873a7 100644 --- a/tests/models_test.py +++ b/tests/models_test.py @@ -18,5 +18,5 @@ def test_error_model() -> None: } ] } - model = ErrorModel.parse_raw(json.dumps(error)) - assert model.dict() == error + model = ErrorModel.model_validate_json(json.dumps(error)) + assert model.model_dump() == error diff --git a/tests/pydantic_test.py b/tests/pydantic_test.py index 161c06c5..70a7037f 100644 --- a/tests/pydantic_test.py +++ b/tests/pydantic_test.py @@ -6,7 +6,7 @@ from datetime import UTC, datetime, timedelta, timezone import pytest -from pydantic import BaseModel, ValidationError, root_validator +from pydantic import BaseModel, ValidationError, model_validator from safir.pydantic import ( CamelCaseModel, @@ -76,21 +76,21 @@ class TestModel(CamelCaseModel): "replace_403": False, "foo_bar_baz": "something", } - data = TestModel.parse_obj(camel) + data = TestModel.model_validate(camel) assert data.minimum_lifetime == 10 assert not data.replace_403 assert data.foo_bar_baz == "something" - assert data.dict() == camel - assert data.dict(by_alias=False) == snake - assert data.json() == json.dumps(camel) - assert data.json(by_alias=False) == json.dumps(snake) + assert data.model_dump() == camel + assert data.model_dump(by_alias=False) == snake + assert json.loads(data.model_dump_json()) == camel + assert json.loads(data.model_dump_json(by_alias=False)) == snake - snake_data = TestModel.parse_obj(snake) + snake_data = TestModel.model_validate(snake) assert data.minimum_lifetime == 10 assert not data.replace_403 assert data.foo_bar_baz == "something" - assert snake_data.dict() == data.dict() - assert snake_data.json() == data.json() + assert snake_data.model_dump() == data.model_dump() + assert snake_data.model_dump_json() == data.model_dump_json() def test_validate_exactly_one_of() -> None: @@ -99,35 +99,35 @@ class Model(BaseModel): bar: int | None = None baz: int | None = None - _validate_type = root_validator(allow_reuse=True)( + _validate_type = model_validator(mode="after")( validate_exactly_one_of("foo", "bar", "baz") ) - Model.parse_obj({"foo": 4, "bar": None}) - Model.parse_obj({"baz": 4}) - Model.parse_obj({"bar": 4}) - Model.parse_obj({"foo": None, "bar": 4}) + Model.model_validate({"foo": 4, "bar": None}) + Model.model_validate({"baz": 4}) + Model.model_validate({"bar": 4}) + Model.model_validate({"foo": None, "bar": 4}) with pytest.raises(ValidationError) as excinfo: - Model.parse_obj({"foo": 4, "bar": 3, "baz": None}) + Model.model_validate({"foo": 4, "bar": 3, "baz": None}) assert "only one of foo, bar, and baz may be given" in str(excinfo.value) with pytest.raises(ValidationError) as excinfo: - Model.parse_obj({"foo": None, "baz": None}) + Model.model_validate({"foo": None, "baz": None}) assert "one of foo, bar, and baz must be given" in str(excinfo.value) class TwoModel(BaseModel): foo: int | None = None bar: int | None = None - _validate_type = root_validator(allow_reuse=True)( + _validate_type = model_validator(mode="after")( validate_exactly_one_of("foo", "bar") ) with pytest.raises(ValidationError) as excinfo: - TwoModel.parse_obj({"foo": 3, "bar": 4}) + TwoModel.model_validate({"foo": 3, "bar": 4}) assert "only one of foo and bar may be given" in str(excinfo.value) with pytest.raises(ValidationError) as excinfo: - TwoModel.parse_obj({}) + TwoModel.model_validate({}) assert "one of foo and bar must be given" in str(excinfo.value) From de99989a33609b53dad7945770c22fb358b5c341 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Tue, 12 Sep 2023 10:52:21 -0700 Subject: [PATCH 02/10] Fix some AsyncMultiQueue documentation issues Document the return type of aiter_from, and fix a cross-reference that was still pointing to the end method rather than close. --- src/safir/asyncio.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/safir/asyncio.py b/src/safir/asyncio.py index 6201b5c1..9657398a 100644 --- a/src/safir/asyncio.py +++ b/src/safir/asyncio.py @@ -87,6 +87,11 @@ def aiter_from( timeout waiting for the next item; this is the total execution time of the iterator. + Returns + ------- + AsyncIterator + An async iterator over the contents of the queue. + Raises ------ TimeoutError @@ -176,8 +181,8 @@ def put(self, item: T) -> None: Raises ------ AsyncMultiQueueError - Raised if `put` was called after `end` without an intervening call - to `clear`. + Raised if `put` was called after `close` without an intervening + call to `clear`. """ if self.finished: msg = "end was already called, must call clear before put" From 87291471331a59b36262bc62aba63f18a928ce93 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Tue, 12 Sep 2023 10:52:57 -0700 Subject: [PATCH 03/10] Remove -W from the docs build for now Pydantic v2 uses TYPE_CHECKING more aggressively in ways that sphinx-autodoc-typehints currently doesn't cope with correctly. See https://github.com/tox-dev/sphinx-autodoc-typehints/issues/375. --- Makefile | 2 +- tox.ini | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 80eddb49..7be32eee 100644 --- a/Makefile +++ b/Makefile @@ -24,6 +24,6 @@ init: # level of shell trickery after failed commands. .PHONY: linkcheck linkcheck: - sphinx-build --keep-going -n -W -T -b linkcheck docs \ + sphinx-build --keep-going -n -T -b linkcheck docs \ docs/_build/linkcheck \ || (cat docs/_build/linkcheck/output.txt; exit 1) diff --git a/tox.ini b/tox.ini index 68761334..d1b0e291 100644 --- a/tox.ini +++ b/tox.ini @@ -82,7 +82,7 @@ allowlist_externals = rm commands = rm -rf docs/api - sphinx-build -W --keep-going -n -T -b html -d {envtmpdir}/doctrees docs docs/_build/html + sphinx-build --keep-going -n -T -b html -d {envtmpdir}/doctrees docs docs/_build/html [testenv:docs-linkcheck] description = Check links in the documentation. From 59d2a9ef7ff9f3eed60cea3ab7f8576e93ee9cf4 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Thu, 14 Sep 2023 14:01:42 -0700 Subject: [PATCH 04/10] Use Git version of documenteer The main branch of documenteer is required to work with Pydantic v2. --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index f7fa6d8c..82ed51cd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -56,7 +56,7 @@ dev = [ "types-redis", "uvicorn", # documentation - "documenteer[guide]>=0.7.0,<1", + "documenteer[guide] @ git+https://github.com/lsst-sqre/documenteer@main", "autodoc_pydantic", ] gcs = [ From ad95c752c1bb5ef15a36670e120a96204146dfb8 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Thu, 14 Sep 2023 14:01:58 -0700 Subject: [PATCH 05/10] Replace pre=True with mode="before" Pydantic v2 eliminated the pre=True flag on field validators and replaced it with mode="before". Change the documentation accordingly. --- docs/user-guide/pydantic.rst | 2 +- src/safir/pydantic.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/user-guide/pydantic.rst b/docs/user-guide/pydantic.rst index e177aa72..f0aca94a 100644 --- a/docs/user-guide/pydantic.rst +++ b/docs/user-guide/pydantic.rst @@ -32,7 +32,7 @@ Here's an example of how to use it: examples=[1614986130], ) - _normalize_last_used = field_validator("last_used", pre=True)( + _normalize_last_used = field_validator("last_used", mode="before")( normalize_datetime ) diff --git a/src/safir/pydantic.py b/src/safir/pydantic.py index 210e5bcf..a1103754 100644 --- a/src/safir/pydantic.py +++ b/src/safir/pydantic.py @@ -53,7 +53,7 @@ class Info(BaseModel): examples=[1614986130], ) - _normalize_last_used = field_validator("last_used", pre=True)( + _normalize_last_used = field_validator("last_used", mode="before")( normalize_datetime ) """ @@ -102,7 +102,7 @@ class Info(BaseModel): examples=["2023-01-25T15:44:34Z"], ) - _normalize_last_used = field_validator("last_used", pre=True)( + _normalize_last_used = field_validator("last_used", mode="before")( normalize_isodatetime ) """ From 6ed0396b8566ba73a9e1d34bbbaadd7be04d98a5 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Thu, 14 Sep 2023 15:41:33 -0700 Subject: [PATCH 06/10] Accept any UTC in datetime_to_db With Pydantic v2's JSON deserializer, datetime objects in models are created with their time zone set to TzInfo(UTC) instead of datetime.UTC. This broke datetime_to_db, since it was comparing the time zone for equality with datetime.UTC, and those objects are not equal. Instead, check whether the datetime has a zero UTC offset, and accept any datetime with that property. Add an explicit test against deserialized Pydantic models. --- changelog.d/20230914_153856_rra_DM_40744.md | 3 +++ src/safir/database.py | 4 ++-- src/safir/datetime.py | 6 +++--- tests/database_test.py | 10 ++++++++++ tests/datetime_test.py | 19 +++++++++++++++++++ 5 files changed, 37 insertions(+), 5 deletions(-) create mode 100644 changelog.d/20230914_153856_rra_DM_40744.md diff --git a/changelog.d/20230914_153856_rra_DM_40744.md b/changelog.d/20230914_153856_rra_DM_40744.md new file mode 100644 index 00000000..4867c51f --- /dev/null +++ b/changelog.d/20230914_153856_rra_DM_40744.md @@ -0,0 +1,3 @@ +### Bug fixes + +- `safir.database.datetime_to_db`, `safir.datetime.format_datetime_for_logging`, and `safir.datetime.isodatetime` now accept any `datetime` object with a time zone whose offset from UTC is 0, rather than only the `datetime.UTC` time zone object. diff --git a/src/safir/database.py b/src/safir/database.py index d8b920b8..c8076452 100644 --- a/src/safir/database.py +++ b/src/safir/database.py @@ -4,7 +4,7 @@ import asyncio import time -from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta from typing import overload from urllib.parse import quote, urlparse @@ -140,7 +140,7 @@ def datetime_to_db(time: datetime | None) -> datetime | None: """ if not time: return None - if time.tzinfo != UTC: + if time.utcoffset() != timedelta(seconds=0): raise ValueError(f"datetime {time} not in UTC") return time.replace(tzinfo=None) diff --git a/src/safir/datetime.py b/src/safir/datetime.py index beafd0f8..a9760aa5 100644 --- a/src/safir/datetime.py +++ b/src/safir/datetime.py @@ -2,7 +2,7 @@ from __future__ import annotations -from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta from typing import overload __all__ = [ @@ -75,7 +75,7 @@ def format_datetime_for_logging(timestamp: datetime | None) -> str | None: Raised if the argument is in a time zone other than UTC. """ if timestamp: - if timestamp.tzinfo not in (None, UTC): + if timestamp.utcoffset() != timedelta(seconds=0): raise ValueError(f"datetime {timestamp} not in UTC") if timestamp.microsecond: result = timestamp.isoformat(sep=" ", timespec="milliseconds") @@ -106,7 +106,7 @@ def isodatetime(timestamp: datetime) -> str: ValueError The provided timestamp was not in UTC. """ - if timestamp.tzinfo not in (None, UTC): + if timestamp.utcoffset() != timedelta(seconds=0): raise ValueError(f"datetime {timestamp} not in UTC") return timestamp.strftime("%Y-%m-%dT%H:%M:%SZ") diff --git a/tests/database_test.py b/tests/database_test.py index a093e2e8..be94bca8 100644 --- a/tests/database_test.py +++ b/tests/database_test.py @@ -8,6 +8,7 @@ import pytest import structlog +from pydantic import BaseModel from sqlalchemy import Column, MetaData, String, Table from sqlalchemy.exc import ProgrammingError from sqlalchemy.future import select @@ -188,3 +189,12 @@ def test_datetime() -> None: datetime_to_db(tz_local) with pytest.raises(ValueError, match=r"datetime .* not in UTC"): datetime_from_db(tz_local) + + # Pydantic's JSON decoder uses a TzInfo data structure instead of + # datetime.timezone.utc. Make sure that's still recognized as UTC. + class Test(BaseModel): + time: datetime + + json_model = Test(time=tz_aware).model_dump_json() + model = Test.model_validate_json(json_model) + assert datetime_to_db(model.time) == tz_naive diff --git a/tests/datetime_test.py b/tests/datetime_test.py index 5b95a9fc..2cc0d27e 100644 --- a/tests/datetime_test.py +++ b/tests/datetime_test.py @@ -5,6 +5,7 @@ from datetime import UTC, datetime, timedelta, timezone import pytest +from pydantic import BaseModel from safir.datetime import ( current_datetime, @@ -37,6 +38,15 @@ def test_isodatetime() -> None: with pytest.raises(ValueError, match=r"datetime .* not in UTC"): isodatetime(datetime.fromisoformat("2022-09-16T12:03:45+02:00")) + # Pydantic's JSON decoder uses a TzInfo data structure instead of + # datetime.timezone.utc. Make sure that's still recognized as UTC. + class Test(BaseModel): + time: datetime + + json_model = Test(time=time).model_dump_json() + model = Test.model_validate_json(json_model) + assert isodatetime(model.time) == "2022-09-16T12:03:45Z" + def test_parse_isodatetime() -> None: time = parse_isodatetime("2022-09-16T12:03:45Z") @@ -65,3 +75,12 @@ def test_format_datetime_for_logging() -> None: time = datetime.now(tz=timezone(timedelta(hours=1))) with pytest.raises(ValueError, match=r"datetime .* not in UTC"): format_datetime_for_logging(time) + + # Pydantic's JSON decoder uses a TzInfo data structure instead of + # datetime.timezone.utc. Make sure that's still recognized as UTC. + class Test(BaseModel): + time: datetime + + json_model = Test(time=now).model_dump_json() + model = Test.model_validate_json(json_model) + assert format_datetime_for_logging(model.time) == expected From 870caa26a298828aa7f8c29a71ff13046c23926f Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Mon, 18 Sep 2023 11:21:24 -0700 Subject: [PATCH 07/10] Be more consistent about validator temrinology Use field validator or model validator explicitly instead of talking about Pydantic validators. Remove the linter exclusion for the old root_validator and validator functions, which are no longer used. --- docs/user-guide/pydantic.rst | 8 ++++---- pyproject.toml | 6 ------ src/safir/pydantic.py | 40 ++++++++++++++++++------------------ 3 files changed, 24 insertions(+), 30 deletions(-) diff --git a/docs/user-guide/pydantic.rst b/docs/user-guide/pydantic.rst index f0aca94a..43044c77 100644 --- a/docs/user-guide/pydantic.rst +++ b/docs/user-guide/pydantic.rst @@ -13,7 +13,7 @@ Normalizing datetime fields Pydantic supports several input formats for `~datetime.datetime` fields, but the resulting `~datetime.datetime` object may be timezone-naive. Best practice for Python code is to only use timezone-aware `~datetime.datetime` objects in the UTC time zone. -Pydantic provides a utility function, `~safir.pydantic.normalize_datetime`, that can be used as a validator for a `~datetime.datetime` model field. +Pydantic provides a utility function, `~safir.pydantic.normalize_datetime`, that can be used as a field validator for a `~datetime.datetime` model field. It ensures that any input is converted to UTC and is always timezone-aware. Here's an example of how to use it: @@ -36,14 +36,14 @@ Here's an example of how to use it: normalize_datetime ) -Multiple attributes can be listed as the initial arguments of `~pydantic.validator` if there are multiple fields that need to be checked. +Multiple attributes can be listed as the initial arguments of `~pydantic.field_validator` if there are multiple fields that need to be checked. -This validator accepts all of the input formats that Pydantic accepts. +This field validator accepts all of the input formats that Pydantic accepts. This includes some ambiguous formats, such as an ISO 8601 date without time zone information. All such dates are given a consistent interpretation as UTC, but the results may be surprising if the caller expected local time. In some cases, it may be desirable to restrict input to one unambiguous format. -This can be done by using `~safir.pydantic.normalize_isodatetime` as the validator instead. +This can be done by using `~safir.pydantic.normalize_isodatetime` as the field validator instead. This function only accepts ``YYYY-MM-DDTHH:MM[:SS]Z`` as the input format. The ``Z`` time zone prefix indicating UTC is mandatory. It is called the same way as `~safir.pydantic.normalize_datetime`. diff --git a/pyproject.toml b/pyproject.toml index 82ed51cd..70d84486 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -265,12 +265,6 @@ builtins-ignorelist = [ fixture-parentheses = false mark-parentheses = false -[tool.ruff.pep8-naming] -classmethod-decorators = [ - "pydantic.root_validator", - "pydantic.validator", -] - [tool.ruff.pydocstyle] convention = "numpy" diff --git a/src/safir/pydantic.py b/src/safir/pydantic.py index a1103754..7abdfc98 100644 --- a/src/safir/pydantic.py +++ b/src/safir/pydantic.py @@ -21,12 +21,12 @@ def normalize_datetime(v: int | datetime | None) -> datetime | None: - """Pydantic validator for datetime fields. + """Pydantic field validator for datetime fields. Supports `~datetime.datetime` fields given in either any format supported by Pydantic natively, or in seconds since epoch (which Pydantic doesn't - support). This validator ensures that datetimes are always stored in the - model as timezone-aware UTC datetimes. + support). This field validator ensures that datetimes are always stored + in the model as timezone-aware UTC datetimes. Parameters ---------- @@ -41,12 +41,12 @@ def normalize_datetime(v: int | datetime | None) -> datetime | None: Examples -------- - Here is a partial model that uses this function as a validator. + Here is a partial model that uses this function as a field validator. .. code-block:: python class Info(BaseModel): - last_used: Optional[datetime] = Field( + last_used: datetime | None = Field( None, title="Last used", description="When last used in seconds since epoch", @@ -68,12 +68,12 @@ class Info(BaseModel): def normalize_isodatetime(v: str | None) -> datetime | None: - """Pydantic validator for datetime fields in ISO format. + """Pydantic field validator for datetime fields in ISO format. - This validator requires the ISO 8601 date and time format with ``Z`` as - the time zone (``YYYY-MM-DDTHH:MM:SSZ``). This format is compatible with - Kubernetes and the ISO UWS standard and is the same format produced by - `safir.datetime.isodatetime`. It should be used when the ambiguous + This field validator requires the ISO 8601 date and time format with ``Z`` + as the time zone (``YYYY-MM-DDTHH:MM:SSZ``). This format is compatible + with Kubernetes and the ISO UWS standard and is the same format produced + by `safir.datetime.isodatetime`. It should be used when the ambiguous formats supported by Pydantic by default (such as dates and times without time zone information) shouldn't be allowed. @@ -90,12 +90,12 @@ def normalize_isodatetime(v: str | None) -> datetime | None: Examples -------- - Here is a partial model that uses this function as a validator. + Here is a partial model that uses this function as a field validator. .. code-block:: python class Info(BaseModel): - last_used: Optional[datetime] = Field( + last_used: datetime | None = Field( None, title="Last used", description="Date and time last used", @@ -216,13 +216,13 @@ def model_dump_json(self, **kwargs: Any) -> str: def validate_exactly_one_of( *settings: str, ) -> Callable[[BaseModel], BaseModel]: - """Generate a validator imposing a one and only one constraint. + """Generate a model validator imposing a one and only one constraint. Sometimes, models have a set of attributes of which one and only one may be set. Ideally this is represented properly in the type system, but - occasionally it's more convenient to use a validator. This is a validator - generator that can produce a validator function that ensures one and only - one of an arbitrary set of attributes must be set. + occasionally it's more convenient to use a model validator. This is a + model validator generator that can produce a model validator function that + ensures one and only one of an arbitrary set of attributes must be set. Parameters ---------- @@ -237,7 +237,7 @@ def validate_exactly_one_of( Examples -------- - Use this inside a Pydantic class as a validator as follows: + Use this inside a Pydantic class as a model validator as follows: .. code-block:: python @@ -250,9 +250,9 @@ class Foo(BaseModel): validate_exactly_one_of("foo", "bar", "baz") ) - The attribute listed as the first argument to the ``validator`` call must - be the last attribute in the model definition so that any other attributes - have already been seen. + The attribute listed as the first argument to the ``model_validator`` call + must be the last attribute in the model definition so that any other + attributes have already been seen. """ if len(settings) < 2: msg = "validate_exactly_one_of takes at least two field names" From 69cbcf937a5ec758ee5217794e417290982bf3fd Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Mon, 18 Sep 2023 11:47:44 -0700 Subject: [PATCH 08/10] Treat GitHub app and installation IDs as ints Gidgethub treats app and installation IDs as str, but the GitHub API appears to return int. Use int in GitHubAppClientFactory for consistency with what GitHub returns, and convert them to str when passing them to Gidgethub. Add a changelog entry and a note on the class about this out of an abundance of caution. --- changelog.d/20230918_114322_rra_DM_40744.md | 3 +++ src/safir/github/_client.py | 16 +++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) create mode 100644 changelog.d/20230918_114322_rra_DM_40744.md diff --git a/changelog.d/20230918_114322_rra_DM_40744.md b/changelog.d/20230918_114322_rra_DM_40744.md new file mode 100644 index 00000000..e8eef5ec --- /dev/null +++ b/changelog.d/20230918_114322_rra_DM_40744.md @@ -0,0 +1,3 @@ +### Backwards-incompatible changes + +- `safir.github.GitHubAppClientFactory` now expects the application ID and installation ID (for `create_installation_client`) to be of type `int`, not `str`. This appears to match what GitHub's API returns, but not what Gidgethub expects. The ID is converted to a string when passing it to Gidgethub. diff --git a/src/safir/github/_client.py b/src/safir/github/_client.py index cf64e286..5608add0 100644 --- a/src/safir/github/_client.py +++ b/src/safir/github/_client.py @@ -21,10 +21,16 @@ class GitHubAppClientFactory: from (e.g. ``lsst-sqre/times-square``). http_client The httpx client. + + Notes + ----- + Gidgethub treats the application ID and installation ID as strings, but + GitHub's API appears to return them as integers. This class expects them + to be integers and converts them to strings when calling Gidgethub. """ def __init__( - self, *, id: str, key: str, name: str, http_client: httpx.AsyncClient + self, *, id: int, key: str, name: str, http_client: httpx.AsyncClient ) -> None: self.app_id = id self.app_key = key @@ -43,7 +49,7 @@ def get_app_jwt(self) -> str: The JWT token. """ return gidgethub.apps.get_jwt( - app_id=self.app_id, private_key=self.app_key + app_id=str(self.app_id), private_key=self.app_key ) def _create_client(self, *, oauth_token: str | None = None) -> GitHubAPI: @@ -72,7 +78,7 @@ def create_app_client(self) -> GitHubAPI: return self._create_client(oauth_token=self.get_app_jwt()) async def create_installation_client( - self, installation_id: str + self, installation_id: int ) -> GitHubAPI: """Create a client authenticated as an installation of the GitHub App for a specific repository or organization. @@ -93,8 +99,8 @@ async def create_installation_client( anon_client = self.create_anonymous_client() token_info = await gidgethub.apps.get_installation_access_token( anon_client, - installation_id=installation_id, - app_id=self.app_id, + installation_id=str(installation_id), + app_id=str(self.app_id), private_key=self.app_key, ) return self._create_client(oauth_token=token_info["token"]) From fca3f55ae5735b9f4f84e141d442d9553c56626b Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Mon, 18 Sep 2023 13:55:26 -0700 Subject: [PATCH 09/10] Properly test the Pydantic validators The Pydantic field validators were being tested without models, which wasn't ideal since they wouldn't be tested for compatibility with newer versions of Pydantic. Add model-based tests. normalize_datetime now explicitly rejects input other than seconds since epoch or datetime objects with a validation error rather than attempting to treat the input as a datetime object and potentially throwing more obscure errors. --- changelog.d/20230918_134911_rra_DM_40744.md | 3 ++ src/safir/pydantic.py | 14 ++++--- tests/pydantic_test.py | 43 +++++++++++++++------ 3 files changed, 43 insertions(+), 17 deletions(-) create mode 100644 changelog.d/20230918_134911_rra_DM_40744.md diff --git a/changelog.d/20230918_134911_rra_DM_40744.md b/changelog.d/20230918_134911_rra_DM_40744.md new file mode 100644 index 00000000..a88ab71b --- /dev/null +++ b/changelog.d/20230918_134911_rra_DM_40744.md @@ -0,0 +1,3 @@ +### Bug fixes + +- `safir.pydantic.normalize_datetime` now explicitly rejects input other than seconds since epoch or datetime objects with a validation error rather than attempting to treat the input as a datetime object and potentially throwing more obscure errors. diff --git a/src/safir/pydantic.py b/src/safir/pydantic.py index 7abdfc98..8bed00d7 100644 --- a/src/safir/pydantic.py +++ b/src/safir/pydantic.py @@ -20,18 +20,18 @@ ] -def normalize_datetime(v: int | datetime | None) -> datetime | None: +def normalize_datetime(v: Any) -> datetime | None: """Pydantic field validator for datetime fields. - Supports `~datetime.datetime` fields given in either any format supported - by Pydantic natively, or in seconds since epoch (which Pydantic doesn't - support). This field validator ensures that datetimes are always stored - in the model as timezone-aware UTC datetimes. + Supports `~datetime.datetime` fields given as either datetime objects or + seconds since epoch (not the other types Pydantic natively supports) and + ensures that the resulting datetime object is timezone-aware and in the + UTC timezone. Parameters ---------- v - The field representing a `~datetime.datetime`. + Field representing a `~datetime.datetime`. Returns ------- @@ -61,6 +61,8 @@ class Info(BaseModel): return v elif isinstance(v, int): return datetime.fromtimestamp(v, tz=UTC) + elif not isinstance(v, datetime): + raise ValueError("Must be a datetime or seconds since epoch") elif v.tzinfo and v.tzinfo.utcoffset(v) is not None: return v.astimezone(UTC) else: diff --git a/tests/pydantic_test.py b/tests/pydantic_test.py index 70a7037f..4fad7b46 100644 --- a/tests/pydantic_test.py +++ b/tests/pydantic_test.py @@ -6,7 +6,12 @@ from datetime import UTC, datetime, timedelta, timezone import pytest -from pydantic import BaseModel, ValidationError, model_validator +from pydantic import ( + BaseModel, + ValidationError, + field_validator, + model_validator, +) from safir.pydantic import ( CamelCaseModel, @@ -18,39 +23,55 @@ def test_normalize_datetime() -> None: - assert normalize_datetime(None) is None + class TestModel(BaseModel): + time: datetime | None + + _val = field_validator("time", mode="before")(normalize_datetime) + + assert TestModel(time=None).time is None date = datetime.fromtimestamp(1668814932, tz=UTC) - assert normalize_datetime(1668814932) == date + model = TestModel(time=1668814932) # type: ignore[arg-type] + assert model.time == date mst_zone = timezone(-timedelta(hours=7)) mst_date = datetime.now(tz=mst_zone) utc_date = mst_date.astimezone(UTC) - assert normalize_datetime(mst_date) == utc_date + assert TestModel(time=mst_date).time == utc_date naive_date = datetime.utcnow() # noqa: DTZ003 - aware_date = normalize_datetime(naive_date) + aware_date = TestModel(time=naive_date).time assert aware_date == naive_date.replace(tzinfo=UTC) assert aware_date.tzinfo == UTC + with pytest.raises(ValueError, match=r"Must be a datetime or seconds .*"): + TestModel(time="2023-01-25T15:44:00+00:00") # type: ignore[arg-type] + def test_normalize_isodatetime() -> None: - assert normalize_isodatetime(None) is None + class TestModel(BaseModel): + time: datetime | None + + _val = field_validator("time", mode="before")(normalize_isodatetime) + + assert TestModel(time=None).time is None date = datetime.fromisoformat("2023-01-25T15:44:34+00:00") - assert date == normalize_isodatetime("2023-01-25T15:44:34Z") + model = TestModel(time="2023-01-25T15:44:34Z") # type: ignore[arg-type] + assert model.time == date date = datetime.fromisoformat("2023-01-25T15:44:00+00:00") - assert date == normalize_isodatetime("2023-01-25T15:44Z") + model = TestModel(time="2023-01-25T15:44Z") # type: ignore[arg-type] + assert model.time == date with pytest.raises(ValueError, match=r"Must be a string in .* format"): - normalize_isodatetime("2023-01-25T15:44:00+00:00") + TestModel(time="2023-01-25T15:44:00+00:00") # type: ignore[arg-type] with pytest.raises(ValueError, match=r"Must be a string in .* format"): - normalize_isodatetime(1668814932) # type: ignore[arg-type] + TestModel(time=1668814932) # type: ignore[arg-type] with pytest.raises(ValueError, match=r"Must be a string in .* format"): - normalize_isodatetime("next thursday") + TestModel(time="next thursday") # type: ignore[arg-type] def test_to_camel_case() -> None: From 65fb84de0e0d6b4d07b9283937b1f21fc75c1adf Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Mon, 18 Sep 2023 14:32:52 -0700 Subject: [PATCH 10/10] Use alpha version of documenteer --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 70d84486..280ff559 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -56,7 +56,7 @@ dev = [ "types-redis", "uvicorn", # documentation - "documenteer[guide] @ git+https://github.com/lsst-sqre/documenteer@main", + "documenteer[guide]>=1.0.0a7", "autodoc_pydantic", ] gcs = [